1464374 - Use JS::Realm more in Gecko for JSPrincipals*-related APIs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

Some APIs do a JSCompartment* to JS::Realm* cast and we need to stop using those APIs.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Some minor changes

Some minor things:

* Renames SetCompartmentValidAccessPtr to SetRealmValidAccessPtr and asserts |global| is indeed a global object.

* Adds JS::GetRealmPrincipals. JS_GetCompartmentPrincipals will need to go away eventually.

* Added js::GetContextRealm in addition to js::GetContextCompartment.

* Then I used these APIs for a few of the simplest cases.

Comment 2

[Jan de Mooij [:jandem]]

Oh and I added JS::GetRealmPrincipals to jsfriendapi because JS_GetCompartmentPrincipals is also defined there.

I could also move it to jsapi or public/Realm.h but it doesn't matter too much I think. I want to do some cleanup of these APIs anyway after the dust settles.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Use JS::Realm in CallSetup, bindings

We currently call js::GetObjectCompartment in a few places and then pass this compartment to CallSetup. In ShouldRethrowException we then get the compartment's principals, so we really want CallSetup to have a realm instead of a compartment.

We could add js::GetObjectRealm, but we don't want this to be called for CCWs, so this patch adds js::GetNonCCWObjectRealm which asserts !IsCrossCompartmentWrapper in debug builds (suggestions for better names etc welcome).

bz, please double check these objects really can't be CCWs.
* One of the calls is immediately after an unwrap call (in ShouldRethrowException).
* Another is always on a global, IIUC (scopeObj).
* I'm a bit less sure about the one in Codegen.py where we check unwrappedObj because we check for Xrays there but I don't know the invariants of that code.

This is green on Try.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8980557 [details] [diff] [review]
Part 1 - Some minor changes

r=me with a commit message added....

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8980694 [details] [diff] [review]
Part 2 - Use JS::Realm in CallSetup, bindings

> CallbackObject::CallSetup::ShouldRethrowException(JS::Handle<JS::Value> aException)
>     // At this point mCx is in the compartment of our unwrapped callback, so

s/compartment/realm/

>+++ b/dom/bindings/Codegen.py
>+                argsPost.append("js::GetNonCCWObjectRealm(unwrappedObj ? *unwrappedObj : obj)")

Here we could be passing a wrapper to GetNonCCWObjectRealm.

Specifically, this can happen when we're doing a call to a JS-implemented webidl API (which is the only case when this line is reached), and the "this" value is a cross-compartment wrapper.  Something like this should probably reproduce:

  <iframe></iframe>
  <script>
    var obj = new frames[0].RTCPeerConnection();
    RTCPeerConnection.prototype.getIdentityAssertion.call(obj);
  </script>

We should add a test to this effect using our test js-implemented interface.  See the interface definition in dom/webidl/TestInterfaceJS.webidl and implementation in dom/bindings/test/TestInterfaceJS.js.  We can probably use one of the existing methods on it; just need to add a debug-only test.

What we can probably do is rewrite this code like so:

  JS::Realm* realm = unwrappedObj ? js::GetNonCCWObjectRealm(unwrappedObj) : js::GetContextRealm(cx);

I _think_ at this point obj and cx are always same-compartment right now... and more importantly our caller is who will be getting the exception and that's in the realm of cx here, I am pretty sure.

>+GetNonCCWObjectRealm(JSObject* obj)

Another option is GetNonWrapperRealm.  But I agree that the naming is hard here.

Comment 6

[Jan de Mooij [:jandem]]

Thanks for the reviews. I'll add a test later and request another review (and add a commit message, I usually do that before committing but I really need to change that..)

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Pass JS::Realm* instead of JSCompartment* to CallSetup

Now with the suggested fix and a test that asserted without it.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> >+GetNonCCWObjectRealm(JSObject* obj)
> 
> Another option is GetNonWrapperRealm.

