Closed Bug 337716 Opened 18 years ago Closed 18 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: jst)

References

Details

(5 keywords, Whiteboard: [sg:critical?] testcase reused for bug 367123)

Crash Data

Attachments

(2 files)

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.
Attached file testcase
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.
OS: Windows XP → All
Hardware: PC → All
Depends on: 52209
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Whiteboard: [sg:critical?]
Taking bug.
Assignee: general → jst
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
Depends on: 364657
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.
Status: NEW → ASSIGNED
Attachment #250526 - Flags: superreview?(bugmail)
Attachment #250526 - Flags: review?(mrbkap)
Attachment #250526 - Flags: review?(mrbkap) → review+
Attachment #250526 - Flags: superreview?(bugmail) → superreview+
Fixed on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #250526 - Flags: approval1.8.1.2?
Attachment #250526 - Flags: approval1.8.0.10?
Comment on attachment 250526 [details] [diff] [review]
Fix per mrbkap's comment.

Approved for both branches branches, a=jay for drivers.
Attachment #250526 - Flags: approval1.8.1.2?
Attachment #250526 - Flags: approval1.8.1.2+
Attachment #250526 - Flags: approval1.8.0.10?
Attachment #250526 - Flags: approval1.8.0.10+
Fixed on the branches.
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?

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.
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?
Attached file 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.
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.
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
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.
Status: RESOLVED → VERIFIED
Depends on: 367123
Whiteboard: [sg:critical?] → [sg:critical?] wait for bug 367123
Group: security
Whiteboard: [sg:critical?] wait for bug 367123 → [sg:critical?] testcase reused for bug 367123
Flags: in-testsuite?
Crash Signature: [@ js_GetSlotThreadSafe nsGetInterface::operator JS_GetParent]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: