Closed Bug 583225 Opened 11 years ago Closed 11 years ago
We use the wrong DOMCI for Document
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.
The testcase only needs privileges to force a GC and make the crash appear sooner.
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
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.
Attachment #461522 - Flags: review?(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.
Attachment #461522 - Flags: review?(jst) → review+
Would it be possible to add some assertion somewhere to prevent this kind of problem?
There is already an assertion in XPCWrappedNative::ReparentWrapperIfFound that catches this.
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).
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 461522 [details] [diff] [review] v1 Fixes a crash found using the cross_fuzz fuzzer, accessing a stale pointer.
Comment on attachment 461522 [details] [diff] [review] v1 a=LegNeato for 18.104.22.168 and 22.214.171.124. 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.
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
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.
(In reply to comment #1) > Created attachment 461518 [details] > Testcase (crashes browser) I'm not seeing a crash with this testcase in either 126.96.36.199 or 188.8.131.52. Is there a particular trick to it. I am testing XO.
(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
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
(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 184.108.40.206 or 220.127.116.11. Is > there a particular trick to it. I am testing XP. I still cannot reproduce the crash. I need some viable STR here.
Testcase crashes immediately for me in a 18.104.22.168 build (ran testcase from file://).
Peter, what operating system? I'm using Windows XP. If I open the attached testcase in 22.214.171.124 or 126.96.36.199, I do not get a crash.
OS X, but I used Windows XP when originally recording the crash so should work there too.
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:188.8.131.52pre) 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:184.108.40.206pre) Gecko/20100817 Shiretoko/3.5.12pre.
You need to log in before you can comment on or make changes to this bug.