583225 - We use the wrong DOMCI for DocumentFragment

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

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]]

Attachment: Testcase (crashes browser)

Comment 2

[Peter Van der Beken [:peterv]]

The testcase only needs privileges to force a GC and make the crash appear sooner.

Comment 3

[Martijn Wargers (dead)]

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]]

Attachment: 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)]

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][[e-mail]]]

Would it be possible to add some assertion somewhere to prevent this kind of problem?

Comment 7

[Peter Van der Beken [:peterv]]

There is already an assertion in XPCWrappedNative::ReparentWrapperIfFound that catches this.

Comment 8

[Peter Van der Beken [:peterv]]

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]]

Comment on attachment 461522 [details] [diff] [review]
v1

Fixes a crash found using the cross_fuzz fuzzer, accessing a stale pointer.

Comment 10

[LegNeato]

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]]

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]

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 - ex-MoCo]]

(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]]

(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]]

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]]

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 - ex-MoCo]]

(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]]

Testcase crashes immediately for me in a 1.9.2.8 build (ran testcase from file://).

Comment 19

[Al Billings [:abillings - ex-MoCo]]

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]]

OS X, but I used Windows XP when originally recording the crash so should work there too.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

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.