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

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

12.07 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
23.38 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.27 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.99 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.76 KB, patch
luke
: review+
Details | Diff | Splinter Review
2.38 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.54 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.08 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.43 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Assignee

Description

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

Comment 1

a year ago
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.
Attachment #8980557 - Flags: review?(bzbarsky)
Assignee

Comment 2

a year ago
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.
Assignee

Comment 3

a year ago
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.
Attachment #8980694 - Flags: review?(luke)
Attachment #8980694 - Flags: review?(bzbarsky)
Attachment #8980694 - Flags: review?(luke) → review+
Comment on attachment 8980557 [details] [diff] [review]
Part 1 - Some minor changes

r=me with a commit message added....
Attachment #8980557 - Flags: review?(bzbarsky) → review+
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.
Attachment #8980694 - Flags: review?(bzbarsky) → review+
Assignee

Comment 6

a year ago
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..)
Assignee

Comment 7

a year ago
Now with the suggested fix and a test that asserted without it.
Attachment #8980694 - Attachment is obsolete: true
Attachment #8980976 - Flags: review?(bzbarsky)
Assignee

Comment 8

a year ago
(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.
Assignee

Comment 11

a year ago
I verified we call these with cx->global() as argument.
Attachment #8981073 - Flags: review?(luke)
Assignee

Comment 13

a year ago
This will become GetRealmName at some point.
Attachment #8981075 - Flags: review?(luke)
Assignee

Comment 16

a year ago
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.
Summary: Use JS::Realm more in Gecko → Use JS::Realm more in Gecko for JSPrincipals*-related APIs
Attachment #8981073 - Flags: review?(luke) → review+
Attachment #8981075 - Flags: review?(luke) → review+
Priority: -- → P2
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
Attachment #8980976 - Flags: review?(bzbarsky) → review+
Comment on attachment 8981069 [details] [diff] [review]
Part 3 - Use GetRealmPrincipals in generated bindings to get subject principal

r=me
Attachment #8981069 - Flags: review?(bzbarsky) → review+
Comment on attachment 8981070 [details] [diff] [review]
Part 4 - Remove unused nsScriptSecurityManager::doGetObjectPrincipal

This is unused?  That's so awesome.  :)
Attachment #8981070 - Flags: review?(bzbarsky) → review+
Comment on attachment 8981074 [details] [diff] [review]
Part 6 - Use GetRealmPrincipals in Scriptability constructor

r=me
Attachment #8981074 - Flags: review?(bzbarsky) → review+
Comment on attachment 8981076 [details] [diff] [review]
Part 8 - Use GetRealmPrincipals in nsHTMLDocument::Open assertion

r=me
Attachment #8981076 - Flags: review?(bzbarsky) → review+
Comment on attachment 8981077 [details] [diff] [review]
Part 9 - Turn JS_SetCompartmentPrincipals into JS::SetRealmPrincipals

r=me
Attachment #8981077 - Flags: review?(bzbarsky) → review+
Assignee

Updated

a year ago
See Also: → 1465700

Comment 24

a year ago
Pushed by jandemooij@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.