Bug 937766, part 2 - Make SnowWhiteKiller remove dying things from the CC graph. r=smaug
authorAndrew McCreight <continuation@gmail.com>
Fri, 06 Dec 2013 10:17:20 -0800
changeset 173977 b015eac4566f51574e0beac410bf9db937adbc09
parent 173976 4dcb040e3c84e69e9c3be4cee4d5e4b9ee995285
child 173978 9a4f57330003dac8bb6d9e986a8b777e0dbabf29
push id3224
push userlsblakk@mozilla.com
push dateTue, 04 Feb 2014 01:06:49 +0000
treeherdermozilla-beta@60c04d0987f1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs937766
milestone28.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 937766, part 2 - Make SnowWhiteKiller remove dying things from the CC graph. r=smaug If we purge snow white objects while ICC is in progress, we need to make sure to remove anything from the CC graph to avoid dangling pointers. We don't need to do that after shutdown.
xpcom/base/nsCycleCollector.cpp
--- a/xpcom/base/nsCycleCollector.cpp
+++ b/xpcom/base/nsCycleCollector.cpp
@@ -37,18 +37,49 @@
 //
 // XPCOM objects participating in garbage-cycle collection are obliged
 // to inform us when they ought to turn purple; that is, when their
 // refcount transitions from N+1 -> N, for nonzero N. Furthermore we
 // require that *after* an XPCOM object has informed us of turning
 // purple, they will tell us when they either transition back to being
 // black (incremented refcount) or are ultimately deleted.
 
-
-// Safety:
+// Incremental cycle collection
+//
+// Beyond the simple state machine required to implement incremental
+// collection, the CC needs to be able to compensate for things the browser
+// is doing during the collection. There are two kinds of problems. For each
+// of these, there are two cases to deal with: purple-buffered C++ objects
+// and JS objects.
+
+// The first problem is that an object in the CC's graph can become garbage.
+// This is bad because the CC touches the objects in its graph at every
+// stage of its operation.
+//
+// All cycle collected C++ objects that die during a cycle collection
+// will end up actually getting deleted by the SnowWhiteKiller. Before
+// the SWK deletes an object, it checks if an ICC is running, and if so,
+// if the object is in the graph. If it is, the CC clears mPointer and
+// mParticipant so it does not point to the raw object any more. Because
+// objects could die any time the CC returns to the mutator, any time the CC
+// accesses a PtrInfo it must perform a null check on mParticipant to
+// ensure the object has not gone away.
+//
+// JS objects don't always run finalizers, so the CC can't remove them from
+// the graph when they die. Fortunately, JS objects can only die during a GC,
+// so if a GC is begun during an ICC, the browser synchronously finishes off
+// the ICC, which clears the entire CC graph. If the GC and CC are scheduled
+// properly, this should be rare.
+//
+// The second problem is that objects in the graph can be changed, say by
+// being addrefed or released, or by having a field updated, after the object
+// has been added to the graph. This will be addressed in bug 937818.
+
+
+// Safety
 //
 // An XPCOM object is either scan-safe or scan-unsafe, purple-safe or
 // purple-unsafe.
 //
 // An nsISupports object is scan-safe if:
 //
 //  - It can be QI'ed to |nsXPCOMCycleCollectionParticipant|, though
 //    this operation loses ISupports identity (like nsIClassInfo).
@@ -398,19 +429,23 @@ private:
 
 public:
 
     PtrInfo(void *aPointer, nsCycleCollectionParticipant *aParticipant)
         : mPointer(aPointer),
           mParticipant(aParticipant),
           mColor(grey),
           mInternalRefs(0),
-          mRefCount(0),
+          mRefCount(UINT32_MAX - 1),
           mFirstChild()
     {
+        // We initialize mRefCount to a large non-zero value so
+        // that it doesn't look like a JS object to the cycle collector
+        // in the case where the object dies before being traversed.
+
         MOZ_ASSERT(aParticipant);
     }
 
     // Allow NodePool::Block's constructor to compile.
     PtrInfo() {
         NS_NOTREACHED("should never be called");
     }
 
