1523609 - nsChromeRegistryChrome::CheckForNewChrome should not be called during shutdown

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement, P2)

Description

[Florian Quèze [:florian]]

See this portion of a shutdown profile: http://bit.ly/2CMRhdO

nsChromeRegistryChrome::CheckForNewChrome is called at least 3 times, causing main thread I/O at least 16 times in this profile (see in the "Marker Chart" panel the "stat" and "close" markers). It's fast-ish in this profile (about 30ms) because the profile was captured on a high-end machine with a fast SSD, but this has the potential to be very slow on slower hardware.

This seems to be triggered by the chromeHandle.destruct line at https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/browser/extensions/formautofill/api.js#141 and by the Components.manager.removeBootstrappedManifestLocation calls at https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/browser/extensions/formautofill/api.js#145 and https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/browser/extensions/webcompat-reporter/experimentalAPIs/l10n.js#21

A simple fix would be to just make these onShutdown methods return early during Firefox shutdown using the 'reason' parameter.

But this would remain a footgun for system add-on authors, so a better fix could be to make these methods not call nsChromeRegistryChrome::CheckForNewChrome when they are called during Firefox shutdown.

Comment 1

[Florian Quèze [:florian]]

Attachment: Bug 1523609 - avoid triggering nsChromeRegistryChrome::CheckForNewChrome (which does main thread I/O) during shutdown of system add-ons, r=kmag.

Comment 2

[Florian Quèze [:florian]]

Attachment: chrome-registry-shutdown

The patch on which I requested review was done based on local testing on Mac. Would strongly prefer this to be covered by a test so that we know it's OK on all platforms and all branches.

I'm attaching now the WIP where I tried to add an assertion (which I was expecting to make debug only once it would have worked) but couldn't figure out a way to do it, because AsyncShutdown receives the quit-application-granted notification and triggers the add-on shutdown before my observer in nsChromeRegistryChrome is called.

Yoric, is this the intended behavior? Currently AsyncShutdown spins the event loop but doesn't return from the observer notification, so other random runnables can run, but other observers of the same notification can't receive it.

If this is the intended behavior, is there another way to check from C++ if we are currently shutting down?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I seem to remember that AsyncShutdown should eventually return from the observer notification, once all shutdown blockers have been resolved. Isn't this the case?

At least according to https://searchfox.org/mozilla-central/source/toolkit/components/asyncshutdown/AsyncShutdown.jsm#530-540, the loop should eventually terminate. If that's not the case, I believe it's a bug.

Now, if you wish to add a C++ shutdown blocker, you can do this with the XPCOM API: https://searchfox.org/mozilla-central/source/toolkit/components/asyncshutdown/nsIAsyncShutdown.idl . Would this fulfill your needs?

Comment 4

[Florian Quèze [:florian]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #3)

I seem to remember that AsyncShutdown should eventually return from the observer notification, once all shutdown blockers have been resolved. Isn't this the case?

Yes, that's what it does, which blocks all the code trying to observe shutdown notifications without going through AsyncShutdown.

The case I have is some code that wants to be notified that shutdown is happening, without needing to block shutdown.

Even better would be a simple way to answer the question "are we currently shutting down?".

Comment 5

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] from comment #4)

Even better would be a simple way to answer the question "are we currently shutting down?".

There's currently no way to answer that other than adding an observer or blocker which sets a flag. It would be nice to have a standard place to get that information, given how much code we already have which follows that pattern, but we currently do not.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

It would be pretty easy to add an attribute to nsIAsyncShutdown specifying the latest shutdown phase we have encountered.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

If you have someone to write a patch to extend nsIAsyncShutdownService to specify the latest shutdown phase, I'll be happy to review it. Would this fulfill your need?

Comment 8

[Kris Maglione [:kmag]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #6)

It would be pretty easy to add an attribute to nsIAsyncShutdown specifying the latest shutdown phase we have encountered.

I don't think that's a good idea. Any reasonably sane caller that wanted to check that would have to compare it against a list of the phase they care about and all later phases. Which could, of course, change.

It would make more sense if each barrier had a flag for whether it had started or finished running.

Comment 9

[Florian Quèze [:florian]]

(In reply to Kris Maglione [:kmag] from comment #8)

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #6)

It would be pretty easy to add an attribute to nsIAsyncShutdown specifying the latest shutdown phase we have encountered.

I don't think that's a good idea. Any reasonably sane caller that wanted to check that would have to compare it against a list of the phase they care about and all later phases. Which could, of course, change.

Do you have realistic examples in mind for callers that would want to do more than "if shutdown has started, return early"?

Comment 10

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] from comment #9)

Do you have realistic examples in mind for callers that would want to do more than "if shutdown has started, return early"?

If all we care about is "if shutdown has started, return early" then that should just be a boolean flag. If we care about specific phases, we should store a flag on those barriers. Storing the last seen phase is just going to encourage people to do things they shouldn't.

That said, there are definitely cases where callers need to care about more than whether shutdown has started. We shut things down in phases, which means that after a given phase has finished, we need to not touch anything that was used in that phase. Doing so tends to result either in harmless/harmful errors, or in certain cases reinitializing things that were supposed to have been shut down already.

We can touch OS.File any time until its profileBeforeChange blocker has completed, for instance. If we touch it any time after that, we run into problems.

Comment 11

[Florian Quèze [:florian]]

Services.startup.shuttingDown already exists and seems good enough for this.

Comment 12

[Florian Quèze [:florian]]

Mochitests are green-ish on try with the patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08159d3b89826d226ccc4a4182744ecf7e5f27ca

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99b6adafbfa3
avoid triggering nsChromeRegistryChrome::CheckForNewChrome (which does main thread I/O) during shutdown of system add-ons, r=kmag.

Comment 14

[Andrei Ciure[:aciure]]

Backed out changeset 99b6adafbfa3 (bug 1523609) for causing nsCOMPtr asertion failures and nsIAppStartup failures

push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=235456474&revision=99b6adafbfa350899f7bc75b5cb8e2c9cd5d4d68

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=f857b15c8d3c48cda12f50db07445b4a04ce1743&selectedJob=235461206&searchStr=linux%2Cx64%2Cdebug%2Creftests%2Cwith%2Ce10s%2Ctest-linux64%2Fdebug-crashtest-e10s%2Cr-e10s%28c%29

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=f857b15c8d3c48cda12f50db07445b4a04ce1743&selectedJob=235463072&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Cwith%2Ce10s%2Ctest-linux64-asan%2Fopt-mochitest-browser-chrome-e10s-1%2Cm-e10s%28bc1%29

backout: https://hg.mozilla.org/integration/autoland/rev/1197bfc392f80694ea941efd156069f917ef3b27

Comment 15

[Florian Quèze [:florian]]

The crash is because RegistryEntries::Destruct() [AddonManagerStartup.cpp] is called late enough during shutdown that appStartup is null. I think we should just return when appStartup is null, I pushed this to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c4c653d766b654ca9129fa375579eb78be1ab16

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25a055417a5c
avoid triggering nsChromeRegistryChrome::CheckForNewChrome (which does main thread I/O) during shutdown of system add-ons, r=kmag.

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/25a055417a5c