Created attachment 582388 [details] testcase (crashes Firefox) ###!!! ASSERTION: Cycle collection participant didn't traverse to preserved wrapper! This will probably crash.: 'callback.mFound', file /builds/slave/m-cen-osx64-dbg/build/content/base/src/nsContentUtils.cpp, line 5471 And then things *really* go wrong. Regression from bug 680937?
This test puts the style field of a DOM node into a weak map. Looking at nsGenericElement, the style field has type nsICSSDeclaration. nsComputedDOMStyle and nsDOMCSSAttrDeclaration are both subtypes of that class and of nsWrapperCache. NS_IMPL_CYCLE_COLLECTION_1(nsDOMCSSAttributeDeclaration, mElement) NS_IMPL_CYCLE_COLLECTION_1(nsComputedDOMStyle, mContent) So these classes seem to be missing the wrapper cache glue in the cycle collector declarations, as can be seen in nsHTMLOptionCollection. specifically: NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in Traverse, the NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER in Unlink, as well as NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsFoo) NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER NS_IMPL_CYCLE_COLLECTION_TRACE_END It seems to me that that's a bug in those classes, but maybe I'm wrong? I'm going to look at every nsWrapperCache to see any others are missing this.
All other uses look okay to me. I don't see anything for node lists, but I assume that with the node bindings they aren't using the existing CC macros.
These classes did have the preserved wrapper stuff at one point, but Peter removed them in bug 437449.
Created attachment 582433 [details] [diff] [review] alternate way to preserve wrappers This is an alternate way to preserve wrappers that I found, which seems to pass my existing tests and not crash with Jesse's test case, but I have no idea where this PreserveWrapper is implemented, which is disturbing. Thanks for the test case, Jesse.
Another simpler solution would be to just QI to nsINode or whatever. That will cover the common cases, and fail on the weird CSS things.
Created attachment 582582 [details] [diff] [review] QI wrapped native weak map keys to nsINode, not nsWrapperCache This is a simple fix to the particular problem. Instead of QIing to nsWrapperCache, which has some subclasses that can't have their wrappers preserved, we QI to nsINode, which traces its wrapper, and thus is able to have its wrapper preserved. This should cover almost any case we actually care about. I also added Jesse's test case.
Comment on attachment 582582 [details] [diff] [review] QI wrapped native weak map keys to nsINode, not nsWrapperCache Stealing review. I think this is a good stop gap here until we figure out the "right" way to do this, which may not happen before we have new DOM bindings. The most important case here is IMO to do this right for DOM nodes, and this does exactly that.
Try run looked good. https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b4b0ce8ad5
What's wrong with do_QueryInterface(native, &rv)?
I'm just not very familiar with all of the various ways to QI, so it looks like I ended up using a lower level one. Good to know.
(In reply to Johnny Stenback (:jst, firstname.lastname@example.org) from comment #7) > Stealing review. I think this is a good stop gap here until we figure out > the "right" way to do this, which may not happen before we have new DOM > bindings. The most important case here is IMO to do this right for DOM > nodes, and this does exactly that. Except that in Bug 669240 we want to be able to stick nsIFrameMessageManagers into a weak map, and this moves us farther away from that.