Closed Bug 683217 Opened 13 years ago Closed 12 years ago

{Cu} = require("chrome"); doesn't let you use Cu.import("somejsmodule");

Categories

(Add-on SDK Graveyard :: General, defect, P1)

Product:
Add-on SDK Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: KWierso, Assigned: irakli)

References

Details

Attachments

(1 file)

Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/500
355 bytes, text/html
ochameau
: review+
Details 
While playing around adding an async read method to the file module, I had to import the NetUtils JS module.
So I added "Cu" to the require("chrome") line at the top of file.js, then used Cu.import() to try to import NetUtils.
I then tried to use functions provided by NetUtils, and it gave me an error.

But then I added "components" to the require("chrome") line, and used components.utils.import() to import NetUtils, and the function then worked just fine when I used it later in the file.

I don't have a simplified testcase handy, but I could make one if needed.
Assignee: nobody → rFobic
Priority: -- → P1
Target Milestone: --- → 1.2
If you rewrite your example as follows it will work:

var { Cu } = require("chrome");
var { ctypes } = Cu.import("resource://gre/modules/ctypes.jsm");
// ...

Also, this code is better as you can see exactly what has been imported. Assuming this works for you I'd suggest closing this bug as invalid.
Perhaps this should be documented in the chrome authority section? This example came from someone asking on IRC yesterday, and I hit it working on the file IO stuff.

(And it's not needed in the Cu.import documentation on MDN: https://developer.mozilla.org/en/Components.utils.import }
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.2 → ---
I think it'll makes more sense if wbamberg will pick it up, as he will know better where to add it ;)

wbamberg: Feel free to re-assign this back to me if you disagree.
Assignee: rFobic → wbamberg
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #5)
> I think it'll makes more sense if wbamberg will pick it up, as he will know
> better where to add it ;)
> 
> wbamberg: Feel free to re-assign this back to me if you disagree.

I'm happy to pick it up, but I'm not sure that simply documenting this is the right thing to do, at least without knowing more about it.

As I understand it, Cu is an alias for Components.utils, so it's entirely reasonable to expect Cu.import(...) to work too. If there's a good reason why it shouldn't, then I'm happy to document it. But just because there's an easy workaround isn't in itself a reason for just documenting it, as this is clearly something people run into, and people don't always read all the documentation before proceeding :).
gozala: I'm assigning this back because as comment 6 says, it's not obvious to me why Cu.import does not work in the way a user might expect, and in the way the MDN docs explain.

Let me know if there's some good reason for this and I'll happily document it, otherwise it seems to me that it ought to be fixed in the code.
Assignee: wbamberg → rFobic
I suspect it's because the `Cu` that a module gets from require("chrome") belongs to the scope of the `cuddlefish` module, and thus Cu.import("..."), whose default behavior is to add the JSM's symbols to the global object of the importing scope, imports symbols into the global object for the `cuddlefish` module rather than the global object for the module doing the importing.

