337716 - Crash [@ js_GetSlotThreadSafe nsGetInterface::operator JS_GetParent] with testcase, using iframe/timers/alert

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

See upcoming testcase, that I'll attach.
You need to let it run at least 5 seconds or so, to let it build up multiple alert dialogs.
After waiting, click them away.
At this point, I crash often (50% of the time).

I get backtraces with different signatures, so that's why I'm filing it as security sensitive.

Some talkback ID's width different signatures:
TB18597168W
0xb82c0278
js_GetSlotThreadSafe   JS_GetParent   nsDOMClassInfo::WrapNative   nsWindowSH::SetProperty   XPC_WN_Helper_SetProperty   js_SetProperty   JS_SetUCProperty   nsWindowSH::SetProperty   XPC_WN_Helper_SetProperty   js_SetProperty   js_Interpret   0x0188ba40

TB18597034E
0x024a6ff4
nsGetInterface::operator()   nsCOMPtr_base::assign_from_helper   nsCOMPtr<nsIScriptGlobalObject>::nsCOMPtr<nsIScriptGlobalObject>   nsLocationSH::PreCreate   XPCWrappedNative::GetNewOrUsed   XPCConvert::NativeInterface2JSObject   nsXPConnect::WrapNative   nsDOMClassInfo::WrapNative   nsWindowSH::SetProperty   XPC_WN_Helper_SetProperty   js_SetProperty   JS_SetUCProperty   nsWindowSH::SetProperty   XPC_WN_Helper_SetProperty   js_SetProperty   js_Interpret   0x0188c9e8

TB18588913W
JS_GetParent   nsDOMClassInfo::WrapNative   nsWindowSH::SetProperty   XPC_WN_Helper_SetProperty   js_SetProperty   JS_SetUCProperty   nsWindowSH::SetProperty   XPC_WN_Helper_SetProperty   js_SetProperty   js_Interpret   0x02080760

This seems to have regressed between 2005-08-01 and 2005-08-02:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-01+06&maxdate=2005-08-02+08&cvsroot=%2Fcvsroot
I suspect bug 302889 might be the culprit.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I even asked jst who marks the inner window's JS object yesterday ;-).

The answer to my question is that the inner window's JS object is rooted through the outer window's mInnerWindowHolder. This is usually enough keep the inner window alive, but nsWindowSH::SetProperty first calls SetHref() (which gives the outer window a new inner window, unrooting the inner window's global JS object) and then tries to use the inner window's global JS object to wrap the location object. Martijn's alerts allow a GC to nest in between and we access the inner window's collected JS object and crash.

Unfortunately, when I swapped the two calls (so that I wrapped the location object before the call to SetHref) I hit a JS assertion.

Comment 3

[Johnny Stenback (:jst)]

Taking bug.

Comment 4

[Johnny Stenback (:jst)]

Blake's attempt to swap the order of changing the location and wrapping the location object does fix this problem and is IMO the right fix here. And now that the assertion is fixed (bug 364657), swapping the two calls is safe. Patch coming up.

Comment 5

[Johnny Stenback (:jst)]

Attachment: Fix per mrbkap's comment.

Comment 6

[Johnny Stenback (:jst)]

Fixed on the trunk.

Comment 7

[Jay Patel [:jay]]

Comment on attachment 250526 [details] [diff] [review]
Fix per mrbkap's comment.

Approved for both branches branches, a=jay for drivers.

Comment 8

[Johnny Stenback (:jst)]

Fixed on the branches.

Comment 9

[alice nodelman [:alice] [:anode]]

Testing with 2.0.0.2pre and the provided testcase still caused a crash.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2pre) Gecko/2007011103 BonEcho/2.0.0.2pre

Should the testcase no longer crash at all, or am I just looking for a 'secure' crash?

Comment 10

[Johnny Stenback (:jst)]

Hmm, the testcase should not crash at all. I wonder if there's other bugs that are causing a crash with the same testcase, maybe mac only? I've ran the testcase a *lot* with my fix and haven't seen a crash. Please re-test with another build and another platform too if you can and report back (and reopen the bug if it's still crashing for you). Oh, and talkback data would be useful too.

Comment 11

[Martijn Wargers (dead)]

This is fixed for me, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070111 BonEcho/2.0.0.2pre
and current trunk build.
I'm on windows, so perhaps a Mac specific crash?

Comment 12

[alice nodelman [:alice] [:anode]]

Attachment: mac crash report

Reproduced the crash on mac for 2.0.0.2pre.  It was triggered after alternating clicking on the 'okay' button on the alert and the 'back' arrow on the browser - it was the combination of these two actions that caused the crash.

Comment 13

[Martijn Wargers (dead)]

Ok, indeed, with the steps to reproduce in comment 12, I can too reproduce this crash on the latest branch build, talkback ID: TB28292905Y
Probably a new bug should be filed on that issue, so this bug can stay fixed then.

Comment 14

[alice nodelman [:alice] [:anode]]

Now I have another issue.  The testcase causes 1.5.0.10pre to hang.  After running the testcase, having the alert raised, waiting 5 seconds and then clicking the alert the browser becomes unresponsive.  Clicking on buttons has no effect - though I can command-Q to quit.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.10pre) Gecko/20070111 Firefox/1.5.0.10pre

Comment 15

[Martijn Wargers (dead)]

Ok, I have filed bug 367123 for the crash mentioned in commment 9 and further.
That crash is a different issue, because it has a different way of reproducing and has a different stacktrace.
The original bug is fixed on trunk and branches, this was verified by me and Alice.