Fix another miranda method moving GC bug

Need to copy miranda methods over before we allocate the new vtable
or else we may have stale miranda gc roots.

Bug: 21664466
Change-Id: Ib3e415bb9e7df7abfa18c98fe01f790fa39622dc

2 files changed, 28 insertions, 22 deletions

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index d2805cdbd6..5240447c75 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -1310,7 +1310,7 @@ void ClassLinker::VisitClassRoots(RootVisitor* visitor, VisitRootFlags flags) {
 // reinit references to when reinitializing a ClassLinker from a
 // mapped image.
 void ClassLinker::VisitRoots(RootVisitor* visitor, VisitRootFlags flags) {
-  class_roots_.VisitRoot(visitor, RootInfo(kRootVMInternal));
+  class_roots_.VisitRootIfNonNull(visitor, RootInfo(kRootVMInternal));
   Thread* const self = Thread::Current();
   {
     ReaderMutexLock mu(self, dex_lock_);
@@ -1333,9 +1333,9 @@ void ClassLinker::VisitRoots(RootVisitor* visitor, VisitRootFlags flags) {
     }
   }
   VisitClassRoots(visitor, flags);
-  array_iftable_.VisitRoot(visitor, RootInfo(kRootVMInternal));
-  for (size_t i = 0; i < kFindArrayCacheSize; ++i) {
-    find_array_class_cache_[i].VisitRootIfNonNull(visitor, RootInfo(kRootVMInternal));
+  array_iftable_.VisitRootIfNonNull(visitor, RootInfo(kRootVMInternal));
+  for (GcRoot<mirror::Class>& root : find_array_class_cache_) {
+    root.VisitRootIfNonNull(visitor, RootInfo(kRootVMInternal));
   }
 }
 
@@ -4928,8 +4928,7 @@ bool ClassLinker::LinkInterfaceMethods(Thread* self, Handle<mirror::Class> klass
             }
           }
           if (miranda_method == nullptr) {
-            size_t size = ArtMethod::ObjectSize(image_pointer_size_);
-            miranda_method = reinterpret_cast<ArtMethod*>(allocator.Alloc(size));
+            miranda_method = reinterpret_cast<ArtMethod*>(allocator.Alloc(method_size));
             CHECK(miranda_method != nullptr);
             // Point the interface table at a phantom slot.
             new(miranda_method) ArtMethod(*interface_method, image_pointer_size_);
@@ -4968,34 +4967,42 @@ bool ClassLinker::LinkInterfaceMethods(Thread* self, Handle<mirror::Class> klass
         ++out;
       }
     }
+    StrideIterator<ArtMethod> out(
+        reinterpret_cast<uintptr_t>(virtuals) + old_method_count * method_size, method_size);
+    // Copy over miranda methods before copying vtable since CopyOf may cause thread suspension and
+    // we want the roots of the miranda methods to get visited.
+    for (ArtMethod* mir_method : miranda_methods) {
+      out->CopyFrom(mir_method, image_pointer_size_);
+      out->SetAccessFlags(out->GetAccessFlags() | kAccMiranda);
+      move_table.emplace(mir_method, &*out);
+      ++out;
+    }
     UpdateClassVirtualMethods(klass.Get(), virtuals, new_method_count);
-    // Done copying methods, they are all reachable from the class now, so we can end the no thread
+    // Done copying methods, they are all roots in the class now, so we can end the no thread
     // suspension assert.
     self->EndAssertNoThreadSuspension(old_cause);
 
-    size_t old_vtable_count = vtable->GetLength();
+    const size_t old_vtable_count = vtable->GetLength();
     const size_t new_vtable_count = old_vtable_count + miranda_methods.size();
+    miranda_methods.clear();
     vtable.Assign(down_cast<mirror::PointerArray*>(vtable->CopyOf(self, new_vtable_count)));
     if (UNLIKELY(vtable.Get() == nullptr)) {
       self->AssertPendingOOMException();
       return false;
     }
-    StrideIterator<ArtMethod> out(
+    out = StrideIterator<ArtMethod>(
         reinterpret_cast<uintptr_t>(virtuals) + old_method_count * method_size, method_size);
-    for (auto* mir_method : miranda_methods) {
-      ArtMethod* out_method = &*out;
-      out->CopyFrom(mir_method, image_pointer_size_);
+    size_t vtable_pos = old_vtable_count;
+    for (size_t i = old_method_count; i < new_method_count; ++i) {
       // Leave the declaring class alone as type indices are relative to it
-      out_method->SetAccessFlags(out_method->GetAccessFlags() | kAccMiranda);
-      out_method->SetMethodIndex(0xFFFF & old_vtable_count);
-      vtable->SetElementPtrSize(old_vtable_count, out_method, image_pointer_size_);
-      move_table.emplace(mir_method, out_method);
+      out->SetMethodIndex(0xFFFF & vtable_pos);
+      vtable->SetElementPtrSize(vtable_pos, &*out, image_pointer_size_);
       ++out;
-      ++old_vtable_count;
+      ++vtable_pos;
     }
-
+    CHECK_EQ(vtable_pos, new_vtable_count);
     // Update old vtable methods.
-    for (size_t i = 0; i < old_vtable_count - miranda_methods.size(); ++i) {
+    for (size_t i = 0; i < old_vtable_count; ++i) {
       auto* m = vtable->GetElementPtrSize<ArtMethod*>(i, image_pointer_size_);
       DCHECK(m != nullptr) << PrettyClass(klass.Get());
       auto it = move_table.find(m);
@@ -5006,7 +5013,6 @@ bool ClassLinker::LinkInterfaceMethods(Thread* self, Handle<mirror::Class> klass
       }
     }
     klass->SetVTable(vtable.Get());
-    CHECK_EQ(old_vtable_count, new_vtable_count);
     // Go fix up all the stale miranda pointers.
     for (size_t i = 0; i < ifcount; ++i) {
       for (size_t j = 0, count = iftable->GetMethodArrayCount(i); j < count; ++j) {
diff --git a/runtime/stride_iterator.h b/runtime/stride_iterator.h
index 5971524d81..bd622f3709 100644
--- a/runtime/stride_iterator.h
+++ b/runtime/stride_iterator.h
@@ -22,7 +22,7 @@
 namespace art {
 
 template<typename T>
-class StrideIterator : public std::iterator<std::random_access_iterator_tag, T> {
+class StrideIterator : public std::iterator<std::forward_iterator_tag, T> {
  public:
   StrideIterator(const StrideIterator&) = default;
   StrideIterator(StrideIterator&&) = default;
@@ -62,7 +62,7 @@ class StrideIterator : public std::iterator<std::random_access_iterator_tag, T>
 
  private:
   uintptr_t ptr_;
-  const size_t stride_;
+  size_t stride_;
 };
 
 }  // namespace art