Closed Bug 606709 Opened 10 years ago Closed 10 years ago

Crash [@ XPCWrappedNative::GetWrappedNativeOfJSObject]

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jruderman, Assigned: jst)

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical?][compartments])

Crash Data

Attachments

(3 files, 1 obsolete file)

1. Temporarily install 'DOM Fuzz Lite' from
    https://www.squarefree.com/extensions/domFuzzLite.xpi
2. Load the testcase.

Result: Crash [@ XPCWrappedNative::GetWrappedNativeOfJSObject] with reason KERN_INVALID_ADDRESS (0xffffffffdadadb1e).
Attached file crash report
blocking2.0: --- → ?
b8+, maybe bN+ if needs be
blocking2.0: ? → beta8+
Whiteboard: [sg:critical?] → [sg:critical?][compartments]
Assignee: nobody → gal
Still happens on trunk.
This doesn't look to meet the standard of a beta8 blocker. Punting to betaN, feel free to disagree and/or triage to a different beta.
blocking2.0: beta8+ → betaN+
I was able to reproduce this locally and got dvander to help look into this a bit. While it's hard to tell for sure, this looks like it's due to the ".call" part of the line frameDoc.hasAttributes.call(null) that we crash, if we make that line simply be frameDoc.hasAttributes() we were unable to cause the crash. Cc:ing dvander as he wanted to look more into some .call() related optimizations that they've done in the JS engine lately, which could be the cause of this.
Oh, and this crash does seem to go away if I disable the JITs.
A little investigation: the DOM getter is calling JS_THIS(). vp[1] is null, so it goes into global computation (vp[0]->getGlobal()->thisObject(cx)). This calls into XPC's this hook which gets the outer object, I think.

So something relating to that is returning the dead reference. Not clear how the JIT is affecting this yet.
Attached patch Fix. (obsolete) — Splinter Review
dvander told me later on today that he traced down where the dead object comes from and it came from the this object hook, which in this case ends up calling the outer object hook, which finds a dead outer object. This patch adds a trace hook to all windows and in that hook we make sure we trace the outer window, which ensures that an inner can never outlive its outer window.
Attachment #494326 - Flags: review?(mrbkap)
Actually, that patch isn't quite right, the GetOuterWindowInternal() call never returns null, this code should probably check whether the window is an inner, and deal with the fact that GetOuterWindowInternal() returns this if there is no outer... But that's a minor detail, I'd still like feedback on whether adding this trace hook is the best approach here.
Attached patch Different fix.Splinter Review
Talked this over with mrbkap and he'd prefer not to add a trace hook, which I agree with. What typically holds the outer window alive is the "window" property on the inner, and the reason that doesn't save us in this case is that here we've already cleared the inner scope, which drops the "window" property. This patch preserves the "window" property when clearing the inner, and thus fixes this crash.
Attachment #494809 - Flags: review?(mrbkap)
Attachment #494326 - Attachment is obsolete: true
Attachment #494326 - Flags: review?(mrbkap)
Comment on attachment 494809 [details] [diff] [review]
Different fix.

>+    if (!JS_GetProperty(mContext, obj, "window", &window)) {
>+      window = JSVAL_VOID;

Add a JS_ClearPendingException(mContext) here.

>+    if (window != JSVAL_VOID) {
>+      JS_DefineProperty(mContext, obj, "window", window,
>+                        JS_PropertyStub, JS_PropertyStub,
>+                        JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);

Same here, if JS_DefineProperty fails.
Attachment #494809 - Flags: review?(mrbkap) → review+
Assignee: gal → jst
Landed, marking FIXED.

http://hg.mozilla.org/mozilla-central/rev/f1a5bea1d022
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Crash Signature: [@ XPCWrappedNative::GetWrappedNativeOfJSObject]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.