[vm] Check Zones are only [un]chained on the correct side of a safepoint transition.

Fix order in Dart_CreateIsolate and some tests.

Change-Id: Iceb748be3f7aa6b68ce7abb7f65a3c54ad63c1d8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/100795
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>

runtime/vm/dart_api_impl.cc

@@ -1093,8 +1093,10 @@ static Dart_Isolate CreateIsolate(const char* script_uri,
     }
     return reinterpret_cast<Dart_Isolate>(NULL);
   }
+
+  Thread* T = Thread::Current();
+  bool success = false;
   {
-    Thread* T = Thread::Current();
     StackZone zone(T);
     HANDLESCOPE(T);
     // We enter an API scope here as InitializeIsolate could compile some
@@ -1112,25 +1114,27 @@ static Dart_Isolate CreateIsolate(const char* script_uri,
         Library::CheckFunctionFingerprints();
       }
 #endif  // defined(DART_NO_SNAPSHOT) && !defined(PRODUCT).
-      // We exit the API scope entered above.
-      T->ExitApiScope();
-      // A Thread structure has been associated to the thread, we do the
-      // safepoint transition explicitly here instead of using the
-      // TransitionXXX scope objects as the reverse transition happens
-      // outside this scope in Dart_ShutdownIsolate/Dart_ExitIsolate.
-      T->set_execution_state(Thread::kThreadInNative);
-      T->EnterSafepoint();
-      if (error != NULL) {
-        *error = NULL;
-      }
-      return Api::CastIsolate(I);
-    }
-    if (error != NULL) {
+      success = true;
+    } else if (error != NULL) {
       *error = strdup(error_obj.ToErrorCString());
     }
     // We exit the API scope entered above.
     T->ExitApiScope();
   }
+
+  if (success) {
+    // A Thread structure has been associated to the thread, we do the
+    // safepoint transition explicitly here instead of using the
+    // TransitionXXX scope objects as the reverse transition happens
+    // outside this scope in Dart_ShutdownIsolate/Dart_ExitIsolate.
+    T->set_execution_state(Thread::kThreadInNative);
+    T->EnterSafepoint();
+    if (error != NULL) {
+      *error = NULL;
+    }
+    return Api::CastIsolate(I);
+  }
+
   Dart::ShutdownIsolate();
   return reinterpret_cast<Dart_Isolate>(NULL);
 }

runtime/vm/dart_api_impl_test.cc

@@ -3469,8 +3469,8 @@ VM_UNIT_TEST_CASE(DartAPI_LocalHandles) {
   ApiLocalScope* scope = thread->api_top_scope();
   Dart_Handle handles[300];
   {
-    StackZone zone(thread);
     TransitionNativeToVM transition1(thread);
+    StackZone zone(thread);
     HANDLESCOPE(thread);
     Smi& val = Smi::Handle();
     TransitionVMToNative transition2(thread);

runtime/vm/snapshot_test.cc

@@ -2030,7 +2030,7 @@ VM_UNIT_TEST_CASE(PostCObject) {
   Dart_ExitScope();
 }
 
-TEST_CASE(OmittedObjectEncodingLength) {
+ISOLATE_UNIT_TEST_CASE(OmittedObjectEncodingLength) {
   StackZone zone(Thread::Current());
   MessageWriter writer(true);
   writer.WriteInlinedObjectHeader(kOmittedObjectId);

runtime/vm/thread_test.cc

@@ -376,6 +376,7 @@ TEST_CASE(ThreadRegistry) {
   TestCase::CreateTestIsolate();
   {
     // Create a stack resource this time, and exercise it.
+    TransitionNativeToVM transition(Thread::Current());
     StackZone stack_zone(Thread::Current());
     Zone* zone1 = Thread::Current()->zone();
     EXPECT(zone1 != zone0);

runtime/vm/zone.cc

@@ -337,11 +337,20 @@ StackZone::StackZone(ThreadState* thread) : StackResource(thread), zone_() {
                  reinterpret_cast<intptr_t>(this),
                  reinterpret_cast<intptr_t>(&zone_));
   }
+
+  // This thread must be preventing safepoints or the GC could be visiting the
+  // chain of handle blocks we're about the mutate.
+  ASSERT(Thread::Current()->MayAllocateHandles());
+
   zone_.Link(thread->zone());
   thread->set_zone(&zone_);
 }
 
 StackZone::~StackZone() {
+  // This thread must be preventing safepoints or the GC could be visiting the
+  // chain of handle blocks we're about the mutate.
+  ASSERT(Thread::Current()->MayAllocateHandles());
+
   ASSERT(thread()->zone() == &zone_);
   thread()->set_zone(zone_.previous_);
   if (FLAG_trace_zones) {

runtime/vm/zone_test.cc

@@ -18,6 +18,7 @@ VM_UNIT_TEST_CASE(AllocateZone) {
   Thread* thread = Thread::Current();
   EXPECT(thread->zone() == NULL);
   {
+    TransitionNativeToVM transition(thread);
     StackZone stack_zone(thread);
     EXPECT(thread->zone() != NULL);
     Zone* zone = stack_zone.GetZone();
@@ -78,6 +79,7 @@ VM_UNIT_TEST_CASE(AllocGeneric_Success) {
   Thread* thread = Thread::Current();
   EXPECT(thread->zone() == NULL);
   {
+    TransitionNativeToVM transition(thread);
     StackZone zone(thread);
     EXPECT(thread->zone() != NULL);
     uintptr_t allocated_size = 0;
@@ -131,6 +133,7 @@ VM_UNIT_TEST_CASE(ZoneAllocated) {
 
   // Create a few zone allocated objects.
   {
+    TransitionNativeToVM transition(thread);
     StackZone zone(thread);
     EXPECT_EQ(0UL, zone.SizeInBytes());
     SimpleZoneObject* first = new SimpleZoneObject();
@@ -156,6 +159,7 @@ VM_UNIT_TEST_CASE(ZoneAllocated) {
 }
 
 TEST_CASE(PrintToString) {
+  TransitionNativeToVM transition(Thread::Current());
   StackZone zone(Thread::Current());
   const char* result = zone.GetZone()->PrintToString("Hello %s!", "World");
   EXPECT_STREQ("Hello World!", result);
@@ -219,7 +223,7 @@ TEST_CASE(StressMallocDirectly) {
 #endif  // !defined(PRODUCT)
 }
 
-TEST_CASE(StressMallocThroughZones) {
+ISOLATE_UNIT_TEST_CASE(StressMallocThroughZones) {
 #if !defined(PRODUCT)
   int64_t start_rss = Service::CurrentRSS();
 #endif  // !defined(PRODUCT)