SpecialPowers.wrap(Components).utils.import(url, window) is broken

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: MattN, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
mozilla17
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Posted patch Test case (obsolete) — Splinter Review
See the attached mochitest-plain test.

Workaround is to do:

const Cu = SpecialPowers.wrap(Components).utils;
const Services = Cu.import("resource://gre/modules/Services.jsm").Services;
Alternatively, you can do Cu.import(url, window), which imports it into |window|'s scope. See the docs here: https://developer.mozilla.org/en/Components.utils.import

There's not a lot we can do here to fix the bug as filed. Cu.import(url) imports the JSM into the caller's scope. SpecialPowers.wrap explicitly proxies operations into a special chrome scope so that they run with privileges. Matt, do you have any ideas on how this could be made nicer? Or should we WONTFIX?
(In reply to Bobby Holley (:bholley) from comment #1)
> Alternatively, you can do Cu.import(url, window), which imports it into
> |window|'s scope. See the docs here:
> https://developer.mozilla.org/en/Components.utils.import

Yep, that didn't work either but I forgot to include it in the testcase.  I guess I should have mentioned that.  I tried:
Cu.import(url, window)
and 
Cu.import(url, this) (which would have been the window) as well.

I've now updated the test case. Is Cu.import(url, window) supposed to work there?

> There's not a lot we can do here to fix the bug as filed. Cu.import(url)
> imports the JSM into the caller's scope. SpecialPowers.wrap explicitly
> proxies operations into a special chrome scope so that they run with
> privileges. Matt, do you have any ideas on how this could be made nicer? Or
> should we WONTFIX?

If Cu.import(url, window) would work then it would be nicer.
Attachment #640434 - Attachment is obsolete: true
(In reply to Matthew N. [:MattN] from comment #2)

> If Cu.import(url, window) would work then it would be nicer.

Agreed! Let's morph this bug into that issue.
Summary: SpecialPowers.wrap(Components).utils.import doesn't import symbols in mochitest-plain → SpecialPowers.wrap(Components).utils.import(url, window) is broken
Ah, so the properties are ending up on the Xray holder for the chrome view on the content window.

I think we should just unwrap here and enter the compartment. Patching coming right up.
Updated tests. r=bholley
Attachment #640541 - Attachment is obsolete: true
Attachment #640567 - Flags: review+
Comment on attachment 640568 [details] [diff] [review]
Unwrap target objects for Cu.import. v1

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

Doesn't this risk introducing bug 653926 here? If I'm reading the component loader properly, we'll end up with scripts with script->principal being system, but the compartment being non-system. I think we need to avoid that.
Attachment #640568 - Flags: review?(mrbkap)
I'm not sure I grok all of the implications of the old approach, but I totally agree
that cavalier unwrapping should be avoided. Let's waive Xray instead.
Attachment #640568 - Attachment is obsolete: true
Attachment #644344 - Flags: review?(mrbkap)
(fixed the 'dump callers' typo locally)
Comment on attachment 644344 [details] [diff] [review]
Waive Xray for target objects in Cu.import. v1

Thanks.
Attachment #644344 - Flags: review?(mrbkap) → review+
And that one was busted because I asserted AccessCheck::callerIsChrome(), which fails for tests that use UniversalXPConnect to call Cu.import. Switched to nsContentUtils::CallerHasUniversalXPConnect() and repushed:


https://tbpl.mozilla.org/?tree=Try&rev=cd60bc464319
https://hg.mozilla.org/mozilla-central/rev/cd5e3b073284
https://hg.mozilla.org/mozilla-central/rev/8485eb46a4d6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.