Bug 1551275 - Refactor jsapi weak map tests r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Mon, 13 May 2019 19:08:10 +0100
changeset 474717 b993d1489eee99b2227aaa1ff829d868437f1316
parent 474458 72731b10931017b247d79f754145726e52672b71
child 474718 5d9d8566c195a2602a47cf4f9973a6e672fe7a16
push id36045
push userrmaries@mozilla.com
push dateTue, 21 May 2019 16:30:25 +0000
treeherdermozilla-central@3c0f78074b72 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1551275
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1551275 - Refactor jsapi weak map tests r=sfink This removes a bunch of repeated code and hopefully makes it easier to see what we're testing. When marking two things the same color this now checks both orders (e.g. key before map, map before key). I removed individual test cases and generate all possiblities with for loops. The expected marking state is determined by functions factored out from the verifier. The tests for JS WeakMap and internal weakmaps are slightly different because I wanted to cover all existing test cases without making things too complicated. This means we don't test marking the key and delegate different colors for the former. Differential Revision: https://phabricator.services.mozilla.com/D30948
js/src/gc/Verifier.cpp
js/src/gc/Verifier.h
js/src/jsapi-tests/testGCGrayMarking.cpp
--- a/js/src/gc/Verifier.cpp
+++ b/js/src/gc/Verifier.cpp
@@ -1,14 +1,16 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
  * vim: set ts=8 sts=2 et sw=2 tw=80:
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+#include "gc/Verifier.h"
+
 #include "mozilla/DebugOnly.h"
 #include "mozilla/IntegerPrintfMacros.h"
 #include "mozilla/Sprintf.h"
 
 #ifdef MOZ_VALGRIND
 #  include <valgrind/memcheck.h>
 #endif
 
@@ -442,33 +444,16 @@ void js::gc::GCRuntime::finishVerifier()
     verifyPreData = nullptr;
   }
 }
 
 #endif /* JS_GC_ZEAL */
 
 #if defined(JS_GC_ZEAL) || defined(DEBUG)
 
