Enable delayed background page startup on Nightly

VERIFIED FIXED in Firefox 62

Status

P1
normal
VERIFIED FIXED
11 months ago
8 months ago

People

(Reporter: aswan, Assigned: aswan)

Tracking

(Depends on: 1 bug)

unspecified
mozilla62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 verified)

Details

Attachments

(2 attachments)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8972235 - Flags: review?(kmaglione+bmo)
Attachment #8972236 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

11 months ago
Priority: -- → P1
(Assignee)

Updated

11 months ago
Attachment #8972235 - Flags: review?(tomica)
Attachment #8972236 - Flags: review?(tomica)

Comment 4

11 months ago
mozreview-review
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("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
>  
> +Services.prefs.setBoolPref("extensions.webextensions.background-delayed-startup", false);

How come this isn't required for all xpcshell tests?
Attachment #8972236 - Flags: review?(tomica) → review+
(Assignee)

Comment 5

11 months ago
mozreview-review-reply
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

11 months ago
mozreview-review
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
Attachment #8972235 - Flags: review?(tomica) → review+
(Assignee)

Comment 7

11 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/933708924c6f
https://hg.mozilla.org/mozilla-central/rev/7201982e0481
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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

Updated

11 months ago
Depends on: 1460607

Comment 13

10 months ago
Can you please add some STRs to this bug(and test extension if possible) or mark it as "qe-verify-" ?
Flags: needinfo?(aswan)
(Assignee)

Comment 14

10 months ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=1447551#c23 and also separate email thread with addonsqa
Flags: needinfo?(aswan)

Updated

9 months ago
Product: Toolkit → WebExtensions
Attachment #8972236 - Flags: review?(kmaglione+bmo)
Attachment #8972235 - Flags: review?(kmaglione+bmo)

Comment 15

8 months ago
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.)
Status: RESOLVED → VERIFIED

Updated

8 months ago
status-firefox62: fixed → verified
You need to log in before you can comment on or make changes to this bug.