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

chrome/browser/extensions/extension_gcm_app_handler_unittest.cc

 // Copyright 2014 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 "chrome/browser/extensions/extension_gcm_app_handler.h"
 
 #include <vector>
 
 #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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/services/gcm/fake_signin_manager.h"
 #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"
 #include "components/gcm_driver/fake_gcm_client.h"
 #include "components/gcm_driver/fake_gcm_client_factory.h"
 #include "components/gcm_driver/gcm_client_factory.h"
 #include "components/gcm_driver/gcm_driver.h"
 #include "components/keyed_service/core/keyed_service.h"
 #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"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/permissions/api_permission.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if !defined(OS_ANDROID)
 #include "chrome/browser/extensions/api/gcm/gcm_api.h"
@@ skipping 75 matching lines @@
   scoped_ptr<base::RunLoop> run_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(Waiter);
 };
 
 class FakeExtensionGCMAppHandler : public ExtensionGCMAppHandler {
  public:
   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() {
   }
 
   virtual void OnMessage(
       const std::string& app_id,
       const gcm::GCMClient::IncomingMessage& message) OVERRIDE {
   }
 
   virtual void OnMessagesDeleted(const std::string& app_id) OVERRIDE {
   }
 
   virtual void OnSendError(
       const std::string& app_id,
       const gcm::GCMClient::SendErrorDetails& send_error_details) OVERRIDE {
   }
 
   virtual void OnUnregisterCompleted(const std::string& app_id,
                                      gcm::GCMClient::Result result) OVERRIDE {
     unregistration_result_ = result;
     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);
 };
 
 class ExtensionGCMAppHandlerTest : public testing::Test {
  public:
   static KeyedService* BuildGCMProfileService(
       content::BrowserContext* context) {
     return new gcm::GCMProfileService(
         Profile::FromBrowserContext(context),
         scoped_ptr<gcm::GCMClientFactory>(new gcm::FakeGCMClientFactory(
             gcm::FakeGCMClient::NO_DELAY_START,
             content::BrowserThread::GetMessageLoopProxyForThread(
                 content::BrowserThread::UI),
             content::BrowserThread::GetMessageLoopProxyForThread(
                 content::BrowserThread::IO))));
   }
 
   ExtensionGCMAppHandlerTest()
       : extension_service_(NULL),
         registration_result_(gcm::GCMClient::UNKNOWN_ERROR),
         unregistration_result_(gcm::GCMClient::UNKNOWN_ERROR) {
   }
 
   virtual ~ExtensionGCMAppHandlerTest() {
   }
 
   // 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());
 #endif
 
     // Create a new profile.
     TestingProfile::Builder builder;
     builder.AddTestingFactory(SigninManagerFactory::GetInstance(),
                               gcm::FakeSigninManager::Build);
     profile_ = builder.Build();
     signin_manager_ = static_cast<gcm::FakeSigninManager*>(
         SigninManagerFactory::GetInstance()->GetForProfile(profile_.get()));
 
     // 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);
 
     // Create GCMProfileService that talks with fake GCMClient.
     gcm::GCMProfileServiceFactory::GetInstance()->SetTestingFactoryAndUse(
         profile(), &ExtensionGCMAppHandlerTest::BuildGCMProfileService);
 
     // Create a fake version of ExtensionGCMAppHandler.
     gcm_app_handler_.reset(new FakeExtensionGCMAppHandler(profile(), &waiter_));
   }
 
   virtual void TearDown() OVERRIDE {
 #if defined(OS_CHROMEOS)
     test_user_manager_.reset();
 #endif
 
     waiter_.PumpUILoop();
   }
 
   // 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);
     base::ListValue* permission_list = new base::ListValue;
     permission_list->Append(base::Value::CreateStringValue("gcm"));
     manifest.Set(manifest_keys::kPermissions, permission_list);
 
     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(
         extension->permissions_data()->HasAPIPermission(APIPermission::kGcm));
 
     return extension;
   }
 
   void LoadExtension(const Extension* extension) {
     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);
   }
 
   void EnableExtension(const Extension* extension) {
     extension_service_->EnableExtension(extension->id());
   }
 
   void UninstallExtension(const Extension* extension) {
@@ skipping 40 matching lines @@
   }
   gcm::GCMClient::Result registration_result() const {
     return registration_result_;
   }
   gcm::GCMClient::Result unregistration_result() const {
     return unregistration_result_;
   }
 
  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)
   chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;
   chromeos::ScopedTestCrosSettings test_cros_settings_;
   scoped_ptr<chromeos::ScopedTestUserManager> test_user_manager_;
 #endif
 
   Waiter waiter_;
   scoped_ptr<FakeExtensionGCMAppHandler> gcm_app_handler_;
@@ skipping 49 matching lines @@
   // Unregistration should be triggered when the extension is uninstalled.
   UninstallExtension(extension);
   waiter()->WaitUntilCompleted();
   EXPECT_EQ(gcm::GCMClient::SUCCESS,
             gcm_app_handler()->unregistration_result());
 
   // Clean up.
   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