[vm, gc] Require a safepoint for heap iteration.

runtime/vm/thread_test.cc

Index: runtime/vm/thread_test.cc
diff --git a/runtime/vm/thread_test.cc b/runtime/vm/thread_test.cc
index befe7a8ac62fcf18ea010a0dc85514f3e7e4d624..54ecfe1a81cca8595e4d13e26eb43e535a964eb6 100644
--- a/runtime/vm/thread_test.cc
+++ b/runtime/vm/thread_test.cc
@@ -130,10 +130,11 @@ class TaskWithZoneAllocation : public ThreadPool::Task {
       Smi& smi = Smi::Handle(zone, Smi::New(unique_smi));
       EXPECT(smi.Value() == unique_smi);
       {
+        HeapIterationScope iteration;
         ObjectCounter counter(isolate_, &smi);
         // Ensure that our particular zone is visited.
-        isolate_->IterateObjectPointers(&counter,
-                                        StackFrameIterator::kValidateFrames);
+        iteration.IterateStackPointers(&counter,

[danunez, 2017/08/05 17:24:11]

Why did we change to iterating over just the stack pointers?

The original function also visited class tables and object stores. Are those
visits not necessary for the test?

[rmacnak, 2017/08/07 22:37:51]

The test only needs to visit zone handles.

+                                       StackFrameIterator::kValidateFrames);
         EXPECT_EQ(1, counter.count());
       }
       char* unique_chars = zone->PrintToString("unique_str_%" Pd, id_);
@@ -146,10 +147,11 @@ class TaskWithZoneAllocation : public ThreadPool::Task {
       }
       EXPECT(unique_str.Equals(unique_chars));
       {
+        HeapIterationScope iteration;
         ObjectCounter str_counter(isolate_, &unique_str);
         // Ensure that our particular zone is visited.
-        isolate_->IterateObjectPointers(&str_counter,
-                                        StackFrameIterator::kValidateFrames);
+        iteration.IterateStackPointers(&str_counter,

[danunez, 2017/08/05 17:24:11]

Ditto.

+                                       StackFrameIterator::kValidateFrames);
         // We should visit the string object exactly once.
         EXPECT_EQ(1, str_counter.count());
       }
@@ -173,7 +175,7 @@ ISOLATE_UNIT_TEST_CASE(ManyTasksWithZones) {
   const int kTaskCount = 100;
   Monitor sync[kTaskCount];
   bool done[kTaskCount];
-  Isolate* isolate = Thread::Current()->isolate();
+  Isolate* isolate = thread->isolate();
   EXPECT(isolate->heap()->GrowthControlState());
   isolate->heap()->DisableGrowthControl();
   for (int i = 0; i < kTaskCount; i++) {
@@ -181,20 +183,27 @@ ISOLATE_UNIT_TEST_CASE(ManyTasksWithZones) {
     Dart::thread_pool()->Run(
         new TaskWithZoneAllocation(isolate, &sync[i], &done[i], i));
   }
+  bool in_isolate = true;
   for (int i = 0; i < kTaskCount; i++) {
     // Check that main mutator thread can still freely use its own zone.
     String& bar = String::Handle(String::New("bar"));
     if (i % 10 == 0) {
       // Mutator thread is free to independently move in/out/between isolates.
       Thread::ExitIsolate();
+      in_isolate = false;
     }
     MonitorLocker ml(&sync[i]);
     while (!done[i]) {
-      ml.Wait();
+      if (in_isolate) {
+        ml.WaitWithSafepointCheck(thread);
+      } else {
+        ml.Wait();
+      }
     }
     EXPECT(done[i]);
     if (i % 10 == 0) {
       Thread::EnterIsolate(isolate);
+      in_isolate = true;
     }
     EXPECT(bar.Equals("bar"));
   }
@@ -421,10 +430,11 @@ class SafepointTestTask : public ThreadPool::Task {
         TransitionVMToBlocked transition(thread);
       } else {
         // But occasionally, organize a rendezvous.
-        SafepointOperationScope safepoint_scope(thread);
+        HeapIterationScope iteration;  // Establishes a safepoint.
+        ASSERT(thread->IsAtSafepoint());
         ObjectCounter counter(isolate_, &smi);
-        isolate_->IterateObjectPointers(&counter,
-                                        StackFrameIterator::kValidateFrames);
+        iteration.IterateStackPointers(&counter,

[danunez, 2017/08/05 17:24:11]

Ditto.

+                                       StackFrameIterator::kValidateFrames);
         {
           MonitorLocker ml(monitor_);
           EXPECT_EQ(*expected_count_, counter.count());