In bug 792861, I have a patch that checks that a) HoldJSObject is only done when something isn't in the JS holders table already b) DropJSObjects is only done when something is in the JS holders table https://tbpl.mozilla.org/?tree=Try&rev=9aa3ce78d219 M1, M-C, J: tries to DropJSObjects an object that isn't in the holders table nsContentUtils::ReleaseWrapper nsBaseContentList::cycleCollection::UnlinkImpl M2, X: tries to Hold something already held object nsContentUtils::PreserveWrapper nsEventTargetSH::AddProperty M3, M4, R are okay M5 tries to drop something that isn't held nsContentUtils::ReleaseWrapper nsComputedDOMStyle::cycleCollection::UnlinkImpl bc drops an unheld thing nsContentUtils::ReleaseWrapper nsDOMCSSAttributeDeclaration::cycleCollection::UnlinkImpl C drops an unheld thing nsContentUtils::ReleaseWrapper mozilla::css::DOMCSSStyleRule::cycleCollection::UnlinkImpl So it looks like it is a widespread enough problem we should just bail on it. Another approach would just be to release layout statics when the mJSHolders table is empty, and hold it when it becomes non-empty, but that could be annoying in its own way.

Could we try having hold/drop increment/decrement a counter in the table instead of just adding/removing?

I think the "counter in the table" is just going to be the size of the table, so there's no real need to explicitly maintain a separate one.

The benefit of doing counter in table (per entry, note!) is that doing hold, hold, drop, drop in that order is no longer a footgun...

(In reply to Andrew McCreight [:mccr8] from comment #0) > Another approach would just be to release layout statics when the mJSHolders > table is empty, and hold it when it becomes non-empty. I like this approach.

(In reply to Boris Zbarsky (:bz) from comment #3) > The benefit of doing counter in table (per entry, note!) is that doing hold, > hold, drop, drop in that order is no longer a footgun... Oh I see what you mean. Still, that won't help with things like hold, hold, drop. The minimum requirement here is that we keep XPConnect alive while there are still things in the table. Do you want any further checking of hold/drop beyond that?

> Still, that won't help with things like hold, hold, drop. Help in what sense? Right now, actually dropping in anything other than unlink or your destructor is a footgun of ginormous proportions, because we have macros for wrappercached stuff that hide hold/drop calls. So you might not even realize that you've done "hold hold drop", and will be holding garbage JS object pointers. What I'd like, ideally, is a way to not have that footgun. But yes, that can be a separate issue from just keeping xpconnect alive...

Ideally, we'd NULL out all JS pointers when we do the DROP, but I can't think of any great way to do that aside from creating a new callback that is to Trace what Unlink is to Traverse, which is awful in its own way. In practice, we're probably mostly okay, because Unlinked objects are usually only going to run their destructor.

> Ideally, we'd NULL out all JS pointers when we do the DROP How does that work with inheritance? In fact, how does the whole hold/drop thing work with inheritance?

I'm not going to be able to get to this right away.

This catches quite some cases I'll need to change.

Let's see if something else need to be still fixed. https://tbpl.mozilla.org/?tree=Try&rev=0ca477b9d082 dtors may cause problems, and if so, I may need to add some flag to not do the tracing when drop is calling in dtor.

Comment on attachment 683074 [details] [diff] [review] fixes, WIP Review of attachment 683074 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioBuffer.cpp @@ +53,5 @@ > } > > AudioBuffer::~AudioBuffer() > { > + mChannels.Clear(); You probably want to do this in Unlink, too, to break any potential cycles. ::: dom/base/nsJSTimeoutHandler.cpp @@ +151,2 @@ > } else { > NS_WARNING("No func and no expr - roots may not have been removed"); This last case looks impossible, so you could eliminate it and refactor a little bit.

IDB needs still some fixes...

Getting closer...

Comment on attachment 682638 [details] [diff] [review] WIP, assertions Not super pretty but works.

Comment on attachment 683788 [details] [diff] [review] fixes, WIP, v4 IDBRequest requires that wrapper is caches when mResultVal is set. Wrapper cache is cleared in nsDOMEventTargetHelper (which IDBWrapperCache inherits)

Comment on attachment 682557 [details] [diff] [review] WIP - base, without assertions Review of attachment 682557 [details] [diff] [review]: ----------------------------------------------------------------- Can't you just do the nsLayoutStatics::AddRef/Release inside of XPCJSRuntime? Then you'd avoid having to thread the boolean all over the place. Also, please remove the declarations of sJSGCThingRootCount from nsContentUtils.h and nsContentUtils.cpp.

(In reply to Andrew McCreight [:mccr8] from comment #27) > Can't you just do the nsLayoutStatics::AddRef/Release inside of > XPCJSRuntime? Then you'd avoid having to thread the boolean all over the > place. Hmm, that is indeed better. > Also, please remove the declarations of sJSGCThingRootCount from > nsContentUtils.h and nsContentUtils.cpp Uh, how did I forgot that. I was going to do it.

Comment on attachment 684152 [details] [diff] [review] base Review of attachment 684152 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this is much nicer. You still forgot to remove the declarations of sJSGCThingRootCount from nsContentUtils.h and nsContentUtils.cpp. :) r=me with that.

Comment on attachment 684153 [details] [diff] [review] assert Review of attachment 684153 [details] [diff] [review]: ----------------------------------------------------------------- I don't really like how spread all over the place this ends up being. You could move the call of AssertNoObjectsToTrace to XPCJSRuntime::RemoveJSHolder, move all of the supporting stuff from nsContentUtils to XPCJSRuntime, and make the interface AssertNoObjectsToTrace instead of GetJSHolderTracer, and you'd only have to expose AssertNoObjectsToTrace on nsCycleCollectionJSRuntime instead of nsIXPConnect. nsCycleCollector could call it by doing if (mJSRuntime) mJSRuntime->AssertNoObjectsToTrace(...) How does that sound to you? ::: content/base/src/nsContentUtils.cpp @@ +4521,5 @@ > return sXPConnect->AddJSHolder(aScriptObjectHolder, aTracer); > } > > +#ifdef DEBUG > +extern void* gCycleCollectorUnlinkObject; You should add a comment about where this is ultimately defined. @@ +4532,5 @@ > +} > + > +void AssertNoObjectsToTrace(void* aPossibleJSHolder) > +{ > + if (nsContentUtils::XPConnect()) { Maybe use an early return here? @@ +4547,5 @@ > nsresult > nsContentUtils::DropJSObjects(void* aScriptObjectHolder) > { > +#ifdef DEBUG > + if (aScriptObjectHolder != gCycleCollectorUnlinkObject) { If you set gCycleCollectorUnlinkObject to null before the call to AssertNoObjectsToTrace in CollectWhite, then you can move this gCycleCollectorUnlinkObject check inside AssertNoObjectsToTrace. You should add a comment about why this check is needed.

Comment on attachment 685303 [details] [diff] [review] assert Review of attachment 685303 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this is a lot nicer! ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +281,5 @@ > { > +#ifdef DEBUG > + // Assert that the holder doesn't try to keep any GC things alive. > + // In case of unlinking cycle collector calls AssertNoObjectsToTrace > + // manually. If you could throw something at the end here like "because we don't want to check the holder before we are finished unlinking it" that would be good. ::: js/xpconnect/src/xpcprivate.h @@ +485,5 @@ > static XPCJSRuntime* GetRuntimeInstance(); > XPCJSRuntime* GetRuntime() {return mRuntime;} > > +#ifdef DEBUG > + void ObjectToUnlink(void* aObject); Should this be called something like SetObjectToUnlink? @@ +830,5 @@ > nsresult AddJSHolder(void* aHolder, nsScriptObjectTracer* aTracer); > nsresult RemoveJSHolder(void* aHolder); > nsresult TestJSHolder(void* aHolder, bool* aRetval); > +#ifdef DEBUG > + void ObjectToUnlink(void* aObject) { mObjectToUnlink = aObject; } Likewise. ::: xpcom/base/nsCycleCollector.cpp @@ +2382,5 @@ > for (uint32_t i = 0; i < count; ++i) { > PtrInfo *pinfo = mWhiteNodes->ElementAt(i); > +#ifdef DEBUG > + if (mJSRuntime) > + mJSRuntime->ObjectToUnlink(pinfo->mPointer); nit: brace the body of the if

Comment on attachment 685304 [details] [diff] [review] fixes, v6, removes also sJSGCThingRootCount Review of attachment 685304 [details] [diff] [review]: ----------------------------------------------------------------- The IndexedDB changes are a little more dramatic than the rest, maybe it would be good to get an IDB peer to glance at them. ::: dom/base/nsJSTimeoutHandler.cpp @@ +140,5 @@ > > void > nsJSScriptTimeoutHandler::ReleaseJSObjects() > { > if (mExpr || mFunObj) { nit: I think you could reorganize the body of this |if| to be: if (mExpr) { mExpr = nullptr; } else { mFunObj = nullptr; } NS_DROP_JS_OBJECTS(this,nsJSScriptTimeoutHandler); The NS_WARNING case will never be hit, and you might as well factor out the DROP to be less confusing. Not a big deal, it is just a little confusing as is.

Comment on attachment 685304 [details] [diff] [review] fixes, v6, removes also sJSGCThingRootCount Kyle, could you look at the IDB changes? The idea is to assert that after drop/unlink there is nothing to trace.

It might be a good idea to land the fixes earlier in the stack than the assertions, to avoid causing bisections to blow up. Hopefully that won't be that hard.

Comment on attachment 685304 [details] [diff] [review] fixes, v6, removes also sJSGCThingRootCount Review of attachment 685304 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBCursor.cpp @@ +427,5 @@ > return NS_OK; > } > > +void > +IDBCursor::DropJSObjects() I would prefer that this function went after the dtor (so the header order matches the cpp order to the extent possible). @@ +429,5 @@ > > +void > +IDBCursor::DropJSObjects() > +{ > + if (mRooted) { Can we just do if (!mRooted) { return; } ? ::: dom/indexedDB/IDBKeyRange.cpp @@ +308,5 @@ > > +void > +IDBKeyRange::DropJSObjects() > +{ > + if (mRooted) { Same here, I think we should early return. ::: dom/indexedDB/IDBTransaction.cpp @@ -189,5 @@ > mActorChild->Send__delete__(mActorChild); > NS_ASSERTION(!mActorChild, "Should have cleared in Send__delete__!"); > } > - > - nsContentUtils::ReleaseWrapper(static_cast<nsIDOMEventTarget*>(this), this); There's one of these in IDBDatabase too.

https://hg.mozilla.org/mozilla-central/rev/a196bff3a967 https://hg.mozilla.org/mozilla-central/rev/aaf77faa6886 https://hg.mozilla.org/mozilla-central/rev/6c23f41b0747

This is the same as for m-c, but with the null check for xpconnect I added in "fixes". https://tbpl.mozilla.org/?tree=Try&rev=dee2522a8d13

Comment on attachment 686139 [details] [diff] [review] Base patch for Aurora [Approval Request Comment] Bug caused by (feature/regressing bug #): Old problem which is getting worse the more we add new APIs User impact if declined: Possibly odd shutdown crashes Testing completed (on m-c, etc.): landed today Risk to taking this patch (and alternatives if risky): Should be relatively safe String or UUID changes made by this patch: NA I'd like to take the patch in order to avoid problems like bug 810618.

Comment on attachment 686139 [details] [diff] [review] Base patch for Aurora Without having a specific unresolved crasher that this prevents, let's instead let this ride the trains.

(In reply to Alex Keybl [:akeybl] from comment #47) > Comment on attachment 686139 [details] [diff] [review] > Base patch for Aurora > > Without having a specific unresolved crasher that this prevents, let's > instead let this ride the trains. Now we have a bad crash in bug 848153.