-// Like gc::MarkColor but allows the possibility of the cell being
-// unmarked. Order is important here, with white being 'least marked'
-// and black being 'most marked'.
-enum class CellColor : uint8_t { White = 0, Gray = 1, Black = 2 };
-
-static CellColor GetCellColor(Cell* cell) {
-  if (cell->isMarkedBlack()) {
-    return CellColor::Black;
-  }
-
-  if (cell->isMarkedGray()) {
-    return CellColor::Gray;
-  }
-
-  return CellColor::White;
-}
-
 static const char* CellColorName(CellColor color) {
   switch (color) {
     case CellColor::White:
       return "white";
     case CellColor::Black:
       return "black";
     case CellColor::Gray:
       return "gray";
@@ -793,17 +778,17 @@ bool js::gc::CheckWeakMapEntryMarking(co
   // Values belonging to other runtimes or in uncollected zones are treated as
   // black.
   CellColor valueColor = CellColor::Black;
   if (value->runtimeFromAnyThread() == zone->runtimeFromAnyThread() &&
       valueZone->isGCMarking()) {
     valueColor = GetCellColor(value);
   }
 
-  if (valueColor < Min(mapColor, keyColor)) {
+  if (valueColor < ExpectedWeakMapValueColor(mapColor, keyColor)) {
     fprintf(stderr, "WeakMap value is less marked than map and key\n");
     fprintf(stderr, "(map %p is %s, key %p is %s, value %p is %s)\n", map,
             CellColorName(mapColor), key, CellColorName(keyColor), value,
             CellColorName(valueColor));
     ok = false;
   }
 
   // Debugger weak maps map have keys in zones that are not or are
@@ -822,17 +807,17 @@ bool js::gc::CheckWeakMapEntryMarking(co
   CellColor delegateColor;
   if (delegate->zone()->isGCMarking() || delegate->zone()->isGCSweeping()) {
     delegateColor = GetCellColor(delegate);
   } else {
     // IsMarked() assumes cells in uncollected zones are marked.
     delegateColor = CellColor::Black;
   }
 
-  if (keyColor < Min(mapColor, delegateColor)) {
+  if (keyColor < ExpectedWeakMapKeyColor(mapColor, delegateColor)) {
     fprintf(stderr, "WeakMap key is less marked than map and delegate\n");
     fprintf(stderr, "(map %p is %s, delegate %p is %s, key %p is %s)\n", map,
             CellColorName(mapColor), delegate, CellColorName(delegateColor),
             key, CellColorName(keyColor));
     ok = false;
   }
 
   return ok;
new file mode 100644
--- /dev/null
+++ b/js/src/gc/Verifier.h
@@ -0,0 +1,62 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
+ * vim: set ts=8 sts=2 et sw=2 tw=80:
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/*
+ * Internal header for definitions shared between the verifier and jsapi tests.
+ */
+
+#ifndef gc_Verifier_h
+#define gc_Verifier_h
+
+#include "gc/Cell.h"
+
+namespace js {
+namespace gc {
+
+// Like gc::MarkColor but allows the possibility of the cell being
+// unmarked. Order is important here, with white being 'least marked'
+// and black being 'most marked'.
+enum class CellColor : uint8_t { White = 0, Gray = 1, Black = 2 };
+
+static constexpr CellColor AllCellColors[] = {
+  CellColor::White, CellColor::Gray, CellColor::Black
+};
+
+static constexpr CellColor MarkedCellColors[] = {
+  CellColor::Gray, CellColor::Black
+};
+
+inline CellColor GetCellColor(Cell* cell) {
+  if (cell->isMarkedBlack()) {
+    return CellColor::Black;
+  }
+
+  if (cell->isMarkedGray()) {
+    return CellColor::Gray;
+  }
+
+  return CellColor::White;
+}
+
+inline CellColor ExpectedWeakMapValueColor(CellColor keyColor,
+                                           CellColor mapColor) {
+  return Min(keyColor, mapColor);
+}
+
+inline CellColor ExpectedWeakMapKeyColor(CellColor mapColor,
+                                         CellColor delegateColor) {
+  return Min(mapColor, delegateColor);
+}
+
+inline CellColor ExpectedKeyAndDelegateColor(CellColor keyColor,
+                                             CellColor delegateColor) {
+  return Max(keyColor, delegateColor);
+}
+
+} // namespace gc
+} // namespace js
+
+#endif /* gc_Verifier_h */
--- a/js/src/jsapi-tests/testGCGrayMarking.cpp
+++ b/js/src/jsapi-tests/testGCGrayMarking.cpp
@@ -1,16 +1,17 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
  * vim: set ts=8 sts=2 et sw=2 tw=80:
  */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "gc/Heap.h"
+#include "gc/Verifier.h"
 #include "gc/WeakMap.h"
 #include "gc/Zone.h"
 #include "js/Proxy.h"
 #include "jsapi-tests/tests.h"
 
 using namespace js;
 using namespace js::gc;
 
@@ -51,17 +52,17 @@ BEGIN_TEST(testGCGrayMarking) {
   AutoLeaveZeal nozeal(cx);
 #endif /* JS_GC_ZEAL */
 
   CHECK(InitGlobals());
   JSAutoRealm ar(cx, global1);
 
   InitGrayRootTracer();
 
-  bool ok = TestMarking() && TestWeakMaps() && TestUnassociatedWeakMaps() &&
+  bool ok = TestMarking() && TestJSWeakMaps() && TestInternalWeakMaps() &&
             TestCCWs() && TestGrayUnmarking();
 
   global1 = nullptr;
   global2 = nullptr;
   RemoveGrayRootTracer();
 
   return ok;
 }
@@ -144,288 +145,191 @@ bool TestMarking() {
   blackRoot1 = nullptr;
   blackRoot2 = nullptr;
   grayRoots.grayRoot1 = nullptr;
   grayRoots.grayRoot2 = nullptr;
 
   return true;
 }
 
-bool TestWeakMaps() {
-  JSObject* weakMap = JS::NewWeakMapObject(cx);
-  CHECK(weakMap);
+static const CellColor DontMark = CellColor::White;
+
+enum MarkKeyOrDelegate : bool { MarkKey = true, MarkDelegate = false };
 
-  JSObject* key;
-  JSObject* value;
-  {
-    JS::RootedObject rootedMap(cx, weakMap);
-
-    key = AllocWeakmapKeyObject();
-    CHECK(key);
-
-    value = AllocPlainObject();
-    CHECK(value);
-
-    weakMap = rootedMap;
+bool TestJSWeakMaps() {
+  for (auto keyOrDelegateColor : MarkedCellColors) {
+    for (auto mapColor : MarkedCellColors) {
+      for (auto markKeyOrDelegate : { MarkKey, MarkDelegate }) {
+        CellColor expected = ExpectedWeakMapValueColor(keyOrDelegateColor,
+                                                       mapColor);
+        CHECK(TestJSWeakMap(markKeyOrDelegate, keyOrDelegateColor, mapColor,
+                            expected));
+      }
+    }
   }
 
-  {
-    JS::RootedObject rootedMap(cx, weakMap);
-    JS::RootedObject rootedKey(cx, key);
-    JS::RootedValue rootedValue(cx, ObjectValue(*value));
-    CHECK(SetWeakMapEntry(cx, rootedMap, rootedKey, rootedValue));
+  return true;
+}
+
+bool TestInternalWeakMaps() {
+  for (auto keyMarkColor : AllCellColors) {
+    for (auto delegateMarkColor : AllCellColors) {
+      if (keyMarkColor == CellColor::White &&
+          delegateMarkColor == CellColor::White) {
+        continue;
+      }
+
+      CellColor keyOrDelegateColor =
+          ExpectedKeyAndDelegateColor(keyMarkColor, delegateMarkColor);
+      CellColor expected = ExpectedWeakMapValueColor(keyOrDelegateColor,
+                                                     CellColor::Black);
+      CHECK(TestInternalWeakMap(keyMarkColor, delegateMarkColor, expected));
+    }
   }
 
-  // Test the value of a weakmap entry is marked gray by GC if both the
-  // weakmap and key are marked gray.
-
-  grayRoots.grayRoot1 = weakMap;
-  grayRoots.grayRoot2 = key;
-  JS_GC(cx);
-  CHECK(IsMarkedGray(weakMap));
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(value));
-
-  // Test the value of a weakmap entry is marked gray by GC if one of the
-  // weakmap and the key is marked gray and the other black.
+  return true;
+}
 
-  JS::RootedObject blackRoot1(cx);
-  blackRoot1 = weakMap;
-  grayRoots.grayRoot1 = key;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(weakMap));
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(value));
-
-  blackRoot1 = key;
-  grayRoots.grayRoot1 = weakMap;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedGray(weakMap));
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedGray(value));
+bool TestJSWeakMap(MarkKeyOrDelegate markKey, CellColor weakMapMarkColor,
+                   CellColor keyOrDelegateMarkColor,
+                   CellColor expectedValueColor) {
+  // Test marking a JS WeakMap object.
+  //
+  // This marks the map and one of the key or delegate. The key/delegate and the
+  // value can end up different colors depending on the color of the map.
 
-  // Test the value of a weakmap entry is marked black by GC if both the
-  // weakmap and the key are marked black.
-
-  JS::RootedObject blackRoot2(cx);
-  grayRoots.grayRoot1 = nullptr;
-  grayRoots.grayRoot2 = nullptr;
-
-  blackRoot1 = weakMap;
-  blackRoot2 = key;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(weakMap));
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedBlack(value));
+  JSObject* weakMap;
+  JSObject* key;
+  JSObject* value;
 
-  blackRoot1 = key;
-  blackRoot2 = weakMap;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(weakMap));
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedBlack(value));
-
-  // Test that a weakmap key is marked gray if it has a gray delegate and the
-  // map is either gray or black.
+  // If both map and key are marked the same color, test both possible
+  // orderings.
+  unsigned markOrderings = weakMapMarkColor == keyOrDelegateMarkColor ? 2 : 1;
 
-  JSObject* delegate = UncheckedUnwrapWithoutExpose(key);
-  blackRoot1 = weakMap;
-  blackRoot2 = nullptr;
-  grayRoots.grayRoot1 = delegate;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedGray(delegate));
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedBlack(weakMap));
-  CHECK(IsMarkedGray(value));
+  for (unsigned markOrder = 0; markOrder < markOrderings; markOrder++) {
+    CHECK(CreateJSWeakMapObjects(&weakMap, &key, &value));
 
-  blackRoot1 = nullptr;
-  blackRoot2 = nullptr;
-  grayRoots.grayRoot1 = weakMap;
-  grayRoots.grayRoot2 = delegate;
-  JS_GC(cx);
-  CHECK(IsMarkedGray(delegate));
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(weakMap));
-  CHECK(IsMarkedGray(value));
-
-  // Test that a weakmap key is marked gray if it has a black delegate but
-  // the map is gray.
+    JSObject* delegate = UncheckedUnwrapWithoutExpose(key);
+    JSObject* keyOrDelegate = markKey ? key : delegate;
 
