Enable ESLint rule mozilla/use-services for security/

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks 2 bugs)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 attachments)

(Assignee)

Description

a year ago
As we've been doing elsewhere, we should turn on mozilla/use-services for security/ so that we're consistent across the tree and get the small perf improvements.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1425832
Comment hidden (mozreview-request)
Comment on attachment 8937283 [details]
Bug 1425688 - Rework definitions of Cu/Cc/etc and inclusion of Services.jsm in pippki.js related files to reduce duplication.

https://reviewboard.mozilla.org/r/207978/#review213994
Attachment #8937283 - Flags: review?(dkeeler) → review+
Comment on attachment 8937284 [details]
Bug 1425688 - Enable ESLint rule mozilla/use-services for security/.

https://reviewboard.mozilla.org/r/207980/#review213930

LGTM. I noticed one place where we actually had some dead code, so if you feel like removing it that would be great. Otherwise, not a big deal. Also, a sandbox peer should look at the security/sandbox changes since I'm not familiar with that code (although I see no reason why it wouldn't be fine to make those changes).

::: security/manager/pki/resources/content/certViewer.js:29
(Diff revision 3)
>  const nsIASN1Tree = Ci.nsIASN1Tree;
>  const nsASN1Tree = "@mozilla.org/security/nsASN1Tree;1";
>  
>  var bundle;
>  
>  function doPrompt(msg) {

Looks like this function is only used in what should be dead code (line 368). Would you mind removing it and the code that calls it? (just change that `else` to `if (tree.currentIndex >= 0)` and remove the first `if` block).

::: security/sandbox/test/browser_content_sandbox_fs.js
(Diff revision 3)
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>   /* import-globals-from browser_content_sandbox_utils.js */
>   "use strict";
>  
> -var prefs = Cc["@mozilla.org/preferences-service;1"]

A sandbox peer should sign off on these changes.
Attachment #8937284 - Flags: review?(dkeeler) → review+
Priority: -- → P1
Whiteboard: [psm-assigned]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
:gcp, please can you review the security/sandbox parts. Thank you.

Comment 11

a year ago
mozreview-review
Comment on attachment 8937284 [details]
Bug 1425688 - Enable ESLint rule mozilla/use-services for security/.

https://reviewboard.mozilla.org/r/207980/#review214844
Attachment #8937284 - Flags: review?(gpascutto) → review+

Comment 12

a year ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd2bf7b7fead
Rework definitions of Cu/Cc/etc and inclusion of Services.jsm in pippki.js related files to reduce duplication. r=keeler
https://hg.mozilla.org/integration/autoland/rev/f73324a4d033
Enable ESLint rule mozilla/use-services for security/. r=gcp,keeler

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd2bf7b7fead
https://hg.mozilla.org/mozilla-central/rev/f73324a4d033
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Updated

a year ago
Depends on: 1427046

Comment 14

a year ago
Can we please get this backed out since it completely breaks the Certificate Manager, see bug 1426866 comment #7.
Flags: needinfo?(ryanvm)
Flags: needinfo?(aryx.bugmail)

Comment 15

a year ago
https://hg.mozilla.org/mozilla-central/rev/c2daa4c032b7
Backed out 2 changesets (bug 1425688) on request from jorgk for breaking the Certificate Manager r=backout a=backout

Sorry Mark, but this caused to much strive, so after Masatoshi-san asked for a backout confirming my thoughts, I had this backed out.
Status: RESOLVED → REOPENED
Flags: needinfo?(standard8)
Flags: needinfo?(ryanvm)
Flags: needinfo?(aryx.bugmail)
Resolution: FIXED → ---
(Assignee)

Comment 16

a year ago
That's fine, I was away, so backing out is a reasonable thing to do. Will try and re-fix this later today.
Flags: needinfo?(standard8)
(Assignee)

Updated

a year ago
Blocks: 1427717
(Assignee)

Comment 17

a year ago
The problem was that I'd missed that Services.jsm was already included in certManager.js, and when I did the first patch I had therefore missed removing it, in preference to the new pippki.js inclusion.

Unfortunately, the only test I could find for the certificate manager seems to open and close it to only check for leaks - it doesn't do any function testing. Additionally, a lot of these dialogs seem to be largely untested. I've therefore filed bug 1427717 to suggest that we create some more tests for these dialogs.

I've also checked around as many of the dialogs that I can find for more potential errors.

Since the fix is simple, I'll update on mozreview and re-push it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

a year ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/64ca68667553
Rework definitions of Cu/Cc/etc and inclusion of Services.jsm in pippki.js related files to reduce duplication. r=keeler
https://hg.mozilla.org/integration/autoland/rev/ed67462e6574
Enable ESLint rule mozilla/use-services for security/. r=gcp,keeler

Comment 21

a year ago
Thanks, Mark!

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64ca68667553
https://hg.mozilla.org/mozilla-central/rev/ed67462e6574
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.