Don't send background app notifications during Browser startup.

This restores behavior broken by 6670c336

Change the name of BackgroundApplicationListModel::is_ready() to
startup_done() to disambiguate from ExtensionService::is_ready().

Move BackgroundContentsService observation to OnExtensionSystemReady()
so we don't run BackgroundApplicationListModel::Update() too early.
Note that it's not sufficient to check whether the ExtensionService is
ready because it may return true before
BackgroundApplicationListModel::OnExtensionSystemReady runs.

Bug: 1008890
Change-Id: Ib3e5013e7857e7a9aa6f94ed6b43f3839e08fe00
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1837486Reviewed-by: Drew Wilson <atwilson@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703319}

chrome/browser/background/background_application_list_model.cc

@@ -147,9 +147,6 @@ BackgroundApplicationListModel::~BackgroundApplicationListModel() = default;
 BackgroundApplicationListModel::BackgroundApplicationListModel(Profile* profile)
     : profile_(profile) {
   DCHECK(profile_);
-  background_contents_service_observer_.Add(
-      BackgroundContentsServiceFactory::GetForProfile(profile));
-
   registrar_.Add(this,
                  extensions::NOTIFICATION_EXTENSION_PERMISSIONS_UPDATED,
                  content::Source<Profile>(profile));
@@ -309,7 +306,6 @@ void BackgroundApplicationListModel::OnExtensionUnloaded(
 }
 
 void BackgroundApplicationListModel::OnExtensionSystemReady() {
-  ready_ = true;
   // All initial extensions will be loaded when extension system ready. So we
   // can get everything here.
   Update();
@@ -321,6 +317,11 @@ void BackgroundApplicationListModel::OnExtensionSystemReady() {
   // for the extension system, which isn't a guarantee. Thus, register here and
   // associate all initial extensions.
   extension_registry_observer_.Add(ExtensionRegistry::Get(profile_));
+
+  background_contents_service_observer_.Add(
+      BackgroundContentsServiceFactory::GetForProfile(profile_));
+
+  startup_done_ = true;
 }
 
 void BackgroundApplicationListModel::OnShutdown(ExtensionRegistry* registry) {
@@ -367,11 +368,9 @@ void BackgroundApplicationListModel::RemoveObserver(Observer* observer) {
 // differs from the old list, it generates OnApplicationListChanged events for
 // each observer.
 void BackgroundApplicationListModel::Update() {
-  if (!ready_)
-    return;
-
   extensions::ExtensionService* service =
       extensions::ExtensionSystem::Get(profile_)->extension_service();
+  DCHECK(service->is_ready());
 
   // Discover current background applications, compare with previous list, which
   // is consistently sorted, and notify observers if they differ.

chrome/browser/background/background_application_list_model.h

@@ -101,9 +101,7 @@ class BackgroundApplicationListModel
   }
 
   // Returns true if all startup notifications have already been issued.
-  bool is_ready() const {
-    return ready_;
-  }
+  bool startup_done() const { return startup_done_; }
 
  private:
   // Contains data associated with a background application that is not
@@ -167,7 +165,7 @@ class BackgroundApplicationListModel
   base::ObserverList<Observer, true>::Unchecked observers_;
   Profile* const profile_;
   content::NotificationRegistrar registrar_;
-  bool ready_{false};
+  bool startup_done_ = false;
 
   // Listens to extension load, unload notifications.
   ScopedObserver<extensions::ExtensionRegistry,

chrome/browser/background/background_application_list_model_unittest.cc

@@ -156,7 +156,7 @@ TEST_F(BackgroundApplicationListModelTest, DISABLED_ExplicitTest) {
   service()->Init();
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(service()->is_ready());
-  ASSERT_TRUE(model()->is_ready());
+  ASSERT_TRUE(model()->startup_done());
 
   ASSERT_TRUE(registry()->enabled_extensions().is_empty());
   ASSERT_EQ(0U, model()->size());
@@ -296,11 +296,11 @@ TEST_F(BackgroundApplicationListModelTest, ExtensionLoadAndUnload) {
 
 TEST_F(BackgroundApplicationListModelTest, LateExtensionSystemReady) {
   ASSERT_FALSE(service()->is_ready());
-  ASSERT_FALSE(model()->is_ready());
+  ASSERT_FALSE(model()->startup_done());
   service()->Init();
   // Model is not ready yet since ExtensionSystem::ready() is dispatched using
   // PostTask to UI Thread. and OnExtensionSystemReady is not called yet.
-  ASSERT_FALSE(model()->is_ready());
+  ASSERT_FALSE(model()->startup_done());
 
   scoped_refptr<Extension> bgapp =
       CreateExtension("background_application", true);
@@ -314,13 +314,13 @@ TEST_F(BackgroundApplicationListModelTest, LateExtensionSystemReady) {
   service()->AddExtension(bgapp.get());
   load_observer.WaitForExtensionLoaded();
   EXPECT_EQ(1U, registry()->enabled_extensions().size());
-  // Model still has 0 item. since OnExtensionSystemReady is not called yet.
+  // Model still has 0 items since OnExtensionSystemReady is not called yet.
   EXPECT_EQ(0U, model()->size());
 
   // Wait Until OnExtensionSystemReady called.
   base::RunLoop().RunUntilIdle();
   // Make sure background model holds extensions.
-  EXPECT_TRUE(model()->is_ready());
+  EXPECT_TRUE(model()->startup_done());
   EXPECT_EQ(1U, model()->size());
 }
 

chrome/browser/background/background_mode_manager.cc

@@ -206,7 +206,7 @@ BackgroundModeManager::BackgroundModeData::GetNewBackgroundApps() {
       current_extensions_.insert(id);
       // If this application has been newly loaded after the initial startup,
       // notify the user.
-      if (applications_->is_ready())
+      if (applications_->startup_done())
         new_apps.insert(application.get());
     }
   }
@@ -744,6 +744,7 @@ void BackgroundModeManager::OnBackgroundClientInstalled(
   EnableBackgroundMode();
   ResumeBackgroundMode();
 
+  ++client_installed_notifications_;
   // Notify the user that a background client has been installed.
   DisplayClientInstalledNotification(name);
 }

chrome/browser/background/background_mode_manager.h

@@ -108,6 +108,10 @@ class BackgroundModeManager : public content::NotificationObserver,
   // For testing purposes.
   size_t NumberOfBackgroundModeData();
 
+  int client_installed_notifications_for_test() {
+    return client_installed_notifications_;
+  }
+
  private:
   friend class AppBackgroundPageApiTest;
   friend class BackgroundModeManagerTest;
@@ -395,6 +399,10 @@ class BackgroundModeManager : public content::NotificationObserver,
   // app).
   bool keep_alive_for_test_ = false;
 
+  // Tracks the number of "background app installed" notifications shown to the
+  // user. Used for testing.
+  int client_installed_notifications_ = 0;
+
   // Set to true when background mode is suspended.
   bool background_mode_suspended_ = false;
 

chrome/browser/extensions/background_app_browsertest.cc

@@ -9,71 +9,54 @@
 #include "chrome/browser/background/background_mode_manager.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "extensions/browser/test_extension_registry_observer.h"
 
-class TestBackgroundModeManager : public BackgroundModeManager {
- public:
-  TestBackgroundModeManager(const base::CommandLine& command_line,
-                            ProfileAttributesStorage* profile_storage)
-      : BackgroundModeManager(command_line, profile_storage),
-        showed_background_app_installed_notification_for_test_(false) {}
+namespace extensions {
 
-  ~TestBackgroundModeManager() override {}
-
-  void DisplayClientInstalledNotification(const base::string16& name) override {
-    showed_background_app_installed_notification_for_test_ = true;
-  }
-
-  bool showed_background_app_installed_notification_for_test() {
-    return showed_background_app_installed_notification_for_test_;
-  }
-
-  void set_showed_background_app_installed_notification_for_test(
-      bool showed) {
-    showed_background_app_installed_notification_for_test_ = showed;
-  }
-
- private:
-  // Tracks if we have shown a "Background App Installed" notification to the
-  // user.  Used for unit tests only.
-  bool showed_background_app_installed_notification_for_test_;
-
-  FRIEND_TEST_ALL_PREFIXES(BackgroundAppBrowserTest,
-                           ReloadBackgroundApp);
-
-  DISALLOW_COPY_AND_ASSIGN(TestBackgroundModeManager);
-};
-
-using BackgroundAppBrowserTest = extensions::ExtensionBrowserTest;
+using BackgroundAppBrowserTest = ExtensionBrowserTest;
 
 // Tests that if we reload a background app, we don't get a popup bubble
 // telling us that a new background app has been installed.
 IN_PROC_BROWSER_TEST_F(BackgroundAppBrowserTest, ReloadBackgroundApp) {
-  // Pass this in to the browser test.
-  std::unique_ptr<BackgroundModeManager> test_background_mode_manager(
-      new TestBackgroundModeManager(*base::CommandLine::ForCurrentProcess(),
-                                    &(g_browser_process->profile_manager()
-                                          ->GetProfileAttributesStorage())));
-  g_browser_process->set_background_mode_manager_for_test(
-      std::move(test_background_mode_manager));
-  TestBackgroundModeManager* manager =
-      reinterpret_cast<TestBackgroundModeManager*>(
-          g_browser_process->background_mode_manager());
-
+  BackgroundModeManager* manager = g_browser_process->background_mode_manager();
   // Load our background extension
-  ASSERT_FALSE(
-      manager->showed_background_app_installed_notification_for_test());
-  const extensions::Extension* extension = LoadExtension(
-      test_data_dir_.AppendASCII("background_app"));
+  EXPECT_EQ(0, manager->client_installed_notifications_for_test());
+  const Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("background_app"));
+  EXPECT_EQ(1, manager->client_installed_notifications_for_test());
   ASSERT_FALSE(extension == NULL);
 
-  // Set the test flag to not shown.
-  manager->set_showed_background_app_installed_notification_for_test(false);
-
   // Reload our background extension
   ReloadExtension(extension->id());
 
-  // Ensure that we did not see a "Background extension loaded" dialog.
-  EXPECT_FALSE(
-      manager->showed_background_app_installed_notification_for_test());
+  // Ensure that we did not see another "Background extension loaded" dialog.
+  EXPECT_EQ(1, manager->client_installed_notifications_for_test());
+}
+
+// Make sure that the background mode notification is sent for an app install,
+// but not again on browser restart. Regression test for
+// https://crbug.com/1008890
+IN_PROC_BROWSER_TEST_F(BackgroundAppBrowserTest, PRE_InstallBackgroundApp) {
+  InstallExtension(test_data_dir_.AppendASCII("background_app"), 1);
+  EXPECT_EQ(1, g_browser_process->background_mode_manager()
+                   ->client_installed_notifications_for_test());
 }
+
+IN_PROC_BROWSER_TEST_F(BackgroundAppBrowserTest, InstallBackgroundApp) {
+  // Verify the installed extension is still here.
+  const ExtensionSet& extensions = extension_registry()->enabled_extensions();
+  EXPECT_TRUE(
+      std::any_of(extensions.begin(), extensions.end(),
+                  [](scoped_refptr<const Extension> extension) {
+                    return extension->description() ==
+                           "A simple app with background permission set.";
+                  }));
+  // Verify the installed extension did not pop up a background mode
+  // notification.
+  EXPECT_EQ(0, g_browser_process->background_mode_manager()
+                   ->client_installed_notifications_for_test());
+}
+
+}  // namespace extensions