1789727 - Default to the component version of Screenshots on Nightly

Status: RESOLVED FIXED

(Firefox :: Screenshots, task, P2)

Description

[Sam Foster [:sfoster] (he/him)]

Switch the screenshots.browser.component.enabled pref to true for nightly only

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r=sfoster

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ece3e5e72f03
Default to the component implementation of Screenshots for nightly builds. r=niklas

Comment 3

[Cristina Horotan [:chorotan]]

Backed out changeset ece3e5e72f03 (bug 1789727) for causing telemetry failures at test_subsession_management.py

Backout: https://hg.mozilla.org/integration/autoland/rev/58e3241edab78a271810244d5a6838c771434308

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=ece3e5e72f033ff947400e1c496c5e72123c04f3&selectedTaskRun=dUMZpaZcQL2j1y-10LlULA.0

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=440774527&repo=autoland&lineNumber=2791
https://treeherder.mozilla.org/logviewer?job_id=440773305&repo=autoland&lineNumber=3456

Comment 4

[Sam Foster [:sfoster] (he/him)]

There's 2 failures there. I don't really understand how the telemetry failures from test_subsession_management.py could be related to this patch. But perhaps the browser_startup_content_mainthreadio.js one is a clue? It says there was an unexpected close on /builds/worker/workspace/build/application/firefox/browser/features/[e-mail] in WebExtensions process. Ideally we shouldn't load the screenshots extension at all - loading and unloading/closing it during startup is a problem. But I don't know enough about how these built-in extensions (or extensions in general) interact with startup to say more. Does the presence of the extension implicitly require loading it at startup?

It looks like we call _monitorScreenshotsPref from _onWindowsRestored in BrowserGlue. Is that already too late?

mconley, I'm picking on you as I know you've spent time in the startup sequence, but please redirect if necessary. Can you advise on how to proceed here? Are the results expected given the pref change, or do I need to do more to ensure we only load the extension when necessary? Am I even understanding the problem correctly?

Comment 5

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

Hey sfoster,

It's unclear if the Telemetry failure is related to the mainthread IO test failure. I certainly support investigating one at a time, though, in the hopes that fixing one fixes the other.

Ideally we shouldn't load the screenshots extension at all - loading and unloading/closing it during startup is a problem. But I don't know enough about how these built-in extensions (or extensions in general) interact with startup to say more. Does the presence of the extension implicitly require loading it at startup?

It looks like we call _monitorScreenshotsPref from _onWindowsRestored in BrowserGlue. Is that already too late?

Looking at _monitorScreenshotsPref, what I infer is that by default we have the screenshots XPI loaded, but if the component pref is set, then we unload that extension. I suspect this is what's causing the "close" disk event in the WebExtension process main thread - it's receiving the message to close its file handle to the screenshots XPI.

That failing test uses the StartupRecorder, which basically dumps a profile from process start up and looks for file IO markers on the main threads of any opened processes. That'd certainly encompass _onWindowsRestored.

There are a few ways to approach this:

1. We could add an exception here, with the understanding that this exception will be removed once the _monitorScreenshotsPref and the screenshots XPI is removed once and for all. This is probably simplest, but might also be my least favourite, since it wallpapers over the main-thread IO that we're adding here by shutting down the screenshots XPI.
2. Try moving _monitorScreenshotsPref out from _onWindowsRestored to one of the tasks in _scheduleStartupIdleTasks. This might require you to update your tests to ensure that the component has initialized before the tests run. Before you do that though, I'd be curious to know if moving that out to _scheduleStartupIdleTasks gets rid of one (or both!) of these failures, before you look at adjusting the screenshots tests.

And if (2) ends up being a big hairy ball, then I'd be fine falling back to (1), since it'd be temporary anyways.

Comment 6

[Sam Foster [:sfoster] (he/him)]

:mixedpuppy, I think we're running into an issue here where the built-in screenshots extension is automatically loaded and made active. So reading the pref in the _onWindowsRestored method in BrowserGlue where we find out we didn't want the extension - is already too late. Ideally we would not load it at all for the default case where the component (which replaces this built-in extension) is pref'd on. Is that possible? (And for nightly only.)

