Last Comment Bug 772288 - SpecialPowers.wrap(Components).utils.import(url, window) is broken
: SpecialPowers.wrap(Components).utils.import(url, window) is broken
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 762492
  Show dependency treegraph
 
Reported: 2012-07-09 16:54 PDT by Matthew N. [:MattN] (In Taipei until Sep. 23)
Modified: 2012-07-24 03:02 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case (1.99 KB, patch)
2012-07-09 16:54 PDT, Matthew N. [:MattN] (In Taipei until Sep. 23)
no flags Details | Diff | Splinter Review
Test case v2 [added Cu.import(url, window) test] (2.41 KB, patch)
2012-07-10 02:33 PDT, Matthew N. [:MattN] (In Taipei until Sep. 23)
no flags Details | Diff | Splinter Review
Tests. v2 r=bholley (2.45 KB, patch)
2012-07-10 04:23 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Unwrap target objects for Cu.import. v1 (2.91 KB, patch)
2012-07-10 04:23 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Waive Xray for target objects in Cu.import. v1 (4.28 KB, patch)
2012-07-20 08:05 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review

Description Matthew N. [:MattN] (In Taipei until Sep. 23) 2012-07-09 16:54:44 PDT
Created attachment 640434 [details] [diff] [review]
Test case

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;
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-07-10 02:16:05 PDT
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?
Comment 2 Matthew N. [:MattN] (In Taipei until Sep. 23) 2012-07-10 02:33:53 PDT
Created attachment 640541 [details] [diff] [review]
Test case v2 [added Cu.import(url, window) test]

(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.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-07-10 02:42:53 PDT
(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.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-07-10 04:05:21 PDT
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.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-07-10 04:23:16 PDT
Created attachment 640567 [details] [diff] [review]
Tests. v2 r=bholley

Updated tests. r=bholley
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-07-10 04:23:34 PDT
Created attachment 640568 [details] [diff] [review]
Unwrap target objects for Cu.import. v1
Comment 7 Blake Kaplan (:mrbkap) 2012-07-17 18:43:55 PDT
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.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-07-20 08:05:56 PDT
Created attachment 644344 [details] [diff] [review]
Waive Xray for target objects in Cu.import. v1

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.
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-07-20 08:07:30 PDT
(fixed the 'dump callers' typo locally)
Comment 10 Blake Kaplan (:mrbkap) 2012-07-20 12:29:38 PDT
Comment on attachment 644344 [details] [diff] [review]
Waive Xray for target objects in Cu.import. v1

Thanks.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-07-23 06:08:53 PDT
Twiddled with null-ness and repushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=230fd7f7903c
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-07-23 06:25:06 PDT
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
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-07-23 07:49:19 PDT
Looks green. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/8485eb46a4d6
http://hg.mozilla.org/integration/mozilla-inbound/rev/cd5e3b073284

Note You need to log in before you can comment on or make changes to this bug.