1464772 - Make some RealmOptions APIs take a realm instead of compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Make JS_GetIsSecureContext take a realm instead of compartment and move to JS namespace

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove RealmBehaviorsRef(obj), change RealmBehaviorsRef(compartment) to take a realm

At some point I'll audit the obj->realm() calls for CCWs; removing RealmBehaviorsRef(obj) prepares for that.

Also, RealmBehaviorsRef(realm) could be removed and we could use RealmBehaviorsRef(cx) everywhere, but it seems reasonable to keep it.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Remove unused RealmCreationOptionsRef(obj)

Removes an obj->realm() call, similar to the previous patch.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Use cx->realm()->creationOptions() instead of RealmCreationOptionsRef in NowAsMillis

Inside the JS engine there's no need to call the non-inlined RealmCreationOptionsRef API.

Comment 5

[Jan de Mooij [:jandem]]

At this point, the only caller of RealmCreationOptionsRef(compartment) is IsSecureContextOrObjectIsFromSecureContext [0]. (Note that we could also use JS::GetIsSecureContext(realm) here, see part 1, but that doesn't really matter.)

The first call there can just s/GetContextCompartment/GetContextRealm/. The second one is harder because that wants to know the object's realm, so we need to know if the object can be a CCW.

We have the following callers of IsSecureContextOrObjectIsFromSecureContext:

(1) ConstructorEnabled in generated bindings. ConstructorEnabled has the following callers:
    (a) WebIDLGlobalNameHash::GetNames passes a Window, so it should be fine to get its realm.
    (b) WebIDLGlobalNameHash::DefineIfEnabled does a CheckedUnwrap and then also passes the global object IIUC.

(2) nsGlobalWindowInner::OfflineCacheAllowedForContext. Caller is ConstructorEnabled, similar to (1).

(3) nsGlobalWindowInner::RegisterProtocolHandlerAllowedForContext. This is stored in PrefableDisablers, see (4).

(4) PrefableDisablers::isEnabled. This is called by Prefable::isEnabled, and *that* has about 7 callers [1] and I'm not sure if they can pass wrappers.

[0] https://searchfox.org/mozilla-central/rev/b47af5169898b156429c8cdaf415afdaa3c43a7b/dom/bindings/DOMJSClass.h#61

[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZNK7mozilla3dom8Prefable9isEnabledEP9JSContextN2JS6HandleIP8JSObjectEE&redirect=false

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> (2) nsGlobalWindowInner::OfflineCacheAllowedForContext. Caller is
> ConstructorEnabled, similar to (1).

Oh and this one is also stored in PrefableDisablers, similar to (3).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8981088 [details] [diff] [review]
Part 1 - Make JS_GetIsSecureContext take a realm instead of compartment and move to JS namespace

r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

> (2) nsGlobalWindowInner::OfflineCacheAllowedForContext. Caller is ConstructorEnabled, similar to (1).

It's a little more complicated than that.  This function can be called in two ways: from ConstructorEnabled when looking up the "OfflineResourceList" property on a window and from PrefableDisablers::isEnabled when looking up the "applicationCache" property.  This second case falls into (4) from comment 5.

> (4) PrefableDisablers::isEnabled. This is called by Prefable::isEnabled, and *that* has about 7 callers [1] and I'm not sure if they can pass wrappers.

Right.  So looking at those callers:

1)  DefinePrefable is called during DOM prototype/constructor setup.  The argument is guaranteed to be a DOM prototype, constructor, or instance (in the case of the global) object, 1465602not a wrapper.  We could add a MOZ_ASSERT that the incoming object's JSClass tests true for either IsDOMClass() or IsDOMIfaceAndProtoClass().

2) XrayResolveAttribute has the Xray as "wrapper" and the Xray target as "obj".  The latter is never a CCW.

3) XrayResolveMethod, same.

4) XrayResolveConstant, same.

5, 6) XrayAppendPropertyKeys is called from XrayOwnPropertyKeys which has the
      same setup and is passing "obj" to XrayAppendPropertyKeys.

7) IsGetterEnabled is ... buggy!  We don't have great tests for it, so I only tested it by code-inspection.  But it won't do the right thing in some cases, precisely because it _can_ pass a cross-compartment wrapper.  I filed bug 1465602 on this.

Comment 9

[Jan de Mooij [:jandem]]

Thanks Boris, that helps a lot. It looks like we can go ahead with this after your patch lands.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)
> This second case falls into (4) from comment 5.

See comment 6, FWIW.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> We could add a MOZ_ASSERT that the incoming object's JSClass tests true for either IsDOMClass() or IsDOMIfaceAndProtoClass()

Actually, it could be a plain object too (for the Document case, say).  We can definitely assert !js::IsWrapper(), though.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26cbc312b5cb
part 1 - Make JS_GetIsSecureContext take a realm instead of compartment and move to JS namespace. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/f58f5333e375
part 2 - Remove RealmBehaviorsRef(obj), change RealmBehaviorsRef(compartment) to take a realm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3a6af0b27ec
part 3 - Remove unused RealmCreationOptionsRef(obj). r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4187e4b06ee
part 4 - Use cx->realm()->creationOptions() instead of RealmCreationOptionsRef in NowAsMillis. r=luke

Comment 12

[Jan de Mooij [:jandem]]

Still needs a part 5 for RealmCreationOptionsRef(compartment) that we can fix now.

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/26cbc312b5cb
https://hg.mozilla.org/mozilla-central/rev/f58f5333e375
https://hg.mozilla.org/mozilla-central/rev/e3a6af0b27ec
https://hg.mozilla.org/mozilla-central/rev/a4187e4b06ee

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Use GetIsSecureContext instead of RealmCreationOptionsRef in IsSecureContextOrObjectIsFromSecureContext

Also changes RealmCreationOptionsRef to take a realm instead of compartment (this function is now unused but still potentially useful).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8982976 [details] [diff] [review]
Part 5 - Use GetIsSecureContext instead of RealmCreationOptionsRef in IsSecureContextOrObjectIsFromSecureContext

r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11aacf3b7e93
part 5 - Use GetIsSecureContext instead of RealmCreationOptionsRef in IsSecureContextOrObjectIsFromSecureContext. r=bz

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/11aacf3b7e93