Fix mark stack overflow bug in WeakMap marking.

Summary: I figured out a bug in WeakMap marking: if mark stack overflow occurs, we shouldn't assume that reachability has been computed correctly, so we shouldn't clear WeakMap entries with unreachable keys.

Reviewed By: kodafb

Differential Revision: D19199569

fbshipit-source-id: b0e2a3a4a51c81133e7d62b81106635922e5fcf5

include/hermes/VM/GCBase-inline.h

@@ -144,14 +144,16 @@ template <
     typename Acceptor,
     typename ObjIsMarkedFunc,
     typename MarkFromValFunc,
-    typename DrainMarkStackFunc>
+    typename DrainMarkStackFunc,
+    typename CheckMarkStackOverflowFunc>
 gcheapsize_t GCBase::completeWeakMapMarking(
     GC *gc,
     Acceptor &acceptor,
     std::vector<JSWeakMap *> &reachableWeakMaps,
     ObjIsMarkedFunc objIsMarked,
     MarkFromValFunc markFromVal,
-    DrainMarkStackFunc drainMarkStack) {
+    DrainMarkStackFunc drainMarkStack,
+    CheckMarkStackOverflowFunc checkMarkStackOverflow) {
   // If a WeakMap is present as a key in this map, the corresponding list
   // is a superset of the unreachable keys in the WeakMap.  (The set last
   // found to be unreachable, some of which may now be reachable.)
@@ -217,6 +219,14 @@ gcheapsize_t GCBase::completeWeakMapMarking(
     }
   } while (newReachableValueFound);
 
+  // If mark stack overflow occurred, terminate.
+  if (checkMarkStackOverflow()) {
+    // We return 0 in this case; the reachable WeakMaps will be
+    // discovered again, and we'll compute their total size in the iteration
+    // that eventually succeeds.
+    return 0;
+  }
+
   for (auto *weakMap : reachableWeakMaps) {
     clearEntriesWithUnreachableKeys(gc, weakMap, objIsMarked);
     // Previously we scanned the weak map while its value storage was
@@ -234,6 +244,9 @@ gcheapsize_t GCBase::completeWeakMapMarking(
     GCBase::markCell(weakMap, gc, acceptor);
     drainMarkStack(acceptor);
   }
+  // Because of the limited nature of the marking done above, we can
+  // assert that overflow did not occur.
+  assert(!checkMarkStackOverflow());
 
   return weakMapAllocBytes;
 }

include/hermes/VM/GCBase.h

@@ -708,6 +708,9 @@ class GCBase {
   ///    Ensures that the mark stack used by the collector is empty;
   ///    the transitive closure of the original contents is marked.
   ///
+  ///  * checkMarkStackOverflow: () ==> bool
+  ///    Returns whether mark stack overflow has occurred.
+  ///
   /// Some collectors compute the allocated bytes during GC.  If this
   /// is done in \p drainMarkStack, that will cover all objects except
   /// WeakWaps, which are never pushed on the mark stack.  Thus:
@@ -716,14 +719,16 @@ class GCBase {
       typename Acceptor,
       typename ObjIsMarkedFunc,
       typename MarkFromValFunc,
-      typename DrainMarkStackFunc>
+      typename DrainMarkStackFunc,
+      typename CheckMarkStackOverflowFunc>
   static gcheapsize_t completeWeakMapMarking(
       GC *gc,
       Acceptor &acceptor,
       std::vector<JSWeakMap *> &reachableWeakMaps,
       ObjIsMarkedFunc objIsMarked,
       MarkFromValFunc markFromVal,
-      DrainMarkStackFunc drainMarkStack);
+      DrainMarkStackFunc drainMarkStack,
+      CheckMarkStackOverflowFunc checkMarkStackOverflow);
 
   /// @}
 

lib/VM/gcs/GenGCNC.cpp

@@ -669,7 +669,9 @@ void GenGC::completeWeakMapMarking() {
       /*drainMarkStack*/
       [this](CompleteMarkState::FullMSCMarkTransitiveAcceptor &acceptor) {
         markState_.drainMarkStack(this, acceptor);
-      });
+      },
+      /*checkMarkStackOverflow*/
+      [this]() { return markState_.markStackOverflow_; });
 
   markState_.currentParPointer = nullptr;
   markState_.reachableWeakMaps_.clear();