-  blackRoot1 = delegate;
-  blackRoot2 = nullptr;
-  grayRoots.grayRoot1 = weakMap;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(delegate));
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(weakMap));
-  CHECK(IsMarkedGray(value));
+    RootedObject blackRoot1(cx);
+    RootedObject blackRoot2(cx);
 
-  blackRoot1 = delegate;
-  blackRoot2 = nullptr;
-  grayRoots.grayRoot1 = weakMap;
-  grayRoots.grayRoot2 = key;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(delegate));
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(weakMap));
-  CHECK(IsMarkedGray(value));
-
-  // Test that a weakmap key is marked black if it has a black delegate and
-  // the map is black.
+    RootObject(weakMap, weakMapMarkColor, blackRoot1, grayRoots.grayRoot1);
+    RootObject(keyOrDelegate, keyOrDelegateMarkColor, blackRoot2,
+               grayRoots.grayRoot2);
 
-  blackRoot1 = delegate;
-  blackRoot2 = weakMap;
-  grayRoots.grayRoot1 = nullptr;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(delegate));
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedBlack(weakMap));
-  CHECK(IsMarkedBlack(value));
+    if (markOrder != 0) {
+      mozilla::Swap(blackRoot1.get(), blackRoot2.get());
+      mozilla::Swap(grayRoots.grayRoot1, grayRoots.grayRoot2);
+    }
 
