951847 - Stop pushing a cx for each XPCCallContext

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

What I see is that my 'obj' is a XPConnect object. Calling JS_CallFunctionName (with cx and obj in the same compartment) we land to XPC_WN_NoHelper_Resolve, which enters the default compartment of cx and then tries to operate on obj.

I'm assigning this bug to bholley as bz suggested.

Comment 1

[Boris Zbarsky [:bzbarsky]]

More precisely, Andrea is using JS_CallFunctionName to call some method on a chrome JS object.  This then runs some script that tries to do something on some XPConnect object and lands in XPC_WN_NoHelper_Resolve.  This constructs the ccx, which pushes the cx and in the process changes its compartment.  And then it does this:

#0  0x00007ffff280d384 in js::CompartmentChecker::fail (c1=0x2f65b00, c2=0x2e232d0) at /home/baku/Sources/m/console/src/js/src/jscntxtinlines.h:40
#1  0x00007ffff280d418 in js::CompartmentChecker::check (this=0x7fffffffa530, c=0x2e232d0) at /home/baku/Sources/m/console/src/js/src/jscntxtinlines.h:61
#2  0x00007ffff280d44f in js::CompartmentChecker::check (this=0x7fffffffa530, obj=0x7fff4fa60670) at /home/baku/Sources/m/console/src/js/src/jscntxtinlines.h:72
#3  0x00007ffff28165b9 in js::assertSameCompartment<JS::Rooted<JSObject*> > (cx=0x24b6170, t1=...) at /home/baku/Sources/m/console/src/js/src/jscntxtinlines.h:147
#4  0x00007ffff2b79c4c in js::NewFunctionByIdWithReserved (cx=0x24b6170, native=0x7ffff0505de8 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, nargs=0, flags=0, parentArg=0x7fff4fa60670, id=...)
    at /home/baku/Sources/m/console/src/js/src/jsfriendapi.cpp:516
#5  0x00007ffff04fde91 in XPCNativeMember::Resolve (this=0x258fb48, ccx=..., iface=0x258fb20, parent=..., vp=0x7fffffffa8d0) at /home/baku/Sources/m/console/src/js/xpconnect/src/XPCWrappedNativeInfo.cpp:98
#6  0x00007ffff04fdb48 in XPCNativeMember::NewFunctionObject (this=0x258fb48, ccx=..., iface=0x258fb20, parent=..., pval=0x7fffffffa8d0)
    at /home/baku/Sources/m/console/src/js/xpconnect/src/XPCWrappedNativeInfo.cpp:46
#7  0x00007ffff05023a2 in DefinePropertyIfFound (ccx=..., obj=..., idArg=..., set=0x23244a0, iface=0x258fb20, member=0x258fb48, scope=0x30baa60, reflectToStringAndToSource=true, 
    wrapperToReflectInterfaceNames=0x290dd50, wrapperToReflectDoubleWrap=0x290dd50, scriptableInfo=0x0, propFlags=7, resolved=0x0)
    at /home/baku/Sources/m/console/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:386
#8  0x00007ffff050338b in XPC_WN_NoHelper_Resolve (cx=0x24b6170, obj=..., id=...) at /home/baku/Sources/m/console/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:638

Comment 2

[Bobby Holley (:bholley)]

I'm pretty sure we don't need to be pushing during XPCCallContext construction anymore.

https://tbpl.mozilla.org/?tree=Try&rev=624ac1cb62e3

Comment 3

[Bobby Holley (:bholley)]

Opt looks green. Debug failed during build for stupid reasons - repushing that, though I _think_ that compartment mismatches should be detected on opt builds as well:

https://tbpl.mozilla.org/?tree=Try&rev=70207e2ddd5f

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Push a cx in TestShellParent. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Don't construct an XPCCallContext just to find a JSContext. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make the |cx| param to XPCCallContext mandatory. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Stop pushing a cx in the XPCCallContext constructor. v2

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #5)
> Created attachment 8350207 [details] [diff] [review]
> Part 2 - Don't construct an XPCCallContext just to find a JSContext. v1

-    return XPCCallContext::GetDefaultJSContext();
+    // Fall back to the safe JSContext.
+    return stack->GetSafeJSContext();

This seems wrong to me. The original one returned the cx from the stack and if there weren't any fall back to the safe one, and you seem like ignoring the cx's on the stack and fall back to the safe one right away. I think this would affect JS stack tracing, but can be wrong... Could you elaborate on this change? I might be missing something but I can also imagine that we just lack of tests coverage in this area.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> (In reply to Bobby Holley (:bholley) from comment #5)
> > Created attachment 8350207 [details] [diff] [review]
> > Part 2 - Don't construct an XPCCallContext just to find a JSContext. v1
> 
> -    return XPCCallContext::GetDefaultJSContext();
> +    // Fall back to the safe JSContext.
> +    return stack->GetSafeJSContext();
> 
> This seems wrong to me. The original one returned the cx from the stack and
> if there weren't any fall back to the safe one, and you seem like ignoring
> the cx's on the stack and fall back to the safe one right away. I think this
> would affect JS stack tracing, but can be wrong... Could you elaborate on
> this change? I might be missing something but I can also imagine that we
> just lack of tests coverage in this area.

We only hit this line if the cx stack is empty - See the "// First, try the cx stack." part at the top of the function.

Comment 10

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8350207 [details] [diff] [review]
Part 2 - Don't construct an XPCCallContext just to find a JSContext. v1

Review of attachment 8350207 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bobby Holley (:bholley) from comment #9)
> We only hit this line if the cx stack is empty - See the "// First, try the
> cx stack." part at the top of the function.

Ah right, I lost the context while I compared those two lines I guess... That null check for the stack at the top is kind of meaningless as well right?
If so we should remove it (or turn it into an assert...)

Comment 11

[Bobby Holley (:bholley)]

Thanks for the reviews :-)

https://tbpl.mozilla.org/?tree=Try&rev=68a12b91f6a7

Comment 12

[Bobby Holley (:bholley)]

Green.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/71a771fd043b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ae54dffda253
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/54c69b807cb1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/53d78589f349

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71a771fd043b
https://hg.mozilla.org/mozilla-central/rev/ae54dffda253
https://hg.mozilla.org/mozilla-central/rev/54c69b807cb1
https://hg.mozilla.org/mozilla-central/rev/53d78589f349