Closed
Bug 1472973
Opened 6 years ago
Closed 6 years ago
Make sure js::GetGlobalForObjectCrossCompartment is not called on CCWs
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(16 files)
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.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8989946 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8989947 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8989949 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #8989967 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #8989978 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8989980 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•6 years ago
|
||
js::GetJSMEnvironmentOfScriptedCaller returns either nullptr or a NonSyntacticVariablesObject.
Attachment #8989982 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #8989989 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #8989992 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•6 years ago
|
||
FWIW, at this point there are about 20 GetGlobalForObjectCrossCompartment callers left.
Comment 11•6 years ago
|
||
Comment on attachment 8989946 [details] [diff] [review]
Part 1 - Use JS::GetNonCCWObjectGlobal in Codegen.py
r=me
Attachment #8989946 -
Flags: review?(bzbarsky) → review+
Comment 12•6 years ago
|
||
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 13•6 years ago
|
||
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 14•6 years ago
|
||
Comment on attachment 8989967 [details] [diff] [review]
Part 4 - Use JS::GetNonCCWObjectGlobal in PrefableDisablers::isEnabled
r=me
Attachment #8989967 -
Flags: review?(bzbarsky) → review+
Comment 15•6 years ago
|
||
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 16•6 years ago
|
||
Comment on attachment 8989980 [details] [diff] [review]
Part 6 - Use JS::GetNonCCWObjectGlobal in subscript loader
r=me
Attachment #8989980 -
Flags: review?(bzbarsky) → review+
Comment 17•6 years ago
|
||
Comment on attachment 8989982 [details] [diff] [review]
Part 7 - Use JS::GetNonCCWObjectGlobal in mozJSComponentLoader::FindTargetObject
r=me
Attachment #8989982 -
Flags: review?(bzbarsky) → review+
Comment 18•6 years ago
|
||
Comment on attachment 8989989 [details] [diff] [review]
Part 8 - Use JS::GetNonCCWObjectGlobal in XrayAwareCalleeGlobal
r=me
Attachment #8989989 -
Flags: review?(bzbarsky) → review+
Comment 19•6 years ago
|
||
Comment on attachment 8989992 [details] [diff] [review]
Part 9 - Use JS::GetNonCCWObjectGlobal in worker wrap-callback
r=me
Attachment #8989992 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 20•6 years ago
|
||
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..
Comment 21•6 years ago
|
||
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?
Assignee | ||
Comment 22•6 years ago
|
||
(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 :)
Assignee | ||
Comment 23•6 years ago
|
||
Attachment #8990261 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 24•6 years ago
|
||
Attachment #8990263 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 25•6 years ago
|
||
Attachment #8990264 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 26•6 years ago
|
||
Attachment #8990265 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 27•6 years ago
|
||
Attachment #8990267 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 28•6 years ago
|
||
Attachment #8990268 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 29•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 30•6 years ago
|
||
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 31•6 years ago
|
||
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 32•6 years ago
|
||
Comment on attachment 8990263 [details] [diff] [review]
Part 11 - Use JS::GetNonCCWObjectGlobal in nsIDocument::SetScriptGlobalObject
r=me
Attachment #8990263 -
Flags: review?(bzbarsky) → review+
Comment 33•6 years ago
|
||
Comment on attachment 8990264 [details] [diff] [review]
Part 12 - Use JS::GetNonCCWObjectGlobal in CheckForOutdatedParent
r=me
Attachment #8990264 -
Flags: review?(bzbarsky) → review+
Comment 34•6 years ago
|
||
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 35•6 years ago
|
||
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 36•6 years ago
|
||
Comment on attachment 8990268 [details] [diff] [review]
Part 15 - Use JS::GetNonCCWObjectGlobal in SandboxCallableProxyHandler::call
r=me
Attachment #8990268 -
Flags: review?(bzbarsky) → review+
Comment 37•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a8c62e8a6080
https://hg.mozilla.org/mozilla-central/rev/7049feb994ee
https://hg.mozilla.org/mozilla-central/rev/5bcad8d82168
https://hg.mozilla.org/mozilla-central/rev/aaaa74019db7
https://hg.mozilla.org/mozilla-central/rev/9d097737be52
https://hg.mozilla.org/mozilla-central/rev/21f5ed1760b9
https://hg.mozilla.org/mozilla-central/rev/def39aedc2a6
https://hg.mozilla.org/mozilla-central/rev/6462a8c5ad53
https://hg.mozilla.org/mozilla-central/rev/e17b68432e95
Assignee | ||
Comment 38•6 years ago
|
||
(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)
Comment 39•6 years ago
|
||
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)
Comment 40•6 years ago
|
||
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
Comment 41•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5dded93548e6
https://hg.mozilla.org/mozilla-central/rev/e0247ffaa0e9
https://hg.mozilla.org/mozilla-central/rev/2a0a0805ce4f
https://hg.mozilla.org/mozilla-central/rev/bf7444af18cf
https://hg.mozilla.org/mozilla-central/rev/314d501fe26f
https://hg.mozilla.org/mozilla-central/rev/c4dce7639098
Assignee | ||
Comment 42•6 years ago
|
||
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.
Assignee | ||
Comment 43•6 years ago
|
||
When the dependent bugs land, there will be no uses left.
Attachment #8996614 -
Flags: review?(luke)
Comment 44•6 years ago
|
||
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+
Comment 45•6 years ago
|
||
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6b15672ed7
part 16 - Remove js::GetGlobalForObjectCrossCompartment. r=luke
Assignee | ||
Comment 46•6 years ago
|
||
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
Comment 47•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•