Test extension reloading behavior.

chrome/browser/extensions/extension_service_unittest.cc

Index: chrome/browser/extensions/extension_service_unittest.cc
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index a6068b5d2c4d97bb3c4a13771d4c17b7024ee903..2fc0bad2c7e90b2b7845a330da2fca4a5434fb3e 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -58,6 +58,7 @@
 #include "chrome/browser/prefs/pref_service_mock_builder.h"
 #include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_paths.h"
@@ -68,12 +69,14 @@
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_l10n_util.h"
 #include "chrome/common/extensions/extension_manifest_constants.h"
+#include "chrome/common/extensions/extension_messages.h"
 #include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h"
 #include "chrome/common/extensions/manifest_handlers/requirements_handler.h"
 #include "chrome/common/extensions/manifest_url_handler.h"
 #include "chrome/common/extensions/permissions/permission_set.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/dom_storage_context.h"
@@ -82,10 +85,12 @@
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/plugin_service.h"
+#include "content/public/browser/site_instance.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/content_constants.h"
 #include "content/public/common/gpu_info.h"
 #include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_notification_tracker.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension_resource.h"
 #include "extensions/common/url_pattern.h"
@@ -102,6 +107,7 @@
 #include "sync/protocol/app_specifics.pb.h"
 #include "sync/protocol/extension_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 #include "webkit/database/database_tracker.h"
@@ -411,6 +417,7 @@ class MockProviderVisitor
 // Our message loop may be used in tests which require it to be an IO loop.
 ExtensionServiceTestBase::ExtensionServiceTestBase()
     : loop_(MessageLoop::TYPE_IO),
+      local_state_(TestingBrowserProcess::GetGlobal()),
       service_(NULL),
       management_policy_(NULL),
       expected_extensions_count_(0),
