Closed
Bug 1513277
Opened 6 years ago
Closed 6 years ago
Cu.importGlobalProperties uses the current global, can define things on the wrong global
Categories
(Core :: XPConnect, enhancement, P2)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(2 files)
Cu.importGlobalProperties is used like this:
Cu.importGlobalProperties(["crypto", "TextEncoder"]);
Now if Cu.importGlobalProperties is a cross-realm function, we can end up importing the properties on the wrong global because we switch to that realm when invoking the native function (this is similar to bug 1512417).
Our options are:
(1) Require passing |this| as argument to importGlobalProperties. Doable but there are hundreds of callers...
(2) Do something like this in nsXPCComponents_Utils::ImportGlobalProperties:
RootedObject global(cx, JS::GetScriptedCallerGlobal(cx));
MOZ_ASSERT(global);
MOZ_ASSERT(js::GetObjectCompartment(global) == js::GetContextCompartment(cx));
JSAutoRealm ar(cx, global);
I tried (2) and it fixes the test failure I was debugging.
Assignee | ||
Comment 1•6 years ago
|
||
Maybe mozJSComponentLoader::FindTargetObject should do something similar. We are already using js::GetJSMEnvironmentOfScriptedCaller there so it seems JS::GetScriptedCallerGlobal would fit in nicely.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Comment 2•6 years ago
|
||
(2) makes a lot of sense to me.
Assignee | ||
Comment 3•6 years ago
|
||
Hopefully this will fix the remaining oranges I didn't track down yet. Fingers crossed...
Comment 4•6 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)
> (2) makes a lot of sense to me.
Agreed.
Comment 5•6 years ago
|
||
+1. We could also use IncumbentGlobal, but ScriptedCallerGlobal more directly does what we want.
Assignee | ||
Comment 6•6 years ago
|
||
With same-compartment-realms enabled we can call a cross-realm Cu.importGlobalProperties
and we ended up defining properties on the wrong global.
Assignee | ||
Comment 7•6 years ago
|
||
We have a few places where C++ calls ChromeUtils::Import directly.
I fixed these to pass the target object directly instead of an empty Optional<>.
Depends on D14179
Assignee | ||
Comment 8•6 years ago
|
||
Hm part 2 doesn't work because the ChromeUtils.import call here can end up going through a CCW:
https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/testing/mochitest/chrome-harness.js#7
Then scriptedCallerGlobal->compartment != cx->compartment and we fail the AssertSameCompartment I added. cx->global is a Window and scriptedCallerGlobal is a Sandbox.
Maybe mozJSComponentLoader::ImportInto should emplace a Maybe<JSAutoRealm> after calling FindTargetObject? It feels like I might be opening a can of worms but using the scripted caller's global is the behavior we want I think...
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 9•6 years ago
|
||
Also, any idea how this happens to work right now? We define things on the wrong global I think, but maybe I'm missing something.
Comment 10•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #9)
> Also, any idea how this happens to work right now? We define things on the
> wrong global I think, but maybe I'm missing something.
The only time that happens is when we load the script into a sandbox whose prototype is the global ChromeUtils.import comes from. In that case, defining the variables on the wrong global will actually still work. But in that case, they also happen to already be defined.
We should perhaps just return null in that case rather than asserting, and have that script explicitly pass `this` to ChromeUtils.import.
Flags: needinfo?(kmaglione+bmo)
Updated•6 years ago
|
Priority: -- → P2
Comment 11•6 years ago
|
||
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63993693c624
part 1 - Use the scripted caller's global in Cu.importGlobalProperties. r=kmag
https://hg.mozilla.org/integration/autoland/rev/c38131baf660
part 2 - Use the scripted caller's global in mozJSComponentLoader::FindTargetObject. r=kmag
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/63993693c624
https://hg.mozilla.org/mozilla-central/rev/c38131baf660
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•