lib/VM/gcs/MallocGC.cpp

@@ -405,7 +405,9 @@ void MallocGC::completeWeakMapMarking(MarkingAcceptor &acceptor) {
         return true;
       },
       /*drainMarkStack*/
-      [this](MarkingAcceptor &acceptor) { drainMarkStack(acceptor); });
+      [this](MarkingAcceptor &acceptor) { drainMarkStack(acceptor); },
+      /*checkMarkStackOverflow (MallocGC does not have mark stack overflow)*/
+      []() { return false; });
 
   acceptor.reachableWeakMaps_.clear();
   // drainMarkStack will have added the size of every object popped

public/hermes/Public/GCConfig.h

@@ -128,7 +128,7 @@ enum class GCEventKind {
   /* Whether to enable "proper" (spec-compliant) WeakMap marking.         \
      (There have been bugs, and perf issues, so we want to be able        \
      to revert to the previous, non-spec-compliant, behavior.) */         \
-  F(constexpr, bool, ProperWeakMapMarking, false)                         \
+  F(constexpr, bool, ProperWeakMapMarking, true)                          \
                                                                           \
   /* Pointer to the memory profiler (Memory Event Tracker). */            \
   F(HERMES_NON_CONSTEXPR,                                                 \

test/hermes/weak-map-key-in-value.js.hide → test/hermes/weakmap-key-in-value.js

@@ -5,11 +5,8 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-// RUN: %hermes -O -Xhermes-internal-test-methods %s | %FileCheck --match-full-lines %s
-// RUN: %hermes -O -emit-binary -out %t.hbc %s && %hermes -Xhermes-internal-test-methods %t.hbc | %FileCheck --match-full-lines %s
-// Currently, only gengc has the finer-grained marking that this test tests.
-// TODO(T57727796): leaving that task open for removing the "REQUIRES" below when
-// that is implemented for GenGC as well.
+// RUN: %hermes -gc-init-heap=4M -O -Xhermes-internal-test-methods %s | %FileCheck --match-full-lines %s
+// RUN: %hermes -O -emit-binary -out %t.hbc %s && %hermes -gc-init-heap=4M -Xhermes-internal-test-methods %t.hbc | %FileCheck --match-full-lines %s
 
 "use strict"
 

test/hermes/weakmap-stack-overflow-test.js

@@ -0,0 +1,46 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes -gc-init-heap=4M -O -Xhermes-internal-test-methods %s | %FileCheck --match-full-lines %s
+// RUN: %hermes -O -emit-binary -out %t.hbc %s && %hermes -gc-init-heap=4M -Xhermes-internal-test-methods %t.hbc | %FileCheck --match-full-lines %s
+
+// This test is specific to the implementation of GenGC.
+// REQUIRES: gengc
+
+"use strict"
+
+// CHECK-LABEL: Start
+print("Start");
+
+// This tests that WeakMap marking works even in the presence of mark stack
+// overflow.  (This test failed before a bug fix.)
+function foo7() {
+  function makeList(n, lastVal) {
+    if (n == 0) {
+      return {p: lastVal};
+    } else {
+      return {p: {}, tail: makeList(n-1, lastVal)};
+    }
+  }
+  var key0 = {};
+  var key1 = {};
+  var map = new WeakMap();
+  // 1000 is the mark stack limit; exceed that.
+  map.set(key0, makeList(1200, key1));
+  map.set(key1, {y: 17});
+  return [map, key0];
+}
+var pair7 = foo7();
+gc();
+
+// Make sure a mark stack overflow occurred.
+var stats = HermesInternal.getInstrumentedStats();
+// CHECK-NEXT: 1
+print(stats.js_markStackOverflows);
+
+// CHECK-NEXT: 2
+print(HermesInternal.getWeakSize(pair7[0]));