Last Comment Bug 583225 - We use the wrong DOMCI for DocumentFragment
: We use the wrong DOMCI for DocumentFragment
Status: RESOLVED FIXED
[sg:critical?]
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla2.0b3
Assigned To: Peter Van der Beken [:peterv] - away till Aug 1st
:
Mentors:
Depends on:
Blocks: crossfuzz
  Show dependency treegraph
 
Reported: 2010-07-30 05:59 PDT by Peter Van der Beken [:peterv] - away till Aug 1st
Modified: 2010-09-27 18:32 PDT (History)
14 users (show)
peterv: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+
.9+
.9-fixed
.12+
.12-fixed


Attachments
Testcase (crashes browser) (648 bytes, text/html)
2010-07-30 06:00 PDT, Peter Van der Beken [:peterv] - away till Aug 1st
no flags Details
v1 (1.42 KB, patch)
2010-07-30 06:16 PDT, Peter Van der Beken [:peterv] - away till Aug 1st
jst: review+
christian: approval1.9.2.9+
christian: approval1.9.1.12+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] - away till Aug 1st 2010-07-30 05:59:08 PDT
We use the wrong DOM classinfo for DocumentFragment nodes (nsDOMGenericSH instead of nsNodeSH). This means we don't force the parent of the DocumentFragment nodes to be their ownerDocument. If the ownerDocument is reparented then the wrapper for the ownerDocument and the DocumentFragment node end up in different XPCWrappedNativeScopes. If then at some point we forcibly try to reparent the wrapper for the DocumentFragment node (for example through adoptNode) we try to remove the wrapper from the XPCWrappedNativeScope of the ownerDocument's wrapper, but we don't find it there. We then add the wrapper to a different XPCWrappedNativeScope and so the wrapper is now in two XPCWrappedNativeScopes. When the wrapper is then destroyed it remove itself from one of the two XPCWrappedNativeScopes, but we end up with a stale reference to it in the other XPCWrappedNativeScope. At some point we'll enumerate the wrapper in all the scopes and hit the stale reference.

It looks like this might lead to several different crashes. One is in WrappedNativeMarker (bug 520554). I've also seen it crash in WrappedNativeTearoffSweeper, and crashes in XPCWrappedNative::IsValid (no bug) and XPCWrappedNativeScope::TraceJS (bug 537822) look suspicious too.

This looks a lot like the Pwn2Own 2010 bug (bug 555109), with a slightly different cause.
Comment 1 Peter Van der Beken [:peterv] - away till Aug 1st 2010-07-30 06:00:13 PDT
Created attachment 461518 [details]
Testcase (crashes browser)
Comment 2 Peter Van der Beken [:peterv] - away till Aug 1st 2010-07-30 06:01:34 PDT
The testcase only needs privileges to force a GC and make the crash appear sooner.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2010-07-30 06:14:08 PDT
Confirmed:
http://crash-stats.mozilla.com/report/index/1ba14588-3fdb-48e4-b3bd-ffd812100730
0  	mozjs.dll  	js_CallGCMarker  	 js/src/jsgc.cpp:2338
1 	xul.dll 	WrappedNativeJSGCThingTracer 	js/src/xpconnect/src/xpcwrappednativescope.cpp:373
2 	mozjs.dll 	JS_DHashTableEnumerate 	js/src/jsdhash.cpp:743
3 	xul.dll 	XPCWrappedNativeScope::TraceJS 	js/src/xpconnect/src/xpcwrappednativescope.cpp:390
4 	mozjs.dll 	mozjs.dll@0x6938f
Comment 4 Peter Van der Beken [:peterv] - away till Aug 1st 2010-07-30 06:16:55 PDT
Created attachment 461522 [details] [diff] [review]
v1

I've looked over nsNodeSH and nsEventReceiverSH and I think everything is able to deal with DocumentFragment nodes, but it'd be good if you could check too.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2010-07-30 15:51:01 PDT
Comment on attachment 461522 [details] [diff] [review]
v1