-  blackRoot1 = delegate;
-  blackRoot2 = weakMap;
-  grayRoots.grayRoot1 = key;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(delegate));
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedBlack(weakMap));
-  CHECK(IsMarkedBlack(value));
-
-  // Test what happens if there is a delegate but it is not marked for both
-  // black and gray cases.
+    JS_GC(cx);
 
-  delegate = nullptr;
-  blackRoot1 = key;
-  blackRoot2 = weakMap;
-  grayRoots.grayRoot1 = nullptr;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedBlack(weakMap));
-  CHECK(IsMarkedBlack(value));
+    ClearGrayRoots();
 
-  blackRoot1 = nullptr;
-  blackRoot2 = nullptr;
-  grayRoots.grayRoot1 = weakMap;
-  grayRoots.grayRoot2 = key;
-  JS_GC(cx);
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(weakMap));
-  CHECK(IsMarkedGray(value));
-
-  blackRoot1 = nullptr;
-  blackRoot2 = nullptr;
-  grayRoots.grayRoot1 = nullptr;
-  grayRoots.grayRoot2 = nullptr;
+    CHECK(GetCellColor(weakMap) == weakMapMarkColor);
+    CHECK(GetCellColor(keyOrDelegate) == keyOrDelegateMarkColor);
+    CHECK(GetCellColor(value) == expectedValueColor);
+  }
 
   return true;
 }
 
