1360261 - GMPInstallManager causes jank off a timer in _delayedStartup

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[(no longer active)]

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.

Comment 2

[(no longer active)]

(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.

Comment 3

[Dão Gottwald [:dao]]

(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

Comment 4

[:shell escalante]

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

Comment 5

[Dale Harvey (:daleharvey)]

Attachment: remove-preference.patch

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 6

[Florian Quèze [:florian]]

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 7

[Dale Harvey (:daleharvey)]

Attachment: Bug 1360261 - Remove Preferences.jsm usage in GMPUtils.

Review commit: https://reviewboard.mozilla.org/r/154952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154952/

Comment 8

[Florian Quèze [:florian]]

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 9

[Dale Harvey (:daleharvey)]

Comment on attachment 8883995 [details]
Bug 1360261 - Remove Preferences.jsm usage in GMPUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154952/diff/1-2/

Comment 10

[Florian Quèze [:florian]]

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.

Comment 11

[Dale Harvey (:daleharvey)]

Comment on attachment 8883995 [details]
Bug 1360261 - Remove Preferences.jsm usage in GMPUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154952/diff/2-3/

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 8883995 [details]
Bug 1360261 - Remove Preferences.jsm usage in GMPUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154952/diff/3-4/

Comment 13

[Dale Harvey (:daleharvey)]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eab68649cc1b
Remove Preferences.jsm usage in GMPUtils. r=florian

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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.

Comment 16

[Florian Quèze [:florian]]

(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.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eab68649cc1b