commit a8d034f3e176732cf31df2c7b4ab1e3b673930b3
author Alexander Markov <alexmarkov@google.com> Tue Apr 30 17:07:20 2019 +0000
committer commit-bot@chromium.org <commit-bot@chromium.org> Tue Apr 30 17:07:20 2019 +0000
tree 967ac7e42298975aa2e6dbf68774a3e03aa565ff
parent 25daff618d393961359166227b496ae126897ac3

[vm/reload] Make sure classes in saved class table are visited by GC

Previously, IsolateReloadContext::CheckpointClasses() was filling
local_saved_class_table with raw pointers to classes and at the same
time it was adding classes to the set which is based on Array.
After that local_saved_class_table was saved into saved_class_table_
were it becomes visible to GC. However, adding an element to the set
may trigger GC in the middle and pointers for already processed
classes in local_saved_class_table are not visited and are not updated
if GC moves a Class object.

Also, IsolateReloadContext::VisitObjectPointers was not visiting
classes in saved_class_table_ properly as it was interpreting it as
an array of saved_num_cids_ pointers (while it is an array of
ClassAndSize elements).

This change fixes flaky crashes in vm/dart/trigger_gc_in_native_test.

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

runtime/vm/class_table.h

diff --git a/runtime/vm/class_table.h b/runtime/vm/class_table.h
index fd2bdc1..9404617 100644
--- a/runtime/vm/class_table.h
+++ b/runtime/vm/class_table.h
@@ -36,6 +36,7 @@
   intptr_t size_;
 
   friend class ClassTable;
+  friend class IsolateReloadContext;  // For VisitObjectPointers.
 };
 
 #if defined(ARCH_IS_32_BIT)

runtime/vm/isolate_reload.cc

diff --git a/runtime/vm/isolate_reload.cc b/runtime/vm/isolate_reload.cc
index 76e8e9d..6413b63 100644
--- a/runtime/vm/isolate_reload.cc
+++ b/runtime/vm/isolate_reload.cc
@@ -987,25 +987,40 @@
   ClassAndSize* local_saved_class_table = reinterpret_cast<ClassAndSize*>(
       malloc(sizeof(ClassAndSize) * saved_num_cids_));
 
+  // Copy classes into saved_class_table_ first. Make sure there are no
+  // safepoints until saved_class_table_ is filled up and saved so class raw
+  // pointers in saved_class_table_ are properly visited by GC.
+  {
+    NoSafepointScope no_safepoint_scope(Thread::Current());
+
+    for (intptr_t i = 0; i < saved_num_cids_; i++) {
+      if (class_table->IsValidIndex(i) && class_table->HasValidClassAt(i)) {
+        // Copy the class into the saved class table.
+        local_saved_class_table[i] = class_table->PairAt(i);
+      } else {
+        // No class at this index, mark it as NULL.
+        local_saved_class_table[i] = ClassAndSize(NULL);
+      }
+    }
+
+    // Elements of saved_class_table_ are now visible to GC.
+    saved_class_table_ = local_saved_class_table;
+  }
+
+  // Add classes to the set. Set is stored in the Array, so adding an element
+  // may allocate Dart object on the heap and trigger GC.
   Class& cls = Class::Handle();
   UnorderedHashSet<ClassMapTraits> old_classes_set(old_classes_set_storage_);
   for (intptr_t i = 0; i < saved_num_cids_; i++) {
     if (class_table->IsValidIndex(i) && class_table->HasValidClassAt(i)) {
-      // Copy the class into the saved class table and add it to the set.
-      local_saved_class_table[i] = class_table->PairAt(i);
       if (i != kFreeListElement && i != kForwardingCorpse) {
         cls = class_table->At(i);
         bool already_present = old_classes_set.Insert(cls);
         ASSERT(!already_present);
       }
-    } else {
-      // No class at this index, mark it as NULL.
-      local_saved_class_table[i] = ClassAndSize(NULL);
     }
   }
   old_classes_set_storage_ = old_classes_set.Release().raw();
-  // Assigning the field must be done after saving the class table.
-  saved_class_table_ = local_saved_class_table;
   TIR_Print("---- System had %" Pd " classes\n", saved_num_cids_);
 }
 
@@ -1840,8 +1855,10 @@
 void IsolateReloadContext::VisitObjectPointers(ObjectPointerVisitor* visitor) {
   visitor->VisitPointers(from(), to());
   if (saved_class_table_ != NULL) {
-    visitor->VisitPointers(
-        reinterpret_cast<RawObject**>(&saved_class_table_[0]), saved_num_cids_);
+    for (intptr_t i = 0; i < saved_num_cids_; i++) {
+      visitor->VisitPointer(
+          reinterpret_cast<RawObject**>(&(saved_class_table_[i].class_)));
+    }
   }
 }