Closed Bug 1270619 Opened 4 years ago Closed 4 years ago

[ESLint] Remove incorrect globals

Categories

(DevTools :: General, enhancement)

enhancement
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: jryans, Assigned: jryans)

Details

Attachments

(3 files)

Some of the globals we have listed in devtools/.eslintrc aren't actually global, but instead are just "commonly used".  We should remove them so we get errors when they have not been imported.
Comment on attachment 8749373 [details]
MozReview Request: Bug 1270619 - Remove globals that aren't actually global. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50913/diff/1-2/
Comment on attachment 8749374 [details]
MozReview Request: Bug 1270619 - Add missing imports instead of using globals. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50915/diff/1-2/
Comment on attachment 8749373 [details]
MozReview Request: Bug 1270619 - Remove globals that aren't actually global. r=pbro

https://reviewboard.mozilla.org/r/50913/#review48009
Attachment #8749373 - Flags: review?(pbrosset) → review+
Attachment #8749942 - Flags: review?(pbrosset) → review+
Comment on attachment 8749942 [details]
MozReview Request: Bug 1270619 - Remove .js from module IDs. r=pbro

https://reviewboard.mozilla.org/r/51253/#review48011
Comment on attachment 8749374 [details]
MozReview Request: Bug 1270619 - Add missing imports instead of using globals. r=pbro

https://reviewboard.mozilla.org/r/50915/#review48013

Looks good to me, but I realize that I don't understand why it worked before. It looks like Task, EventEmitter and XPCOMUtils (maybe more) are indeed globally available.

::: devtools/client/animationinspector/components/animation-details.js:11
(Diff revision 2)
>  
>  "use strict";
>  
>  const {Cu} = require("chrome");
>  const {Task} = Cu.import("resource://gre/modules/Task.jsm", {});
> +const EventEmitter = require("devtools/shared/event-emitter");

I understand why we need this, but I don't understand why it worked without it. Can you explain to me what was defining EventEmitter before this?

::: devtools/client/dom/dom-panel.js:14
(Diff revision 2)
>  const { defer } = require("sdk/core/promise");
>  const { ObjectClient } = require("devtools/shared/client/main");
>  
>  const promise = require("promise");
>  const EventEmitter = require("devtools/shared/event-emitter");
> +const { Task } = require("resource://gre/modules/Task.jsm");

Same question here. Why is Task defined even when it's not required?

::: devtools/client/inspector/rules/views/rule-editor.js:8
(Diff revision 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const {Ci} = require("chrome");
> +const {XPCOMUtils} = require("resource://gre/modules/XPCOMUtils.jsm");

Here too, about XPCOMUtils.
Attachment #8749374 - Flags: review?(pbrosset) → review+
https://reviewboard.mozilla.org/r/50915/#review48013

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.

In this bug, I have only fixed up a few cases of single arg `Cu.import` as directed by ESLint errors.  However, we should probably fix this everywhere (and consider a custom rule perhaps) to remove confusion.  It looks like :ochameau filed bug 1193390 about this issue a while back, so I'll add more comments there about the general problem.

[1]: https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/js/xpconnect/idl/xpccomponents.idl#213-215
[2]: https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/devtools/shared/Loader.jsm#138
Severity: normal → enhancement
https://hg.mozilla.org/mozilla-central/rev/723fc96632f7
https://hg.mozilla.org/mozilla-central/rev/a9b22026d93f
https://hg.mozilla.org/mozilla-central/rev/0530f8d36de8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.