@@ -657,16 +692,17 @@ public:
         mWeakMaps.Clear();
         mRootCount = 0;
         PL_DHashTableFinish(&mPtrToNodeMap);
         mPtrToNodeMap.ops = nullptr;
     }
 
     PtrInfo* FindNode(void *aPtr);
     PtrToNodeEntry* AddNodeToMap(void *aPtr);
+    void RemoveNodeFromMap(void *aPtr);
 
     uint32_t MapCount() const
     {
         return mPtrToNodeMap.entryCount;
     }
 
     void SizeOfExcludingThis(MallocSizeOf aMallocSizeOf,
                              size_t *aNodesSize, size_t *aEdgesSize,
@@ -696,16 +732,22 @@ GCGraph::AddNodeToMap(void *aPtr)
     PtrToNodeEntry *e = static_cast<PtrToNodeEntry*>(PL_DHashTableOperate(&mPtrToNodeMap, aPtr, PL_DHASH_ADD));
     if (!e) {
         // Caller should track OOMs
         return nullptr;
     }
     return e;
 }
 
+void
+GCGraph::RemoveNodeFromMap(void *aPtr)
+{
+    PL_DHashTableOperate(&mPtrToNodeMap, aPtr, PL_DHASH_REMOVE);
+}
+
 
 static nsISupports *
 CanonicalizeXPCOMParticipant(nsISupports *in)
 {
     nsISupports* out;
     in->QueryInterface(NS_GET_IID(nsCycleCollectionISupports),
                        reinterpret_cast<void**>(&out));
     return out;
@@ -1057,16 +1099,19 @@ public:
     }
 
     void Suspect(void *n, nsCycleCollectionParticipant *cp,
                  nsCycleCollectingAutoRefCnt *aRefCnt);
     uint32_t SuspectedCount();
     void ForgetSkippable(bool aRemoveChildlessNodes, bool aAsyncSnowWhiteFreeing);
     bool FreeSnowWhite(bool aUntilNoSWInPurpleBuffer);
 
+    // This method assumes its argument is already canonicalized.
+    void RemoveObjectFromGraph(void *aPtr);
+
     bool Collect(ccType aCCType,
                  SliceBudget &aBudget,
                  nsICycleCollectorListener *aManualListener);
     void Shutdown();
 
     NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,
                               nsISupports* aData);
 
@@ -2059,34 +2104,37 @@ struct SnowWhiteObject
   void* mPointer;
   nsCycleCollectionParticipant* mParticipant;
   nsCycleCollectingAutoRefCnt* mRefCnt;
 };
 
 class SnowWhiteKiller
 {
 public:
-    SnowWhiteKiller(uint32_t aMaxCount)
+    SnowWhiteKiller(nsCycleCollector *aCollector, uint32_t aMaxCount)
+        : mCollector(aCollector)
     {
+        MOZ_ASSERT(mCollector, "Calling SnowWhiteKiller after nsCC went away");
         while (true) {
             if (mObjects.SetCapacity(aMaxCount)) {
                 break;
             }
             if (aMaxCount == 1) {
                 NS_RUNTIMEABORT("Not enough memory to even delete objects!");
             }
             aMaxCount /= 2;
         }
     }
 
     ~SnowWhiteKiller()
     {
         for (uint32_t i = 0; i < mObjects.Length(); ++i) {
             SnowWhiteObject& o = mObjects[i];
             if (!o.mRefCnt->get() && !o.mRefCnt->IsInPurpleBuffer()) {
+                mCollector->RemoveObjectFromGraph(o.mPointer);
                 o.mRefCnt->stabilizeForDeletion();
                 o.mParticipant->DeleteCycleCollectable(o.mPointer);
             }
         }
     }
 
     void
     Visit(nsPurpleBuffer& aBuffer, nsPurpleBufferEntry* aEntry)
@@ -2102,28 +2150,30 @@ public:
             }
         }
     }
 
     bool HasSnowWhiteObjects() const
     {
       return mObjects.Length() > 0;
     }
