Closed Bug 1425688 Opened 6 years ago Closed 6 years ago

Enable ESLint rule mozilla/use-services for security/

Categories

(Core :: Security: PSM, enhancement, P1)

Product:
Core
Component:
Security: PSM
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files)

Bug 1425688 - Rework definitions of Cu/Cc/etc and inclusion of Services.jsm in pippki.js related files to reduce duplication.
59 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1425688 - Enable ESLint rule mozilla/use-services for security/.
59 bytes, text/x-review-board-request
keeler
: review+
gcp
: review+
Details 
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.
Blocks: 1425832
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]
:gcp, please can you review the security/sandbox parts. Thank you.
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+
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
https://hg.mozilla.org/mozilla-central/rev/bd2bf7b7fead
https://hg.mozilla.org/mozilla-central/rev/f73324a4d033
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1427046
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)
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 → ---
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)
Blocks: 1427717
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.
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
Thanks, Mark!
https://hg.mozilla.org/mozilla-central/rev/64ca68667553
https://hg.mozilla.org/mozilla-central/rev/ed67462e6574
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.