If that's what's happening, it means CommonJS modules can modify `cuddlefish` by importing a JSM, violating the principle of module isolation and potentially creating a cross-module attack vector.
Hm, that's certainly annoying, and should be fixed if possible. OTOH, once you've done require("chrome"), you own the browser: the fact/suspicion that Cu.import() modifies cuddlefish's scope instead of the caller's is not an escalation of authority.
+1 to mentioning this in the official documentation. I just ran into this problem and I was able to realize it is a bug just because I saw some example code using Component.utils.import even after aliasing Ci and Cc, so I thought I would give it a try even though, as wbamberg said, it does make any sense to extension developers as Cu is supposed to be an alias. Until you fix the bug, a link to this bug report in the official documentation wouldn't hurt.
Is there any way to make cuddlefish.js somehow bind the `Cu` it returns to some new empty scope, on each call, so at least folks who use it this way won't clobber the loader itself? Doesn't sound likely (I think it'd involve making require("chrome").Cu be an object that inherits from the real Components.Utilities but overrides the .import method with a wrapper that somehow creates a new global each time it's invoked), but might be worth a shot.
(In reply to Brian Warner [:warner :bwarner] from comment #11)
> Is there any way to make cuddlefish.js somehow bind the `Cu` it returns to
> some new empty scope, on each call, so at least folks who use it this way
> won't clobber the loader itself? Doesn't sound likely (I think it'd involve
> making require("chrome").Cu be an object that inherits from the real
> Components.Utilities but overrides the .import method with a wrapper that
> somehow creates a new global each time it's invoked), but might be worth a
> shot.

What if following line threw exception:

Cu.import('resource://gre/modules/ctypes.jsm')

with a following message:

```
Please use following instead:
var ctypes = require('chrome').import('resource://gre/modules/ctypes.jsm')
```

Where `require('chrome').import` would not leak anything into cuttlefish, nor inject new globals.

I believe this would solve all the confusion and encourage better pattern.
Hm.. what would the new "import" function look like.. would it be like?:

 var newobj = {};
 Cu.import(name, newobj);
 return newobj;

I guess that could work.. I'd kind of like to avoid stealing such a big name like "import". How about require("chrome").Cu_import(name) ?
(In reply to Brian Warner [:warner :bwarner] from comment #13)
> Hm.. what would the new "import" function look like.. would it be like?:
> 
>  var newobj = {};
>  Cu.import(name, newobj);
>  return newobj;
> 

Yeap exactly that!

> I guess that could work.. I'd kind of like to avoid stealing such a big name
> like "import". How about require("chrome").Cu_import(name) ?

I don't think we'll add yet another method to `import` things at least not to the `chrome` with that assumption I would prefer `require("chrome").import(name)` specially since Cu_import is non-javascripty :)
I do think we should fix Cu.import so it does not pollutes scope it comes from and make it clear in documentation that user must use:

let { NetUtil } = Cu.import('resource://gre/modules/NetUtil.jsm')

Instead of expecting that NetUtil will be injected to the module scope when
calling:

Cu.import('resource://gre/modules/NetUtil.jsm')


I do think it's very important that we do not pollute module scope that is not compliant with a future module spec. I also think this is very good compromise
for users as they still will be able to use `Cu.import` while transitioning to
to pattern that will be enforced in a future. Removing `Cu.import` entirely will
harm users, make it behave as it does today will harm users even stronger in a future as they will have to pay second transition cost.
If we're going to change it's behaviour I'd rather we change the name too so it is clearer that it behaves differently.
Attachment #643069 - Flags: review?(poirot.alex)
(In reply to Dave Townsend (:Mossop) from comment #16)
> If we're going to change it's behaviour I'd rather we change the name too so
> it is clearer that it behaves differently.

I don't particularly mind that, but the problem is that there are lots of users who already use `Cu.import` without problems. I don't think breaking them is a good idea.

Also if someone try to relay `Cu.import`'s behavior that is not supported by SDK error will raised that hopefully will make them look into docs.
Maybe we should also modify MDN pages to mention Cu.import behavior in SDK ?
Seriously, why fighting so hard to make the use of JSM different in the SDK.
We should concentrate on selling CommonJS benefits and encourage people to completely switch to CommonJS/harmony/whatever. But trying to make special stuff around JSM just make the whole story more complex.
I insist that we should aim on providing a regular behavior for Cu.import.
I think that it would be more productive to figure out why people are still using JSM and give them necessary documentation/API to avoid doing so. In the meantime keep it simple, let JSM be JSM and not jetpack-jsm-thingy.
Attachment #643069 - Flags: review?(poirot.alex) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/e9989c480add10b3b7588aa1af2f01cc78eb4513
Merge pull request #500 from Gozala/bug/Cu.import@683217

Bug 683217 - Fix Cu.import so that it no longer pollutes global scope. r=@ochameau
(In reply to Alexandre Poirot (:ochameau) from comment #20)
> Seriously, why fighting so hard to make the use of JSM different in the SDK.

1. In order to make a change it should be worth it, so far I have not seen major complains about this.
2. Dynamic scope mutation is incompatible with a direction JS is going (`with` has being deprecated & removed from strict mode, harmony modules will be statically scoped). Enabling users to make mistakes that they will have to pay for is inappropriate choice.
3. There is nothing wrong with writing .jsm style modules if you want regular .jsm behavior. It's just if you import such modules into CommonJS environment you should
play nicely with that environment (do not introduce implicit bindings). Having clean break between two environment is advantage not a disadvantage, as long as you
can still require one from another.
4. Migration from CommonJS to harmony is straight forward and hopefully even can be automated. On the other hand suggested `Cu.import` behavior completely vanishes that.

> I insist that we should aim on providing a regular behavior for Cu.import.
> I think that it would be more productive to figure out why people are still using > JSM and give them necessary documentation/API to avoid doing so. In the meantime > keep it simple, let JSM be JSM and not jetpack-jsm-thingy.

I totally agree, but I think JSM should be JSM and commonjs module should be commonjs module. Use one or another or import one from another but don't imply incompatible behavior from one to another.
Ok so we put us in akward position.
On tuesday I looked at Cu.import and tried to fix it on FF13 (was still broken),
but then I looked at it again today but it took me some time to figure out that it has been fixed in FF14 !!
But ... it has been broken with comment 21's patch :-(
So this patch fixed loader.js pollution in FF13-, but disallow gaining FF14's fix.

There isn't any modification around Cu.import platform code in FF14. I tried to ask #content if they happen to know what could have fixed that, but no response for now.

I'm suggesting to revert back this change and gain from FF14 automatic fix.
The only issue is that is will still leave the pollution of loader scope when addons are used in FF13-.
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/063a6c5f998a9cbb2901a40ecefc9d578d7cc7a0
Bug 683217: Revert "Merge pull request #500 from Gozala/bug/Cu.import@683217" a=@gozala

This reverts commit e9989c480add10b3b7588aa1af2f01cc78eb4513, reversing
changes made to 13eb5e5cbfd6104e18cf76aadf276eabfc3f5265.
So with this patch reverted,
  let { Cu } = require("chrome");
  Cu.import("...")
Will now behave correctly as soon as you have FF14. It should work on whatever version of the SDK you use, but we can ensure that it works on 1.10.
Target Milestone: --- → 1.10
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: