Closed Bug 711616 Opened 9 years ago Closed 9 years ago
.style to a Weak Map makes the CC go boom
###!!! 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.
Assignee: nobody → continuation
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.
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.
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.
Attachment #582582 - Flags: review?(mrbkap) → review+
Try run looked good. https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b4b0ce8ad5
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla11
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.