1270619 - [ESLint] Remove incorrect globals

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1270619 - Remove globals that aren't actually global. r=pbro

Review commit: https://reviewboard.mozilla.org/r/50913/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50913/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1270619 - Add missing imports instead of using globals. r=pbro

Review commit: https://reviewboard.mozilla.org/r/50915/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50915/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d7c45105179

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1270619 - Remove .js from module IDs. r=pbro

Review commit: https://reviewboard.mozilla.org/r/51253/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51253/

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=227376603684

Comment 8

[Patrick Brosset <:pbro>]

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

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8749942 [details]
MozReview Request: Bug 1270619 - Remove .js from module IDs. r=pbro

https://reviewboard.mozilla.org/r/51253/#review48011

Comment 10

[Patrick Brosset <:pbro>]

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.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/723fc96632f7
https://hg.mozilla.org/integration/fx-team/rev/a9b22026d93f
https://hg.mozilla.org/integration/fx-team/rev/0530f8d36de8

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/723fc96632f7
https://hg.mozilla.org/mozilla-central/rev/a9b22026d93f
https://hg.mozilla.org/mozilla-central/rev/0530f8d36de8