I guess this is bug 1489527 again. But in lieu of that, what is my next-best option here?

Comment 7

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

I'm catching up with the patch attached to this bugzilla issue and comments related to the failure hit by that patch, so that I can provide perspective from the AddonManager side about what would be the best path forward, and so I'm redirecting the needinfo to myself

Comment 8

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

(In reply to Sam Foster [:sfoster] (he/him) from comment #6)

:mixedpuppy, I think we're running into an issue here where the built-in screenshots extension is automatically loaded and made active. So reading the pref in the _onWindowsRestored method in BrowserGlue where we find out we didn't want the extension - is already too late. Ideally we would not load it at all for the default case where the component (which replaces this built-in extension) is pref'd on. Is that possible? (And for nightly only.)

I guess this is bug 1489527 again. But in lieu of that, what is my next-best option here?

Hi Sam, apologies for letting you wait a bit more, I managed to catch up with this bug yesterday and look into this a bit more today (from both "new profile" and "existing profile" perspectives) to gather some thoughts and pass them through some of my teammates to also gather their thoughts about it.

The very short summary is that, the easiest path forward to have this landed in nightly would be to defer during startup to disable the addon (to avoid triggering additional work due to the extension shutdown during the browser startup, and avoiding to hit the failure on that content mainthead io smoke test as a side effect of that), especially if the extension is meant to be still enabled/disabled at runtime by the pref controlling it.

If I'm not misreading your last comment on phabricator, this is a change you have already applied to the attached patch, does that patch still hit the two test failure that triggered a backout of the previous version of the patch?
(I was not able to hit the one from browser_startup_content_mainthreadio.js when I tried it on the current version of the patch, at least not on my linux dev machine)

---

As side notes about instead avoiding to start the extension all together:

For new profiles: if we would like to avoid even installing the screenshot extension unless necessary, we could consider removing it from the built_in_addons.json file and instead installing it if/when necessary (e.g. if the pref is flipped) explicitly using AddonManager.maybeInstallBuiltinAddon (e.g. [e-mail] is a builtin using this approach to be installed), and that should be simple enough and it wouldn't require any change to the AOM or XPIProvider internals.

For existing profiles: things are a bit trickier, and in particular we have to account for the fact that the [e-mail] builtin is actually already installed and enabled, which means that:

• XPIProvider.startup will find an entry for the screenshots builtin in the data loaded earlier on startup from the "addonStartup.json.lz4" file (we call this XPIStates) and will be starting it up pretty earlier in the application startup
• and in addition to the XPIStates, there will be also an entry for the screenshots builtin in the XPI DB loaded from the "extensions.json" file stored in the profile, and the two entries are meant to stay in sync with each other (I know, that's kind of obvious).

Making sure that we account for the pref (e.g. through a mapping table between addon ids and a lazy pref getter) during the XPIProvider.startup to avoid starting the screenshot extension would be possible but it will require a bit more work, and more carefullness to be sure to get it right and don't introduce inconsistences in the XPIStates and XPI DB, and so it may not be something we may want to block this bug on (given that I think we want to have it baking in nightly sooner rather than later).

Comment 9

[Niklas Baumgardner [:niklas]]

Attachment: WIP: Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r?niklas!

• Flip the component pref to true by default for nightly builds only
• Move the pref check and initialization to a startup idle task
• And be a bit smarter about when we get and disable the addon
• Fix a bug where we try to communicate with the overlay after the window actor is destroyed when
  the component pref gets flipped off during use

Comment 10

[Sandor Molnar[:smolnar]]

Backed out for causing telemetry/marionette/tests/client/test_subsession_management.py

Backout link: https://hg.mozilla.org/integration/autoland/rev/45e7664976394cd541a72b2ea43873d1ca46efda

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | telemetry/marionette/tests/client/test_subsession_management.py TestSubsessionManagement.test_subsession_management | AssertionError: 'ef616f3a-7680-4cee-94b2-56f32cb778c3' is not None

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b4da905ce36
Default to the component implementation of Screenshots for nightly builds. r=extension-reviewers,sfoster,robwu,chutten

Comment 12

[Noemi Erli[:noemi_erli]]

Backed out changeset 9b4da905ce36 (Bug 1789727) for causing failures in browser_asrouter_toolbarbadge.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=U7FksB2rSu2oU61HeBCVhg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=9b4da905ce367275c1a76655834ec86d6d3f90be

Log: https://treeherder.mozilla.org/logviewer?job_id=448999171&repo=autoland&lineNumber=4504

Backout: https://hg.mozilla.org/integration/autoland/rev/b80118ceef958531ef4972eed8ec1b19535e9b55

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b88e57227bc
Default to the component implementation of Screenshots for nightly builds. r=extension-reviewers,sfoster,robwu,chutten

Comment 14

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures @ browser_feature_callout_in_chrome.js & browser_asrouter_toolbarbadge

Backout link: https://hg.mozilla.org/integration/autoland/rev/af32c3581b958ea86afab3e8d3b020fef3c794ec

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/components/asrouter/tests/browser/browser_asrouter_toolbarbadge.js | A promise chain failed to handle a rejection: The ExtensionSettingsStore was accessed before the initialize

Failure log-> TEST-UNEXPECTED-FAIL | browser/components/asrouter/tests/browser/browser_feature_callout_in_chrome.js | A promise chain failed to handle a rejection: The ExtensionSettingsStore was accessed before the initialize

Comment 15

[Niklas Baumgardner [:niklas]]

Attachment: Bug 1789727 - Initialize ExtensionSettingsStore before use to prevent test failures. r=mconley

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a66012be9d9
Default to the component implementation of Screenshots for nightly builds. r=extension-reviewers,sfoster,robwu,chutten
https://hg.mozilla.org/integration/autoland/rev/5b706eb21870
Initialize ExtensionSettingsStore before use to prevent test failures. r=mconley,settings-reviewers

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7a66012be9d9
https://hg.mozilla.org/mozilla-central/rev/5b706eb21870

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This seems like something we should call out in the Nightly release notes. Please add a relnote-firefox:? nomination if you agree.

Comment 20

[Florin Bilt]

(In reply to Pulsebot from comment #16)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a66012be9d9
Default to the component implementation of Screenshots for nightly builds.
r=extension-reviewers,sfoster,robwu,chutten
https://hg.mozilla.org/integration/autoland/rev/5b706eb21870
Initialize ExtensionSettingsStore before use to prevent test failures.
r=mconley,settings-reviewers

== Change summary for alert #41678 (as of Wed, 06 Mar 2024 18:17:17 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
0.34%	Base Content JS	windows11-64-2009-shippable-qr	fission	1,510,984.00 -> 1,505,833.33
0.29%	Base Content JS	linux1804-64-shippable-qr	fission	1,505,080.00 -> 1,500,697.33

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41678

Comment 24

[Alex Finder]

(In reply to Sandor Molnar[:smolnar] from comment #14)

Backed out for causing bc failures @ browser_feature_callout_in_chrome.js & browser_asrouter_toolbarbadge

Backout link: https://hg.mozilla.org/integration/autoland/rev/af32c3581b958ea86afab3e8d3b020fef3c794ec

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/components/asrouter/tests/browser/browser_asrouter_toolbarbadge.js | A promise chain failed to handle a rejection: The ExtensionSettingsStore was accessed before the initialize

Failure log-> TEST-UNEXPECTED-FAIL | browser/components/asrouter/tests/browser/browser_feature_callout_in_chrome.js | A promise chain failed to handle a rejection: The ExtensionSettingsStore was accessed before the initialize

== Change summary for alert #41704 (as of Fri, 08 Mar 2024 03:57:11 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
23%	amazon LastVisualChange	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	1,459.24 -> 1,794.37	Before/After

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
3%	reddit FirstVisualChange	android-hw-a51-11-0-aarch64-shippable-qr	cold webrender	1,063.29 -> 1,026.58	Before/After

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41704