[eslint] Catch more cases of importing globals from "var foo = Cu.import("...");"

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

3 Branch
mozilla53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
In working on more no-undef failures, I realised that although bug 1328565 covered the 

`var foo = Cu.import("...", this);`

case, it didn't cover:

`var foo = Cu.import("...");`

i.e. without the scope supplied.

There's about 96 of these cases, mainly in tests, and there's definitely one or two where we're relying on the side effects, so I think its worth catching these as well.

A lot of these are also importing into variables within functions, but then end up affecting the global scope.
Assignee

Updated

2 years ago
Depends on: 1329621
Assignee

Comment 2

2 years ago
This has been run through try already:

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

(ignore the eslint failures there, I'd left the no-undef rule uncommented).
Comment on attachment 8825431 [details]
Bug 1329614 - [eslint] Catch more cases of importing globals from 'var foo = Cu.import('...');'.

https://reviewboard.mozilla.org/r/103596/#review104246

::: toolkit/components/telemetry/tests/unit/head.js:236
(Diff revision 1)
> -  let module = Cu.import("resource://gre/modules/TelemetrySend.jsm");
> +  let module = Cu.import("resource://gre/modules/TelemetrySend.jsm", {});
>    let obj = Cu.cloneInto({set, clear}, module, {cloneFunctions:true});
>    module.Policy.setSchedulerTickTimeout = obj.set;
>    module.Policy.clearSchedulerTickTimeout = obj.clear;
>  }
>  
>  function fakeMidnightPingFuzzingDelay(delayMs) {
> -  let module = Cu.import("resource://gre/modules/TelemetrySend.jsm");
> +  let module = Cu.import("resource://gre/modules/TelemetrySend.jsm", {});
>    module.Policy.midnightPingFuzzingDelay = () => delayMs;

Since TelemetrySend is now defined explicitly in the global scope, we should change these to use TelemetrySend instead of 'module'.
Attachment #8825431 - Flags: review?(jaws) → review+
Assignee

Comment 4

2 years ago
mozreview-review-reply
Comment on attachment 8825431 [details]
Bug 1329614 - [eslint] Catch more cases of importing globals from 'var foo = Cu.import('...');'.

https://reviewboard.mozilla.org/r/103596/#review104246

> Since TelemetrySend is now defined explicitly in the global scope, we should change these to use TelemetrySend instead of 'module'.

TelemetrySend.jsm doesn't mark `Policy` as exported, so this is using an alternative to get access to it for the purposes of that bit of the test. The rest of the test needs `TelemetrySend` and that was being side-imported via these imports.

Comment 5

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd9a337129ad
[eslint] Catch more cases of importing globals from 'var foo = Cu.import('...');'. r=jaws

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd9a337129ad
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

a year ago
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.