Last Comment Bug 711616 - Adding elem.style to a WeakMap makes the CC go boom
: Adding elem.style to a WeakMap makes the CC go boom
Status: RESOLVED FIXED
[sg:critical]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla11
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on:
Blocks: 326633 680937
  Show dependency treegraph
 
Reported: 2011-12-16 14:25 PST by Jesse Ruderman
Modified: 2012-03-26 19:48 PDT (History)
10 users (show)
continuation: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
unaffected


Attachments
testcase (crashes Firefox) (80 bytes, text/html)
2011-12-16 14:25 PST, Jesse Ruderman
no flags Details
alternate way to preserve wrappers (1.23 KB, patch)
2011-12-16 16:46 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
QI wrapped native weak map keys to nsINode, not nsWrapperCache (2.21 KB, patch)
2011-12-17 12:56 PST, Andrew McCreight [:mccr8]
jst: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-12-16 14:25:21 PST
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?
Comment 1 Andrew McCreight [:mccr8] 2011-12-16 15:12:04 PST
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.
Comment 2 Andrew McCreight [:mccr8] 2011-12-16 15:23:28 PST
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.
Comment 3 Andrew McCreight [:mccr8] 2011-12-16 15:33:06 PST
These classes did have the preserved wrapper stuff at one point, but Peter removed them in bug 437449.
Comment 4 Andrew McCreight [:mccr8] 2011-12-16 16:46:19 PST
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.
Comment 5 Andrew McCreight [:mccr8] 2011-12-16 18:05:31 PST
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.
Comment 6 Andrew McCreight [:mccr8] 2011-12-17 12:56:11 PST
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 7 Johnny Stenback (:jst, jst@mozilla.com) 2011-12-17 18:36:53 PST
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.
Comment 8 Andrew McCreight [:mccr8] 2011-12-17 22:13:39 PST
Try run looked good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b4b0ce8ad5
Comment 9 :Ms2ger 2011-12-18 02:17:49 PST
What's wrong with do_QueryInterface(native, &rv)?
Comment 10 Andrew McCreight [:mccr8] 2011-12-18 05:42:35 PST
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.
Comment 11 Matt Brubeck (:mbrubeck) 2011-12-18 15:35:11 PST
https://hg.mozilla.org/mozilla-central/rev/b0b4b0ce8ad5
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-21 14:07:21 PST
(In reply to Johnny Stenback (:jst, jst@mozilla.com) 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.

Note You need to log in before you can comment on or make changes to this bug.