[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>

runtime/bin/main.cc

@@ -705,11 +705,6 @@ char* BuildIsolateName(const char* script_name, const char* func_name) {
 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();
 

runtime/include/dart_api.h

@@ -1175,29 +1175,6 @@ DART_EXPORT bool Dart_IsPausedOnExit();
  */
 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.
  *

runtime/vm/dart_api_impl.cc

@@ -1391,43 +1391,6 @@ DART_EXPORT void Dart_SetPausedOnExit(bool paused) {
 #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_EXPORT Dart_Handle Dart_RunLoop() {
     }
   }
   ::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());

runtime/vm/dart_api_impl_test.cc

@@ -3696,22 +3696,6 @@ VM_UNIT_TEST_CASE(DartAPI_SetMessageCallbacks) {
   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"

runtime/vm/isolate.cc

@@ -1699,11 +1699,8 @@ static void ShutdownIsolate(uword parameter) {
   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 @@ void Isolate::TrackDeoptimizedCode(const Code& code) {
   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 @@ Thread* Isolate::ScheduleThread(bool is_mutator, bool bypass_safepoint) {
     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();

runtime/vm/isolate.h

@@ -606,11 +606,8 @@ class Isolate : public BaseIsolate {
   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);
 

runtime/vm/object.cc

@@ -866,12 +866,13 @@ void Object::Init(Isolate* isolate) {
   // 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());

runtime/vm/service.cc

@@ -3142,7 +3142,8 @@ static bool ReloadSources(Thread* thread, JSONStream* js) {
                    "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 "

runtime/vm/service_isolate.cc

@@ -381,10 +381,6 @@ class RunServiceTask : public ThreadPool::Task {
       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));

runtime/vm/service_test.cc

@@ -140,49 +140,6 @@ static void HandleRootMessage(const Array& message) {
   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();

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();
+  }
+}