Bug 1335751 - Check all gray marking state before cycle collection in debug builds r=mccr8
authorJon Coppeard <jcoppeard@mozilla.com>
Sun, 05 Mar 2017 09:23:33 +0000
changeset 375057 3c4a6d36149c38695e0cdccd9e7bbe261d1e17c8
parent 375056 7d3562cfc9723146311fa66e1e748c1735a6979b
child 375058 376d3e4d4a750adf58e011f65b530ecf65726652
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1335751
milestone54.0a1
Bug 1335751 - Check all gray marking state before cycle collection in debug builds r=mccr8
xpcom/base/CycleCollectedJSContext.cpp
xpcom/base/CycleCollectedJSContext.h
xpcom/base/nsCycleCollector.cpp
--- a/xpcom/base/CycleCollectedJSContext.cpp
+++ b/xpcom/base/CycleCollectedJSContext.cpp
@@ -222,17 +222,54 @@ NoteWeakMapsTracer::trace(JSObject* aMap
     // if we haven't already.
     if (!mChildTracer.mTracedAny &&
         aKey && JS::GCThingIsMarkedGray(aKey) && kdelegate) {
       mCb.NoteWeakMapping(aMap, aKey, kdelegate, nullptr);
     }
   }
 }
 
-// This is based on the logic in FixWeakMappingGrayBitsTracer::trace.
+// Report whether the key or value of a weak mapping entry are gray but need to
+// be marked black.
+static void
+ShouldWeakMappingEntryBeBlack(JSObject* aMap, JS::GCCellPtr aKey, JS::GCCellPtr aValue,
+                              bool* aKeyShouldBeBlack, bool* aValueShouldBeBlack)
+{
+  *aKeyShouldBeBlack = false;
+  *aValueShouldBeBlack = false;
+
+  // If nothing that could be held alive by this entry is marked gray, return.
+  bool keyMightNeedMarking = aKey && JS::GCThingIsMarkedGray(aKey);
+  bool valueMightNeedMarking = aValue && JS::GCThingIsMarkedGray(aValue) &&
+    aValue.kind() != JS::TraceKind::String;
+  if (!keyMightNeedMarking && !valueMightNeedMarking) {
+    return;
+  }
+
+  if (!AddToCCKind(aKey.kind())) {
+    aKey = nullptr;
+  }
+
+  if (keyMightNeedMarking && aKey.is<JSObject>()) {
+    JSObject* kdelegate = js::GetWeakmapKeyDelegate(&aKey.as<JSObject>());
+    if (kdelegate && !JS::ObjectIsMarkedGray(kdelegate) &&
+        (!aMap || !JS::ObjectIsMarkedGray(aMap)))
+    {
+      *aKeyShouldBeBlack = true;
+    }
+  }
+
+  if (aValue && JS::GCThingIsMarkedGray(aValue) &&
+      (!aKey || !JS::GCThingIsMarkedGray(aKey)) &&
+      (!aMap || !JS::ObjectIsMarkedGray(aMap)) &&
+      aValue.kind() != JS::TraceKind::Shape) {
+    *aValueShouldBeBlack = true;
+  }
+}
+
 struct FixWeakMappingGrayBitsTracer : public js::WeakMapTracer
 {
   explicit FixWeakMappingGrayBitsTracer(JSContext* aCx)
     : js::WeakMapTracer(aCx)
   {
   }
 
   void
@@ -241,52 +278,73 @@ struct FixWeakMappingGrayBitsTracer : pu
     do {
       mAnyMarked = false;
       js::TraceWeakMaps(this);
     } while (mAnyMarked);
   }
 
   void trace(JSObject* aMap, JS::GCCellPtr aKey, JS::GCCellPtr aValue) override
   {
-    // If nothing that could be held alive by this entry is marked gray, return.
-    bool keyMightNeedMarking = aKey && JS::GCThingIsMarkedGray(aKey);
-    bool valueMightNeedMarking = aValue && JS::GCThingIsMarkedGray(aValue) &&
-                                 aValue.kind() != JS::TraceKind::String;
-    if (!keyMightNeedMarking && !valueMightNeedMarking) {
-      return;
-    }
-
-    if (!AddToCCKind(aKey.kind())) {
-      aKey = nullptr;
+    bool keyShouldBeBlack;
+    bool valueShouldBeBlack;
+    ShouldWeakMappingEntryBeBlack(aMap, aKey, aValue,
+                                  &keyShouldBeBlack, &valueShouldBeBlack);
+    if (keyShouldBeBlack && JS::UnmarkGrayGCThingRecursively(aKey)) {
+      mAnyMarked = true;
     }
 
-    if (keyMightNeedMarking && aKey.is<JSObject>()) {
-      JSObject* kdelegate = js::GetWeakmapKeyDelegate(&aKey.as<JSObject>());
-      if (kdelegate && !JS::ObjectIsMarkedGray(kdelegate) &&
-          (!aMap || !JS::ObjectIsMarkedGray(aMap)))
-      {
-        if (JS::UnmarkGrayGCThingRecursively(aKey)) {
-          mAnyMarked = true;
-        }
-      }
-    }
-
-    if (aValue && JS::GCThingIsMarkedGray(aValue) &&
-        (!aKey || !JS::GCThingIsMarkedGray(aKey)) &&
-        (!aMap || !JS::ObjectIsMarkedGray(aMap)) &&
-        aValue.kind() != JS::TraceKind::Shape) {
-      if (JS::UnmarkGrayGCThingRecursively(aValue)) {
-        mAnyMarked = true;
-      }
+    if (valueShouldBeBlack && JS::UnmarkGrayGCThingRecursively(aValue)) {
+      mAnyMarked = true;
     }
   }
 
   MOZ_INIT_OUTSIDE_CTOR bool mAnyMarked;
 };
 
+#ifdef DEBUG
+// Check whether weak maps are marked correctly according to the logic above.
+struct CheckWeakMappingGrayBitsTracer : public js::WeakMapTracer
+{
+  explicit CheckWeakMappingGrayBitsTracer(JSContext* aCx)
+    : js::WeakMapTracer(aCx), mFailed(false)
+  {
+  }
+
+  static bool
+  Check(JSContext* aCx)
+  {
+    CheckWeakMappingGrayBitsTracer tracer(aCx);
+    js::TraceWeakMaps(&tracer);
+    return !tracer.mFailed;
+  }
+
+  void trace(JSObject* aMap, JS::GCCellPtr aKey, JS::GCCellPtr aValue) override
+  {
+    bool keyShouldBeBlack;
+    bool valueShouldBeBlack;
+    ShouldWeakMappingEntryBeBlack(aMap, aKey, aValue,
+                                  &keyShouldBeBlack, &valueShouldBeBlack);
+
+    if (keyShouldBeBlack) {
+      fprintf(stderr, "Weak mapping key %p of map %p should be black\n",
+              aKey.asCell(), aMap);
+      mFailed = true;
+    }
+
+    if (valueShouldBeBlack) {
+      fprintf(stderr, "Weak mapping value %p of map %p should be black\n",
+              aValue.asCell(), aMap);
+      mFailed = true;
+    }
+  }
+
+  bool mFailed;
+};
+#endif // DEBUG
+
 static void
 CheckParticipatesInCycleCollection(JS::GCCellPtr aThing, const char* aName,
                                    void* aClosure)
 {
   bool* cycleCollectionEnabled = static_cast<bool*>(aClosure);
 
   if (*cycleCollectionEnabled) {
     return;
@@ -1250,16 +1308,26 @@ CycleCollectedJSContext::FixWeakMappingG
 {
   MOZ_ASSERT(mJSContext);
   MOZ_ASSERT(!JS::IsIncrementalGCInProgress(mJSContext),
              "Don't call FixWeakMappingGrayBits during a GC.");
   FixWeakMappingGrayBitsTracer fixer(mJSContext);
   fixer.FixAll();
 }
 
+void
+CycleCollectedJSContext::CheckGrayBits() const
+{
+  MOZ_ASSERT(mJSContext);
+  MOZ_ASSERT(!JS::IsIncrementalGCInProgress(mJSContext),
+             "Don't call CheckGrayBits during a GC.");
+  MOZ_ASSERT(js::CheckGrayMarkingState(mJSContext));
+  MOZ_ASSERT(CheckWeakMappingGrayBitsTracer::Check(mJSContext));
+}
+
 bool
 CycleCollectedJSContext::AreGCGrayBitsValid() const
 {
   MOZ_ASSERT(mJSContext);
   return js::AreGCGrayBitsValid(mJSContext);
 }
 
 void
--- a/xpcom/base/CycleCollectedJSContext.h
+++ b/xpcom/base/CycleCollectedJSContext.h
@@ -308,16 +308,17 @@ public:
   std::queue<nsCOMPtr<nsIRunnable>>& GetDebuggerPromiseMicroTaskQueue();
 
   nsCycleCollectionParticipant* GCThingParticipant();
   nsCycleCollectionParticipant* ZoneParticipant();
 
   nsresult TraverseRoots(nsCycleCollectionNoteRootCallback& aCb);
   virtual bool UsefulToMergeZones() const;
   void FixWeakMappingGrayBits() const;
+  void CheckGrayBits() const;
   bool AreGCGrayBitsValid() const;
   void GarbageCollect(uint32_t aReason) const;
 
   void NurseryWrapperAdded(nsWrapperCache* aCache);
   void NurseryWrapperPreserved(JSObject* aWrapper);
   void JSObjectsTenured();
 
   void DeferredFinalize(DeferredFinalizeAppendFunction aAppendFunc,
--- a/xpcom/base/nsCycleCollector.cpp
+++ b/xpcom/base/nsCycleCollector.cpp
@@ -3827,16 +3827,19 @@ nsCycleCollector::BeginCollection(ccType
   bool forceGC = isShutdown || (mLogger && mLogger->IsAllTraces());
 
   // BeginCycleCollectionCallback() might have started an IGC, and we need
   // to finish it before we run FixGrayBits.
   FinishAnyIncrementalGCInProgress();
   timeLog.Checkpoint("Pre-FixGrayBits finish IGC");
 
   FixGrayBits(forceGC, timeLog);
+  if (mJSContext) {
+    mJSContext->CheckGrayBits();
+  }
 
   FreeSnowWhite(true);
   timeLog.Checkpoint("BeginCollection FreeSnowWhite");
 
   if (mLogger && NS_FAILED(mLogger->Begin())) {
     mLogger = nullptr;
   }