Yeah I considered that one, but we have wrappers that are not CCWs and these are still fine I think, so that's a bit annoying.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Use GetRealmPrincipals in generated bindings to get subject principal

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Remove unused nsScriptSecurityManager::doGetObjectPrincipal

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Use GetRealmPrincipals in AsmJSCacheOpenEntryFor{Read,Write}

I verified we call these with cx->global() as argument.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Use GetRealmPrincipals in Scriptability constructor

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Use GetRealmPrincipals in GetCompartmentName

This will become GetRealmName at some point.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Use GetRealmPrincipals in nsHTMLDocument::Open assertion

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Part 9 - Turn JS_SetCompartmentPrincipals into JS::SetRealmPrincipals

Comment 16

[Jan de Mooij [:jandem]]

These patches eliminate most of the calls to JS_GetCompartmentPrincipals. The remaining ones are harder because they are for things like RecomputeWrappers (called by ContentPrincipal::SetDomain) and that's at the core of the wrapper machinery we'll have to change.

Boris, I think it's probably best if I file a follow-up bug to fix the remaining JS_GetCompartmentPrincipals calls, and then you or bholley can fix these as part of the Gecko changes.

When we split JSCompartment and JS::Realm in completely separate classes, I'll be able to work around it by release-asserting in JS_GetCompartmentPrincipals that a compartment has 1 realm and then we can just get that realm's principals. For now.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment 16 makes sense to me.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8980976 [details] [diff] [review]
Part 2 - Pass JS::Realm* instead of JSCompartment* to CallSetup

>+++ b/dom/bindings/test/test_jsimplemented_cross_realm_this.html
>+  <iframe></iframe>

Putting this inside the <head> is a bit odd.  What you probably want to do is put this, and the <script> tag, into the <body>.

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8981069 [details] [diff] [review]
Part 3 - Use GetRealmPrincipals in generated bindings to get subject principal

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8981070 [details] [diff] [review]
Part 4 - Remove unused nsScriptSecurityManager::doGetObjectPrincipal

This is unused?  That's so awesome.  :)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8981074 [details] [diff] [review]
Part 6 - Use GetRealmPrincipals in Scriptability constructor

r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8981076 [details] [diff] [review]
Part 8 - Use GetRealmPrincipals in nsHTMLDocument::Open assertion

r=me

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8981077 [details] [diff] [review]
Part 9 - Turn JS_SetCompartmentPrincipals into JS::SetRealmPrincipals

r=me

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2b90c69db5d
part 2 - Pass JS::Realm* instead of JSCompartment* to CallSetup. r=bz,luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/0eed247fcc7b
part 3 - Use GetRealmPrincipals in generated bindings to get subject principal. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c84746c20d3
part 4 - Remove unused nsScriptSecurityManager::doGetObjectPrincipal. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/98f31ca33d2a
part 5 - Use GetRealmPrincipals in AsmJSCacheOpenEntryFor{Read,Write}. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/f520a79d0bc1
part 6 - Use GetRealmPrincipals in Scriptability constructor. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/99519e97491a
part 7 - Use GetRealmPrincipals in GetCompartmentName. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d2a0e01533
part 8 - Use GetRealmPrincipals in nsHTMLDocument::Open assertion. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6e49ba2e89
part 9 - Turn JS_SetCompartmentPrincipals into JS::SetRealmPrincipals. r=bz

Comment 25

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e2b90c69db5d
https://hg.mozilla.org/mozilla-central/rev/0eed247fcc7b
https://hg.mozilla.org/mozilla-central/rev/9c84746c20d3
https://hg.mozilla.org/mozilla-central/rev/98f31ca33d2a
https://hg.mozilla.org/mozilla-central/rev/f520a79d0bc1
https://hg.mozilla.org/mozilla-central/rev/99519e97491a
https://hg.mozilla.org/mozilla-central/rev/70d2a0e01533
https://hg.mozilla.org/mozilla-central/rev/ab6e49ba2e89