Closed
Bug 951847
Opened 11 years ago
Closed 11 years ago
Stop pushing a cx for each XPCCallContext
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: baku, Assigned: bholley)
References
Details
(Whiteboard: [qa-])
Attachments
(4 files)
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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
Assignee | ||
Comment 2•11 years ago
|
||
I'm pretty sure we don't need to be pushing during XPCCallContext construction anymore.
https://tbpl.mozilla.org/?tree=Try&rev=624ac1cb62e3
Assignee | ||
Comment 3•11 years ago
|
||
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
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8350206 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8350207 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #8350208 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8350209 -
Flags: review?(gkrizsanits)
Comment 8•11 years ago
|
||
(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.
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #8350206 -
Flags: review?(gkrizsanits) → review+
Comment 10•11 years ago
|
||
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...)
Attachment #8350207 -
Flags: review?(gkrizsanits) → review+
Updated•11 years ago
|
Attachment #8350208 -
Flags: review?(gkrizsanits) → review+
Updated•11 years ago
|
Attachment #8350209 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 11•11 years ago
|
||
Thanks for the reviews :-)
https://tbpl.mozilla.org/?tree=Try&rev=68a12b91f6a7
Assignee | ||
Updated•11 years ago
|
Summary: XPC_WN_NoHelper_Resolve resets the cx compartment to the default one → Stop pushing a cx for each XPCCallContext
Assignee | ||
Comment 12•11 years ago
|
||
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•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•11 years ago
|
Whiteboard: [qa-]
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•