Closed Bug 1523609 Opened 11 months ago Closed 9 months ago

nsChromeRegistryChrome::CheckForNewChrome should not be called during shutdown

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: florian, Assigned: kmag)

Details

(Whiteboard: [fxperf:p3])

Attachments

(2 files)

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.

Whiteboard: [fxperf] → [fxperf:p3]
Assignee: nobody → kmaglione+bmo
Priority: -- → P2

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?

Flags: needinfo?(dteller)

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?

Flags: needinfo?(dteller)

(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?".

(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.

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

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?

Flags: needinfo?(florian)

(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.

(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"?

(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.

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

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

Flags: needinfo?(florian)
Pushed by fqueze@mozilla.com:
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.

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

Pushed by fqueze@mozilla.com:
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.
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.