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 b1253ebc3b3b74a79ad79ccf9283a23627d150a4..8e18b60e756a5cffb290dd9d88f86ffebb947945 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -68,6 +68,7 @@
 #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_handler.h"
 #include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h"
 #include "chrome/common/extensions/manifest_handlers/requirements_handler.h"
@@ -86,7 +87,10 @@
 #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/mock_render_process_host.h"
 #include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_content_client_initializer.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"
@@ -103,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"
@@ -508,9 +513,11 @@ void ExtensionServiceTestBase::InitializeEmptyExtensionService() {
 }
 
 void ExtensionServiceTestBase::InitializeExtensionProcessManager() {
-  static_cast<extensions::TestExtensionSystem*>(
-      ExtensionSystem::Get(profile_.get()))->
-      CreateExtensionProcessManager();
+  extensions::TestExtensionSystem* system =
+      static_cast<extensions::TestExtensionSystem*>(
+          ExtensionSystem::Get(profile_.get()));
+  system->CreateExtensionProcessManager();
+  system->process_manager()->SetRenderProcessHostFactoryForTest(&rph_factory_);
 }
 
 void ExtensionServiceTestBase::InitializeExtensionServiceWithUpdater() {
@@ -3612,10 +3619,66 @@ TEST_F(ExtensionServiceTest, ReloadExtensions) {
   EXPECT_EQ(0u, service_->disabled_extensions()->size());
 }
 
+std::vector<uint32> MessageTypes(const IPC::TestSink& sink) {
+  std::vector<uint32> message_types;
+  for (size_t i = 0; i < sink.message_count(); ++i) {
+    message_types.push_back(sink.GetMessageAt(i)->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;
+};
+
 // Tests reloading an extension.
 TEST_F(ExtensionServiceTest, ReloadExtension) {
+  ProcessObserver observer(&rph_factory_);
+
+  content::TestNotificationTracker notifications;
+  notifications.ListenForAll(chrome::NOTIFICATION_EXTENSIONS_READY);

[Jeffrey Yasskin, 2013/04/04 09:34:35]

I think it makes a lot of sense to assert against the EXTENSION notifications,
since they're part of the API ExtensionService exposes.

The RENDERER_PROCESS ones are less obviously-correct: they help me test how
ExtensionService creates processes, but maybe I should do that entirely through
the ProcessObserver (which would require extending the MockRenderProcessHost
interface a bit, I think).

[Matt Perry, 2013/04/04 20:54:26]

Yeah, I really like testing for notifications and messages explicitly. We've had
bugs in the past where we changed the order of some notifications, and things
broke.

+  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();

[Matt Perry, 2013/04/04 20:54:26]

So this test used to work without ever calling Init() on ExtensionService?

[Jeffrey Yasskin, 2013/04/05 13:14:01]

Yep. I it required the omission, or else it hit the ProfileManager and then
needed several other bits of the system to be set up.

+  EXPECT_THAT(notifications.GetTypesAndReset(),
+              testing::ElementsAre(chrome::NOTIFICATION_EXTENSIONS_READY));
 
   // Simple extension that should install without error.
   const char* extension_id = "behllobkkfkfnphdnhnkndlbkcpglgmj";
@@ -3627,23 +3690,118 @@ 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());
+  {
+    testing::Matcher<uint32> expected_types[] = {

[Jeffrey Yasskin, 2013/04/04 09:34:35]

The long lists of notifications and message types should get shorter as I fix
bugs here, but they'll still be annoyingly long. I'd welcome suggestions for
more concise ways to test this aspect of ExtensionService behavior.

[Matt Perry, 2013/04/04 20:54:26]

Some messages we probably don't care about. Might be good to add a testing
primitive that says: "actual results must match the order specified in this
expected list, but ignore anything that shows up in actual that expected doesn't
mention."

[Jeffrey Yasskin, 2013/04/05 13:14:01]

That's not quite right, since we want to assert that there aren't any extra
ExtensionMsgs that we didn't expect. I can easily filter out messages by class
so we'd only get ExtensionMsgs, although I think there's an argument that we
care about these ViewMsgs too.

+      ExtensionMsg_Loaded::ID,
+      ExtensionMsg_ActivateExtension::ID,
+      ExtensionMsg_Loaded::ID,

[Jeffrey Yasskin, 2013/04/04 09:34:35]

Why do we send Loaded/ActivateExtension 3 times? Probably a bug, the fixing of
which is the reason I'm writing this test.

[Matt Perry, 2013/04/04 20:54:26]

Yikes! Good find.

+      ExtensionMsg_ActivateExtension::ID,
+      testing::_,  // ViewMsg_New::ID,

[Jeffrey Yasskin, 2013/04/04 09:34:35]

DEPS prevents me from #including content/common/view_messages.h, so I can't
check exactly how the ExtensionService tells its renderers to start up. I could
check that these are View messages in general, or filter to just the
ExtensionMsgs, or add something to the MockRenderProcessHost to report view
operations semantically, somehow.

+      testing::_,  // ViewMsg_AllowBindings::ID,
+      ExtensionMsg_Loaded::ID,
+      ExtensionMsg_ActivateExtension::ID,
+      ExtensionMsg_NotifyRenderViewType::ID,
+      testing::_,  // ViewMsg_Close::ID,
+      testing::_,  // ViewMsg_Navigate::ID
+    };
+    IPC::TestSink& sink = *observer.process_messages[0];
+    EXPECT_THAT(MessageTypes(sink), testing::ElementsAreArray(expected_types));
+    sink.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(MessageTypes(*observer.process_messages[0]),
+              testing::ElementsAre(testing::_ /*ViewMsg_Close::ID*/));
+  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.

[Matt Perry, 2013/04/04 20:54:26]

I'm not sure I agree with the second half. It makes sense that calling
ReloadExtension twice should trigger 2 reloads.

[Jeffrey Yasskin, 2013/04/05 13:14:01]

I think that would be plausible behavior, but it would actually be harder to
achieve given the current implementation. ReloadExtension() just unloads the
extension and posts tasks saying to reload it, and in order to cause a second
reload, we'd have to attach another task starting the reload to run at the end
of the first load.

In any case, the current behavior is that you always get 2 reloads, no matter
how many times you call ReloadExtension() before the first reload finishes, and
I just want to get that down to 1 reload. Making extra ReloadExtension() calls
cause extra reloads is out of scope.

I could reword the comment if you want, to say that the "no effect" behavior is
just an artifact of the current implementation.

   service_->ReloadExtension(extension_id);

[Jeffrey Yasskin, 2013/04/04 09:34:35]

Commenting this line out has no effect on the test, so the extra reload below is
a pure bug in ExtensionService.

+  EXPECT_THAT(notifications.GetTypesAndReset(),
+              testing::ElementsAre());
+  ASSERT_EQ(1u, observer.process_messages.size());
+  EXPECT_THAT(MessageTypes(*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,

[Jeffrey Yasskin, 2013/04/04 09:34:35]

Note the extra reload here.

+                  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());

[Jeffrey Yasskin, 2013/04/04 09:34:35]

This should become "2" when I fix the extra-reload bug.

+  EXPECT_THAT(observer.destroyed_processes,
+              testing::ElementsAre(observer.process_ids[0],
+                                   observer.process_ids[1]));
+  EXPECT_THAT(MessageTypes(*observer.process_messages[0]),
+              testing::ElementsAre());
+  observer.process_messages[0]->ClearMessages();
+  {
+    testing::Matcher<uint32> expected_types[] = {
+      ExtensionMsg_Loaded::ID,
+      ExtensionMsg_ActivateExtension::ID,
+      ExtensionMsg_Loaded::ID,
+      ExtensionMsg_ActivateExtension::ID,
+      testing::_,  // ViewMsg_New::ID,
+      testing::_,  // ViewMsg_AllowBindings::ID,
+      ExtensionMsg_Loaded::ID,
+      ExtensionMsg_ActivateExtension::ID,
+      ExtensionMsg_NotifyRenderViewType::ID,
+      testing::_,  // ViewMsg_Close::ID,
+      testing::_,  // ViewMsg_Navigate::ID,
+      testing::_,  // ViewMsg_Close::ID
+    };
+    EXPECT_THAT(MessageTypes(*observer.process_messages[1]),
+                testing::ElementsAreArray(expected_types));
+    observer.process_messages[1]->ClearMessages();
+  }
+  {
+    testing::Matcher<uint32> expected_types[] = {
+      ExtensionMsg_Loaded::ID,
+      ExtensionMsg_ActivateExtension::ID,
+      ExtensionMsg_Loaded::ID,
+      ExtensionMsg_ActivateExtension::ID,
+      testing::_,  // ViewMsg_New::ID
+      testing::_,  // ViewMsg_AllowBindings::ID,
+      ExtensionMsg_Loaded::ID,
+      ExtensionMsg_ActivateExtension::ID,
+      ExtensionMsg_NotifyRenderViewType::ID,
+      testing::_,  // ViewMsg_Close::ID,
+      testing::_,  // ViewMsg_Navigate::ID
+    };
+    EXPECT_THAT(MessageTypes(*observer.process_messages[2]),
+                testing::ElementsAreArray(expected_types));
+    observer.process_messages[2]->ClearMessages();
+  }
+
   // Extension should be enabled again.
   EXPECT_EQ(1u, service_->extensions()->size());
   EXPECT_EQ(0u, service_->disabled_extensions()->size());