+
 private:
+    nsCycleCollector *mCollector;
     FallibleTArray<SnowWhiteObject> mObjects;
 };
 
 class RemoveSkippableVisitor : public SnowWhiteKiller
 {
 public:
     RemoveSkippableVisitor(nsCycleCollector* aCollector,
                            uint32_t aMaxCount, bool aRemoveChildlessNodes,
                            bool aAsyncSnowWhiteFreeing,
                            CC_ForgetSkippableCallback aCb)
-        : SnowWhiteKiller(aAsyncSnowWhiteFreeing ? 0 : aMaxCount),
+        : SnowWhiteKiller(aCollector, aAsyncSnowWhiteFreeing ? 0 : aMaxCount),
           mRemoveChildlessNodes(aRemoveChildlessNodes),
           mAsyncSnowWhiteFreeing(aAsyncSnowWhiteFreeing),
           mDispatchedDeferredDeletion(false),
           mCallback(aCb)
     {}
 
     ~RemoveSkippableVisitor()
     {
@@ -2181,17 +2231,17 @@ nsPurpleBuffer::RemoveSkippable(nsCycleC
 
 bool
 nsCycleCollector::FreeSnowWhite(bool aUntilNoSWInPurpleBuffer)
 {
     CheckThreadSafety();
 
     bool hadSnowWhiteObjects = false;
     do {
-        SnowWhiteKiller visitor(mPurpleBuf.Count());
+        SnowWhiteKiller visitor(this, mPurpleBuf.Count());
         mPurpleBuf.VisitEntries(visitor);
         hadSnowWhiteObjects = hadSnowWhiteObjects ||
                               visitor.HasSnowWhiteObjects();
         if (!visitor.HasSnowWhiteObjects()) {
             break;
         }
     } while (aUntilNoSWInPurpleBuffer);
     return hadSnowWhiteObjects;
@@ -2934,16 +2984,31 @@ nsCycleCollector::Shutdown()
     if (PR_GetEnv("XPCOM_CC_RUN_DURING_SHUTDOWN"))
 #endif
     {
         ShutdownCollect();
     }
 }
 
 void
+nsCycleCollector::RemoveObjectFromGraph(void *aObj)
+{
+    if (mIncrementalPhase == IdlePhase) {
+        return;
+    }
+
+    if (PtrInfo *pinfo = mGraph.FindNode(aObj)) {
+        mGraph.RemoveNodeFromMap(aObj);
+
+        pinfo->mPointer = nullptr;
+        pinfo->mParticipant = nullptr;
+    }
+}
+
+void
 nsCycleCollector::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf,
                                       size_t *aObjectSize,
                                       size_t *aGraphNodesSize,
                                       size_t *aGraphEdgesSize,
                                       size_t *aWeakMapsSize,
                                       size_t *aPurpleBufferSize) const
 {
     *aObjectSize = aMallocSizeOf(this);
@@ -3118,16 +3183,17 @@ DeferredFinalize(DeferredFinalizeAppendF
 
 MOZ_NEVER_INLINE static void
 SuspectAfterShutdown(void* n, nsCycleCollectionParticipant* cp,
                      nsCycleCollectingAutoRefCnt* aRefCnt,
                      bool* aShouldDelete)
 {
     if (aRefCnt->get() == 0) {
         if (!aShouldDelete) {
+            // The CC is shut down, so we can't be in the middle of an ICC.
             CanonicalizeParticipant(&n, &cp);
             aRefCnt->stabilizeForDeletion();
             cp->DeleteCycleCollectable(n);
         } else {
             *aShouldDelete = true;
         }
     } else {
         // Make sure we'll get called again.