[bgmode] Skip some KeepAlive registrations on shutdown.

Registering a KeepAlive during shtudown is forbidden and hits
a CHECK, so we should avoid starting some operations that do
it.

This CL adds checks to avoid such situations:
- push notifications received from GCM while shutting down
will be dropped early instead of causing a crash and being
dropped anyway.
- Extension functions using chrome_details validate before
running that the browser process is not shutting down.

BUG=625646

Review-Url: https://codereview.chromium.org/2118473002
Cr-Commit-Position: refs/heads/master@{#406001}

chrome/browser/extensions/chrome_extension_function_unittest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+
+#include "chrome/browser/extensions/extension_service_test_base.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "extensions/browser/extension_function.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace extensions {
+
+namespace {
+
+void SuccessCallback(bool* did_respond,
+                     ExtensionFunction::ResponseType type,
+                     const base::ListValue& results,
+                     const std::string& error,
+                     functions::HistogramValue histogram_value) {
+  EXPECT_EQ(ExtensionFunction::ResponseType::SUCCEEDED, type);
+  *did_respond = true;
+}
+
+void FailCallback(bool* did_respond,
+                  ExtensionFunction::ResponseType type,
+                  const base::ListValue& results,
+                  const std::string& error,
+                  functions::HistogramValue histogram_value) {
+  EXPECT_EQ(ExtensionFunction::ResponseType::FAILED, type);
+  *did_respond = true;
+}
+
+class ValidationFunction : public UIThreadExtensionFunction {
+ public:
+  explicit ValidationFunction(bool should_succeed)
+      : should_succeed_(should_succeed), did_respond_(false) {
+    set_response_callback(base::Bind(
+        (should_succeed ? &SuccessCallback : &FailCallback), &did_respond_));
+  }
+
+  ResponseAction Run() override {
+    EXPECT_TRUE(should_succeed_);
+    return RespondNow(NoArguments());
+  }
+
+  bool did_respond() { return did_respond_; }
+
+ private:
+  ~ValidationFunction() override {}
+  bool should_succeed_;
+  bool did_respond_;
+};
+}  // namespace
+
+using ChromeExtensionFunctionUnitTest = ExtensionServiceTestBase;
+
+TEST_F(ChromeExtensionFunctionUnitTest, SimpleFunctionTest) {
+  scoped_refptr<ValidationFunction> function(new ValidationFunction(true));
+  function->RunWithValidation()->Execute();
+  EXPECT_TRUE(function->did_respond());
+}
+
+TEST_F(ChromeExtensionFunctionUnitTest, BrowserShutdownValidationFunctionTest) {
+  TestingBrowserProcess::GetGlobal()->SetShuttingDown(true);
+  scoped_refptr<ValidationFunction> function(new ValidationFunction(false));
+  function->RunWithValidation()->Execute();
+  TestingBrowserProcess::GetGlobal()->SetShuttingDown(false);
+  EXPECT_TRUE(function->did_respond());
+}
+
+}  // namespace extensions

chrome/browser/lifetime/scoped_keep_alive.h

@@ -12,6 +12,8 @@ enum class KeepAliveRestartOption;
 
 // Registers with KeepAliveRegistry on creation and unregisters them
 // on destruction. Use these objects with a scoped_ptr for easy management.
+// Note: The registration will hit a CHECK if it happens while we are
+// shutting down. Caller code should make sure that this can't happen.
 class ScopedKeepAlive {
  public:
   ScopedKeepAlive(KeepAliveOrigin origin, KeepAliveRestartOption restart);

chrome/browser/notifications/message_center_notification_manager.cc

@@ -145,10 +145,12 @@ bool MessageCenterNotificationManager::Update(const Notification& notification,
 
       // Add/remove notification in the local list but just update the same
       // one in MessageCenter.
-      delete old_notification;
-      profile_notifications_.erase(old_id);
       ProfileNotification* new_notification =
           new ProfileNotification(profile, notification);
+      // Delete the old one after the new one is created to ensure we don't run
+      // out of KeepAlives.
+      delete old_notification;
+      profile_notifications_.erase(old_id);
       profile_notifications_[new_notification->notification().id()] =
           new_notification;
 

chrome/browser/push_messaging/push_messaging_service_impl.cc

@@ -195,6 +195,12 @@ void PushMessagingServiceImpl::ShutdownHandler() {
 
 void PushMessagingServiceImpl::OnMessage(const std::string& app_id,
                                          const gcm::IncomingMessage& message) {
+  // We won't have time to process and act on the message.
+  // TODO(peter) This should be checked at the level of the GCMDriver, so that
+  // the message is not consumed. See https://crbug.com/612815
+  if (g_browser_process->IsShuttingDown())
+    return;
+
   in_flight_message_deliveries_.insert(app_id);
 
 #if BUILDFLAG(ENABLE_BACKGROUND)

chrome/chrome_tests_unit.gypi

@@ -485,6 +485,7 @@
       'browser/extensions/bookmark_app_helper_unittest.cc',
       'browser/extensions/chrome_app_sorting_unittest.cc',
       'browser/extensions/chrome_component_extension_resource_manager_unittest.cc',
+      'browser/extensions/chrome_extension_function_unittest.cc',
       'browser/extensions/chrome_info_map_unittest.cc',
       'browser/extensions/component_loader_unittest.cc',
       'browser/extensions/component_migration_helper_unittest.cc',

extensions/browser/api/guest_view/web_view/web_view_internal_api.cc

@@ -260,6 +260,9 @@ bool LegacyWebViewInternalExtensionFunction::RunAsync() {
 }
 
 bool WebViewInternalExtensionFunction::PreRunValidation(std::string* error) {
+  if (!UIThreadExtensionFunction::PreRunValidation(error))
+    return false;
+
   int instance_id = 0;
   EXTENSION_FUNCTION_PRERUN_VALIDATE(args_->GetInteger(0, &instance_id));
   guest_ = WebViewGuest::From(render_frame_host()->GetProcess()->GetID(),

extensions/browser/extension_function.cc

@@ -488,6 +488,26 @@ UIThreadExtensionFunction::AsUIThreadExtensionFunction() {
   return this;
 }
 
+bool UIThreadExtensionFunction::PreRunValidation(std::string* error) {
+  if (!ExtensionFunction::PreRunValidation(error))
+    return false;
+
+  // TODO(crbug.com/625646) This is a partial fix to avoid crashes when certain
+  // extension functions run during shutdown. Browser or Notification creation
+  // for example create a ScopedKeepAlive, which hit a CHECK if the browser is
+  // shutting down. This fixes the current problem as the known issues happen
+  // through synchronous calls from Run(), but posted tasks will not be covered.
+  // A possible fix would involve refactoring ExtensionFunction: unrefcount
+  // here and use weakptrs for the tasks, then have it owned by something that
+  // will be destroyed naturally in the course of shut down.
+  if (extensions::ExtensionsBrowserClient::Get()->IsShuttingDown()) {
+    *error = "The browser is shutting down.";
+    return false;
+  }
+
+  return true;
+}
+
 bool UIThreadExtensionFunction::OnMessageReceived(const IPC::Message& message) {
   return false;
 }

extensions/browser/extension_function.h

@@ -493,6 +493,8 @@ class UIThreadExtensionFunction : public ExtensionFunction {
 
   UIThreadExtensionFunction* AsUIThreadExtensionFunction() override;
 
+  bool PreRunValidation(std::string* error) override;
+
   void set_test_delegate(DelegateForTests* delegate) {
     delegate_ = delegate;
   }