Stop loading Preferences.jsm before first paint

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: florian, Assigned: marco)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

Attachments

(16 attachments, 6 obsolete attachments)

9.48 KB, patch
lina
: review+
marco
: checkin+
Details | Diff | Splinter Review
2.43 KB, patch
kmag
: review+
marco
: checkin+
Details | Diff | Splinter Review
5.09 KB, patch
jaws
: review+
marco
: checkin+
Details | Diff | Splinter Review
1.55 KB, patch
MattN
: review+
marco
: checkin+
Details | Diff | Splinter Review
1.54 KB, patch
bzbarsky
: review+
marco
: checkin+
Details | Diff | Splinter Review
2.51 KB, patch
MattN
: review+
marco
: checkin+
Details | Diff | Splinter Review
1.22 KB, patch
MattN
: review+
marco
: checkin+
Details | Diff | Splinter Review
7.38 KB, patch
MattN
: review+
marco
: checkin+
Details | Diff | Splinter Review
58.69 KB, patch
rhelmer
: review+
Details | Diff | Splinter Review
5.99 KB, patch
mchang
: review+
marco
: checkin+
Details | Diff | Splinter Review
3.30 KB, patch
ato
: review+
marco
: checkin+
Details | Diff | Splinter Review
49.44 KB, patch
Dexter
: review+
marco
: checkin+
Details | Diff | Splinter Review
44.42 KB, patch
marco
: review+
marco
: checkin+
Details | Diff | Splinter Review
36.86 KB, patch
Gijs
: review+
marco
: checkin+
Details | Diff | Splinter Review
53.93 KB, patch
Gijs
: review+
marco
: checkin+
Details | Diff | Splinter Review
1.37 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
With bug 1338306 and bug 1345294 fixed, most of the reasons for using Preferences.jsm are now gone, and using this module adds some overhead, so we should consider removing it.

If there's no simple way to do it for the whole tree, we should start with removing it from the startup path, and removing it from content processes.
(Reporter)

Updated

2 years ago
Blocks: 1350472
Depends on: 1359040

Comment 1

2 years ago
Florian, with your scripts is it easy to find files that both:

- import Preferences.jsm in some way
- only use Preferences.get / .set

and blanket-patch those usecases to use Services.prefs.get/set*Pref(..., (...)) instead? That feels like it'll be 99% of the work here.
Flags: needinfo?(florian)
(Reporter)

Comment 2

