1251011 - Enable ESLint "no-undef" rule for PSM

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

This rule catches references to undefined variables, as well as accidentally setting global variables by doing something like |foo = bar;| instead of |let foo = bar;|.

Comment 1

[:Cykesiopka]

Attachment: MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop

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

Comment 2

[:Cykesiopka]

https://reviewboard.mozilla.org/r/39955/#review36509

::: security/manager/ssl/tests/mochitest/mixedcontent/test_unsecureCSS.html:12
(Diff revision 1)
>    
>    <link rel="stylesheet" type="text/css" 

Trailing whitespace.

::: testing/xpcshell/xpcshell.eslintrc:32
(Diff revision 1)
> +    "greater": false,
> +    "greaterOrEqual": false,

I apparently don't know that g comes after e in the alphabet.

Comment 3

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8730509 [details]
MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop

https://reviewboard.mozilla.org/r/39955/#review36681

Great!

::: security/manager/pki/resources/content/createCertInfo.js:34
(Diff revision 1)
>    keygenThread.startKeyGeneration(obs);
>  }
>  
>  function onClose()
>  {
> -  setCursor("default");
> +  window.setCursor("default");

Huh - I think this should have always been "auto" instead of "default".

::: security/manager/pki/resources/content/protectedAuth.js:40
(Diff revision 1)
>    }
>  }
>  
>  function onClose()
>  {
> -  setCursor("default");
> +  window.setCursor("default");

Same here.

::: security/manager/ssl/tests/mochitest/mixedcontent/test_secureAll.html:13
(Diff revision 1)
>    <script type="text/javascript" src="/MochiKit/Signal.js"></script>
>    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>    <script type="text/javascript" src="mixedContentTest.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>  
>    <link rel="stylesheet" type="text/css" 

Trailing whitespace here if you feel like fixing it.

::: security/manager/ssl/tests/unit/test_toolkit_securityreporter.js:20
(Diff revision 1)
>  
>  "use strict";
>  const CC = Components.Constructor;
>  const Cm = Components.manager;
>  
>  Cu.import("resource://testing-common/AppInfo.jsm");

Hmmm - can we do const \{ AppInfo \} = Cu.import("resource://testing-common/AppInfo.jsm", \{\}); AppInfo.updateAppInfo(); ?
No big deal, though - looks like other lint files handle this similarly.

::: testing/xpcshell/xpcshell.eslintrc:45
(Diff revision 1)
>      "notEqual": false,
>      "notStrictEqual": false,
>      "ok": false,
>      "run_next_test": false,
>      "run_test": false,
> +    "run_test_in_child": false,

Since changes here affect all xpcshell tests that have linting enabled, I feel like you should get sign-off from maybe a test harness peer. Perhaps Ted Mielczarek?

Comment 4

[:Cykesiopka]

https://reviewboard.mozilla.org/r/39955/#review36681

Thanks for the review!

> Huh - I think this should have always been "auto" instead of "default".

Hmm, indeed. I'll update this.

> Hmmm - can we do const \{ AppInfo \} = Cu.import("resource://testing-common/AppInfo.jsm", \{\}); AppInfo.updateAppInfo(); ?
> No big deal, though - looks like other lint files handle this similarly.

Sadly, no. AppInfo.jsm exports its methods directly, so we can do this:
> const { updateAppInfo } = Cu.import("resource://testing-common/AppInfo.jsm", {});
... but that looks kinda weird.

> Since changes here affect all xpcshell tests that have linting enabled, I feel like you should get sign-off from maybe a test harness peer. Perhaps Ted Mielczarek?

Sure.

Comment 5

[:Cykesiopka]

Comment on attachment 8730509 [details]
MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39955/diff/1-2/

Comment 6

[:Cykesiopka]

https://reviewboard.mozilla.org/r/39955/#review36805

Ted: If you could take a look at the xpcshell.eslintrc changes, that would be great. If not, feel free to redirect.
Thanks.

Comment 7

[(not currently active) Ted Mielczarek]

Despite being the module owner here I'm not familiar with these eslintrc files. I can't edit your MozReview request, but I'd recommend you tag Mossop for review since he authored that file.

Comment 8

[:Cykesiopka]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Despite being the module owner here I'm not familiar with these eslintrc
> files. I can't edit your MozReview request, but I'd recommend you tag Mossop
> for review since he authored that file.

I'll do that, thanks.

Comment 9

[:Cykesiopka]

Comment on attachment 8730509 [details]
MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39955/diff/2-3/

Comment 10

[:Cykesiopka]

Comment on attachment 8730509 [details]
MozReview Request: Bug 1251011 - Enable ESLint "no-undef" rule for PSM. r=keeler,mossop

(Bugzilla set r+ and MozReview flags don't mesh that well ATM I guess.)

Comment 11

[:Cykesiopka]

Thanks for the reviews!

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0ff2a3c740

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e0ff2a3c740