PREF_SSL_IMPACT is visible in startup profiles

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: prasanthmani2010, Mentored)

Tracking

(Blocks: 2 bugs, {good-first-bug})

Trunk
Firefox 57
good-first-bug
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
https://perf-html.io/public/6801d5dff8f8e5e1505b145df68b2bf49ba191cb/calltree/?hiddenThreads=&implementation=js&range=0.0000_14.0519&search=PREF_SSL_IMPACT&thread=0&threadOrder=0-2-1

The PREF_SSL_IMPACT array shouldn't be created before first paint. Either it should become a lazy getter, or it should be computed whenever it's used (which doesn't seem to be often). Also, I think precomputing the value may give incorrect results if preferences without default values are created in these branches between startup and when we actually use the array.

The profile shows it in browser.js (http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/browser.js#2895) but it's most likely slow at http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/content.js#254 too.
Looks like a good first/mentored bug to me.
Mentor: jhofmann
Flags: qe-verify-
Keywords: good-first-bug
(Assignee)

Comment 2

2 years ago
I am interested in working with this bug.
Great! As Florian mentioned, it is probably easiest to inline the variable in these two places:

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/browser.js#2975
http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/content.js#464

instead of making it a global const.

You can verify that your patch doesn't break bug 1252068 by running

./mach mochitest browser/base/content/test/about/browser_aboutNetError.js

Let me know if you have any questions.

Thanks!
Assignee: nobody → prasanthmani2010
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
I made the changes as per your suggestion and have run the mochitest , It passed all the 3 cases. How to proceed further ?
Hey, that's cool! We need to get this uploaded for review now.

Since I know from IRC that you have a patch but are not able to use mozreview for unknown reasons (sorry for that), you could try manually attaching the patch to Bugzilla via the "Attach File" link on this page. You can set the review? flag on the patch to me (:johannh).

Let me know if that works.
(Assignee)

Comment 7

2 years ago
Posted patch mypatch.patchSplinter Review
Attachment #8891723 - Flags: review?(jhofmann)
Comment on attachment 8891723 [details] [diff] [review]
mypatch.patch

Review of attachment 8891723 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This patch would probably have the effect we're looking for, but we can still improve it a bit.

::: browser/base/content/browser.js
@@ +2867,4 @@
>    },
>  
>    receiveMessage(msg) {
> +    const PREF_SSL_IMPACT = PREF_SSL_IMPACT_ROOTS.reduce((prefs, root) => {

Please use

let prefSSLImpact = ...

or a similar name instead :)

@@ +2868,5 @@
>  
>    receiveMessage(msg) {
> +    const PREF_SSL_IMPACT = PREF_SSL_IMPACT_ROOTS.reduce((prefs, root) => {
> +      return prefs.concat(Services.prefs.getChildList(root));
> +    }, []);

It's probably not great for performance to build this every time receiveMessage is received (when any kind of message is received). We should only do it inside the switch .. case branch that needs it.

::: browser/base/content/content.js
@@ +371,4 @@
>    },
>  
>    changedCertPrefs() {
> +    const PREF_SSL_IMPACT = PREF_SSL_IMPACT_ROOTS.reduce((prefs, root) => {

See above, please use let instead of const.
Attachment #8891723 - Flags: review?(jhofmann) → feedback+
(Assignee)

Comment 9

2 years ago
have made the changes as per the discussion. kindly check if I have implemented them.
Attachment #8892767 - Flags: review?(jhofmann)
Comment on attachment 8892767 [details] [diff] [review]
mypatch2.patch

Yay, these are the correct changes, but please amend your original patch. Both patches merged would get an r+ from me.

You can use something like hg histedit to combine your two patches and upload the unified patch here for a final round of review :)

Let me know if you need help with that. (I'm also available on IRC).
Attachment #8892767 - Flags: review?(jhofmann) → feedback+
I merged the two changesets with hg histedit (retaining your authorship information). Thanks!
Attachment #8898711 - Flags: review?(jhofmann)
Assignee: prasanthmani2010 → jhofmann
Assignee: jhofmann → prasanthmani2010
Attachment #8898711 - Flags: review?(jhofmann) → review+
I'll land this as soon as inbound opens...

Comment 13

2 years ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d824da178034
PREF_SSL_IMPACT array declaration made into inline. r=johannh
https://hg.mozilla.org/mozilla-central/rev/d824da178034
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.