Extensions: Keep track of loaded extensions in RendererStartupHelper.

extensions/browser/renderer_startup_helper_unittest.cc

Index: extensions/browser/renderer_startup_helper_unittest.cc
diff --git a/extensions/browser/renderer_startup_helper_unittest.cc b/extensions/browser/renderer_startup_helper_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d43c39ff5fc1ada309926aef88d807eb69d31dd6
--- /dev/null
+++ b/extensions/browser/renderer_startup_helper_unittest.cc
@@ -0,0 +1,306 @@
+// Copyright 2017 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 "extensions/browser/renderer_startup_helper.h"
+
+#include "base/memory/ptr_util.h"
+#include "base/stl_util.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/test/mock_render_process_host.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_registry_factory.h"
+#include "extensions/browser/extensions_test.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/extension_messages.h"
+
+namespace extensions {
+namespace test {

[Devlin, 2017/03/27 14:47:58]

We don't frequently use namespace test {} in extensions unless it's designed to
be a shared class (and even then, relatively rarely).  Any particular reason for
doing it here?

[karandeepb, 2017/04/04 00:49:42]

Removed. Reason was that it couldn't be in an anon namespace (due to friend
declaration) and I didn't want to put test code in the extensions namespace.

+
+class RendererStartupHelperTest : public ExtensionsTest {
+ public:
+  RendererStartupHelperTest() {}
+  ~RendererStartupHelperTest() override {}
+
+  void SetUp() override {
+    ExtensionsTest::SetUp();
+    helper_ = base::MakeUnique<RendererStartupHelper>(browser_context());
+    registry_ =
+        ExtensionRegistryFactory::GetForBrowserContext(browser_context());
+    render_process_host_ =
+        base::MakeUnique<content::MockRenderProcessHost>(browser_context());
+    extension_ = CreateExtension("ext_1");
+  }
+
+  void TearDown() override {
+    render_process_host_.reset();
+    helper_.reset();
+    ExtensionsTest::TearDown();
+  }
+
+ protected:
+  void SimulateRenderProcessCreated(content::RenderProcessHost* rph) {
+    content::NotificationService::current()->Notify(
+        content::NOTIFICATION_RENDERER_PROCESS_CREATED,
+        content::Source<content::RenderProcessHost>(rph),
+        content::NotificationService::NoDetails());
+  }
+
+  void SimulateRenderProcessTerminated(content::RenderProcessHost* rph) {
+    content::NotificationService::current()->Notify(
+        content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+        content::Source<content::RenderProcessHost>(rph),
+        content::NotificationService::NoDetails());
+  }
+
+  scoped_refptr<Extension> CreateExtension(const std::string& extension_id) {
+    std::unique_ptr<base::DictionaryValue> manifest =
+        DictionaryBuilder()
+            .Set("name", "extension")
+            .Set("description", "an extension")
+            .Set("manifest_version", 2)
+            .Set("version", "0.1")
+            .Build();
+    return ExtensionBuilder()
+        .SetManifest(std::move(manifest))
+        .SetID(extension_id)
+        .Build();
+  }
+
+  scoped_refptr<Extension> CreateTheme(const std::string& extension_id) {
+    std::unique_ptr<base::DictionaryValue> manifest =
+        DictionaryBuilder()
+            .Set("name", "theme")
+            .Set("description", "a theme")
+            .Set("theme", DictionaryBuilder().Build())
+            .Set("manifest_version", 2)
+            .Set("version", "0.1")
+            .Build();
+    return ExtensionBuilder()
+        .SetManifest(std::move(manifest))
+        .SetID(extension_id)
+        .Build();
+  }
+
+  void AddExtensionToRegistry(scoped_refptr<Extension> extension) {
+    registry_->AddEnabled(extension);
+  }
+
+  void RemoveExtensionFromRegistry(scoped_refptr<Extension> extension) {
+    registry_->RemoveEnabled(extension->id());
+  }
+
+  bool IsProcessInitialized(content::RenderProcessHost* rph) {
+    return base::ContainsKey(helper_->loaded_extensions_, rph);
+  }
+
+  bool IsExtensionLoadedInProcess(const Extension& extension,
+                                  content::RenderProcessHost* rph) {
+    return IsProcessInitialized(rph) &&
+           base::ContainsKey(helper_->loaded_extensions_[rph], extension.id());
+  }
+
+  bool IsExtensionPendingActivationInProcess(const Extension& extension,
+                                             content::RenderProcessHost* rph) {
+    return base::ContainsKey(helper_->pending_active_extensions_, rph) &&
+           base::ContainsKey(helper_->pending_active_extensions_[rph],
+                             extension.id());
+  }
+
+  std::unique_ptr<RendererStartupHelper> helper_;
+  ExtensionRegistry* registry_;  // Weak.
+  std::unique_ptr<content::MockRenderProcessHost> render_process_host_;
+  scoped_refptr<Extension> extension_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(RendererStartupHelperTest);
+};
+
+// Tests extension loading, unloading and activation and render process creation
+// and termination.
+TEST_F(RendererStartupHelperTest, NormalExtensionLifecycle) {
+  // Initialize render process.
+  EXPECT_FALSE(IsProcessInitialized(render_process_host_.get()));
+  SimulateRenderProcessCreated(render_process_host_.get());
+  EXPECT_TRUE(IsProcessInitialized(render_process_host_.get()));
+
+  IPC::TestSink& sink = render_process_host_->sink();
+
+  // Enable extension.
+  sink.ClearMessages();
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  AddExtensionToRegistry(extension_);
+  helper_->OnExtensionLoaded(*extension_);
+  EXPECT_TRUE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  EXPECT_FALSE(IsExtensionPendingActivationInProcess(
+      *extension_, render_process_host_.get()));
+  EXPECT_EQ(1u, sink.message_count());

[Devlin, 2017/03/27 14:47:58]

The checks for count may as well be ASSERTs since you'll crash if it's 0.

[karandeepb, 2017/04/04 00:49:42]

Done.

+  EXPECT_EQ(ExtensionMsg_Loaded::ID, sink.GetMessageAt(0)->type());
+
+  // Activate extension.
+  sink.ClearMessages();
+  helper_->ActivateExtensionInProcess(*extension_, render_process_host_.get());
+  EXPECT_FALSE(IsExtensionPendingActivationInProcess(
+      *extension_, render_process_host_.get()));
+  EXPECT_EQ(1u, sink.message_count());
+  EXPECT_EQ(ExtensionMsg_ActivateExtension::ID, sink.GetMessageAt(0)->type());
+
+  // Disable extension.
+  sink.ClearMessages();
+  RemoveExtensionFromRegistry(extension_);
+  helper_->OnExtensionUnloaded(*extension_);
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  EXPECT_EQ(1u, sink.message_count());
+  EXPECT_EQ(ExtensionMsg_Unloaded::ID, sink.GetMessageAt(0)->type());
+
+  // Extension enabled again.
+  sink.ClearMessages();
+  AddExtensionToRegistry(extension_);
+  helper_->OnExtensionLoaded(*extension_);
+  EXPECT_TRUE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  EXPECT_FALSE(IsExtensionPendingActivationInProcess(
+      *extension_, render_process_host_.get()));
+  EXPECT_EQ(1u, sink.message_count());
+  EXPECT_EQ(ExtensionMsg_Loaded::ID, sink.GetMessageAt(0)->type());
+
+  // Render Process terminated.
+  SimulateRenderProcessTerminated(render_process_host_.get());
+  EXPECT_FALSE(IsProcessInitialized(render_process_host_.get()));
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  EXPECT_FALSE(IsExtensionPendingActivationInProcess(
+      *extension_, render_process_host_.get()));
+}
+
+// Test loading an extension again in a render process is a no-op.
+TEST_F(RendererStartupHelperTest, LoadTwice) {
+  // Initialize render process.
+  EXPECT_FALSE(IsProcessInitialized(render_process_host_.get()));
+  SimulateRenderProcessCreated(render_process_host_.get());
+  EXPECT_TRUE(IsProcessInitialized(render_process_host_.get()));
+
+  IPC::TestSink& sink = render_process_host_->sink();
+
+  // Enable and load extension.
+  sink.ClearMessages();
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  AddExtensionToRegistry(extension_);
+  helper_->OnExtensionLoaded(*extension_);
+  EXPECT_TRUE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  EXPECT_EQ(1u, sink.message_count());
+  EXPECT_EQ(ExtensionMsg_Loaded::ID, sink.GetMessageAt(0)->type());
+
+  // Loading extension again is a no-op.
+  sink.ClearMessages();
+  helper_->OnExtensionLoaded(*extension_);
+  EXPECT_EQ(0u, sink.message_count());
+}
+
+// Test unloading an extension in a render process in which it isn't loaded is a
+// no-op.
+TEST_F(RendererStartupHelperTest, UnloadBeforeLoad) {

[Devlin, 2017/03/27 14:47:58]

As mentioned in the other comment, I'd prefer that we have a stricter contract
here, since many of these would be indicative of a bug we need to fix.

[karandeepb, 2017/03/27 20:54:11]

So one of the reasons of doing this was that with
https://codereview.chromium.org/2766263003/, I was hitting a NOTREACHED. If we
enabled an extension for incognito(which was enabled, but disabled for
incognito), the extension prefs would be changed to reflect that the extension
was enabled for incognito and then the extension would be reloaded. During
reloading, the extension would be unloaded from the incognito renderer process
(since it's now enabled in incognito) which would cause the NOTEACHED, since it
wasn't loaded into it earlier. 

I guess the right thing to do there then is to change the enable incognito code
to:
-Disable the extension.
- Enable it for incognito.
- Enable the extension?

[Devlin, 2017/03/28 23:10:08]

Doing this reordering would actually be non-trivial, since currently the
unload-and-disable-for-reload and reload process are fairly intertwined, and I'm
not sure we'd want to expose certain elements (e.g. disabling for
DISABLE_REASON_RELOAD) outside of their current scope.

That said, this scenario sounds like something that should be caught by the
check in renderer_startup_helper.cc where we don't dispatch an unload IPC if the
extension was never unloaded in the process.  That aspect seems reasonable,
since the contract with consumers of calling ExtensionLoaded before
ExtensionUnloaded wasn't violated; instead it was just that the extension wasn't
loaded in a given process.

So I think what we should do here is:
- Make sure that we have strict checks in RendererStartupHelper for when we
expect loaded/unloaded/etc to be called, and enforce them
- Allow RendererStartupHelper to maintain its state and determine whether or not
to dispatch appropriate IPCs
- Update this test to follow the possible scenario you described.

SGTY?

[karandeepb, 2017/04/04 00:49:42]

Done.

+  // Initialize render process.
+  EXPECT_FALSE(IsProcessInitialized(render_process_host_.get()));
+  SimulateRenderProcessCreated(render_process_host_.get());
+  EXPECT_TRUE(IsProcessInitialized(render_process_host_.get()));
+
+  IPC::TestSink& sink = render_process_host_->sink();
+
+  // Unloading should be a no-op since the extension isn't loaded yet.
+  sink.ClearMessages();
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  helper_->OnExtensionUnloaded(*extension_);
+  EXPECT_EQ(0u, sink.message_count());
+}
+
+// Test activating an extension in an initialized process in which it isn't
+// loaded is a no-op.
+TEST_F(RendererStartupHelperTest, ActivatedBeforeLoad) {
+  // Initialize render process.
+  EXPECT_FALSE(IsProcessInitialized(render_process_host_.get()));
+  SimulateRenderProcessCreated(render_process_host_.get());
+  EXPECT_TRUE(IsProcessInitialized(render_process_host_.get()));
+
+  IPC::TestSink& sink = render_process_host_->sink();
+
+  // Activating the extension should be a no-op, since it isn't loaded in the
+  // render process.
+  sink.ClearMessages();
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  helper_->ActivateExtensionInProcess(*extension_, render_process_host_.get());
+  EXPECT_EQ(0u, sink.message_count());
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  EXPECT_FALSE(IsExtensionPendingActivationInProcess(
+      *extension_, render_process_host_.get()));
+}
+
+// Tests that activating an extension in an uninitialized render process works
+// fine.
+TEST_F(RendererStartupHelperTest, EnabledBeforeProcessInitialized) {
+  EXPECT_FALSE(IsProcessInitialized(render_process_host_.get()));
+  IPC::TestSink& sink = render_process_host_->sink();
+
+  // Enable extension. The render process isn't initialized yet, so the
+  // extension should be added to the list of extensions awaiting activation.
+  sink.ClearMessages();
+  AddExtensionToRegistry(extension_);
+  helper_->OnExtensionLoaded(*extension_);
+  helper_->ActivateExtensionInProcess(*extension_, render_process_host_.get());
+  EXPECT_EQ(0u, sink.message_count());
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  EXPECT_TRUE(IsExtensionPendingActivationInProcess(
+      *extension_, render_process_host_.get()));
+
+  // Initialize the render process.
+  SimulateRenderProcessCreated(render_process_host_.get());
+  // The renderer would have been sent both the loading and activation IPC
+  // messages.
+  EXPECT_LE(2u, sink.message_count());
+  EXPECT_TRUE(IsProcessInitialized(render_process_host_.get()));
+  EXPECT_TRUE(
+      IsExtensionLoadedInProcess(*extension_, render_process_host_.get()));
+  EXPECT_FALSE(IsExtensionPendingActivationInProcess(
+      *extension_, render_process_host_.get()));
+}
+
+// Tests that themes aren't loaded in a renderer process.
+TEST_F(RendererStartupHelperTest, LoadTheme) {
+  // Initialize render process.
+  EXPECT_FALSE(IsProcessInitialized(render_process_host_.get()));
+  SimulateRenderProcessCreated(render_process_host_.get());
+  EXPECT_TRUE(IsProcessInitialized(render_process_host_.get()));
+
+  scoped_refptr<Extension> extension(CreateTheme("theme"));
+  ASSERT_TRUE(extension->is_theme());
+
+  IPC::TestSink& sink = render_process_host_->sink();
+
+  // Enable the theme. Verify it can't be loaded and activated in the renderer.
+  sink.ClearMessages();
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension, render_process_host_.get()));
+  AddExtensionToRegistry(extension_);
+  helper_->OnExtensionLoaded(*extension);
+  EXPECT_EQ(0u, sink.message_count());
+  EXPECT_FALSE(
+      IsExtensionLoadedInProcess(*extension, render_process_host_.get()));
+
+  helper_->ActivateExtensionInProcess(*extension, render_process_host_.get());
+  EXPECT_EQ(0u, sink.message_count());
+  EXPECT_FALSE(IsExtensionPendingActivationInProcess(
+      *extension, render_process_host_.get()));
+}
+
+}  // namespace test
+}  // namespace extensions