Open Bug 1789727 Opened 2 years ago Updated 3 days ago

Default to the component version of Screenshots on Nightly

Categories

(Firefox :: Screenshots, task, P2)

task

Tracking

()

ASSIGNED

People

(Reporter: sfoster, Assigned: sfoster)

References

(Depends on 8 open bugs, Blocks 16 open bugs)

Details

Attachments

(1 file)

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

Depends on: 1748730
Depends on: 1777579
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: 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!
You need to log in before you can comment on or make changes to this bug.