Don't stop and restart GCM when extension/app is being updated

chrome/browser/extensions/extension_gcm_app_handler_unittest.cc

Index: chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
diff --git a/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc b/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
index 24d802f5013cb41cd872060c7197d0d2fb38617e..934c1e5dda9c81ffb3bd9dd5a6473bd8b3cb6c3b 100644
--- a/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
+++ b/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
@@ -9,14 +9,18 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
+#include "base/file_util.h"
 #include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
+#include "base/path_service.h"
 #include "base/prefs/pref_service.h"
 #include "base/run_loop.h"
 #include "base/values.h"
+#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/test_extension_service.h"
 #include "chrome/browser/extensions/test_extension_system.h"
@@ -25,6 +29,7 @@
 #include "chrome/browser/services/gcm/gcm_profile_service.h"
 #include "chrome/browser/services/gcm/gcm_profile_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/gcm_driver/fake_gcm_app_handler.h"
@@ -36,6 +41,7 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
@@ -131,7 +137,9 @@ class FakeExtensionGCMAppHandler : public ExtensionGCMAppHandler {
   FakeExtensionGCMAppHandler(Profile* profile, Waiter* waiter)
       : ExtensionGCMAppHandler(profile),
         waiter_(waiter),
-        unregistration_result_(gcm::GCMClient::UNKNOWN_ERROR) {
+        unregistration_result_(gcm::GCMClient::UNKNOWN_ERROR),
+        add_count_(0),
+        remove_count_(0) {
   }
 
   virtual ~FakeExtensionGCMAppHandler() {
@@ -156,13 +164,31 @@ class FakeExtensionGCMAppHandler : public ExtensionGCMAppHandler {
     waiter_->SignalCompleted();
   }
 
+  virtual void AddAppHandler(const std::string& app_id) OVERRIDE {

[fgorski, 2014/06/12 21:38:32]

There is a DCHECK for not adding duplicates in GCMDriver::AddAppHandler, so what
you really want to test for is that you never enter this method, rather than the
old_size is different than new size.

I looked at GCMDriver::RemoveAppHandler and it does a different set of checks.
Your code makes more sense in that case, do you think we could update the
production code to check that the handler is registered first?

[jianli, 2014/06/12 21:48:31]

Then it will get caught by the DCHECK assert. Here I am just adding an extra
check.
Not sure I understand "update the

[fgorski, 2014/06/13 15:20:21]

Let's discuss it today.

+    size_t old_size = GetGCMDriver()->app_handlers().size();
+    ExtensionGCMAppHandler::AddAppHandler(app_id);
+    if (old_size < GetGCMDriver()->app_handlers().size())
+      add_count_++;
+  }
+
+  virtual void RemoveAppHandler(const std::string& app_id) OVERRIDE{
+    size_t old_size = GetGCMDriver()->app_handlers().size();
+    ExtensionGCMAppHandler::RemoveAppHandler(app_id);
+    if (old_size > GetGCMDriver()->app_handlers().size())
+      remove_count_++;
+  }
+
   gcm::GCMClient::Result unregistration_result() const {
     return unregistration_result_;
   }
+  int add_count() const { return add_count_; }
+  int remove_count() const { return remove_count_; }
 
  private:
   Waiter* waiter_;
   gcm::GCMClient::Result unregistration_result_;
+  int add_count_;
+  int remove_count_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeExtensionGCMAppHandler);
 };
@@ -192,10 +218,16 @@ class ExtensionGCMAppHandlerTest : public testing::Test {
 
   // Overridden from test::Test:
   virtual void SetUp() OVERRIDE {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+
     // Make BrowserThread work in unittest.
     thread_bundle_.reset(new content::TestBrowserThreadBundle(
         content::TestBrowserThreadBundle::REAL_IO_THREAD));
 
+    // Allow extension update to unpack crx in process.
+    in_process_utility_thread_helper_.reset(
+        new content::InProcessUtilityThreadHelper);
+
     // This is needed to create extension service under CrOS.
 #if defined(OS_CHROMEOS)
     test_user_manager_.reset(new chromeos::ScopedTestUserManager());
@@ -212,9 +244,14 @@ class ExtensionGCMAppHandlerTest : public testing::Test {
     // Create extension service in order to uninstall the extension.
     TestExtensionSystem* extension_system(
         static_cast<TestExtensionSystem*>(ExtensionSystem::Get(profile())));
+    base::FilePath extensions_install_dir =
+        temp_dir_.path().Append(FILE_PATH_LITERAL("Extensions"));
     extension_system->CreateExtensionService(
-        CommandLine::ForCurrentProcess(), base::FilePath(), false);
+        CommandLine::ForCurrentProcess(), extensions_install_dir, false);
     extension_service_ = extension_system->Get(profile())->extension_service();
+    extension_service_->set_extensions_enabled(true);
+    extension_service_->set_show_extensions_prompts(false);
+    extension_service_->set_install_updates_when_idle_for_test(false);
 
     // Enable GCM such that tests could be run on all channels.
     profile()->GetPrefs()->SetBoolean(prefs::kGCMChannelEnabled, true);
@@ -237,12 +274,6 @@ class ExtensionGCMAppHandlerTest : public testing::Test {
 
   // Returns a barebones test extension.
   scoped_refptr<Extension> CreateExtension() {
-#if defined(OS_WIN)
-    base::FilePath path(FILE_PATH_LITERAL("c:\\foo"));
-#elif defined(OS_POSIX)
-    base::FilePath path(FILE_PATH_LITERAL("/foo"));
-#endif
-
     base::DictionaryValue manifest;
     manifest.SetString(manifest_keys::kVersion, "1.0.0.0");
     manifest.SetString(manifest_keys::kName, kTestExtensionName);
@@ -252,10 +283,11 @@ class ExtensionGCMAppHandlerTest : public testing::Test {
 
     std::string error;
     scoped_refptr<Extension> extension = Extension::Create(
-        path.AppendASCII(kTestExtensionName),
-        Manifest::INVALID_LOCATION,
+        temp_dir_.path(),
+        Manifest::UNPACKED,
         manifest,
         Extension::NO_FLAGS,
+        "ldnnhddmnhbkjipkidpdiheffobcpfmf",
         &error);
     EXPECT_TRUE(extension.get()) << error;
     EXPECT_TRUE(
@@ -268,6 +300,37 @@ class ExtensionGCMAppHandlerTest : public testing::Test {
     extension_service_->AddExtension(extension);
   }
 
+  static bool IsCrxInstallerDone(extensions::CrxInstaller** installer,
+                                 const content::NotificationSource& source,
+                                 const content::NotificationDetails& details) {
+    return content::Source<extensions::CrxInstaller>(source).ptr() ==
+           *installer;
+  }
+
+  void UpdateExtension(const Extension* extension) {
+    base::FilePath data_dir;
+    if (!PathService::Get(chrome::DIR_TEST_DATA, &data_dir)) {
+      ADD_FAILURE();
+      return;
+    }
+    data_dir = data_dir.AppendASCII("extensions");
+    data_dir = data_dir.AppendASCII("gcm2.crx");
+
+    base::FilePath path = temp_dir_.path();
+    path = path.Append(data_dir.BaseName());
+    ASSERT_TRUE(base::CopyFile(data_dir, path));
+
+    extensions::CrxInstaller* installer = NULL;
+    content::WindowedNotificationObserver observer(
+        chrome::NOTIFICATION_CRX_INSTALLER_DONE,
+        base::Bind(&IsCrxInstallerDone, &installer));
+    extension_service_->UpdateExtension(
+        extension->id(), path, true, &installer);
+
+    if (installer)
+      observer.Wait();
+  }
+
   void DisableExtension(const Extension* extension) {
     extension_service_->DisableExtension(
         extension->id(), Extension::DISABLE_USER_ACTION);
@@ -328,9 +391,12 @@ class ExtensionGCMAppHandlerTest : public testing::Test {
 
  private:
   scoped_ptr<content::TestBrowserThreadBundle> thread_bundle_;
+  scoped_ptr<content::InProcessUtilityThreadHelper>
+      in_process_utility_thread_helper_;
   scoped_ptr<TestingProfile> profile_;
   ExtensionService* extension_service_;  // Not owned.
   gcm::FakeSigninManager* signin_manager_;  // Not owned.
+  base::ScopedTempDir temp_dir_;
 
   // This is needed to create extension service under CrOS.
 #if defined(OS_CHROMEOS)
@@ -400,4 +466,22 @@ TEST_F(ExtensionGCMAppHandlerTest, UnregisterOnExtensionUninstall) {
   GetGCMDriver()->RemoveAppHandler("Foo");
 }
 
+TEST_F(ExtensionGCMAppHandlerTest, UpdateExtension) {
+  scoped_refptr<Extension> extension(CreateExtension());
+
+  // App handler is added when extension is loaded.
+  LoadExtension(extension);
+  waiter()->PumpUILoop();
+  EXPECT_TRUE(HasAppHandlers(extension->id()));
+  EXPECT_EQ(1, gcm_app_handler()->add_count());
+  EXPECT_EQ(0, gcm_app_handler()->remove_count());
+
+  // App handler is not affected when extension is updated.
+  UpdateExtension(extension);
+  waiter()->PumpUILoop();
+  EXPECT_TRUE(HasAppHandlers(extension->id()));
+  EXPECT_EQ(1, gcm_app_handler()->add_count());
+  EXPECT_EQ(0, gcm_app_handler()->remove_count());
+}
+
 }  // namespace extensions