Closed Bug 1193390 Opened 4 years ago Closed 3 years ago

Remove all usage of Cu.import without a second argument

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: ochameau, Assigned: jryans)

References

Details

Attachments

(2 files)

While working on bug 1192863, I realized there is a very bad side effect between SDK Loader and Cu.import.
If you don't pass a second argument to Cu.import, it will automagically import all JSM symbols onto all existing sandboxes/modules and bring a lot of "What is going on?!?" questions if you happened to hit such bad behavior.

We should always pass a second argument to Cu.import or stop using jsm if we can easily do that. (Services for ex, or XPCOMUtils.lazy getters)
This issue came to light again in bug 1270619, where I cleaned up some ESLint globals that were not actually global.  :pbro was puzzled about how modules were able to access certain global-seeming values, so here's my expanded explanation:

The form `Cu.import("path")` installs the `EXPORTED_SYMBOLS` onto the _caller's_ global object[1].  We also use a memory-saving option in our loader that makes it use the same sandbox[2] for the modules it loads.  When you combine these two points, it means that any usage of `Cu.import("path")` exposes new globals to any modules in our loader.

So, these lines happened to work because some module loaded before this one happened to run `Cu.import("resource://devtools/shared/event-emitter.js");`.

To avoid this confusion, we should load JSMs through `require` where possible, or at least pass some object as the second arg to `Cu.import`, as in `const {EventEmitter} = Cu.import("resource://devtools/shared/event-emitter.js", {});`, which makes it install onto the "target object" (the second arg), instead of the caller's global.
We may want to consider an ESLint rule to specifically disallow single argument Cu.import for DevTools as well.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> We may want to consider an ESLint rule to specifically disallow single
> argument Cu.import for DevTools as well.

I think it's definitely a good idea; however I imagine we'll have to remove
all non-server uses of Cu.import for the de-chrome-ification anyway.
(In reply to Tom Tromey :tromey from comment #3)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> > We may want to consider an ESLint rule to specifically disallow single
> > argument Cu.import for DevTools as well.
> 
> I think it's definitely a good idea; however I imagine we'll have to remove
> all non-server uses of Cu.import for the de-chrome-ification anyway.

Yes, that's true.  The server ones probably here to stay though, so the rule still seems useful for that side at least.
Client-side removal is bug 1276341.
More time wasted on this today, so let's take care of it for real.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
A few miscellaneous linting issues also addressed near the lines involved.

Review commit: https://reviewboard.mozilla.org/r/58314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58314/
Comment on attachment 8760916 [details]
Bug 1193390 - Add ESLint rule to check for single argument Cu.import.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58312/diff/1-2/
Comment on attachment 8760917 [details]
Bug 1193390 - Remove single arg Cu.import from /devtools.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58314/diff/1-2/
Comment on attachment 8760916 [details]
Bug 1193390 - Add ESLint rule to check for single argument Cu.import.

https://reviewboard.mozilla.org/r/58312/#review55390

Thank you.

This patch looks good, but I didn't understand why a couple of files were changed.  These changes seem extraneous to me.  If we want them for some other reason, that's fine.

::: testing/eslint/manifest.tt:1
(Diff revision 2)
>  [

I don't understand why this file had to change for this patch.

::: testing/eslint/npm-shrinkwrap.json:1
(Diff revision 2)
>  {

I don't understand why this file had to change, either.
Attachment #8760916 - Flags: review?(ttromey) → review+
Attachment #8760917 - Flags: review?(ttromey) → review+
Comment on attachment 8760917 [details]
Bug 1193390 - Remove single arg Cu.import from /devtools.

https://reviewboard.mozilla.org/r/58314/#review55402

Thank you for writing this.  It looks great.
https://reviewboard.mozilla.org/r/58312/#review55390

> I don't understand why this file had to change for this patch.

As of bug 1265082, changes to the ESLint program and any plugins it uses need to be uploaded to releng's tooltool service.  The goal of this is to allow automation to retrieve all these bits without talking to the external npm registry in automation.

So, these changed files are the output of uploading the new rule to tooltool using the script at testing/eslint/update.  The current design of this has some drawbacks, such as it would be nice to not have to upload the Mozilla plugin at least, since the source is directly in the tree.  Anyway, this is the current process for now.

> I don't understand why this file had to change, either.

This file is also updated by the testing/eslint/update script.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/638225e6200d
Add ESLint rule to check for single argument Cu.import. r=tromey
https://hg.mozilla.org/integration/fx-team/rev/e7474c787d04
Remove single arg Cu.import from /devtools. r=tromey
https://hg.mozilla.org/mozilla-central/rev/638225e6200d
https://hg.mozilla.org/mozilla-central/rev/e7474c787d04
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1279515
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.