@@ -438,6 +445,8 @@ ExtensionServiceTestBase::~ExtensionServiceTestBase() {
   MessageLoop::current()->RunUntilIdle();
   profile_.reset(NULL);
   MessageLoop::current()->RunUntilIdle();
+
+  TestingBrowserProcess::GetGlobal()->SetProfileManager(NULL);
 }
 
 void ExtensionServiceTestBase::InitializeExtensionService(
@@ -507,9 +516,16 @@ void ExtensionServiceTestBase::InitializeEmptyExtensionService() {
 }
 
 void ExtensionServiceTestBase::InitializeExtensionProcessManager() {
-  static_cast<extensions::TestExtensionSystem*>(
-      ExtensionSystem::Get(profile_.get()))->
-      CreateExtensionProcessManager();
+  // Needed in order to look up RenderProcessHosts.
+  TestingBrowserProcess::GetGlobal()->SetProfileManager(
+      new ProfileManagerWithoutInit(temp_dir_.path()));
+
+  extensions::TestExtensionSystem* system =
+      static_cast<extensions::TestExtensionSystem*>(
+          ExtensionSystem::Get(profile_.get()));
+  system->CreateExtensionProcessManager();
+  system->process_manager()->site_instance_for_test()->
+      SetRenderProcessHostFactory(&rph_factory_);
 }
 
 void ExtensionServiceTestBase::InitializeExtensionServiceWithUpdater() {
@@ -3607,10 +3623,70 @@ TEST_F(ExtensionServiceTest, ReloadExtensions) {
   EXPECT_EQ(0u, service_->disabled_extensions()->size());
 }
 
+namespace {
+std::vector<uint32> ExtensionMessageTypes(const IPC::TestSink& sink) {
+  std::vector<uint32> message_types;
+  for (size_t i = 0; i < sink.message_count(); ++i) {
+    uint32 type = sink.GetMessageAt(i)->type();
+    if (IPC_MESSAGE_ID_CLASS(type) == ExtensionMsgStart)
+      message_types.push_back(type);
+  }
+  return message_types;
+}
+
+struct ProcessObserver
+    : public content::MockRenderProcessHostFactory::Observer {
+  explicit ProcessObserver(content::MockRenderProcessHostFactory* factory)
+      : Observer(factory) {}
+
+  ~ProcessObserver() {
+    for (size_t i = 0; i < hosts.size(); ++i) {
+      if (destroyed_processes.count(process_ids[i]) == 0)
+        hosts[i]->sink().RemoveFilter(process_messages[i]);
+      delete process_messages[i];
+    }
+  }
+
+  virtual void OnRenderProcessHostCreated(
+      content::MockRenderProcessHost* host) {
+    IPC::TestSink* sink = new IPC::TestSink;
+    hosts.push_back(host);
+    process_ids.push_back(host->GetID());
+    process_messages.push_back(sink);
+    host->sink().AddFilter(sink);
+  }
+  virtual void OnRenderProcessHostDestroyed(
+      content::MockRenderProcessHost* host) {
+    destroyed_processes.insert(host->GetID());
+  }
+
+  std::vector<content::MockRenderProcessHost*> hosts;
+  std::vector<int> process_ids;
+  std::vector<IPC::TestSink*> process_messages;
+  std::set<int> destroyed_processes;
+};
+}  // namespace
+
 // Tests reloading an extension.
 TEST_F(ExtensionServiceTest, ReloadExtension) {
+  ProcessObserver observer(&rph_factory_);
+
+  content::TestNotificationTracker notifications;
+  notifications.ListenForAll(chrome::NOTIFICATION_EXTENSIONS_READY);
+  notifications.ListenForAll(chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED);
+  notifications.ListenForAll(chrome::NOTIFICATION_EXTENSION_LOADED);
+  notifications.ListenForAll(chrome::NOTIFICATION_EXTENSION_UNLOADED);
+  notifications.ListenForAll(content::NOTIFICATION_RENDERER_PROCESS_CLOSING);
+  notifications.ListenForAll(content::NOTIFICATION_RENDERER_PROCESS_CREATED);
+  notifications.ListenForAll(content::NOTIFICATION_RENDERER_PROCESS_TERMINATED);
+  notifications.ListenForAll(
+      content::NOTIFICATION_RENDER_VIEW_HOST_WILL_CLOSE_RENDER_VIEW);
+
   InitializeEmptyExtensionService();
   InitializeExtensionProcessManager();
+  service_->Init();
+  EXPECT_THAT(notifications.GetTypesAndReset(),
+              testing::ElementsAre(chrome::NOTIFICATION_EXTENSIONS_READY));
 
   // Simple extension that should install without error.
   const char* extension_id = "behllobkkfkfnphdnhnkndlbkcpglgmj";
@@ -3622,23 +3698,90 @@ TEST_F(ExtensionServiceTest, ReloadExtension) {
   extensions::UnpackedInstaller::Create(service_)->Load(ext);
   loop_.RunUntilIdle();
 
+  EXPECT_THAT(notifications.GetTypesAndReset(),
+              testing::ElementsAre(chrome::NOTIFICATION_EXTENSION_LOADED));
+  ASSERT_EQ(1u, observer.process_messages.size());
+  EXPECT_THAT(ExtensionMessageTypes(*observer.process_messages[0]),
+              testing::ElementsAre(ExtensionMsg_Loaded::ID,
+                                   ExtensionMsg_ActivateExtension::ID,
+                                   ExtensionMsg_Loaded::ID,
+                                   ExtensionMsg_ActivateExtension::ID,
+                                   ExtensionMsg_Loaded::ID,

[Jeffrey Yasskin, 2013/04/09 17:58:32]

Why does this extra Loaded/ActivateExtension pair go away under
RenderViewHostTestEnabler? Is it because of a bug in the interaction between the
real RenderViewHostImpl and the fake unittest environment that's been fixed by
using TestRenderViewHostFactory instead, or is it a bug in the live code that's
just hidden because TestRenderViewHostFactory doesn't send as many events as
RenderViewHostImpl? If it's the second, that means we're losing some testing
power by using RenderViewHostTestEnabler.

A point in favor of the second is that the NotifyRenderViewType event also
disappears because it's fired from an observer called by
RenderViewHostImpl::CreateRenderView but not
TestRenderViewHost::CreateRenderView.

I gdb'ed a chrome reloading an unpacked extension and got 4 ExtensionMsg_Loaded
sent on 4 different RenderProcessHostImpls, and then 6 more (for a total of 7)
on the 4th host.

[Matt Perry, 2013/04/09 20:20:03]

Your experience with gdb makes me wonder if the type of host has an effect.
Things like: is the host a regular renderer or extension process, or does the
host have content scripts running in it?

[Jeffrey Yasskin, 2013/04/10 16:18:57]

Right, a live chrome has some other non-extension processes around which perhaps
get notified (although they probably shouldn't be for an extension with no
content scripts; there's space for more tests along these lines). The profile
was blank except for this extension, so I don't think there were other content
scripts running. 

+                                   ExtensionMsg_ActivateExtension::ID,
+                                   ExtensionMsg_NotifyRenderViewType::ID));
+  observer.process_messages[0]->ClearMessages();
+
   EXPECT_EQ(1u, service_->extensions()->size());
   EXPECT_EQ(0u, service_->disabled_extensions()->size());
 
   service_->ReloadExtension(extension_id);
 
+  EXPECT_THAT(notifications.GetTypesAndReset(),
+              testing::ElementsAre(
+                  chrome::NOTIFICATION_EXTENSION_UNLOADED,
+                  content::NOTIFICATION_RENDERER_PROCESS_CLOSING,
+                  chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+                  content::NOTIFICATION_RENDERER_PROCESS_TERMINATED));
+
+  ASSERT_EQ(1u, observer.process_messages.size());
+  EXPECT_THAT(ExtensionMessageTypes(*observer.process_messages[0]),
+              testing::ElementsAre());
+  observer.process_messages[0]->ClearMessages();
+
   // Extension should be disabled now, waiting to be reloaded.
   EXPECT_EQ(0u, service_->extensions()->size());
   EXPECT_EQ(1u, service_->disabled_extensions()->size());
   EXPECT_EQ(Extension::DISABLE_RELOAD,
             service_->extension_prefs()->GetDisableReasons(extension_id));
 
-  // Reloading again should not crash.
+  // Reloading again before iterating the MessageLoop should not crash and
+  // shouldn't cause an extra reload.
   service_->ReloadExtension(extension_id);
+  EXPECT_THAT(notifications.GetTypesAndReset(),
+              testing::ElementsAre());
+  ASSERT_EQ(1u, observer.process_messages.size());
+  EXPECT_THAT(ExtensionMessageTypes(*observer.process_messages[0]),
+              testing::ElementsAre());
+  observer.process_messages[0]->ClearMessages();
 
   // Finish reloading
   loop_.RunUntilIdle();
 
+  EXPECT_THAT(notifications.GetTypesAndReset(),
+              testing::ElementsAre(
+                  chrome::NOTIFICATION_EXTENSION_LOADED,
+                  chrome::NOTIFICATION_EXTENSION_UNLOADED,
+                  content::NOTIFICATION_RENDERER_PROCESS_CLOSING,
+                  chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+                  content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+                  chrome::NOTIFICATION_EXTENSION_LOADED));
+
+  ASSERT_EQ(3u, observer.process_messages.size());
+  EXPECT_THAT(observer.destroyed_processes,
+              testing::ElementsAre(observer.process_ids[0],
+                                   observer.process_ids[1]));
+  EXPECT_THAT(ExtensionMessageTypes(*observer.process_messages[0]),
+              testing::ElementsAre());
+  observer.process_messages[0]->ClearMessages();
+  EXPECT_THAT(ExtensionMessageTypes(*observer.process_messages[1]),
+              testing::ElementsAre(ExtensionMsg_Loaded::ID,
+                                   ExtensionMsg_ActivateExtension::ID,
+                                   ExtensionMsg_Loaded::ID,
+                                   ExtensionMsg_ActivateExtension::ID,
+                                   ExtensionMsg_Loaded::ID,
+                                   ExtensionMsg_ActivateExtension::ID,
+                                   ExtensionMsg_NotifyRenderViewType::ID));
+  observer.process_messages[1]->ClearMessages();
+  EXPECT_THAT(ExtensionMessageTypes(*observer.process_messages[2]),
+              testing::ElementsAre(ExtensionMsg_Loaded::ID,
+                                   ExtensionMsg_ActivateExtension::ID,
+                                   ExtensionMsg_Loaded::ID,
+                                   ExtensionMsg_ActivateExtension::ID,
+                                   ExtensionMsg_Loaded::ID,
+                                   ExtensionMsg_ActivateExtension::ID,
+                                   ExtensionMsg_NotifyRenderViewType::ID));
+  observer.process_messages[2]->ClearMessages();
+
   // Extension should be enabled again.
   EXPECT_EQ(1u, service_->extensions()->size());
   EXPECT_EQ(0u, service_->disabled_extensions()->size());