Make sure js::GetGlobalForObjectCrossCompartment is not called on CCWs

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(16 attachments)

1.30 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.22 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.32 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
992 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.77 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.33 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.04 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.10 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
978 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.05 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
915 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.49 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.29 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.25 KB, patch
luke
: review+
Details | Diff | Splinter Review
There are about 45 callers to audit/change here. Quite a lot, but fortunately most of them are in the "not a CCW" bucket. The plan (as before) is to introduce a new API that asserts non-CCW and then convert callers incrementally.
Depends on: 1473492
js::GetJSMEnvironmentOfScriptedCaller returns either nullptr or a NonSyntacticVariablesObject.
Attachment #8989982 - Flags: review?(bzbarsky)
FWIW, at this point there are about 20 GetGlobalForObjectCrossCompartment callers left.
Comment on attachment 8989946 [details] [diff] [review]
Part 1 - Use JS::GetNonCCWObjectGlobal in Codegen.py

r=me
Attachment #8989946 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989947 [details] [diff] [review]
Part 2 - Use JS::GetNonCCWObjectGlobal in some functions where we unwrapped the object

r=me
Attachment #8989947 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989949 [details] [diff] [review]
Part 3 - Remove some GetGlobalForObjectCrossCompartment calls on globals/WindowProxy

