[vm] Cleanup Dart_Get/Set/HasStickyError API and use isolate's sticky error only as a backup for thread's sticky error
Both thread and isolate have sticky error fields. Dart API was using
isolate's sticky error, while Dart code was using thread's sticky error.
There was a one-way move of a thread's sticky error into isolate
when thread was unscheduled.
This causes a problem as error in the isolate may go unnoticed and
repeated unscheduling/re-scheduling might end up overwriting the error
in the isolate (which triggers the assertion).
To solve this problem, this CL:
* Cleans up Dart API which manipulates isolate's sticky error, so
isolate's sticky error is never set directly.
* When sceduling an isolate to a thread, sticky error is moved back from
isolate (if any).
With this changes, thread's sticky error is always used if thread is running,
and isolate's sticky error is only used to hold sticky error while
isolate has no thread.
Fixes https://github.com/dart-lang/sdk/issues/35590
Change-Id: I99b128cac363ca2df75f6e64c083b1ec36c866ce
Reviewed-on: https://dart-review.googlesource.com/c/89442
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
diff --git a/runtime/bin/main.cc b/runtime/bin/main.cc
index cb1dd8d..97d18ec 100644
--- a/runtime/bin/main.cc
+++ b/runtime/bin/main.cc
@@ -705,11 +705,6 @@
static void OnIsolateShutdown(void* callback_data) {
Dart_EnterScope();
- Dart_Handle sticky_error = Dart_GetStickyError();
- if (!Dart_IsNull(sticky_error) && !Dart_IsFatalError(sticky_error)) {
- Log::PrintErr("%s\n", Dart_GetError(sticky_error));
- }
-
IsolateData* isolate_data = reinterpret_cast<IsolateData*>(callback_data);
isolate_data->OnIsolateShutdown();
diff --git a/runtime/include/dart_api.h b/runtime/include/dart_api.h
index a560eb2..3622eae 100644
--- a/runtime/include/dart_api.h
+++ b/runtime/include/dart_api.h
@@ -1176,29 +1176,6 @@
DART_EXPORT void Dart_SetPausedOnExit(bool paused);
/**
- * Called when the embedder has caught a top level unhandled exception error
- * in the current isolate.
- *
- * NOTE: It is illegal to call this twice on the same isolate without first
- * clearing the sticky error to null.
- *
- * \param error The unhandled exception error.
- */
-DART_EXPORT void Dart_SetStickyError(Dart_Handle error);
-
-/**
- * Does the current isolate have a sticky error?
- */
-DART_EXPORT bool Dart_HasStickyError();
-
-/**
- * Gets the sticky error for the current isolate.
- *
- * \return A handle to the sticky error object or null.
- */
-DART_EXPORT Dart_Handle Dart_GetStickyError();
-
-/**
* Handles the next pending message for the current isolate.
*
* May generate an unhandled exception error.
diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc
index 4f304d9..dbdcc89 100644
--- a/runtime/vm/dart_api_impl.cc
+++ b/runtime/vm/dart_api_impl.cc
@@ -1391,43 +1391,6 @@
#endif
}
-DART_EXPORT void Dart_SetStickyError(Dart_Handle error) {
- Thread* thread = Thread::Current();
- DARTSCOPE(thread);
- Isolate* isolate = thread->isolate();
- CHECK_ISOLATE(isolate);
- NoSafepointScope no_safepoint_scope;
- if ((isolate->sticky_error() != Error::null()) && !::Dart_IsNull(error)) {
- FATAL1("%s expects there to be no sticky error.", CURRENT_FUNC);
- }
- if (!::Dart_IsUnhandledExceptionError(error) && !::Dart_IsNull(error)) {
- FATAL1("%s expects the error to be an unhandled exception error or null.",
- CURRENT_FUNC);
- }
- isolate->SetStickyError(Api::UnwrapErrorHandle(Z, error).raw());
-}
-
-DART_EXPORT bool Dart_HasStickyError() {
- Thread* T = Thread::Current();
- Isolate* isolate = T->isolate();
- CHECK_ISOLATE(isolate);
- NoSafepointScope no_safepoint_scope;
- return isolate->sticky_error() != Error::null();
-}
-
-DART_EXPORT Dart_Handle Dart_GetStickyError() {
- Thread* T = Thread::Current();
- Isolate* I = T->isolate();
- CHECK_ISOLATE(I);
- NoSafepointScope no_safepoint_scope;
- if (I->sticky_error() != Error::null()) {
- TransitionNativeToVM transition(T);
- Dart_Handle error = Api::NewHandle(T, I->sticky_error());
- return error;
- }
- return Dart_Null();
-}
-
DART_EXPORT void Dart_NotifyIdle(int64_t deadline) {
Thread* T = Thread::Current();
CHECK_ISOLATE(T->isolate());
@@ -1608,10 +1571,12 @@
}
}
::Dart_EnterIsolate(Api::CastIsolate(I));
- if (I->sticky_error() != Object::null()) {
+ {
Thread* T = Thread::Current();
- TransitionNativeToVM transition(T);
- return Api::NewHandle(T, I->StealStickyError());
+ if (T->sticky_error() != Object::null()) {
+ TransitionNativeToVM transition(T);
+ return Api::NewHandle(T, T->StealStickyError());
+ }
}
if (FLAG_print_class_table) {
HANDLESCOPE(Thread::Current());
diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc
index 3a3c425..d2fecfa 100644
--- a/runtime/vm/dart_api_impl_test.cc
+++ b/runtime/vm/dart_api_impl_test.cc
@@ -3696,22 +3696,6 @@
Dart_ShutdownIsolate();
}
-TEST_CASE(DartAPI_SetStickyError) {
- const char* kScriptChars = "main() => throw 'HI';";
- Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL);
- Dart_Handle retobj = Dart_Invoke(lib, NewString("main"), 0, NULL);
- EXPECT(Dart_IsError(retobj));
- EXPECT(Dart_IsUnhandledExceptionError(retobj));
- EXPECT(!Dart_HasStickyError());
- EXPECT(Dart_GetStickyError() == Dart_Null());
- Dart_SetStickyError(retobj);
- EXPECT(Dart_HasStickyError());
- EXPECT(Dart_GetStickyError() != Dart_Null());
- Dart_SetStickyError(Dart_Null());
- EXPECT(!Dart_HasStickyError());
- EXPECT(Dart_GetStickyError() == Dart_Null());
-}
-
TEST_CASE(DartAPI_TypeGetNonParamtericTypes) {
const char* kScriptChars =
"class MyClass0 {\n"
diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
index 7f1e8a2..5a6a1a6 100644
--- a/runtime/vm/isolate.cc
+++ b/runtime/vm/isolate.cc
@@ -1699,11 +1699,8 @@
Dart::ShutdownIsolate(isolate);
}
-void Isolate::SetStickyError(RawError* sticky_error) {
- ASSERT(
- ((sticky_error_ == Error::null()) || (sticky_error == Error::null())) &&
- (sticky_error != sticky_error_));
- sticky_error_ = sticky_error;
+void Isolate::ClearStickyError() {
+ sticky_error_ = Error::null();
}
void Isolate::Run() {
@@ -2329,13 +2326,6 @@
deoptimized_code.Add(code);
}
-RawError* Isolate::StealStickyError() {
- NoSafepointScope no_safepoint;
- RawError* return_value = sticky_error_;
- sticky_error_ = Error::null();
- return return_value;
-}
-
#if !defined(PRODUCT)
void Isolate::set_pending_service_extension_calls(
const GrowableObjectArray& value) {
@@ -2837,6 +2827,11 @@
os_thread->set_thread(thread);
if (is_mutator) {
scheduled_mutator_thread_ = thread;
+ if (sticky_error() != Error::null()) {
+ ASSERT(thread->sticky_error() == Error::null());
+ thread->sticky_error_ = sticky_error_;
+ sticky_error_ = Error::null();
+ }
}
Thread::SetCurrent(thread);
os_thread->EnableThreadInterrupts();
diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h
index ffa2a03..f96a728 100644
--- a/runtime/vm/isolate.h
+++ b/runtime/vm/isolate.h
@@ -606,11 +606,8 @@
void set_deoptimized_code_array(const GrowableObjectArray& value);
void TrackDeoptimizedCode(const Code& code);
- // Also sends a paused at exit event over the service protocol.
- void SetStickyError(RawError* sticky_error);
-
RawError* sticky_error() const { return sticky_error_; }
- DART_WARN_UNUSED_RESULT RawError* StealStickyError();
+ void ClearStickyError();
void RetainKernelBlob(const ExternalTypedData& kernel_blob);
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index e680342..da8c753 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -866,12 +866,13 @@
// needs to be created earlier as VM isolate snapshot reader references it
// before Object::FinalizeVMIsolate.
- // Some thread fields need to be reinitialized as null constants have not been
- // initialized until now.
+ // Some thread and isolate fields need to be reinitialized as null constants
+ // have not been initialized until now.
Thread* thr = Thread::Current();
ASSERT(thr != NULL);
thr->ClearStickyError();
thr->clear_pending_functions();
+ isolate->ClearStickyError();
ASSERT(!null_object_->IsSmi());
ASSERT(!null_array_->IsSmi());
diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc
index 580d290..2860a7e 100644
--- a/runtime/vm/service.cc
+++ b/runtime/vm/service.cc
@@ -3142,7 +3142,8 @@
"A library tag handler must be installed.");
return true;
}
- if ((isolate->sticky_error() != Error::null()) ||
+ if ((thread->sticky_error() != Error::null()) ||
+ (isolate->sticky_error() != Error::null()) ||
(Thread::Current()->sticky_error() != Error::null())) {
js->PrintError(kIsolateReloadBarred,
"This isolate cannot reload sources anymore because there "
diff --git a/runtime/vm/service_isolate.cc b/runtime/vm/service_isolate.cc
index 8d13ec6..e645e35 100644
--- a/runtime/vm/service_isolate.cc
+++ b/runtime/vm/service_isolate.cc
@@ -381,10 +381,6 @@
if (!error.IsNull() && !error.IsUnwindError()) {
OS::PrintErr("vm-service: Error: %s\n", error.ToErrorCString());
}
- error = I->sticky_error();
- if (!error.IsNull() && !error.IsUnwindError()) {
- OS::PrintErr("vm-service: Error: %s\n", error.ToErrorCString());
- }
Dart::RunShutdownCallback();
}
ASSERT(ServiceIsolate::IsServiceIsolate(I));
diff --git a/runtime/vm/service_test.cc b/runtime/vm/service_test.cc
index 03c96f2..c9dcb1d 100644
--- a/runtime/vm/service_test.cc
+++ b/runtime/vm/service_test.cc
@@ -140,49 +140,6 @@
Service::HandleRootMessage(message);
}
-ISOLATE_UNIT_TEST_CASE(Service_IsolateStickyError) {
- const char* kScript = "main() => throw 'HI THERE STICKY';\n";
-
- Isolate* isolate = thread->isolate();
- isolate->set_is_runnable(true);
- Dart_Handle result;
- {
- TransitionVMToNative transition(thread);
- Dart_Handle lib = TestCase::LoadTestScript(kScript, NULL);
- EXPECT_VALID(lib);
- result = Dart_Invoke(lib, NewString("main"), 0, NULL);
- EXPECT(Dart_IsUnhandledExceptionError(result));
- EXPECT(!Dart_HasStickyError());
- }
- EXPECT(Thread::Current()->sticky_error() == Error::null());
-
- {
- JSONStream js;
- isolate->PrintJSON(&js, false);
- // No error property and no PauseExit state.
- EXPECT_NOTSUBSTRING("\"error\":", js.ToCString());
- EXPECT_NOTSUBSTRING("HI THERE STICKY", js.ToCString());
- EXPECT_NOTSUBSTRING("PauseExit", js.ToCString());
- }
-
- {
- // Set the sticky error.
- TransitionVMToNative transition(thread);
- Dart_SetStickyError(result);
- Dart_SetPausedOnExit(true);
- EXPECT(Dart_HasStickyError());
- }
-
- {
- JSONStream js;
- isolate->PrintJSON(&js, false);
- // Error and PauseExit set.
- EXPECT_SUBSTRING("\"error\":", js.ToCString());
- EXPECT_SUBSTRING("HI THERE STICKY", js.ToCString());
- EXPECT_SUBSTRING("PauseExit", js.ToCString());
- }
-}
-
ISOLATE_UNIT_TEST_CASE(Service_IdZones) {
Zone* zone = thread->zone();
Isolate* isolate = thread->isolate();
diff --git a/tests/lib_2/isolate/kill_regexp_test.dart b/tests/lib_2/isolate/kill_regexp_test.dart
new file mode 100644
index 0000000..6ce2cde
--- /dev/null
+++ b/tests/lib_2/isolate/kill_regexp_test.dart
@@ -0,0 +1,30 @@
+// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// VMOptions=--intrinsify
+// VMOptions=--no_intrinsify
+
+import "dart:isolate";
+import "dart:async";
+import "package:expect/expect.dart";
+
+isomain1(replyPort) {
+ final regexp = new RegExp('[ab]c');
+ while (true) {
+ Expect.equals(4, regexp.allMatches("acbcacbc").length);
+ }
+}
+
+void main() {
+ for (int i = 0; i < 20; ++i) {
+ ReceivePort reply = new ReceivePort();
+ Isolate.spawn(isomain1, reply.sendPort).then((Isolate isolate) {
+ new Timer(new Duration(milliseconds: 50), () {
+ print('killing isolate $i');
+ isolate.kill(priority: Isolate.immediate);
+ });
+ });
+ reply.close();
+ }
+}
