Closed Bug 1789727 Opened 2 years ago Closed 5 months ago

Default to the component version of Screenshots on Nightly

Categories

(Firefox :: Screenshots, task, P2)

task

Tracking

()

RESOLVED FIXED
125 Branch
Tracking Status
firefox125 --- fixed

People

(Reporter: sfoster, Assigned: niklas)

References

(Depends on 9 open bugs, Blocks 3 open bugs, Regressed 1 open bug)

Details

(Keywords: perf-alert)

Attachments

(3 files)

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

Depends on: 1748730
Depends on: 1777579
Depends on: 1743887, 1785293
Depends on: 1790855
Depends on: 1796205
Depends on: 1799546
Depends on: 1799547
Depends on: 1801019
Depends on: 1801193
Blocks: 1801360
Depends on: 1805984
Depends on: 1805985
Blocks: 1489635
Blocks: 1750854
Depends on: 1820015
No longer depends on: 1796205
Depends on: 1827977
Blocks: 1823423
Blocks: 1830749
Depends on: 1832171
Depends on: 1832667
Depends on: 1851061
Blocks: 1849636
Blocks: 1853295
Blocks: 1854953
Blocks: 1854980
Blocks: 1858053
Blocks: 1860831
Blocks: 1623466
Blocks: 1863148
Blocks: 1870127
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ece3e5e72f03
Default to the component implementation of Screenshots for nightly builds. r=niklas

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/screenshots@mozilla.org.xpi 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?

Flags: needinfo?(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.

Flags: needinfo?(mconley)
Depends on: 1872764

: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?

Flags: needinfo?(mixedpuppy)
Attachment #9369494 - Attachment description: Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r?niklas! → WIP: Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r?niklas!
Depends on: 1873697
Depends on: 1873833
Depends on: 1873862
Depends on: 1873882
Depends on: 1873947
Depends on: 1873977

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

Flags: needinfo?(mixedpuppy) → needinfo?(lgreco)
Depends on: 1874014
Depends on: 1874026
Depends on: 1874096
Depends on: 1874154
Depends on: 1874172
Depends on: 1874230
No longer depends on: 1873862
No longer depends on: 1873882

(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. addon-search-detection@mozilla.com 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 screenshots@mozilla.org 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).

Flags: needinfo?(lgreco)
No longer depends on: 1874026
No longer depends on: 1874230
Blocks: 1870916
Depends on: 1874380
Depends on: 1874401
Depends on: 1874496
Depends on: 1874663
Depends on: 1874671
Depends on: 1874694
Depends on: 1874704
Depends on: 1874707
Depends on: 1874732
Depends on: 1875034
Depends on: 1875486
Blocks: 1876592
Depends on: 1876779
Depends on: 1878628
No longer depends on: 1874172
No longer depends on: 1878628
See Also: → 1878628
Blocks: 1879530
Blocks: 1534076
Depends on: 1881506
Attachment #9369494 - Attachment description: WIP: Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r?niklas! → 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
Attachment #9369494 - Attachment description: Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r?niklas! → WIP: Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r=sfoster
Attachment #9369494 - Attachment description: WIP: Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r=sfoster → Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r=sfoster
Assignee: sfoster → nbaumgardner
See Also: → 1882427
Pushed by nbaumgardner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b4da905ce36
Default to the component implementation of Screenshots for nightly builds. r=extension-reviewers,sfoster,robwu,chutten
Pushed by nbaumgardner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b88e57227bc
Default to the component implementation of Screenshots for nightly builds. r=extension-reviewers,sfoster,robwu,chutten
Pushed by nbaumgardner@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 125 Branch
Depends on: 1883717
Flags: needinfo?(nbaumgardner)

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

Flags: needinfo?(nbaumgardner)
Regressions: 1883765
Depends on: 1883965
No longer blocks: 1876592
Duplicate of this bug: 1876592
Regressions: 1884172
Depends on: 1884625
Depends on: 1884633

(In reply to Pulsebot from comment #16)

Pushed by nbaumgardner@mozilla.com:
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

Keywords: perf-alert
Duplicate of this bug: 1883891
Duplicate of this bug: 1877208
Duplicate of this bug: 1879409

(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

Depends on: 1885512
Depends on: 1885518
Regressions: 1886839
Regressions: 1886843
No longer depends on: 1885512
No longer depends on: 1884625
Depends on: 1890802
Depends on: 1891247
Depends on: 1893460
Depends on: 1896156
Flags: needinfo?(nbaumgardner)
Depends on: 1897152
No longer depends on: 1897152
Depends on: 1897831
No longer regressions: 1897371
Depends on: 1899519
Depends on: 1900331
Regressions: 1902303
Depends on: 1902683
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: