606709 - Crash [@ XPCWrappedNative::GetWrappedNativeOfJSObject]

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires extension for GC)

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).

Comment 1

[Jesse Ruderman]

Attachment: crash report

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

b8+, maybe bN+ if needs be

Comment 3

[Jesse Ruderman]

Still happens on trunk.

Comment 4

[LegNeato]

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.

Comment 5

[Johnny Stenback (:jst)]

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.

Comment 6

[Johnny Stenback (:jst)]

Oh, and this crash does seem to go away if I disable the JITs.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 8

[Johnny Stenback (:jst)]

Attachment: Fix.

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.

Comment 9

[Johnny Stenback (:jst)]

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.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Different fix.

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.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 12

[Johnny Stenback (:jst)]

Landed, marking FIXED.

http://hg.mozilla.org/mozilla-central/rev/f1a5bea1d022