Closed Bug 977340 Opened 6 years ago Closed 6 years ago

Instrument assertSameCompartment to enforce cx stack invariants

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bholley, Assigned: bholley)

Details

Attachments

(4 files)

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.
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)
Comment on attachment 8382561 [details] [diff] [review]
Part 3 - Clean up cx usage in Promises. v1

r=me
Attachment #8382561 - Flags: review?(bzbarsky) → review+
(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 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 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);
(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 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 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+
Attachment #8382560 - Flags: review?(gkrizsanits) → review+
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+
(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. :-)
You need to log in before you can comment on or make changes to this bug.