Stop loading Preferences.jsm before first paint

RESOLVED FIXED in Firefox 57

Status

()

Firefox
General
RESOLVED FIXED
4 months ago
5 hours ago

People

(Reporter: florian, Assigned: marco)

Tracking

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

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

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(16 attachments, 6 obsolete attachments)

9.48 KB, patch
kitcambridge
: 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
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

4 months 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

4 months ago
Blocks: 1350472
Depends on: 1359040

Comment 1

4 months 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

4 months 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

23 days ago
Depends on: 720419
(Assignee)

Comment 3

22 days 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 4

21 days ago
Created attachment 8891485 [details] [diff] [review]
Remove Preferences.jsm usage from some dom/push files
Attachment #8891485 - Flags: review?(kit)
(Assignee)

Comment 5

21 days ago
Created attachment 8891486 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js
Attachment #8891486 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

21 days ago
Created attachment 8891488 [details] [diff] [review]
Remove Preferences.jsm usage from XPCOMUtils.jsm
Attachment #8891488 - Flags: review?(kmaglione+bmo)
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

21 days 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

21 days ago
Created attachment 8891491 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js
Attachment #8891486 - Attachment is obsolete: true
Attachment #8891491 - Flags: review?(bzbarsky)

Updated

21 days ago
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?
Depends on: 1385416
(Assignee)

Comment 11

21 days ago
Created attachment 8891492 [details] [diff] [review]
Remove Preferences.jsm usage from LaterRun.jsm
Attachment #8891492 - Flags: review?(jaws)
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 13

21 days ago
Created attachment 8891494 [details] [diff] [review]
Remove Preferences.jsm import from password manager
Attachment #8891494 - Flags: review?(MattN+bmo)
(Assignee)

Comment 14

21 days ago
Created attachment 8891496 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

Used SpecialPowers.pushPrefEnv.
Assignee: nobody → mcastelluccio
Attachment #8891491 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8891492 - Flags: review?(jaws) → review+
(Assignee)

Updated

21 days 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

21 days ago
Keywords: leave-open

Comment 18

21 days ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b3cced9169
Remove Preferences.jsm usage from workers test. r=bz
(Assignee)

Comment 19

21 days ago
Created attachment 8891523 [details] [diff] [review]
Remove Preferences.jsm usage from addoncompat/Prefetcher.jsm
Attachment #8891523 - Flags: review?(MattN+bmo)
(Assignee)

Comment 20

21 days ago
Created attachment 8891525 [details] [diff] [review]
Avoid leaking Preferences property in viewsource test
Attachment #8891525 - Flags: review?(MattN+bmo)
Attachment #8891494 - Flags: review?(MattN+bmo) → review+
Attachment #8891523 - Flags: review?(MattN+bmo) → review+
Attachment #8891525 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 21

21 days ago
Created attachment 8891530 [details] [diff] [review]
Remove Preferences.jsm usage from some toolkit modules
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

21 days ago
Created attachment 8891541 [details] [diff] [review]
Remove Preferences.jsm usage from some toolkit modules
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

21 days 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 26

21 days ago
Created attachment 8891564 [details] [diff] [review]
Remove Preferences.jsm usage from toolkit/mozapps/extensions/
Attachment #8891564 - Flags: review?(rhelmer)
(Assignee)

Comment 27

21 days ago
Created attachment 8891565 [details] [diff] [review]
Remove Preferences.jsm usage from GfxSanityTest.js
Attachment #8891565 - Flags: review?(mchang)
(Assignee)

Comment 28