>@@ -1174,25 +1174,16 @@ XPCOMObjectToJsval(JSContext* cx, JS::Ha

Why is the assert here being removed?

r=me with this assert left in.
Attachment #8989949 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989967 [details] [diff] [review]
Part 4 - Use JS::GetNonCCWObjectGlobal in PrefableDisablers::isEnabled

r=me
Attachment #8989967 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989978 [details] [diff] [review]
Part 5 - Use JS::CurrentGlobalOrNull in JS shell clone() function

r=me
Attachment #8989978 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989980 [details] [diff] [review]
Part 6 - Use JS::GetNonCCWObjectGlobal in subscript loader

r=me
Attachment #8989980 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989982 [details] [diff] [review]
Part 7 - Use JS::GetNonCCWObjectGlobal in mozJSComponentLoader::FindTargetObject

r=me
Attachment #8989982 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989989 [details] [diff] [review]
Part 8 - Use JS::GetNonCCWObjectGlobal in XrayAwareCalleeGlobal

r=me
Attachment #8989989 - Flags: review?(bzbarsky) → review+
Comment on attachment 8989992 [details] [diff] [review]
Part 9 - Use JS::GetNonCCWObjectGlobal in worker wrap-callback

r=me
Attachment #8989992 - Flags: review?(bzbarsky) → review+
Thanks for all the reviews!

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> >@@ -1174,25 +1174,16 @@ XPCOMObjectToJsval(JSContext* cx, JS::Ha
> 
> Why is the assert here being removed?
> 
> r=me with this assert left in.

What do you want this assert to look like? If I change:

    js::GetGlobalForObjectCrossCompartment(jsobj) == jsobj

to:

    JS_IsGlobalObject(jsobj)

Then the assert in the then-branch is always true because this is exactly the definition of JS_IsGlobalObject:

    NS_ASSERTION(js::GetObjectClass(jsobj)->flags & JSCLASS_IS_GLOBAL,

That's why I removed it..
Hmm.  I guess that used to assert things about js::GetObjectParent back when that was a thing.

In that case, we can just make this function do:

  return NativeInterface2JSObjectAndThrowIfFailed(.....);

right?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #21)
> In that case, we can just make this function do:
> 
>   return NativeInterface2JSObjectAndThrowIfFailed(.....);
> 
> right?

Works for me :)
Things are now becoming a bit harder (for me) to reason about. I'll probably deal with the remaining callers in follow-up bugs.

The patches here are all green on Try btw.
Depends on: 1473865
Keywords: leave-open
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8c62e8a6080
part 1 - Use JS::GetNonCCWObjectGlobal in Codegen.py. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/7049feb994ee
part 2 - Use JS::GetNonCCWObjectGlobal in some functions where we unwrapped the object. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bcad8d82168
part 3 - Remove some GetGlobalForObjectCrossCompartment calls on globals/WindowProxy. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaaa74019db7
part 4 - Use JS::GetNonCCWObjectGlobal in PrefableDisablers::isEnabled. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d097737be52
part 5 - Use JS::CurrentGlobalOrNull in JS shell clone() function. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f5ed1760b9
part 6 - Use JS::GetNonCCWObjectGlobal in subscript loader. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/def39aedc2a6
part 7 - Use JS::GetNonCCWObjectGlobal in mozJSComponentLoader::FindTargetObject. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/6462a8c5ad53
part 8 - Use JS::GetNonCCWObjectGlobal in XrayAwareCalleeGlobal. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17b68432e95
part 9 - Use JS::GetNonCCWObjectGlobal in worker wrap-callback. r=bz
Comment on attachment 8990261 [details] [diff] [review]
Part 10 - Use JS::GetNonCCWObjectGlobal in DOMRequest::Then

We should add a diagnostic assert to Promise::CreateFromExisting that aPromiseObj is JS::IsPromiseObject.  I _think_ that's true, but am not 100% sure.
Attachment #8990261 - Flags: review?(bzbarsky) → review+
Comment on attachment 8990263 [details] [diff] [review]
Part 11 - Use JS::GetNonCCWObjectGlobal in nsIDocument::SetScriptGlobalObject

r=me
Attachment #8990263 - Flags: review?(bzbarsky) → review+
Comment on attachment 8990264 [details] [diff] [review]
Part 12 - Use JS::GetNonCCWObjectGlobal in CheckForOutdatedParent

r=me
Attachment #8990264 - Flags: review?(bzbarsky) → review+
Comment on attachment 8990265 [details] [diff] [review]
Part 13 - Use JS::GetNonCCWObjectGlobal in BindingUtils

>+      JS::GetNonCCWObjectGlobal(&aArgs.callee());

So I _think_ this is OK, because for Xrays we create a separate JSFunction, except for constructors, and those don't use this code...  Pretty subtle, though.

>@@ -1659,17 +1659,17 @@ FindAssociatedGlobal(JSContext* cx, T* p
>   JSObject* obj = WrapNativeHelper<T>::Wrap(cx, p, cache);

This one is also subtle.  It does always return a non-CCW object, but that object may not be in the current compartment of cx.  Worth documenting.

r=me
Attachment #8990265 - Flags: review?(bzbarsky) → review+
Comment on attachment 8990267 [details] [diff] [review]
Part 14 - Use JS::GetNonCCWObjectGlobal in IndexedDatabaseManager::ExperimentalFeaturesEnabled

IndexedDatabaseManager::ExperimentalFeaturesEnabled is only called from PrefableDisablers::isEnabled, so should only get globals passed to start with, right?

We can probably add a diagnostic assert that aGlobal is in fact a global and then take out the "get a global" call.

r=me with that.
Attachment #8990267 - Flags: review?(bzbarsky) → review+
Comment on attachment 8990268 [details] [diff] [review]
Part 15 - Use JS::GetNonCCWObjectGlobal in SandboxCallableProxyHandler::call

r=me
Attachment #8990268 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #31)
> We should add a diagnostic assert to Promise::CreateFromExisting that
> aPromiseObj is JS::IsPromiseObject.  I _think_ that's true, but am not 100%
> sure.

I get some orange on Try with the following stack:

* mozilla::dom::Promise::CreateFromExisting
* mozilla::dom::Promise::Resolve
* mozilla::dom::TestFunctions_Binding::passThroughPromise
...

I think JS::CallOriginalPromiseResolve can return a wrapped promise here:

https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/js/src/builtin/Promise.cpp#2341-2349

For DOMRequest::Then, my justification was that we always create a new DOM + JS Promise here, without ever wrapping it AFAICS:

https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/dom/base/DOMRequest.cpp#180
Flags: needinfo?(bzbarsky)
Depends on: 1474272
You're right.  We do apparently create dom::Promise around CCWs for promises...

You're also right that for the DOMRequest case this can't happen.
Flags: needinfo?(bzbarsky)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dded93548e6
part 10 - Use JS::GetNonCCWObjectGlobal in DOMRequest::Then. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0247ffaa0e9
part 11 - Use JS::GetNonCCWObjectGlobal in nsIDocument::SetScriptGlobalObject. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0a0805ce4f
part 12 - Use JS::GetNonCCWObjectGlobal in CheckForOutdatedParent. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7444af18cf
part 13 - Use JS::GetNonCCWObjectGlobal in BindingUtils. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/314d501fe26f
part 14 - Use JS::GetNonCCWObjectGlobal in IndexedDatabaseManager::ExperimentalFeaturesEnabled. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4dce7639098
part 15 - Use JS::GetNonCCWObjectGlobal in SandboxCallableProxyHandler::call. r=bz
Depends on: 1477705
Current status on m-i tip is there are 9 js::GetGlobalForObjectCrossCompartment calls left:

* 1 in xpc::NativeGlobal. Bug 1474272 is dealing with this.

* 2 in XPCWrappedJSClass. Can definitely be CCWs. I have a patch that's green on Try, but I need to double check the code.

* 3 in dom/xbl. Some of these are likely on CCWs. I'll file a separate bug.
  * 1 related one in GetXBLScopeOrGlobal (this has 1 or 2 problematic callers I think).

* 2 in XrayWrapper.cpp. These are definitely on wrappers.

So the main ones are XBL and Xrays. I'll look into these today and file separate bugs.
Depends on: 1477989
Depends on: 1478275
Depends on: 1478356
When the dependent bugs land, there will be no uses left.
Attachment #8996614 - Flags: review?(luke)
Comment on attachment 8996614 [details] [diff] [review]
Part 16 - Remove js::GetGlobalForObjectCrossCompartment

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

\o/
Attachment #8996614 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6b15672ed7
part 16 - Remove js::GetGlobalForObjectCrossCompartment. r=luke
This whole sub tree of bugs turned out to be bigger and more difficult than I expected. Really glad it's done :)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/bb6b15672ed7
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.