Bug 944492, part 1 - Make XPCWrappedJS use the purple buffer. r=smaug

js/xpconnect/src/XPCWrappedJS.cpp

@@ -16,6 +16,40 @@ using namespace mozilla;
 
 // NOTE: much of the fancy footwork is done in xpcstubs.cpp
 
+
+// nsXPCWrappedJS lifetime.
+//
+// An nsXPCWrappedJS is either rooting its JS object or is subject to finalization.
+// The subject-to-finalization state lets wrappers support
+// nsSupportsWeakReference in the case where the underlying JS object
+// is strongly owned, but the wrapper itself is only weakly owned.
+//
+// A wrapper is rooting its JS object whenever its refcount is greater than 1. In
+// this state, root wrappers are always added to the cycle collector graph. The
+// wrapper keeps around an extra refcount, added in the constructor, to support
+// the possibility of an eventual transition to the subject-to-finalization state.
+// This extra refcount is ignored by the cycle collector, which traverses the "self"
+// edge for this refcount.
+//
+// When the refcount of a rooting wrapper drops to 1, if there is no weak reference
+// to the wrapper (which can only happen for the root wrapper), it is immediately
+// Destroy()'d. Otherwise, it becomes subject to finalization.
+//
+// When a wrapper is subject to finalization, the wrapper has a refcount of 1. It is
+// now owned exclusively by its JS object. Either a weak reference will be turned into
+// a strong ref which will bring its refcount up to 2 and change the wrapper back to
+// the rooting state, or it will stay alive until the JS object dies. If the JS object
+// dies, then when XPCJSRuntime::FinalizeCallback calls FindDyingJSObjects
+// it will find the wrapper and call Release() in it, destroying the wrapper.
+// Otherwise, the wrapper will stay alive, even if it no longer has a weak reference
+// to it.
+//
+// When the wrapper is subject to finalization, it is kept alive by an implicit reference
+// from the JS object which is invisible to the cycle collector, so the cycle collector
+// does not traverse any children of wrappers that are subject to finalization. This will
+// result in a leak if a wrapper in the non-rooting state has an aggregated native that
+// keeps alive the wrapper's JS object.  See bug 947049.
+
 NS_IMETHODIMP
 NS_CYCLE_COLLECTION_CLASSNAME(nsXPCWrappedJS)::Traverse
    (void *p, nsCycleCollectionTraversalCallback &cb)
@@ -37,14 +71,17 @@ NS_CYCLE_COLLECTION_CLASSNAME(nsXPCWrappedJS)::Traverse
         NS_IMPL_CYCLE_COLLECTION_DESCRIBE(nsXPCWrappedJS, refcnt)
     }
 
-    // nsXPCWrappedJS keeps its own refcount artificially at or above 1, see the
-    // comment above nsXPCWrappedJS::AddRef.
+    // A wrapper that is subject to finalization will only die when its JS object dies.
+    if (tmp->IsSubjectToFinalization())
+        return NS_OK;
+
+    // Don't let the extra reference for nsSupportsWeakReference keep a wrapper that is
+    // not subject to finalization alive.
     NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "self");
     cb.NoteXPCOMChild(s);
 
-    if (refcnt > 1) {
-        // nsXPCWrappedJS roots its mJSObj when its refcount is > 1, see
-        // the comment above nsXPCWrappedJS::AddRef.
+    if (tmp->IsValid()) {
+        MOZ_ASSERT(refcnt > 1);
         NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mJSObj");
         cb.NoteJSChild(tmp->GetJSObjectPreserveColor());
     }
@@ -54,7 +91,7 @@ NS_CYCLE_COLLECTION_CLASSNAME(nsXPCWrappedJS)::Traverse
         cb.NoteXPCOMChild(tmp->GetAggregatedNativeObject());
     } else {
         NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "root");
-        cb.NoteXPCOMChild(static_cast<nsIXPConnectWrappedJS*>(tmp->GetRootWrapper()));
+        cb.NoteXPCOMChild(ToSupports(tmp->GetRootWrapper()));
     }
 
     return NS_OK;
@@ -127,37 +164,22 @@ nsXPCWrappedJS::QueryInterface(REFNSIID aIID, void** aInstancePtr)
 }
 
 
