After moving unreachable weak global handles only process harmony collections

Groups and implicit references are no longer relevant at this point.

Also add tests that fail if the first or second round of ephemeral
marking is omitted

BUG=none
R=mstarzinger@chromium.org,hpayer@chromium.org
LOG=n

Review URL: https://codereview.chromium.org/761343004

Cr-Commit-Position: refs/heads/master@{#25606}

src/heap/mark-compact.cc

@@ -2088,13 +2088,16 @@ void MarkCompactCollector::ProcessMarkingDeque() {
 
 // Mark all objects reachable (transitively) from objects on the marking
 // stack including references only considered in the atomic marking pause.
-void MarkCompactCollector::ProcessEphemeralMarking(ObjectVisitor* visitor) {
+void MarkCompactCollector::ProcessEphemeralMarking(
+    ObjectVisitor* visitor, bool only_process_harmony_weak_collections) {
   bool work_to_do = true;
   DCHECK(marking_deque_.IsEmpty());
   while (work_to_do) {
-    isolate()->global_handles()->IterateObjectGroups(
-        visitor, &IsUnmarkedHeapObjectWithHeap);
-    MarkImplicitRefGroups();
+    if (!only_process_harmony_weak_collections) {
+      isolate()->global_handles()->IterateObjectGroups(
+          visitor, &IsUnmarkedHeapObjectWithHeap);
+      MarkImplicitRefGroups();
+    }
     ProcessWeakCollections();
     work_to_do = !marking_deque_.IsEmpty();
     ProcessMarkingDeque();
@@ -2222,26 +2225,26 @@ void MarkCompactCollector::MarkLiveObjects() {
   // The objects reachable from the roots are marked, yet unreachable
   // objects are unmarked.  Mark objects reachable due to host
   // application specific logic or through Harmony weak maps.
-  ProcessEphemeralMarking(&root_visitor);
+  ProcessEphemeralMarking(&root_visitor, false);
 
   // The objects reachable from the roots, weak maps or object groups
-  // are marked, yet unreachable objects are unmarked.  Mark objects
-  // reachable only from weak global handles.
+  // are marked. Objects pointed to only by weak global handles cannot be
+  // immediately reclaimed. Instead, we have to mark them as pending and mark
+  // objects reachable from them.
   //
   // First we identify nonlive weak handles and mark them as pending
   // destruction.
   heap()->isolate()->global_handles()->IdentifyWeakHandles(
       &IsUnmarkedHeapObject);
-  // Then we mark the objects and process the transitive closure.
+  // Then we mark the objects.
   heap()->isolate()->global_handles()->IterateWeakRoots(&root_visitor);
-  while (marking_deque_.overflowed()) {
-    RefillMarkingDeque();
-    EmptyMarkingDeque();
-  }
 
-  // Repeat host application specific and Harmony weak maps marking to
-  // mark unmarked objects reachable from the weak roots.
-  ProcessEphemeralMarking(&root_visitor);
+  // Repeat Harmony weak maps marking to mark unmarked objects reachable from
+  // the weak roots we just marked as pending destruction.
+  //
+  // We only process harmony collections, as all object groups have been fully
+  // processed and no weakly reachable node can discover new objects groups.
+  ProcessEphemeralMarking(&root_visitor, true);
 
   AfterMarking();
 

src/heap/mark-compact.h

@@ -772,7 +772,8 @@ class MarkCompactCollector {
   //    - Processing of objects reachable through Harmony WeakMaps.
   //    - Objects reachable due to host application logic like object groups
   //      or implicit references' groups.
-  void ProcessEphemeralMarking(ObjectVisitor* visitor);
+  void ProcessEphemeralMarking(ObjectVisitor* visitor,
+                               bool only_process_harmony_weak_collections);
 
   // If the call-site of the top optimized code was not prepared for
   // deoptimization, then treat the maps in the code as strong pointers,

test/cctest/test-api.cc

@@ -4430,6 +4430,45 @@ THREADED_TEST(ApiObjectGroupsCycle) {
 }
 
 
+THREADED_TEST(WeakRootsSurviveTwoRoundsOfGC) {
+  LocalContext env;
+  v8::Isolate* iso = env->GetIsolate();
+  HandleScope scope(iso);
+
+  WeakCallCounter counter(1234);
+
+  WeakCallCounterAndPersistent<Value> weak_obj(&counter);
+
+  // Create a weak object that references a internalized string.
+  {
+    HandleScope scope(iso);
+    weak_obj.handle.Reset(iso, Object::New(iso));
+    weak_obj.handle.SetWeak(&weak_obj, &WeakPointerCallback);
+    CHECK(weak_obj.handle.IsWeak());
+    Local<Object>::New(iso, weak_obj.handle.As<Object>())->Set(
+        v8_str("x"),
+        String::NewFromUtf8(iso, "magic cookie", String::kInternalizedString));
+  }
+  // Do a single full GC
+  i::Isolate* i_iso = reinterpret_cast<v8::internal::Isolate*>(iso);
+  i::Heap* heap = i_iso->heap();
+  heap->CollectAllGarbage(i::Heap::kAbortIncrementalMarkingMask);
+
+  // We should have received the weak callback.
+  CHECK_EQ(1, counter.NumberOfWeakCalls());
+
+  // Check that the string is still alive.
+  {
+    HandleScope scope(iso);
+    i::MaybeHandle<i::String> magic_string =
+        i::StringTable::LookupStringIfExists(
+            i_iso,
+            v8::Utils::OpenHandle(*String::NewFromUtf8(iso, "magic cookie")));
+    magic_string.Check();
+  }
+}
+
+
 // TODO(mstarzinger): This should be a THREADED_TEST but causes failures
 // on the buildbots, so was made non-threaded for the time being.
 TEST(ApiObjectGroupsCycleForScavenger) {

test/cctest/test-weakmaps.cc

@@ -97,19 +97,20 @@ TEST(Weakness) {
   }
   CHECK(!global_handles->IsWeak(key.location()));
 
-  // Put entry into weak map.
+  // Put two chained entries into weak map.
   {
     HandleScope scope(isolate);
-    PutIntoWeakMap(weakmap,
-                   Handle<JSObject>(JSObject::cast(*key)),
-                   Handle<Smi>(Smi::FromInt(23), isolate));
+    Handle<Map> map = factory->NewMap(JS_OBJECT_TYPE, JSObject::kHeaderSize);
+    Handle<JSObject> object = factory->NewJSObjectFromMap(map);
+    PutIntoWeakMap(weakmap, Handle<JSObject>(JSObject::cast(*key)), object);
+    PutIntoWeakMap(weakmap, object, Handle<Smi>(Smi::FromInt(23), isolate));
   }
-  CHECK_EQ(1, ObjectHashTable::cast(weakmap->table())->NumberOfElements());
+  CHECK_EQ(2, ObjectHashTable::cast(weakmap->table())->NumberOfElements());
 
   // Force a full GC.
   heap->CollectAllGarbage(false);
   CHECK_EQ(0, NumberOfWeakCalls);
-  CHECK_EQ(1, ObjectHashTable::cast(weakmap->table())->NumberOfElements());
+  CHECK_EQ(2, ObjectHashTable::cast(weakmap->table())->NumberOfElements());
   CHECK_EQ(
       0, ObjectHashTable::cast(weakmap->table())->NumberOfDeletedElements());
 
@@ -128,14 +129,14 @@ TEST(Weakness) {
   // weak references whereas the second one will also clear weak maps.
   heap->CollectAllGarbage(false);
   CHECK_EQ(1, NumberOfWeakCalls);
-  CHECK_EQ(1, ObjectHashTable::cast(weakmap->table())->NumberOfElements());
+  CHECK_EQ(2, ObjectHashTable::cast(weakmap->table())->NumberOfElements());
   CHECK_EQ(
       0, ObjectHashTable::cast(weakmap->table())->NumberOfDeletedElements());
   heap->CollectAllGarbage(false);
   CHECK_EQ(1, NumberOfWeakCalls);
   CHECK_EQ(0, ObjectHashTable::cast(weakmap->table())->NumberOfElements());
-  CHECK_EQ(
-      1, ObjectHashTable::cast(weakmap->table())->NumberOfDeletedElements());
+  CHECK_EQ(2,
+           ObjectHashTable::cast(weakmap->table())->NumberOfDeletedElements());
 }