Closed Bug 1513277 Opened 8 months ago Closed 8 months ago

Cu.importGlobalProperties uses the current global, can define things on the wrong global

Categories

(Core :: XPConnect, enhancement, P2)

enhancement

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.
Maybe mozJSComponentLoader::FindTargetObject should do something similar. We are already using js::GetJSMEnvironmentOfScriptedCaller there so it seems JS::GetScriptedCallerGlobal would fit in nicely.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
(2) makes a lot of sense to me.
Hopefully this will fix the remaining oranges I didn't track down yet. Fingers crossed...
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)
> (2) makes a lot of sense to me.

Agreed.
+1. We could also use IncumbentGlobal, but ScriptedCallerGlobal more directly does what we want.
With same-compartment-realms enabled we can call a cross-realm Cu.importGlobalProperties
and we ended up defining properties on the wrong global.
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
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)
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.
(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)
Priority: -- → P2
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
https://hg.mozilla.org/mozilla-central/rev/63993693c624
https://hg.mozilla.org/mozilla-central/rev/c38131baf660
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Depends on: 1513783
You need to log in before you can comment on or make changes to this bug.