-// Refcounting is now similar to that used in the chained (pre-flattening)
-// wrappednative system.
-//
-// We are now holding an extra refcount for nsISupportsWeakReference support.
-//
-// Non-root wrappers remove themselves from the chain in their destructors.
-// We root the JSObject as the refcount transitions from 1->2. And we unroot
-// the JSObject when the refcount transitions from 2->1.
-//
-// When the transition from 2->1 is made and no one holds a weak ref to the
-// (aggregated) object then we decrement the refcount again to 0 (and
-// destruct) . However, if a weak ref is held at the 2->1 transition, then we
-// leave the refcount at 1 to indicate that state. This leaves the JSObject
-// no longer rooted by us and (as far as we know) subject to possible
-// collection. Code in XPCJSRuntime watches for JS gc to happen and will do
-// the final release on wrappers whose JSObjects get finalized. Note that
-// even after tranistioning to this refcount-of-one state callers might do
-// an addref and cause us to re-root the JSObject and continue on more normally.
+// For a description of nsXPCWrappedJS lifetime and reference counting, see
+// the comment at the top of this file.
 
 nsrefcnt
 nsXPCWrappedJS::AddRef(void)
 {
     if (!MOZ_LIKELY(NS_IsMainThread()))
         MOZ_CRASH();
-    nsrefcnt cnt = ++mRefCnt;
+
+    MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");
+    nsrefcnt cnt = mRefCnt.incr();
     NS_LOG_ADDREF(this, cnt, "nsXPCWrappedJS", sizeof(*this));
 
     if (2 == cnt && IsValid()) {
         GetJSObject(); // Unmark gray JSObject.
-        XPCJSRuntime* rt = mClass->GetRuntime();
-        rt->AddWrappedJSRoot(this);
+        mClass->GetRuntime()->AddWrappedJSRoot(this);
     }
 
     return cnt;
@@ -168,31 +190,44 @@ nsXPCWrappedJS::Release(void)
 {
     if (!MOZ_LIKELY(NS_IsMainThread()))
         MOZ_CRASH();
-    NS_PRECONDITION(0 != mRefCnt, "dup release");
+    MOZ_ASSERT(int32_t(mRefCnt) > 0, "dup release");
+    NS_ASSERT_OWNINGTHREAD(nsXPCWrappedJS);
 
-do_decrement:
-
-    nsrefcnt cnt = --mRefCnt;
+    bool shouldDelete = false;
+    nsISupports *base = NS_CYCLE_COLLECTION_CLASSNAME(nsXPCWrappedJS)::Upcast(this);
+    nsrefcnt cnt = mRefCnt.decr(base, &shouldDelete);
     NS_LOG_RELEASE(this, cnt, "nsXPCWrappedJS");
 
     if (0 == cnt) {
-        delete this;   // also unlinks us from chain
-        return 0;
+        if (MOZ_UNLIKELY(shouldDelete)) {
+            mRefCnt.stabilizeForDeletion();
+            DeleteCycleCollectable();
+        } else {
+            mRefCnt.incr();
+            Destroy();
+            mRefCnt.decr(base);
         }
-    if (1 == cnt) {
+    } else if (1 == cnt) {
         if (IsValid())
             RemoveFromRootSet();
 
-        // If we are not the root wrapper or if we are not being used from a
-        // weak reference, then this extra ref is not needed and we can let
-        // ourself be deleted.
-        // Note: HasWeakReferences() could only return true for the root.
+        // If we are not a root wrapper being used from a weak reference,
+        // then the extra ref is not needed and we can let outselves be
+        // deleted.
         if (!HasWeakReferences())
-            goto do_decrement;
+            return Release();
+
+        MOZ_ASSERT(IsRootWrapper(), "Only root wrappers should have weak references");
     }
     return cnt;
 }
 
+NS_IMETHODIMP_(void)
+nsXPCWrappedJS::DeleteCycleCollectable(void)
+{
+    delete this;
+}
+
 void
 nsXPCWrappedJS::TraceJS(JSTracer* trc)
 {
@@ -345,9 +380,12 @@ nsXPCWrappedJS::nsXPCWrappedJS(JSContext* cx,
 {
     InitStub(GetClass()->GetIID());
 
-    // intentionally do double addref - see Release().
+    // There is an extra AddRef to support weak references to wrappers
+    // that are subject to finalization. See the top of the file for more
+    // details.
     NS_ADDREF_THIS();
     NS_ADDREF_THIS();
+
     NS_ADDREF(aClass);
     NS_IF_ADDREF(mOuter);
 
@@ -358,7 +396,13 @@ nsXPCWrappedJS::nsXPCWrappedJS(JSContext* cx,
 
 nsXPCWrappedJS::~nsXPCWrappedJS()
 {
-    NS_PRECONDITION(0 == mRefCnt, "refcounting error");
+    Destroy();
+}
+
+void
+nsXPCWrappedJS::Destroy()
+{
+    MOZ_ASSERT(1 == int32_t(mRefCnt), "should be stabilized for deletion");
 
     if (IsRootWrapper()) {
         XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();

js/xpconnect/src/xpcprivate.h

@@ -2464,14 +2464,13 @@ class nsXPCWrappedJS : protected nsAutoXPTCStub,
                        public XPCRootSetElem
 {
 public:
-    NS_DECL_THREADSAFE_ISUPPORTS
+    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
     NS_DECL_NSIXPCONNECTJSOBJECTHOLDER
     NS_DECL_NSIXPCONNECTWRAPPEDJS
     NS_DECL_NSISUPPORTSWEAKREFERENCE
     NS_DECL_NSIPROPERTYBAG
 
     NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsXPCWrappedJS, nsIXPConnectWrappedJS)
-    void DeleteCycleCollectable() {}
 
     NS_IMETHOD CallMethod(uint16_t methodIndex,
                           const XPTMethodDescriptor *info,
@@ -2513,12 +2512,9 @@ public:
     bool IsValid() const {return mJSObj != nullptr;}
     void SystemIsBeingShutDown();
 
-    // This is used by XPCJSRuntime::GCCallback to find wrappers that no
-    // longer root their JSObject and are only still alive because they
-    // were being used via nsSupportsWeakReference at the time when their
-    // last (outside) reference was released. Wrappers that fit into that
-    // category are only deleted when we see that their corresponding JSObject
-    // is to be finalized.
+    // These two methods are used by JSObject2WrappedJSMap::FindDyingJSObjects
+    // to find non-rooting wrappers for dying JS objects. See the top of
+    // XPCWrappedJS.cpp for more details.
     bool IsSubjectToFinalization() const {return IsValid() && mRefCnt == 1;}
     bool IsObjectAboutToBeFinalized() {return JS_IsAboutToBeFinalized(&mJSObj);}
 
@@ -2537,6 +2533,7 @@ protected:
                    nsXPCWrappedJS* root,
                    nsISupports* aOuter);
 
+    void Destroy();
     void Unlink();
 
 private: