See bug 942528.

Turns out, the existing traverse method for XPCWrappedJS is only correct when the wrapper doesn't have any weak references. https://tbpl.mozilla.org/?tree=Try&rev=77bb1d9d50b6

I should also turn CanSkipWrappedJS into a normal skippable method, and maybe bake the logic about weak references into the skippable method (eg if we're a root wrapper and we have a weak reference, we're definitely alive).

CanSkipWrappedJS seems a little overly complicated for what it has to do. It has this isRootWrappedJS stuff, but as far as I can see, it is only being called on things in the mWrappedJSRoots list, which I'd think would all have isRootWrappedJS. Olli, am I missing something? Of course, now I need that extra logic.

I don't think you're missing anything. It is just safe to call that method even without non-root stuff. The fact that the object is GetRootWrapper() doesn't in general mean that it is in mWrappedJSRoots

Previous try run looked good. Turned into a normal skippable class, with the CanSkip stuff rewritten a bit: https://tbpl.mozilla.org/?tree=Try&rev=784769303c99

I broke up the giant conditional in CanSkipWrappedJS() a bit to make it easier to understand. I also changed around what it does a little so that should be carefully looked at.

"Normal" is too strong a word to use.

Comment on attachment 8341950 [details] [diff] [review] part 1 - Make XPCWrappedJS use the purple buffer. r=smaug a bit scary stuff, given the complexity of this stuff. Could we land this early in the next cycle?

(In reply to Olli Pettay [:smaug] from comment #10) > Could we land this early in the next cycle? Sure.

Comment on attachment 8341951 [details] [diff] [review] part 2 - Make XPCWrappedJS a proper skippable class. r=smaug >+nsXPCWrappedJS::CanSkip() >+{ >+ if (!nsCCUncollectableMarker::sGeneration) >+ return false; >+ >+ // If this wrapper holds a gray object, need to trace it. >+ JSObject *obj = GetJSObjectPreserveColor(); >+ if (obj && xpc_IsGrayGCThing(obj)) >+ return false; >+ >+ if (IsSubjectToFinalization()) >+ return false; >+ >+ // The extra ref from the weak reference keeps the wrapper alive. >+ if (HasWeakReferences()) >+ return true; I don't understand this. If we have a weak ref and a strong ref, why can we skip?

> I don't understand this. If we have a weak ref and a strong ref, why can we skip? There's an extra AddRef in the XPCWJS constructor, and XPCWJS::Release drops the extra ref if there's no weak reference. Therefore, if anything has a "weak reference" to an XPCWJS (in the SupportsWeakRef sense), the XPCWJS won't be destroyed, so it is definitely alive and we don't need to add it to the CC graph (aside from the usual tracing JS stuff, which the earlier conditions are supposed to handle).

Though I'm not entirely what ends up destroying the XPCWJS when it does have a weak reference, and a refcount of 1. I should figure that out. That could be a problem just with part 1.

Comment on attachment 8341950 [details] [diff] [review] part 1 - Make XPCWrappedJS use the purple buffer. r=smaug Yeah, I think this isn't good either. Having a weakref pointing to XPCWJS shouldn't affect to whether it is deleted or not.

I don't know if you want to look at part 1 or not, Bobby. I consolidated and clarified the documentation of XPCWrappedJS lifetime. These patches should not change the behavior. green L64 debug try run: https://tbpl.mozilla.org/?tree=Try&rev=bbbd2b63b1ef

Comment on attachment 8344215 [details] [diff] [review] part 1 - Make XPCWrappedJS use the purple buffer. Review of attachment 8344215 [details] [diff] [review]: ----------------------------------------------------------------- This comment was very informative. Thank you for writing it! I don't know enough about this code to review it without spending several hours figuring it out myself. I'll let smaug review.

Comment on attachment 8344215 [details] [diff] [review] part 1 - Make XPCWrappedJS use the purple buffer. >+// When a wrapper is subject to finalization, the wrapper has a refcount of 1. It is >+// now owned exclusively by its JS object. Either it will have GetNewOrUsed called on it, >+// 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. Also getting non-weakRef from the weakref ends up increasing refcnt from 1 to 2. No need for GetNewOrUsed >+NS_IMETHODIMP_(void) >+nsXPCWrappedJS::DeleteCycleCollectable(void) >+{ >+ delete(this); I think without () is more common delete this; thanks for the explanation.

Addressed review comments. Carrying forward smaug's r+.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/70b2d51d4f61 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3b50a75aa431

https://hg.mozilla.org/mozilla-central/rev/70b2d51d4f61 https://hg.mozilla.org/mozilla-central/rev/3b50a75aa431