The questions that arose when looking through nsNodeSH and inherited code is what happens with this change if an expando property gets set on a fragment, and AFAICT that is properly dealt with. Looks like the wrapper gets preserved in that case, and gets un-preserved by the cycle collector AFAICT. And also, what about event handlers set through fragment.onclick (or whatever), AFAICT the event handling code can deal, but it might be a good idea to test that we don't leak in such a case.
Comment 6 Olli Pettay [:smaug] 2010-07-31 01:19:49 PDT
Would it be possible to add some assertion somewhere to prevent this kind of problem?
Comment 7 Peter Van der Beken [:peterv] - away till Aug 1st 2010-08-01 06:06:53 PDT
There is already an assertion in XPCWrappedNative::ReparentWrapperIfFound that catches this.
Comment 8 Peter Van der Beken [:peterv] - away till Aug 1st 2010-08-01 13:58:42 PDT
Checked in with a mochitest for onclick and expando:
http://hg.mozilla.org/mozilla-central/rev/b6aaaa3bed5d

Also had to fix a mochitest, because this patch fixed some todos:
http://hg.mozilla.org/mozilla-central/rev/b996972629f7

I haven't checked in the crashtest yet (waiting till this bug is opened up).
Comment 9 Peter Van der Beken [:peterv] - away till Aug 1st 2010-08-01 14:00:51 PDT
Comment on attachment 461522 [details] [diff] [review]
v1

Fixes a crash found using the cross_fuzz fuzzer, accessing a stale pointer.
Comment 10 christian 2010-08-02 14:50:42 PDT
Comment on attachment 461522 [details] [diff] [review]
v1

a=LegNeato for 1.9.2.9 and 1.9.1.12. Please land on mozilla-1.9.2 default as well as mozilla-1.9.1 default. As a reminder, please land with a non-disclosing commit message.
Comment 11 Peter Van der Beken [:peterv] - away till Aug 1st 2010-08-03 05:04:24 PDT
I've used this bugs title as commit message, it shouldn't give away how things go wrong.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/164d166a913f
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/86d3cde1bd51
Comment 12 Robert Kaiser 2010-08-08 07:19:31 PDT
The test for this bug fails on the SeaMonkey2.0 (1.9.1-based) tree with this message:

34384 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_bug583225.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - docFragment.dispatchEvent is not a function

It looks like Firefox3.5 is not running those tests any more, so I can't compare if that happens there as well.
Comment 13 Al Billings [:abillings] 2010-08-09 18:01:20 PDT
(In reply to comment #1)
> Created attachment 461518 [details]
> Testcase (crashes browser)

I'm not seeing a crash with this testcase in either 1.9.1.11 or 1.9.2.8. Is there a particular trick to it. I am testing XO.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-09 18:46:26 PDT
(In reply to comment #12)
> It looks like Firefox3.5 is not running those tests any more, so I can't
> compare if that happens there as well.

It does at least sometimes.  From my push of bug 572232:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1281399011.1281401463.21066.gz&fulltext=1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c54b6c62214d
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-09 19:04:59 PDT
Again here, same push:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1281402241.1281405491.7606.gz&fulltext=1
Comment 16 Peter Van der Beken [:peterv] - away till Aug 1st 2010-08-10 11:41:54 PDT
I backed out the piece of the test that was relying on dispatchEvent on DocumentFragments (which was added post-1.9.1).

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d56e4eec0aea
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e356f6a54a72
Comment 17 Al Billings [:abillings] 2010-08-10 15:59:48 PDT
(In reply to comment #13)
> (In reply to comment #1)
> > Created attachment 461518 [details] [details]
> > Testcase (crashes browser)
> 
> I'm not seeing a crash with this testcase in either 1.9.1.11 or 1.9.2.8. Is
> there a particular trick to it. I am testing XP.

I still cannot reproduce the crash. I need some viable STR here.
Comment 18 Peter Van der Beken [:peterv] - away till Aug 1st 2010-08-11 06:00:07 PDT
Testcase crashes immediately for me in a 1.9.2.8 build (ran testcase from file://).
Comment 19 Al Billings [:abillings] 2010-08-16 17:18:23 PDT
Peter, what operating system? I'm using Windows XP. If I open the attached testcase in 1.9.2.8 or 1.9.1.11, I do not get a crash.
Comment 20 Peter Van der Beken [:peterv] - away till Aug 1st 2010-08-17 06:25:47 PDT
OS X, but I used Windows XP when originally recording the crash so should work there too.
Comment 21 Al Billings [:abillings] 2010-08-17 15:15:44 PDT
Well, I can't verify the crash on XP. 

I was able to verify it on OS X 10.6.4 though. Firefox 3.6.8 crashes with the testcase and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.9pre) Gecko/20100817 Namoroka/3.6.9pre does not.

The same with Firefox 3.5.11 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.12pre) Gecko/20100817 Shiretoko/3.5.12pre.

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