Closed
Bug 977340
Opened 11 years ago
Closed 11 years ago
Instrument assertSameCompartment to enforce cx stack invariants
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: bholley, Assigned: bholley)
Details
Attachments
(4 files)
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
Ever since bug 834732, we've asserted the following invariant:
Any piece of main-thread C++ code that grabs a JSContext from somewhere other than (a) an argument or (b) the top of the JSContext stack must make sure that JSContext is stack-top.
In other words, the stack-top JSContext is a property of Gecko and the JSRuntime, and the fact that we pass it around everywhere is just a historical artifact.
We occasionally make exceptions for cxes used only for rooting and whatnot, but in general this invariant holds, and can be relied on (i.e. by consumers that want to avoid passing a JSContext to callees and just let the callees use AutoJSContext). Indeed, relying on this invariant is actually helpful, because it reduces the amount of code we'll need to touch when we eventually stop using JSContexts entirely in Gecko.
We already enforce this invariant in the wrap callback, because it's the most-frequently-accessed hook from the JS engine. But Boris is skeptical that it holds everywhere, so I want to hook this to the compartment assertions, since those are the most pervasive checks we do in SpiderMonkey.
This will involve fiddling with a couple of corner cases, but none of them should be serious correctness issues.
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ffce0a5bb64a
One orange on the above, so repushing mochitest-3 debug:
https://tbpl.mozilla.org/?tree=Try&rev=f832588804ce
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8382559 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8382560 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 4•11 years ago
|
||
None of these are hazards, because we already make sure to push the JSContext
in the cases where we do anything meaningful in JSAPI. But the current setup
trips the new assertions, and is ugly to boot. Let's fix it.
Attachment #8382561 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8382562 -
Flags: review?(luke)
Attachment #8382562 -
Flags: review?(gkrizsanits)
Comment 6•11 years ago
|
||
Comment on attachment 8382561 [details] [diff] [review]
Part 3 - Clean up cx usage in Promises. v1
r=me
Attachment #8382561 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #1)
> https://tbpl.mozilla.org/?tree=Try&rev=ffce0a5bb64a
>
> One orange on the above, so repushing mochitest-3 debug:
>
> https://tbpl.mozilla.org/?tree=Try&rev=f832588804ce
For posterity: this was green.
Comment 8•11 years ago
|
||
Comment on attachment 8382559 [details] [diff] [review]
Part 1 - Use an AutoJSContext when clearing modules. v1
Review of attachment 8382559 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/xpconnect/loader/mozJSComponentLoader.h
@@ +115,5 @@
> void Clear() {
> getfactoryobj = nullptr;
>
> if (obj) {
> + mozilla::AutoJSContext cx;
What is up with the indentation?
Comment 9•11 years ago
|
||
Comment on attachment 8382562 [details] [diff] [review]
Part 4 - Assert during compartment checking that we're using the stack-top cx. v1
Review of attachment 8382562 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jscntxtinlines.h
@@ +35,5 @@
> + // was going to use.
> + JSContext *activeContext = nullptr;
> + if (cx->isJSContext())
> + activeContext = cx->asJSContext()->runtime()->activeContext;
> + JS_ASSERT_IF(activeContext, cx == activeContext);
Maybe
MOZ_ASSERT_IF(cx->isJSContext(),
cx == cx->asJSContext()->runtime()->activeContext);
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to :Ms2ger from comment #9)
> Maybe
>
> MOZ_ASSERT_IF(cx->isJSContext(),
> cx == cx->asJSContext()->runtime()->activeContext);
No, because we only want to do the assert in the case that activeContext is non-null. So inlining the predicate would involve chasing down activeContext twice.
Comment 11•11 years ago
|
||
Comment on attachment 8382562 [details] [diff] [review]
Part 4 - Assert during compartment checking that we're using the stack-top cx. v1
Review of attachment 8382562 [details] [diff] [review]:
-----------------------------------------------------------------
Cool!
::: js/src/jsfriendapi.h
@@ +1963,5 @@
> + * duty (in debug builds) to verify that it matches the cx being used.
> + */
> +#ifdef DEBUG
> +JS_FRIEND_API(void)
> +SetActiveJSContext(JSRuntime *rt, JSContext *cx);
Could you name this Debug_SetActiveJSContext?
Attachment #8382562 -
Flags: review?(luke) → review+
Comment 12•11 years ago
|
||
Comment on attachment 8382559 [details] [diff] [review]
Part 1 - Use an AutoJSContext when clearing modules. v1
Review of attachment 8382559 [details] [diff] [review]:
-----------------------------------------------------------------
I assume you fixed the indentation already. And sorry for the lag on these reviews.
Attachment #8382559 -
Flags: review?(gkrizsanits) → review+
Updated•11 years ago
|
Attachment #8382560 -
Flags: review?(gkrizsanits) → review+
Comment 13•11 years ago
|
||
Comment on attachment 8382562 [details] [diff] [review]
Part 4 - Assert during compartment checking that we're using the stack-top cx. v1
Review of attachment 8382562 [details] [diff] [review]:
-----------------------------------------------------------------
You could also just use a macro for calling (Debug_)SetActiveJSContext and setting that macro to empty for the non-debug build. Then you don't have to define a noop function for the non-debug builds. But I'm not very pushy about this since I'm sure that any half decent compiler optimizes out that noop function anyway, so I leave it up to you.
Attachment #8382562 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> You could also just use a macro for calling (Debug_)SetActiveJSContext and
> setting that macro to empty for the non-debug build. Then you don't have to
> define a noop function for the non-debug builds. But I'm not very pushy
> about this since I'm sure that any half decent compiler optimizes out that
> noop function anyway, so I leave it up to you.
Yeah. Doing it as a macro can cause weirdness (as I discovered with assertEnteredPolicy in bug 836301). The compiler will definitely optimize out the empty inlined method. :-)
Assignee | ||
Comment 15•11 years ago
|
||
all-platform try push:
https://tbpl.mozilla.org/?tree=Try&rev=18bccc65547a
Assignee | ||
Comment 16•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/681a088dd2b8
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8c8d1b869b
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/219a096d2d1e
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb71a24360b
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/681a088dd2b8
https://hg.mozilla.org/mozilla-central/rev/3c8c8d1b869b
https://hg.mozilla.org/mozilla-central/rev/219a096d2d1e
https://hg.mozilla.org/mozilla-central/rev/8eb71a24360b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•