-bool TestUnassociatedWeakMaps() {
-  // Make a weakmap that's not associated with a JSObject.
-  auto weakMap = cx->make_unique<GCManagedObjectWeakMap>(cx);
+bool CreateJSWeakMapObjects(JSObject** weakMapOut, JSObject** keyOut,
+                            JSObject** valueOut) {
+  RootedObject key(cx, AllocWeakmapKeyObject());
+  CHECK(key);
+
+  RootedObject value(cx, AllocPlainObject());
+  CHECK(value);
+
+  RootedObject weakMap(cx, JS::NewWeakMapObject(cx));
   CHECK(weakMap);
 
-  // Make sure this gets traced during GC.
-  Rooted<GCManagedObjectWeakMap*> rootMap(cx, weakMap.get());
+  JS::RootedValue valueValue(cx, ObjectValue(*value));
+  CHECK(SetWeakMapEntry(cx, weakMap, key, valueValue));
+
+  *weakMapOut = weakMap;
+  *keyOut = key;
+  *valueOut = value;
+  return true;
+}
+
+bool TestInternalWeakMap(CellColor keyMarkColor, CellColor delegateMarkColor,
+                         CellColor expectedColor) {
+  // Test marking for internal weakmaps (without an owning JSObject).
+  //
+  // All of the key, delegate and value are expected to end up the same color.
+
+  UniquePtr<GCManagedObjectWeakMap> weakMap;
+  JSObject* key;
+  JSObject* value;
+
+  // If both key and delegate are marked the same color, test both possible
+  // orderings.
+  unsigned markOrderings = keyMarkColor == delegateMarkColor ? 2 : 1;
+
+  for (unsigned markOrder = 0; markOrder < markOrderings; markOrder++) {
+    CHECK(CreateInternalWeakMapObjects(&weakMap, &key, &value));
+
+    JSObject* delegate = UncheckedUnwrapWithoutExpose(key);
 
-  JSObject* key = AllocWeakmapKeyObject();
+    RootedObject blackRoot1(cx);
+    RootedObject blackRoot2(cx);
+
+    Rooted<GCManagedObjectWeakMap*> rootMap(cx, weakMap.get());
+    RootObject(key, keyMarkColor, blackRoot1, grayRoots.grayRoot1);
+    RootObject(delegate, delegateMarkColor, blackRoot2, grayRoots.grayRoot2);
+
+    if (markOrder != 0) {
+      mozilla::Swap(blackRoot1.get(), blackRoot2.get());
+      mozilla::Swap(grayRoots.grayRoot1, grayRoots.grayRoot2);
+    }
+
+    JS_GC(cx);
+
+    ClearGrayRoots();
+
+    CHECK(GetCellColor(key) == expectedColor);
+    CHECK(GetCellColor(delegate) == expectedColor);
+    CHECK(GetCellColor(value) == expectedColor);
+  }
+
+  return true;
+}
+
+bool CreateInternalWeakMapObjects(UniquePtr<GCManagedObjectWeakMap>* weakMapOut,
+                                  JSObject** keyOut, JSObject** valueOut) {
+  RootedObject key(cx, AllocWeakmapKeyObject());
   CHECK(key);
 
-  JSObject* value = AllocPlainObject();
+  RootedObject value(cx, AllocPlainObject());
   CHECK(value);
 
+  auto weakMap = cx->make_unique<GCManagedObjectWeakMap>(cx);
+  CHECK(weakMap);
+
   CHECK(weakMap->add(cx, key, value));
 
-  // Test the value of a weakmap entry is marked gray by GC if the
-  // key is marked gray.
-
-  grayRoots.grayRoot1 = key;
-  JS_GC(cx);
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(value));
-
-  // Test the value of a weakmap entry is marked gray by GC if the key is marked
-  // gray.
-
-  grayRoots.grayRoot1 = key;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(value));
-
-  // Test the value of a weakmap entry is marked black by GC if the key is
-  // marked black.
-
-  JS::RootedObject blackRoot(cx);
-  blackRoot = key;
-  grayRoots.grayRoot1 = nullptr;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedBlack(value));
-
-  // Test that a weakmap key is marked gray if it has a gray delegate.
-
-  JSObject* delegate = UncheckedUnwrapWithoutExpose(key);
-  blackRoot = nullptr;
-  grayRoots.grayRoot1 = delegate;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedGray(delegate));
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(value));
+  *weakMapOut = std::move(weakMap);
+  *keyOut = key;
+  *valueOut = value;
+  return true;
+}
 
