977340 - Instrument assertSameCompartment to enforce cx stack invariants

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Use an AutoJSContext when clearing modules. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Do some gymnastics to avoid tripping cx assertions when cloning exceptions from evalInWindow calls. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Clean up cx usage in Promises. v1

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.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Assert during compartment checking that we're using the stack-top cx. v1

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8382561 [details] [diff] [review]
Part 3 - Clean up cx usage in Promises. v1

r=me

Comment 7

[Bobby Holley (:bholley)]

(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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 10

[Bobby Holley (:bholley)]

(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

[Luke Wagner [:luke]]

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?

Comment 12

[Gabor Krizsanits (INACTIVE)]

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.

Comment 13

[Gabor Krizsanits (INACTIVE)]

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.

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Bobby Holley (:bholley)]

all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=18bccc65547a

Comment 16

[Bobby Holley (:bholley)]

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

[Ryan VanderMeulen [:RyanVM]]

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