See this profile where GMPInstallManager causes 15ms of jank off a timer started in _delayedStartup: https://perfht.ml/2qjgDbE Here are few things that could be done to reduce this: - use requestIdleCallback instead of the timer at http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/browser/base/content/browser.js#1547 - avoid using Preferences.jsm in GMPUtils.jsm - avoid using NetUtil in GMPUtils.jsm. Is there a reason why getting the locale uses the complicated code at http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/toolkit/modules/UpdateUtils.jsm#119 instead of Services.locale.getRequestedLocale()? - replace UpdateUtils.jsm's formatUpdateURL method with Services.urlFormatter, or if that's not possible for some reason, reimplement it in a more efficient way. Eg. a single .replace call with a callback function that gets the values of the parameters only if they are actually part of the URL (see bug 1356593 for an example of what I mean). Note: GMPInstallManager also causes jank later by doing main thread I/O while installing updates; that's covered by bug 1149732.

I think moving this over to requestIdleCallback is definitely a great first step which should be done independently to making GMPInstallManager itself more efficient. There seems to be no good reason why this needs to run in exactly 1 minute.

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #1) > I think moving this over to requestIdleCallback is definitely a great first > step which should be done independently to making GMPInstallManager itself > more efficient. There seems to be no good reason why this needs to run in > exactly 1 minute. Posted a patch in bug 1360864 for this.