21 days ago
Created attachment 8891571 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/base/content/* files
Attachment #8891571 - Flags: review?(mconley)
(Assignee)

Comment 29

21 days ago
Created attachment 8891572 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/components/* files
Attachment #8891572 - Flags: review?(mconley)
(Assignee)

Comment 30

21 days ago
Created attachment 8891573 [details] [diff] [review]
Remove or delay Preferences.jsm usage from Marionette

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

20 days ago
Created attachment 8891742 [details] [diff] [review]
Remove Preferences.jsm usage from some Telemetry files
Attachment #8891742 - Flags: review?(alessio.placitelli)

Comment 34

19 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b0b3cced9169
https://hg.mozilla.org/mozilla-central/rev/302da7be5b9a
https://hg.mozilla.org/mozilla-central/rev/19e6fdd4a271
https://hg.mozilla.org/mozilla-central/rev/11360bb6d7a1
https://hg.mozilla.org/mozilla-central/rev/8d62cdcccad4
https://hg.mozilla.org/mozilla-central/rev/68c4c9c19f3c
https://hg.mozilla.org/mozilla-central/rev/65105a4351d1
https://hg.mozilla.org/mozilla-central/rev/e05ac296ea14

Comment 35

19 days 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

19 days ago
Attachment #8891485 - Flags: checkin+
(Assignee)

Updated

19 days ago
Attachment #8891488 - Flags: checkin+
(Assignee)

Updated

19 days ago
Attachment #8891492 - Flags: checkin+
(Assignee)

Updated

19 days ago
Attachment #8891494 - Flags: checkin+
(Assignee)

Updated

19 days ago
Attachment #8891496 - Flags: checkin+
(Assignee)

Updated

19 days ago
Attachment #8891523 - Flags: checkin+
(Assignee)

Updated

19 days ago
Attachment #8891525 - Flags: checkin+
(Assignee)

Updated

19 days ago
Attachment #8891541 - Flags: checkin+

Comment 36

19 days 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

19 days ago
Created attachment 8891834 [details] [diff] [review]
Remove Preferences.jsm usage from some Telemetry files

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+
https://hg.mozilla.org/mozilla-central/rev/3362528e5e25
https://hg.mozilla.org/mozilla-central/rev/6025b5b43ec6
(Assignee)

Updated

19 days ago
Attachment #8891573 - Flags: checkin+

Comment 39

19 days 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

19 days 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

19 days 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

19 days 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

19 days 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

19 days ago
Created attachment 8891967 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/base/content/* files

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

19 days ago
Created attachment 8891968 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/components/* files

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

19 days 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

19 days 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

19 days 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

19 days 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

Updated

18 days ago
Attachment #8891565 - Flags: review?(mchang) → review+
(Assignee)

Comment 50

18 days ago
Created attachment 8892075 [details] [diff] [review]
Remove Preferences.jsm usage from some browser/extensions/ modules
Attachment #8892075 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 51

18 days ago
Created attachment 8892078 [details] [diff] [review]
Add Preferences.jsm to browser_startup.js blacklist until "before handling user events" phase

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

18 days 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

18 days ago
Blocks: 1385952
(Assignee)

Updated

18 days ago
Blocks: 1385954
(Assignee)

Updated

18 days 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

18 days 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

18 days 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

18 days 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

18 days 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

Comment 57

18 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1a2f780f966
https://hg.mozilla.org/mozilla-central/rev/e0029dcf7b3e
https://hg.mozilla.org/mozilla-central/rev/10de190a839b
https://hg.mozilla.org/mozilla-central/rev/0f64ae9515a8
https://hg.mozilla.org/mozilla-central/rev/9079c02563e7
https://hg.mozilla.org/mozilla-central/rev/45821738caeb
https://hg.mozilla.org/mozilla-central/rev/fa81cd2510db
https://hg.mozilla.org/mozilla-central/rev/68b1b2f00e3f
(Assignee)

Updated

18 days ago
Attachment #8891565 - Flags: checkin+
(Assignee)

Updated

18 days ago
Attachment #8891834 - Flags: checkin+
(Assignee)

Updated

18 days ago
Attachment #8891967 - Flags: checkin+
(Assignee)

Updated

18 days ago
Attachment #8891968 - Flags: checkin+
(Assignee)

Updated

18 days ago
Attachment #8892075 - Flags: checkin+
(Assignee)

Updated

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

Comment 59

17 days 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

17 days 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

16 days ago
Keywords: leave-open

Comment 61

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

Updated

15 days ago
Depends on: 1386883

Comment 62

9 days 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)
You need to log in before you can comment on or make changes to this bug.