822425 - Expose assertSameCompartment outside the JS engine

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

It would be nice if we had a friend API for this, so XPConnect can use it instead of making ad hoc compartment checking assertions. Probably we only need object and maybe string.

Then I can file a followup to actually use it.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Expose a few simple compartment assertions in jsfriendapi. v1

Only the first one will benefit from our Nightly-mode compartment assertions,
but both are useful for consistency.

Comment 2

[Andrew McCreight [:mccr8]]

In the header file, the one with a context shouldn't be turned into an empty function in a non-debug build, or we won't get release mode assertions.  In the cpp file, you should guard the object-object one inside #ifdef DEBUG.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> In the header file, the one with a context shouldn't be turned into an empty
> function in a non-debug build, or we won't get release mode assertions.  In
> the cpp file, you should guard the object-object one inside #ifdef DEBUG.

Oh hm, that's a good point. It's a shame that this means that we'll have a performance penalty in release builds regardless, but it's probably negligible. And anyway, the once we fold mozjs.dll into libxul on windows, LTO should take care of it.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Expose a few simple compartment assertions in jsfriendapi. v2

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 814898 [details] [diff] [review]
Expose a few simple compartment assertions in jsfriendapi. v2

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

::: js/src/jsfriendapi.h
@@ +483,5 @@
>  JS_FRIEND_API(JSObject *)
>  GetGlobalForObjectCrossCompartment(JSObject *obj);
>  
> +JS_FRIEND_API(void)
> +AssertSameCompartment(JSContext *cx, JSObject *obj);

If you wanted to avoid the call overhead in release builds, you could wrap this in something like |#ifdef JS_CRASH_DIAGNOSTICS or DEBUG|, otherwise define it as empty.  That's what the JS engine uses, and maybe the CRASH flag is defined for browser code, too?  If you do that, it would be good to check.

@@ +487,5 @@
> +AssertSameCompartment(JSContext *cx, JSObject *obj);
> +
> +#ifdef DEBUG
> +JS_FRIEND_API(void)
> +AssertSameCompartment(JSObject *objA, JSObject *objB);

I'll assume you have a use-case for this form.  I don't think the JS engine has any variants without a cx involved. Instead, it has (cx, obj, obj) etc.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3eb51e31e094

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/264163f1b1bc

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7911cdea0b9f
https://hg.mozilla.org/releases/mozilla-esr24/rev/8f19bbb77f47