1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[Mark Banner (:standard8) (afk until 21st July)]

I'm working on rolling out mozilla/use-services across the tree. passwordmgr has a good chunk of changes to do, and I've seen a few issues with tests when changing it.

Hence, separating it out into a separate bug.

Comment 1

[Mark Banner (:standard8) (afk until 21st July)]

Attachment: Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

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

Comment 2

[Mark Banner (:standard8) (afk until 21st July)]

Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

https://reviewboard.mozilla.org/r/194616/#review199610

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-services.js:28
(Diff revision 1)
>    // These interfaces are difficult/not possible to get via processing.
>    result: {
>      "nsIPrefBranch": "prefs",
>      "nsIPrefService": "prefs",
> -    "nsIXULRuntime": "appInfo",
> -    "nsIXULAppInfo": "appInfo",
> +    "nsIXULRuntime": "appinfo",
> +    "nsIXULAppInfo": "appinfo",

Note: this is a correctness thing I noticed whilst working on deploying the rule, the only effect is on the ESLint error message will show the correct replacement to use.

Comment 3

[Mark Banner (:standard8) (afk until 21st July)]

I've seen the test failures in test_autofill_password-only.html on try server - I'll look at those in the morning.

Comment 4

[Mark Banner (:standard8) (afk until 21st July)]

Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

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

Comment 5

[Mark Banner (:standard8) (afk until 21st July)]

I fixed the issue - Services was out of scope in one of the functions that gets run in a parent process.

Comment 6

[Mark Banner (:standard8) (afk until 21st July)]

Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

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

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

https://reviewboard.mozilla.org/r/194616/#review200222

My only concern is about tests which are skipped/disabled either permanently or in e10s. Could you verify that those tests still pass (if they did before your change)?

::: toolkit/components/passwordmgr/test/mochitest/test_prompt_http.html:32
(Diff revision 3)
>  
>  let chromeScript = runInParent(SimpleTest.getTestFileURL("pwmgr_common.js"));
>  
>  runInParent(() => {
>    const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>    Cu.import("resource://gre/modules/Services.jsm");

Should this also be destructured to make the scope clear to tools

::: toolkit/components/passwordmgr/test/pwmgr_common.js:131
(Diff revision 3)
> -  var pwmgr = SpecialPowers.Cc["@mozilla.org/login-manager;1"].
> -              getService(SpecialPowers.Ci.nsILoginManager);
> -  ok(pwmgr != null, "Access LoginManager");
> +  // var pwmgr = SpecialPowers.Cc["@mozilla.org/login-manager;1"].
> +  //             getService(SpecialPowers.Ci.nsILoginManager);
> +  // ok(pwmgr != null, "Access LoginManager");

You can delete this

::: toolkit/components/passwordmgr/test/test_master_password.html:20
(Diff revision 3)
> +// eslint-disable-next-line mozilla/use-services
>  var pwmgr   = SpecialPowers.Cc["@mozilla.org/login-manager;1"]
>                             .getService(SpecialPowers.Ci.nsILoginManager);

Ideally the rule would be smart enough to handle this

Comment 8

[Mark Banner (:standard8) (afk until 21st July)]

Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

https://reviewboard.mozilla.org/r/194616/#review200222

Thanks for the suggestion, looks like there was only one that was skipped for e10s, and I've fixed that up.

> Should this also be destructured to make the scope clear to tools

Yes it should. Missed fixing that one.

> Ideally the rule would be smart enough to handle this

Interestingly we can actually use SpecialPowers.Services. I'll leave it as it is for now, but file a follow-up to improve the suggested information or something and revisit these instances.

Comment 9

[Mark Banner (:standard8) (afk until 21st July)]

I've also just added the rule to toolkit/components/passwordmgr/.eslintrc.js - totally meant to do that before, somehow forgot.

Comment 10

[Mark Banner (:standard8) (afk until 21st July)]

Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cac71ff2481c
Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr. r=MattN

Comment 12

[Matthew N. [:MattN]]

(In reply to Mark Banner (:standard8) from comment #8)
> Thanks for the suggestion, looks like there was only one that was skipped
> for e10s, and I've fixed that up.

Ok, I didn't just mean tests that you were changing though. Some tests never got ported to work with e10s. Could you double-check the following directory locally with --disable-e10s: https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/toolkit/components/passwordmgr/test/mochitest.ini#2

Comment 13

[Attila Craciun [:arny]]

Backed out changeset cac71ff2481c (bug 1412890) possible ESlint failure. Backout requested by Mark Banner.

https://hg.mozilla.org/integration/autoland/rev/d86cc075ca5d2155086cccb5e0086573961447e9

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cac71ff2481c20c77090cb508650d2b36175f2e8&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Comment 14

[Mark Banner (:standard8) (afk until 21st July)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #12)
> (In reply to Mark Banner (:standard8) from comment #8)
> > Thanks for the suggestion, looks like there was only one that was skipped
> > for e10s, and I've fixed that up.
> 
> Ok, I didn't just mean tests that you were changing though. Some tests never
> got ported to work with e10s. Could you double-check the following directory
> locally with --disable-e10s:
> https://dxr.mozilla.org/mozilla-central/rev/
> ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/toolkit/components/passwordmgr/test/
> mochitest.ini#2

Thanks, I certainly wasn't expecting tests in that higher level directory. Looks like there's a few more places for Services -> SpecialPowers.Services. I'll fix those up and re-land later.

Comment 15

[Mark Banner (:standard8) (afk until 21st July)]

Comment on attachment 8923474 [details]
Bug 1412890 - Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194616/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da3d20251959
Enable ESLint rule mozilla/use-services for toolkit/components/passwordmgr. r=MattN

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/da3d20251959