2 years ago
(In reply to :Gijs from comment #1)
> Florian, with your scripts is it easy to find files that both:
> 
> - import Preferences.jsm in some way
> - only use Preferences.get / .set
> 
> and blanket-patch those usecases to use Services.prefs.get/set*Pref(...,
> (...)) instead? That feels like it'll be 99% of the work here.

I don't think it would be easy to script unfortunately. Issues I observed:
- some files do use observers, or reset, or has
- to convert get/set, we need to guess the type of the pref, and that requires guessing the type from the provided default value. I'm not sure that value always has the right type.

We may be able to use a script to prepare the work, but I think the output will need careful review.
Flags: needinfo?(florian)
Depends on: 1359737
(Assignee)

Updated

2 years ago
Depends on: 720419
(Assignee)

Comment 3

2 years ago
I have a dirty WIP patch that would move Preferences.jsm loading to be "before handling user events". I'll try to clean it a bit and post it for review.
Blocks: 1355956
(Assignee)

Comment 5

2 years ago
Attachment #8891486 - Flags: review?(bzbarsky)
Comment on attachment 8891486 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

>+  let rcwnEnabled = Services.prefs.getBoolPref("network.http.rcwn.enabled");

setBoolPref, no?  Was this tested?
Attachment #8891486 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 8

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8891486 [details] [diff] [review]
> Remove Preferences.jsm usage from browser_bug1047663.js
> 
> >+  let rcwnEnabled = Services.prefs.getBoolPref("network.http.rcwn.enabled");
> 
> setBoolPref, no?  Was this tested?

Ooops. Yes, it was even green on try :S
(Assignee)

Comment 9

2 years ago
Attachment #8891486 - Attachment is obsolete: true
Attachment #8891491 - Flags: review?(bzbarsky)
Attachment #8891488 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8891486 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

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

::: dom/workers/test/browser_bug1047663.js
@@ +15,2 @@
>    registerCleanupFunction(()=>{
> +    Services.prefs.getBoolPref("network.http.rcwn.enabled", rcwnEnabled);

Can you please use `SpecialPowers.pushPrefEnv` for this?
Comment on attachment 8891491 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

>+    Services.prefs.getBoolPref("network.http.rcwn.enabled", rcwnEnabled);

This part is still wrong....
Attachment #8891491 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 14

2 years ago
Used SpecialPowers.pushPrefEnv.
Assignee: nobody → mcastelluccio
Attachment #8891491 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8891496 - Flags: review?(bzbarsky)
Comment on attachment 8891496 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

Yes, this is much better!
Attachment #8891496 - Flags: review?(bzbarsky) → review+
Comment on attachment 8891485 [details] [diff] [review]
Remove Preferences.jsm usage from some dom/push files

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

Thanks! Up to you if you want to pass a default argument, or leave this as is and have it throw.

::: dom/push/PushRecord.jsm
@@ +48,5 @@
>  PushRecord.prototype = {
>    setQuota(suggestedQuota) {
>      if (this.quotaApplies()) {
>        let quota = +suggestedQuota;
> +      this.quota = quota >= 0 ? quota : prefs.getIntPref("maxQuotaPerSubscription");

Should we maybe provide a default value as the second argument to `getIntPref`, just in case the pref isn't set or set to the wrong type?
Attachment #8891485 - Flags: review?(kit) → review+
Comment on attachment 8891485 [details] [diff] [review]
Remove Preferences.jsm usage from some dom/push files

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

::: dom/push/PushServiceHttp2.jsm
@@ +430,5 @@
>    },
>  
>    validServerURI: function(serverURI) {
>      if (serverURI.scheme == "http") {
> +      return !!prefs.getBoolPref("testing.allowInsecureServerURL");

This definitely needs to be `getBoolPref("testing.allowInsecureServerURL", false)`, though, since there's no default value for this pref in `all.js`.
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 18

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b3cced9169
Remove Preferences.jsm usage from workers test. r=bz
Attachment #8891494 - Flags: review?(MattN+bmo) → review+
Attachment #8891523 - Flags: review?(MattN+bmo) → review+
Attachment #8891525 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 21

2 years ago
Attachment #8891530 - Flags: review?(MattN+bmo)
Comment on attachment 8891530 [details] [diff] [review]
Remove Preferences.jsm usage from some toolkit modules

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

::: toolkit/modules/Locale.jsm
@@ -3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  this.EXPORTED_SYMBOLS = ["Locale"];
>  
> -const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

I would prefer you didn't change this. At least the front-end prefers to have the common 3/4 aliases in every file once we need one.

::: toolkit/modules/tests/xpcshell/test_client_id.js
@@ +83,5 @@
>  
>    // Assure that cached IDs are being checked for validity.
>    for (let invalidID of invalidIDs) {
>      await ClientID._reset();
> +    Services.prefs.setStringPref(PREF_CACHED_CLIENTID, invalidID);

Hmm… this is changing what the test is testing. It was previously sometimes going through setBoolPref or setIntPref for some of the invalidIDs but now you're always forcing this through setStringPref. Maybe you could have the invalidIDs array also contain tuples of values and pref method names to use. e.g.

const invalidIDs = [
  [-1, "setIntPref"],
…
  ["INVALID-UUID", "setStringPref"],
  [true, "setBoolPref"],
…
Attachment #8891530 - Flags: review?(MattN+bmo)
(Assignee)

Comment 23

2 years ago
Attachment #8891530 - Attachment is obsolete: true
Attachment #8891541 - Flags: review?(MattN+bmo)
Comment on attachment 8891541 [details] [diff] [review]
Remove Preferences.jsm usage from some toolkit modules

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

Thanks

::: toolkit/modules/tests/xpcshell/test_client_id.js
@@ +95,4 @@
>      let cachedID = ClientID.getCachedClientID();
>      Assert.strictEqual(cachedID, null, "ClientID should ignore invalid cached IDs");
> +    Assert.ok(!Services.prefs.prefHasUserValue(PREF_CACHED_CLIENTID), "ClientID should reset invalid cached IDs");
> +    Assert.ok(Services.prefs.getPrefType(PREF_CACHED_CLIENTID) == Ci.nsIPrefBranch.PREF_INVALID, "ClientID should reset invalid cached IDs");

You should probably wrap between some arguments as I think these are over 80 or 100
Attachment #8891541 - Flags: review?(MattN+bmo) → review+

Comment 25

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/302da7be5b9a
Remove Preferences.jsm usage from LaterRun.jsm. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e6fdd4a271
Remove Preferences.jsm import from password manager. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/11360bb6d7a1
Remove Preferences.jsm usage from addoncompat component. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d62cdcccad4
Avoid leaking Preferences property in viewsource test. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c4c9c19f3c
Remove Preferences.jsm usage from some dom/push/* files. r=kitcambridge
https://hg.mozilla.org/integration/mozilla-inbound/rev/65105a4351d1
Remove Preferences.jsm usage from XPCOMUtils.jsm. r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05ac296ea14
Remove Preferences.jsm usage from some toolkit modules. r=MattN
(Assignee)

Comment 30

2 years ago
This is needed in order to add Preferences.jsm to the browser_startup.js test, otherwise Marionette loads it really early.
Attachment #8891573 - Flags: review?(ato)
The reason we have preferred Preferences.jsm in the remote
protocol—and this may be besides the point of this bug—is that
nsIPrefBranch, with its getBoolPref/getCharPref/getIntPref et al.,
appears needlessly verbose next to Preferences.{get,set} which can
deduce a preference’s type.

If you consider
https://bugzilla.mozilla.org/attachment.cgi?id=8891573&action=diff,
the type inference adds code duplication that could be avoided if we
were not using the C++/XPCOM interface directly.
Comment on attachment 8891573 [details] [diff] [review]
Remove or delay Preferences.jsm usage from Marionette

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

::: testing/marionette/components/marionette.js
@@ +71,5 @@
>  
> +function getPrefVal(pref) {
> +  let prefType = Services.prefs.getPrefType(pref);
> +  let prefValue = undefined;
> +  if (prefType == Ci.nsIPrefBranch.PREF_STRING) {

To make this easier on the eye, you could make shorthands for PREF_STRING, PREF_BOOL et al.:

> const {PREF_STRING, PREF_BOOL, PREF_INT, PREF_INVALID} = Ci.nsIPrefBranch;

@@ +78,5 @@
> +    prefValue = Services.prefs.getBoolPref(pref);
> +  } else if (prefType == Ci.nsIPrefBranch.PREF_INT) {
> +    prefValue = Services.prefs.getIntPref(pref);
> +  } else if (prefType == Ci.nsIPrefBranch.PREF_INVALID) {
> +    prefValue = undefined;

I would prefer the use of a switch statement to reduce the amount of code here.

@@ +80,5 @@
> +    prefValue = Services.prefs.getIntPref(pref);
> +  } else if (prefType == Ci.nsIPrefBranch.PREF_INVALID) {
> +    prefValue = undefined;
> +  } else {
> +    throw new Error("Unexpected preference type (" + prefType + ") for " + pref + ".");

Use TypeError and a template string.  Also drop the final stop at the end.
Attachment #8891573 - Flags: review?(ato) → review+
(Assignee)

Comment 33

2 years ago
Attachment #8891742 - Flags: review?(alessio.placitelli)

Comment 35

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3362528e5e25
Remove or delay Preferences.jsm usage from Marionette. r=ato
(Assignee)

Updated

2 years ago
Attachment #8891485 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891488 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891492 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891494 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891496 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891523 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891525 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891541 - Flags: checkin+

Comment 36

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6025b5b43ec6
Fix max-len eslint rule breakage in marionette.js. r=me
(Assignee)

Comment 37

2 years ago
Patch updated to also remove Preferences.jsm from the newly introduced UpdatePing module.
Attachment #8891742 - Attachment is obsolete: true
Attachment #8891742 - Flags: review?(alessio.placitelli)
Attachment #8891834 - Flags: review?(alessio.placitelli)
Attachment #8891834 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Updated

2 years ago
Attachment #8891573 - Flags: checkin+

Comment 39

2 years ago
Comment on attachment 8891571 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/base/content/* files

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

Stealing. :-)

r=me with the below fixed.

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +19,4 @@
>      let iframe = document.getElementById("remote-report");
>      iframe.addEventListener("load", healthReportWrapper.initRemotePage);
>      iframe.src = this._getReportURI().spec;
> +    this.prefs.observe("uploadEnabled", this.updatePrefState, healthReportWrapper);

I think you can remove this, and do:

XPCOMUtils.defineLazyPreferenceGetter(this, /* unused */ "_isUploadEnabled", "datareporting.healthreport.uploadEnabled", false, () => this.updatePrefState());

and remove uninit() and callsites.

::: browser/base/content/browser-captivePortal.js
@@ +11,5 @@
>     * This constant is chosen to be large enough for a portal recheck to complete,
>     * and small enough that the delay in opening a tab isn't too noticeable.
>     * Please see comments for _delayedCaptivePortalDetected for more details.
>     */
> +  PORTAL_RECHECK_DELAY_MS: Services.prefs.getIntPref("captivedetect.portalRecheckDelayMS", 500),

Could use XPCOMUtils.defineLazyPreferenceGetter() to avoid the early read.

::: browser/base/content/sanitize.js
@@ +812,4 @@
>      // Reset the pref, so that if we crash before having a chance to
>      // sanitize on shutdown, we will do at the next startup.
>      // Flushing prefs has a cost, so do this only if necessary.
> +    Services.prefs.clearUserPref(Sanitizer.PREF_SANITIZE_DID_SHUTDOWN);

Given this code, the if() might as well just check .prefHasUserValue(), which though not strictly equivalent to Preferences.has() is more readable, and should work here (no point resetting to defaults if there is only a default value).

::: browser/extensions/shield-recipe-client/test/browser/browser_ClientEnvironment.js
@@ -1,4 @@
>  "use strict";
>  
> -Cu.import("resource://gre/modules/Services.jsm");
> -Cu.import("resource://gre/modules/Preferences.jsm");

The normandy / shield-recipe-client stuff will need upstreaming into the relevant github repo.
Attachment #8891571 - Flags: review?(mconley) → review+

Comment 40

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a2f780f966
Remove Preferences.jsm usage from some Telemetry files. r=Dexter

Comment 41

2 years ago
Comment on attachment 8891572 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/components/* files

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

::: browser/components/distribution.js
@@ +14,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> +                                  "resource://gre/modules/Preferences.jsm");

Could just rewrite the actual uses here to use the prefbranch directly?

::: browser/components/extensions/test/browser/head.js
@@ +33,5 @@
>  PromiseTestUtils.whitelistRejectionsGlobally(/Receiving end does not exist/);
>  
>  const {AppConstants} = Cu.import("resource://gre/modules/AppConstants.jsm", {});
>  const {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm", {});
> +const {Preferences} = Cu.import("resource://gre/modules/Preferences.jsm", {});

Why is this necessary?

::: browser/components/newtab/NewTabPrefsProvider.jsm
@@ +79,5 @@
>      for (let pref of gNewtabPagePrefs) {
> +      results[pref] = null;
> +
> +      if (Services.prefs.getPrefType(pref) != Ci.nsIPrefBranch.PREF_INVALID &&
> +          gPrefsMap.has(pref)) {

Why the check for gPrefsMap.has() ? Both of those lists look constant to me, and all the items in gNewtabPagePrefs are in gPrefsMap, right?
Attachment #8891572 - Flags: review?(mconley)
(Assignee)

Comment 42

2 years ago
(In reply to :Gijs from comment #41)
> Comment on attachment 8891572 [details] [diff] [review]
> Remove or delay Preferences.jsm usage from some browser/components/* files
> 
> Review of attachment 8891572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/distribution.js
> @@ +14,5 @@
> >  
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/Services.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> > +                                  "resource://gre/modules/Preferences.jsm");
> 
> Could just rewrite the actual uses here to use the prefbranch directly?

Do we know the types of the preferences here? Or do we need to implement some kind of type inference like Preferences.jsm does in Preferences.set?

> 
> ::: browser/components/extensions/test/browser/head.js
> @@ +33,5 @@
> >  PromiseTestUtils.whitelistRejectionsGlobally(/Receiving end does not exist/);
> >  
> >  const {AppConstants} = Cu.import("resource://gre/modules/AppConstants.jsm", {});
> >  const {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm", {});
> > +const {Preferences} = Cu.import("resource://gre/modules/Preferences.jsm", {});
> 
> Why is this necessary?

The file is using Preferences.jsm but was not importing it, assuming Preferences would be available as a global.

> 
> ::: browser/components/newtab/NewTabPrefsProvider.jsm
> @@ +79,5 @@
> >      for (let pref of gNewtabPagePrefs) {
> > +      results[pref] = null;
> > +
> > +      if (Services.prefs.getPrefType(pref) != Ci.nsIPrefBranch.PREF_INVALID &&
> > +          gPrefsMap.has(pref)) {
> 
> Why the check for gPrefsMap.has() ? Both of those lists look constant to me,
> and all the items in gNewtabPagePrefs are in gPrefsMap, right?

Right, I can remove this check, assuming this will always be true.

Comment 43

2 years ago
(In reply to Marco Castelluccio [:marco] from comment #42)
> (In reply to :Gijs from comment #41)
> > Comment on attachment 8891572 [details] [diff] [review]
> > Remove or delay Preferences.jsm usage from some browser/components/* files
> > 
> > Review of attachment 8891572 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/components/distribution.js
> > @@ +14,5 @@
> > >  
> > >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > >  Cu.import("resource://gre/modules/Services.jsm");
> > > +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> > > +                                  "resource://gre/modules/Preferences.jsm");
> > 
> > Could just rewrite the actual uses here to use the prefbranch directly?
> 
> Do we know the types of the preferences here? Or do we need to implement
> some kind of type inference like Preferences.jsm does in Preferences.set?

You would need the type inference, so perhaps that means it's not worth it...
(Assignee)

Comment 44

2 years ago
Carrying r+.
There are also a couple of additional tests where I removed Preferences.jsm, but they are really simple fixes. Feel free to double check.
Attachment #8891571 - Attachment is obsolete: true
Attachment #8891967 - Flags: review+
(Assignee)

Comment 45

2 years ago
Fixed what you asked in comment 41 (except distribution.js) and also removed Preferences.jsm usage from Experiments.jsm.
Attachment #8891572 - Attachment is obsolete: true
Attachment #8891968 - Flags: review?(gijskruitbosch+bugs)

Comment 46

2 years ago
Comment on attachment 8891968 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/components/* files

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

::: browser/experiments/Experiments.jsm
@@ +402,3 @@
>      this._log.trace("enabled=" + gExperimentsEnabled + ", " + this.enabled);
>  
> +    Services.prefs.addObserver(PREF_BRANCH + PREF_LOGGING, configureLogging);

This used to be a weak observer via the indirection of Preferences.jsm, and now isn't. It looks like it's a singleton so I imagine it doesn't matter, but I wanted to note it in case there are leaks when you push this.
Attachment #8891968 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 47

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0029dcf7b3e
TelemetrySend crash annotations are controlled by TelemetryEnabled pref on Android, FhrUploadEnabled on other platforms. r=me

Comment 48

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10de190a839b
Remove or delay Preferences.jsm usage from some browser/base/content/* files. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f64ae9515a8
Remove or delay Preferences.jsm usage from some browser/components/* files. r=Gijs

Comment 49

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9079c02563e7
Remove unneeded ternary operators to fix eslint failure. r=me
Attachment #8891565 - Flags: review?(mchang) → review+
(Assignee)

Comment 51

2 years ago
Once all the patches above land, Preferences.jsm is loaded in the "before handling user events" phase, so we can add it to the "before first paint" blacklist.

I'll morph this bug into getting Preferences.jsm out of the startup path, then I'll open a new bug to move it further away (maybe in "before becoming idle") and another bug to consider deprecating the module.
Attachment #8892078 - Flags: review?(gijskruitbosch+bugs)

Comment 52

2 years ago
Comment on attachment 8892078 [details] [diff] [review]
Add Preferences.jsm to browser_startup.js blacklist until "before handling user events" phase

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

rs=me
Attachment #8892078 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

2 years ago
Blocks: 1385952
(Assignee)

Updated

2 years ago
Blocks: 1385954
(Assignee)

Updated

2 years ago
No longer blocks: 1350472
No longer depends on: 1359040, 1385416
Summary: Consider removing Preferences.jsm → Stop loading Preferences.jsm before first paint

Comment 53

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45821738caeb
Disable pref observer TelemetrySend test on platforms where the crash reporter component is not available (like asan). r=me

Comment 54

2 years ago
Comment on attachment 8892075 [details] [diff] [review]
Remove Preferences.jsm usage from some browser/extensions/ modules

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

::: browser/extensions/e10srollout/bootstrap.js
@@ +231,5 @@
>    }
>  
> +  let value = 0.0;
> +  if (prefType == Ci.nsIPrefBranch.PREF_INT) {
> +    value = Services.prefs.getIntPref(pref) / 100;

Nit: please keep the comment ("convert old integer value")

::: browser/extensions/shield-recipe-client/lib/PreferenceExperiments.jsm
@@ +241,5 @@
>        lastSeen: new Date().toJSON(),
>        preferenceName,
>        preferenceValue,
>        preferenceType,
> +      previousPreferenceValue: getPref(preferences, preferenceName, preferenceType, undefined),

Because there's an arg defined it'll get the value undefined without you explicitly passing it in here, so I would just omit it here.
Attachment #8892075 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 55

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa81cd2510db
Remove Preferences.jsm usage from graphics sanity test. r=mchang

Comment 56

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b1b2f00e3f
Remove Preferences.jsm usage from some browser/extensions/ modules. r=Gijs
(Assignee)

Updated

2 years ago
Attachment #8891565 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891834 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891967 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8891968 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8892075 - Flags: checkin+
(Assignee)

Updated

2 years ago
No longer depends on: 720419
Attachment #8891564 - Flags: review?(rhelmer) → review+

Comment 59

2 years ago
Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/f30b605a0cf733e3024de29f534a4ca0462eb653
chore(bootstrap): Backport Preferences -> Services.prefs change from bug 1357517

https://github.com/mozilla/activity-stream/commit/8d800d6172eaba7e9075dc314e7f127a4ff1ba75
Merge pull request #3067 from Mardak/gh3065-backport

chore(bootstrap): Backport Preferences -> Services.prefs change from bug 1357517

Comment 60

2 years ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33788fc85648
Add Preferences.jsm to browser_startup.js blacklist until "before handling user events" phase. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/984c45066b41
Remove Preferences.jsm usage from some toolkit/mozapps/* files. r=rhelmer
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 61

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/33788fc85648
https://hg.mozilla.org/mozilla-central/rev/984c45066b41
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

2 years ago
Depends on: 1386883

Comment 62

2 years ago
Commits pushed to master at https://github.com/mozilla/normandy

https://github.com/mozilla/normandy/commit/912bcaed88db508eb85d3845051a971b4b450792
Bug 1357517 - Remove Preferences.jsm usage from some modules. r=Gijs

https://github.com/mozilla/normandy/commit/ff1b680c19f03653a1c758b724f69541d3822b10
Merge pull request #971 from mythmon/backport-1357517

Bug 1357517 - Remove Preferences.jsm usage from some modules.
Hmm I just noticed, while testing the clicktoplay-rollout addon, that there's a difference in behavior in undefined values. While it's all in JS, not passing an argument, or passing `undefined`, is the same. But it looks like in the XPCOM boundary, there's a difference in an explicit or implicit `undefined`.

Preferences.get("foo", undefined)       -> returns undefined
Preferences.get("foo")                  -> returns undefined

Services.prefs.getStringPref("foo", undefined) -> returns null
Services.prefs.getBoolPref("foo", undefined)   -> returns false
Services.prefs.getIntPref("foo", undefined)    -> returns 0
Services.prefs.getStringPref("foo")            -> *throws*
Services.prefs.getBoolPref("foo")              -> *throws*
Services.prefs.getIntPref("foo")               -> *throws*


So they are all different and should be looked more closely, but the last cases are the worst ones, as it causes the code to throw. For example, it caused this line to fail: https://hg.mozilla.org/mozilla-central/rev/68b1b2f00e3f#l2.32


(Note: this is not very important for the clicktoplay-rollout addon, as I'm removing it on bug 1390706, but I think there are other cases that changed in this bug that needs to be double checked)
Flags: needinfo?(mcastelluccio)
Depends on: 1397379
(Assignee)

Comment 64

2 years ago
Sorry for the delay, I was on PTO.

I did try not to alter the behavior by adding default values when needed or making sure that the calls were in a try block. I could have missed something though.

I've taken another look at all the patches, I believe the only one where I missed it is the clicktoplay rollout addon.
Flags: needinfo?(mcastelluccio)
Depends on: 1401249
Fixing the target milestone here. As can be seen on comment 57, the first pieces of this bug started landing on 56.
status-firefox56: --- → fixed
Target Milestone: Firefox 57 → Firefox 56

Updated

a year ago
Depends on: 1448361
You need to log in before you can comment on or make changes to this bug.