😴 Skip some KeepAlive registrations on shutdown.

chrome/browser/extensions/chrome_extension_function.cc

Index: chrome/browser/extensions/chrome_extension_function.cc
diff --git a/chrome/browser/extensions/chrome_extension_function.cc b/chrome/browser/extensions/chrome_extension_function.cc
index 07c3006d98fb9fc4ef3dbebc71d3783fb0ab1e5a..5dab504dce511377347e9006a6598d170274ffad 100644
--- a/chrome/browser/extensions/chrome_extension_function.cc
+++ b/chrome/browser/extensions/chrome_extension_function.cc
@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/chrome_extension_function_details.h"
 #include "chrome/browser/extensions/window_controller.h"
 #include "chrome/browser/extensions/window_controller_list.h"
@@ -100,6 +101,11 @@ ChromeAsyncExtensionFunction::ChromeAsyncExtensionFunction() {
 ChromeAsyncExtensionFunction::~ChromeAsyncExtensionFunction() {}
 
 ExtensionFunction::ResponseAction ChromeAsyncExtensionFunction::Run() {
+  if (g_browser_process->IsShuttingDown()) {
+    error_ = "Shutting down.";
+    return RespondNow(Error(error_));

[Peter Beverloo, 2016/07/05 12:42:10]

Is this potentially developer-visible at this point in the process' lifetime? If
so, it would be good to be more descriptive about *what* is shutting down— my
gut feel upon receiving this message would be the extension/app.

What about "The browser is shutting down."?

[Devlin, 2016/07/06 16:54:23]

ChromeAsyncExtensionFunction is just one of many ExtensionFunction variants.  If
your intention is to do this for all extension functions, this should be done
somewhere in ExtensionFunction, rather than ChromeAsyncExtensionFunction.  If
you only need this for a specific extension function, then it show go there
(though I'd also be curious why only one needs this treatment).

Additionally, ExtensionFunctions can run very asynchronously.  So let's say
Run() is called and we're not shutting down, but then we post a task for
something else to happen, and then we start shutting down.  The task will be
called, but I don't think we really check IsShuttingDown() in any of them.

The right-est fix for this would probably be to unrefcount ExtensionFunction and
have it use weak ptrs for its tasks, and then have it owned by something
reasonable that will be destroyed naturally in the course of shut down.  But
that will be a massive refactoring.

[dgn, 2016/07/07 15:20:17]

Thanks for the explanation. I'm not too keen on starting a massive refactoring
now. 

The functions I noticed to be a potential issue are WindowsCreateFunction,
TabsCreateFunction, BrowserOpenTabFunction (creates new browser, other functions
seem to either not create any or properly check for a given window id),
NotificationsCreateFunction (creates new notifications = keepAlive). The related
code is synchronous until the point where we care, so checking that we are
shutting down in ExtensionFunction would fix the problem.

But if that is inherently going to be unreliable for other cases, checking for
these functions specifically (through extension_tab_util.cc for example) could
be fine too.

[Devlin, 2016/07/08 01:01:01]

If this solves the immediate problem, we can avoid the longer term solution for
now (since this is strictly better than the current situation).  However, I'd
still like to put this a little higher up (e.g. in ExtensionFunction somewhere
rather than ChromeAsyncExtensionFunction) so that it gets better exposure. 
Additionally, a TODO that summarizes the important pieces here (mostly that any
posted tasks won't be checked and the suggestion about weak ptrs that can be
cleared on shutdown) would be good.

[dgn, 2016/07/12 15:58:19]

Done. I could not make the check in ExtensionFunction because it's not in
//chrome. I used ChromeEFDetails instead.

[Devlin, 2016/07/12 16:19:38]

Instead you can use ExtensionsBrowserClient.  We do this here, too:
https://cs.chromium.org/chromium/src/extensions/browser/extension_function.cc...

Then you don't have to architect quite as much here.  Also note that you can
only put this in a UIThreadExtensionFunction as it can only be accessed on that
thread, whereas this would crash for IOThreadExtension functions.

[dgn, 2016/07/13 20:15:08]

Oh wow it makes it much simpler indeed. Thanks! I'm embarrassed that I didn't
see it before. Anyway, done.

+  }
+
   return RunAsync() ? RespondLater() : RespondNow(Error(error_));
 }