(In reply to Florian Quèze [:florian] [:flo] from comment #0) > See this profile where GMPInstallManager causes 15ms of jank off a timer > started in _delayedStartup: https://perfht.ml/2qjgDbE > > Here are few things that could be done to reduce this: - initialization should move to nsBrowserGlue since the module doesn't seem to care about individual windows

moving over to where code for plugins lives so the right folks can triage

The charPref usage in Services.prefs does not like null / undefined so replaced usage of that with "", I started switching all usage of GMPPrefs to use Services.prefs directly but this ended up a lot clearer a change even if the extra functions in GMPPrefs are a little ott I havent addressed the other issues, will do seperate patches

Comment on attachment 8882000 [details] [diff] [review] remove-preference.patch Review of attachment 8882000 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/GMPInstallManager.jsm @@ +199,1 @@ > DEFAULT_SECONDS_BETWEEN_CHECKS) nit: fix indent ::: toolkit/modules/GMPUtils.jsm @@ +144,5 @@ > * @param aDefaultValue The default value if no preference exists. > * @param aPlugin The plugin to scope the preference to. > * @return The obtained preference value, or the defaultValue if none exists. > */ > get(aKey, aDefaultValue, aPlugin) { This should be called "getString", keeping the generic "get" name is confusing when this method handles only one pref type. @@ +149,5 @@ > if (aKey === this.KEY_APP_DISTRIBUTION || > aKey === this.KEY_APP_DISTRIBUTION_VERSION) { > return Services.prefs.getDefaultBranch(null).getCharPref(aKey, "default"); > } > + return Services.prefs.getCharPref(this.getPrefKey(aKey, aPlugin), aDefaultValue); Preferences.get does not use .getCharPref, under the hood it's .getComplexValue(prefName, Ci.nsISupportsString).data, the modern equivalent of which is .getStringPref @@ +180,5 @@ > * @param aKey The preference key value to use. > * @param aVal The value to set. > * @param aPlugin The plugin to scope the preference to. > */ > set(aKey, aVal, aPlugin) { This should be called setString @@ +181,5 @@ > * @param aVal The value to set. > * @param aPlugin The plugin to scope the preference to. > */ > set(aKey, aVal, aPlugin) { > + Services.prefs.setCharPref(this.getPrefKey(aKey, aPlugin), aVal); use setStringPref instead of setCharPref ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm @@ +175,3 @@ > }, > set userDisabled(aVal) { > + GMPPrefs.setBool(GMPPrefs.KEY_PLUGIN_ENABLED, The indent was already oddly broken here, but let's fix it while we are touching this call. @@ +196,5 @@ > return permissions; > }, > > get updateDate() { > + let time = Number(GMPPrefs.getInt(GMPPrefs.KEY_PLUGIN_LAST_UPDATE, null, Did you mean 0 instead of null? @@ +201,1 @@ > this._plugin.id)); indent @@ +201,2 @@ > this._plugin.id)); > if (!isNaN(time) && this.isInstalled) { Maybe this check needs to be adjusted if 'time' will now be 0 instead of null when the pref doesn't exist.

Comment on attachment 8883995 [details] Bug 1360261 - Remove Preferences.jsm usage in GMPUtils. https://reviewboard.mozilla.org/r/154952/#review160258 ::: toolkit/modules/GMPInstallManager.jsm:70 (Diff revision 1) > // the normal URL but it does not check the cert. > - let url = GMPPrefs.get(GMPPrefs.KEY_URL_OVERRIDE); > + let url = GMPPrefs.getString(GMPPrefs.KEY_URL_OVERRIDE, ""); > if (url) { > log.info("Using override url: " + url); > } else { > - url = GMPPrefs.get(GMPPrefs.KEY_URL); > + url = GMPPrefs.getString(GMPPrefs.KEY_URL); Do you need a default value here, or is this pref always guaranteed to exist? http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/modules/libpref/init/all.js#5477 looks like it will always exist so that's probably fine. ::: toolkit/modules/GMPInstallManager.jsm:103 (Diff revision 1) > let url = this._getURL(); > > let allowNonBuiltIn = true; > let certs = null; > if (!Services.prefs.prefHasUserValue(GMPPrefs.KEY_URL_OVERRIDE)) { > - allowNonBuiltIn = !GMPPrefs.get(GMPPrefs.KEY_CERT_REQUIREBUILTIN, true); > + allowNonBuiltIn = !GMPPrefs.getString(GMPPrefs.KEY_CERT_REQUIREBUILTIN, true); The default value of getStringPref will always be a string, so true will be automatically converted to "true" here, which should be fine. ::: toolkit/modules/GMPUtils.jsm:185 (Diff revision 1) > + * @param aKey The preference key value to use. > + * @param aVal The value to set. > + * @param aPlugin The plugin to scope the preference to. > + */ > + setString(aKey, aVal, aPlugin) { > + Services.prefs.setCharPref(this.getPrefKey(aKey, aPlugin), aVal); setStringPref ::: toolkit/modules/UpdateUtils.jsm:66 (Diff revision 1) > * @param url > * The URL to format. > * @return The formatted URL. > */ > formatUpdateURL(url) { > - url = url.replace(/%PRODUCT%/g, Services.appinfo.name); > + const PARAM_REGEXP = /\%((?:\w+:)?\w+)(\??)\%/g; This can be simplified a lot, you don't need to support prefixes or optional parameters. I think you just need: /%(\w+)%/g And at this point it's probably simple enough that you can just inline it inside the .replace call. ::: toolkit/modules/UpdateUtils.jsm:67 (Diff revision 1) > * The URL to format. > * @return The formatted URL. > */ > formatUpdateURL(url) { > - url = url.replace(/%PRODUCT%/g, Services.appinfo.name); > - url = url.replace(/%VERSION%/g, Services.appinfo.version); > + const PARAM_REGEXP = /\%((?:\w+:)?\w+)(\??)\%/g; > + return url.replace(PARAM_REGEXP, (match, name, optional) => { You don't need the 'optional' parameter. ::: toolkit/modules/UpdateUtils.jsm:68 (Diff revision 1) > * @return The formatted URL. > */ > formatUpdateURL(url) { > - url = url.replace(/%PRODUCT%/g, Services.appinfo.name); > - url = url.replace(/%VERSION%/g, Services.appinfo.version); > - url = url.replace(/%BUILD_ID%/g, Services.appinfo.appBuildID); > + const PARAM_REGEXP = /\%((?:\w+:)?\w+)(\??)\%/g; > + return url.replace(PARAM_REGEXP, (match, name, optional) => { > + if (name === "PRODUCT") return Services.appinfo.name; nit: Our js code usually uses '==', unless '===' is specifically needed. This may be more readable as a switch/case. ::: toolkit/modules/UpdateUtils.jsm:79 (Diff revision 1) > - } > - url = url.replace(/%CHANNEL%/g, this.UpdateChannel); > - url = url.replace(/%PLATFORM_VERSION%/g, Services.appinfo.platformVersion); > - url = url.replace(/%DISTRIBUTION%/g, > - getDistributionPrefValue(PREF_APP_DISTRIBUTION)); > - url = url.replace(/%DISTRIBUTION_VERSION%/g, > + if (name === "CHANNEL") return this.UpdateChannel; > + if (name === "PLATFORM_VERSION") return Services.appinfo.platformVersion; > + if (name === "SYSTEM_CAPABILITIES") > + return getSystemCapabilities(); > + if (name === "CUSTOM") > + return Preferences.get(PREF_APP_UPDATE_CUSTOM, ""); This is the only use of Preferences.jsm in this file, can we remove it? :-) ::: toolkit/modules/UpdateUtils.jsm:83 (Diff revision 1) > - getDistributionPrefValue(PREF_APP_DISTRIBUTION)); > - url = url.replace(/%DISTRIBUTION_VERSION%/g, > - getDistributionPrefValue(PREF_APP_DISTRIBUTION_VERSION)); > - url = url.replace(/%CUSTOM%/g, Preferences.get(PREF_APP_UPDATE_CUSTOM, "")); > - url = url.replace(/\+/g, "%2B"); > - > + if (name === "CUSTOM") > + return Preferences.get(PREF_APP_UPDATE_CUSTOM, ""); > + if (name === "DISTRIBUTION") > + return getDistributionPrefValue(PREF_APP_DISTRIBUTION); > + if (name === "DISTRIBUTION_VERSION") > + return getDistributionPrefValue(PREF_APP_DISTRIBUTION_VERSION); Isn't this function going to cause a JS strict warning because it doesn't always return a value? I think it should 'return match;' at the end. ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm:140 (Diff revision 1) > get gmpPath() { > if (!this._gmpPath && this.isInstalled) { > this._gmpPath = OS.Path.join(OS.Constants.Path.profileDir, > this._plugin.id, > - GMPPrefs.get(GMPPrefs.KEY_PLUGIN_VERSION, > + GMPPrefs.getString(GMPPrefs.KEY_PLUGIN_VERSION, > null, this._plugin.id)); please fix indent here ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm:156 (Diff revision 1) > > get description() { return this._plugin.description; }, > get fullDescription() { return this._plugin.fullDescription; }, > > get version() { > - return GMPPrefs.get(GMPPrefs.KEY_PLUGIN_VERSION, null, > + return GMPPrefs.getString(GMPPrefs.KEY_PLUGIN_VERSION, null, this._plugin.id); I'm pleasantly surprised that getStringPref accepts null as a default value; note: getCharPref throws if null is provided as the default value. Maybe I should fix that to make these 2 consistent. ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm:199 (Diff revision 1) > }, > > get updateDate() { > - let time = Number(GMPPrefs.get(GMPPrefs.KEY_PLUGIN_LAST_UPDATE, null, > + let time = Number(GMPPrefs.getInt(GMPPrefs.KEY_PLUGIN_LAST_UPDATE, 0, > - this._plugin.id)); > + this._plugin.id)); > if (!isNaN(time) && this.isInstalled) { time will always be a number now, should '!isNaN(time)' be changed to 'time' ?

Comment on attachment 8883995 [details] Bug 1360261 - Remove Preferences.jsm usage in GMPUtils. https://reviewboard.mozilla.org/r/154952/#review161282 ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm:140 (Diff revision 2) > get gmpPath() { > if (!this._gmpPath && this.isInstalled) { > this._gmpPath = OS.Path.join(OS.Constants.Path.profileDir, > this._plugin.id, > - GMPPrefs.get(GMPPrefs.KEY_PLUGIN_VERSION, > + GMPPrefs.getString(GMPPrefs.KEY_PLUGIN_VERSION, > null, this._plugin.id)); The indent still needs fixing here.

Fixed the indent, also eslint didnt enjoy the spaced out switch statement so just put them on newlines, try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=97095052fd1ebcb28729360184c47d7b35dbaba9 Cheers

Pushed by dharvey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eab68649cc1b Remove Preferences.jsm usage in GMPUtils. r=florian

(In reply to Florian Quèze [:florian] [:flo] from comment #0) > - avoid using NetUtil in GMPUtils.jsm. Is there a reason why getting the > locale uses the complicated code at > http://searchfox.org/mozilla-central/rev/ > ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/toolkit/modules/UpdateUtils.jsm#119 > instead of Services.locale.getRequestedLocale()? There may be. `getRequestedLocales` may be changed by users by hand and soon from Preferences window. Also, the requested is not necessary the app's locale - user may request ['de', 'fr', 'en-US'], and if we only have locales for 'fr' and 'en-US', then their Firefox will be in 'fr'. There's also `getAppLocales` which shows only negotiated locales (so, ['fr', 'en-US'] in the above case), but this one can also change if the user downloads a 'de' language pack and installs it. Now, if our update mechanism will handle downloading an update for a different locale than the original app was installed for, then I believe that we should use `getAppLocales`. In that scenario the order would be: 1) User downloads Firefox in language A 2) User downloads language pack in language B 3) User switches his locale to B 4) We download an update for Firefox in locale B 5) Profit... But if that doesn't work, and we need to keep the user on the same locale that the original app was downloaded in, then `update.locale` file is the only reliable way to keep it. I would love to see us get rid of this file, but I'm not sure if we can.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15) Ok, it seems we do need to read update.locale, but we should stop doing main thread I/O for it, and stop using NetUtil there. UpdateUtils.Locale is apparently only used by UpdateUtils.formatUpdateURL, which only has 3 callers (+ 2 from tests) : http://searchfox.org/mozilla-central/search?q=formatUpdateURL But this should probably go in a new bug.