1457224 - Enable delayed background page startup on Nightly

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P1)

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1457224 Clean up extension testing helpers

- Remove outdated appChanged logic from test helper function to start
  the AddonManager
- Avoid calling to the AddonManager in webextension tests that don't
  otherwise require the AddonManager

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

Comment 2

[Andrew Swan [:aswan]]

Attachment: Bug 1457224 Enable delayed background page startup on Nightly

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

Comment 3

[Andrew Swan [:aswan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79ae27f13215d9a3dd5a9dc0df8cfb9fb1aa7f5a

Comment 4

[Tomislav Jovanovic :zombie]

Comment on attachment 8972236 [details]
Bug 1457224 Enable delayed background page startup on Nightly

https://reviewboard.mozilla.org/r/240904/#review248044

::: toolkit/components/extensions/test/xpcshell/test_ext_permissions.js:14
(Diff revision 1)
>  
>  AddonTestUtils.init(this);
>  AddonTestUtils.overrideCertDB();
>  AddonTestUtils.createAppInfo("[e-mail]", "XPCShell", "1", "42");
>  
> +Services.prefs.setBoolPref("extensions.webextensions.background-delayed-startup", false);

How come this isn't required for all xpcshell tests?

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8972236 [details]
Bug 1457224 Enable delayed background page startup on Nightly

https://reviewboard.mozilla.org/r/240904/#review248044

> How come this isn't required for all xpcshell tests?

This is only an issue for tests that use the addon manager (ExtensionTestUtils.loadExtension() bypasses the addon manager unless you explicitly supply the useAddonManager option).  And for tests that do use the addon manager, its only an issue if they restart the manager (delayed background script startup only applies during browser startup, the behavior when an extension is installed for the first time isn't affected)

Comment 6

[Tomislav Jovanovic :zombie]

Comment on attachment 8972235 [details]
Bug 1457224 Clean up extension testing helpers

https://reviewboard.mozilla.org/r/240902/#review248048

::: toolkit/components/extensions/ExtensionTestCommon.jsm:390
(Diff revision 1)
>        id,
>        resourceURI: jarURI,
>        cleanupFile: file,
>        signedState,
>        temporarilyInstalled: !!data.temporarilyInstalled,
> +      TEST_NO_ADDON_MANAGER: true,

nit: name isn't exactly in style, don't think we ever use that for something not global or static.

::: toolkit/components/extensions/test/xpcshell/test_ext_webRequest_startup.js:50
(Diff revision 1)
>      },
>    });
>  
>    await extension.startup();
>  
> -  await promiseRestartManager(false);
> +  await promiseRestartManager();

Was passing `false` ever right?  Can you please add a JSDoc to `promiseRestartManager` explaining the correct usage.

Also, looks like you missed one https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_proxy_startup.js#90

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:733
(Diff revision 1)
>    },
>  
>    /**
>     * Starts up the add-on manager as if it was started by the application.
>     *
>     * @param {boolean} [appChanged = true]

Update the JSDoc please.

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:675
(Diff revision 1)
>      Assert.equal(aActual[size], aExpected[size]);
>    }
>  }
>  
> -function startupManager(aAppChanged) {
> -  promiseStartupManager(aAppChanged);
> +function startupManager() {
> +  promiseStartupManager();

This is not returning the promise, and not getting awaited anywhere.  That's either 1) on purpose, or 2) a mistake from when startup was converted to async.  Please document or fix accordingly.


Also, looks like you missed one https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_distribution.js#72

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8972235 [details]
Bug 1457224 Clean up extension testing helpers

https://reviewboard.mozilla.org/r/240902/#review248048

> Was passing `false` ever right?  Can you please add a JSDoc to `promiseRestartManager` explaining the correct usage.
> 
> Also, looks like you missed one https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_proxy_startup.js#90

It might have been right ages ago, this code has evolved quite a bit recently.
I'll fix the proxy test, that was added since I wrote this patch.

> This is not returning the promise, and not getting awaited anywhere.  That's either 1) on purpose, or 2) a mistake from when startup was converted to async.  Please document or fix accordingly.
> 
> 
> Also, looks like you missed one https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_distribution.js#72

Ugh, this is kind of a mess.  Short story is I don't think this is a problem in practice and we want to get rid of all the synchronous test apis in favor of asynchronous ones.  But I'll add some comments to hopefully reduce future confusion.

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8972235 [details]
Bug 1457224 Clean up extension testing helpers

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

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8972236 [details]
Bug 1457224 Enable delayed background page startup on Nightly

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

Comment 10

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/933708924c6fac7fe66962ebf79d9aa91888e960
Bug 1457224 Clean up extension testing helpers r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/7201982e0481cbc237a21756ef88827c4dcfb2c8
Bug 1457224 Enable delayed background page startup on Nightly r=zombie

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/933708924c6f
https://hg.mozilla.org/mozilla-central/rev/7201982e0481

Comment 12

[Ionuț Goldan [:igoldan]]

Congratulations for the perf wins! \0/

== Change summary for alert #13155 (as of Tue, 08 May 2018 16:44:13 GMT) ==

Improvements:

  6%  ts_paint_webext osx-10-10 opt e10s stylo     848.50 -> 796.17
  3%  ts_paint_webext windows7-32 pgo e10s stylo   386.25 -> 373.83
  3%  ts_paint_webext windows7-32 opt e10s stylo   422.33 -> 410.92
  3%  ts_paint_webext windows10-64 opt e10s stylo  385.75 -> 375.58
  3%  ts_paint_webext windows10-64 pgo e10s stylo  358.79 -> 349.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13155

Comment 13

[Victor Carciu]

Can you please add some STRs to this bug(and test extension if possible) or mark it as "qe-verify-" ?

Comment 14

[Andrew Swan [:aswan]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1447551#c23 and also separate email thread with addonsqa

Comment 15

[Victor Carciu]

Tested and verified on Firefox 62.0b15 (Latest) in Windows 10 64Bit and MacOS 10.13.2.
Tested startup scenarios with the most popular Privacy & Security extensions(Adblock Plus, uBlock Origin , Avast Online Security, LastPass Password Manager, etc.)