1946569 - Wait for New Tab built-in addon to be enabled before allowing navigations to about:home and about:newtab to complete

Status: RESOLVED FIXED

(Firefox :: New Tab Page, task)

Description

[Mike Conley (:mconley) (:⚙️)]

Normally, a known built-in addon is initialized very quickly during startup. There is, however, a race here - the initialization of the New Tab built-in addon via the AddonManager is async, and requests to load about:home / about:newtab may arrive before that initialization completes.

We need to solve this. I've been talking to valentin, and it might be possible to suspend the nsIChannel's for about:newtab / about:home until the addon is enabled.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

The following details are likely useful to inform plans around this:

• the WebExtensionPolicy webidl includes a readyPromise attribute which is created when the related extension has initiated its asynchonous startup and then resolved when the extension startup is far enough to be ready to resolve moz-extension url

• WebExtension::ReadyPromise is then used internally by the ExtensionProtocolHandler::SubstituteChannel here to wait for the extension readyPromise to be resolved before starting to serve data from requests to moz-extension urls

The internals described above have been introduced by Bug 1518863 - Blank new tab page displayed on start when an add-on controls the new tab page (due to race between page load and extension startup) to fix a race between Firefox starting up and trying to load a WebExtensions provided new tab page and the extension providing that new tab page starting up, and so it should be relevant and useful also for a buil-in addon provided new tab page..

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1946569 - Part 1: Create nsISuspendableChannelWrapper and implementation. r?valentin!

This implementation of nsIChannel wraps an inner nsIChannel, and blocks all
calls to asyncOpen until allowAsyncOpen is called on the nsISuspendedChannel.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1946569 - Part 2: Split out handling for about:newtab and about:home to a separate nsIAboutModule, replacing nsIAboutNewTabService. r?pdahiya!,Mossop!,#home-newtab-reviewers!

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1946569 - Part 3: Suspend all about:newtab or about:home channels until the built-in addon is ready. r?thecount!,pdahiya!,baku!

This uses the SharedData map to signal to the privileged about content process
that the New Tab built-in addon has initialized. I chose the SharedData map
because it has a pre-existing synchronization mechanism between processes,
and the value that it writes will not need to get refreshed in the event
that the privileged about content process crashes and needs to be respawned.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e6bd9219aac
Part 1: Create nsISuspendableChannelWrapper and implementation. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/1c7217d8e6fb
Part 2: Split out handling for about:newtab and about:home to a separate nsIAboutModule, replacing nsIAboutNewTabService. r=pdahiya,mossop,home-newtab-reviewers,omc-reviewers,chumphreys,thecount
https://hg.mozilla.org/integration/autoland/rev/23c635dd5036
Part 3: Suspend all about:newtab or about:home channels until the built-in addon is ready. r=thecount,pdahiya,home-newtab-reviewers,willdurand

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8e6bd9219aac
https://hg.mozilla.org/mozilla-central/rev/1c7217d8e6fb
https://hg.mozilla.org/mozilla-central/rev/23c635dd5036

Comment 7

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #5)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e6bd9219aac
Part 1: Create nsISuspendableChannelWrapper and implementation.
r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/1c7217d8e6fb
Part 2: Split out handling for about:newtab and about:home to a separate
nsIAboutModule, replacing nsIAboutNewTabService.
r=pdahiya,mossop,home-newtab-reviewers,omc-reviewers,chumphreys,thecount
https://hg.mozilla.org/integration/autoland/rev/23c635dd5036
Part 3: Suspend all about:newtab or about:home channels until the built-in
addon is ready. r=thecount,pdahiya,home-newtab-reviewers,willdurand

Perfherder has detected a browsertime performance change from push 23c635dd50363b9897e7458f19a9435bcf9cff7d.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
6%	welcome loadtime	windows11-64-24h2-shippable	cold fission webrender	37.93 -> 35.77	Before/After
5%	welcome fcp	windows11-64-24h2-shippable	cold fission webrender	73.07 -> 69.64	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 44272

For more information on performance sheriffing please see our FAQ.