-  // Test that a weakmap key is marked black if it has a black delegate.
-
-  blackRoot = delegate;
-  grayRoots.grayRoot1 = nullptr;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(delegate));
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedBlack(value));
-
-  blackRoot = delegate;
-  grayRoots.grayRoot1 = key;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(delegate));
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedBlack(value));
-
-  // Test what happens if there is a delegate but it is not marked for both
-  // black and gray cases.
-
-  delegate = nullptr;
-  blackRoot = key;
-  grayRoots.grayRoot1 = nullptr;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedBlack(key));
-  CHECK(IsMarkedBlack(value));
-
-  blackRoot = nullptr;
-  grayRoots.grayRoot1 = key;
-  grayRoots.grayRoot2 = nullptr;
-  JS_GC(cx);
-  CHECK(IsMarkedGray(key));
-  CHECK(IsMarkedGray(value));
-
-  blackRoot = nullptr;
-  grayRoots.grayRoot1 = nullptr;
-  grayRoots.grayRoot2 = nullptr;
-
-  return true;
+void RootObject(JSObject* object, CellColor color, RootedObject& blackRoot,
+                JS::Heap<JSObject*>& grayRoot) {
+  if (color == CellColor::Black) {
+    blackRoot = object;
+  } else if (color == CellColor::Gray) {
+    grayRoot = object;
+  } else {
+    MOZ_RELEASE_ASSERT(color == CellColor::White);
+  }
 }
 
 bool TestCCWs() {
   JSObject* target = AllocPlainObject();
   CHECK(target);
 
   // Test getting a new wrapper doesn't return a gray wrapper.
 
@@ -589,25 +493,28 @@ bool InitGlobals() {
   global1.init(cx, global);
   if (!createGlobal()) {
     return false;
   }
   global2.init(cx, global);
   return global2 != nullptr;
 }
 
-void InitGrayRootTracer() {
+void ClearGrayRoots() {
   grayRoots.grayRoot1 = nullptr;
   grayRoots.grayRoot2 = nullptr;
+}
+
+void InitGrayRootTracer() {
+  ClearGrayRoots();
   JS_SetGrayGCRootsTracer(cx, TraceGrayRoots, &grayRoots);
 }
 
 void RemoveGrayRootTracer() {
-  grayRoots.grayRoot1 = nullptr;
-  grayRoots.grayRoot2 = nullptr;
+  ClearGrayRoots();
   JS_SetGrayGCRootsTracer(cx, nullptr, nullptr);
 }
 
 static void TraceGrayRoots(JSTracer* trc, void* data) {
   auto grayRoots = static_cast<GrayRoots*>(data);
   TraceEdge(trc, &grayRoots->grayRoot1, "gray root 1");
   TraceEdge(trc, &grayRoots->grayRoot2, "gray root 2");
 }