[Extensions] Fix chrome url override settings

Previously, chrome url overrides were added and removed at extension load and
unload. They should instead be added at install time, updated when the extension
is unloaded, and removed when the extension is uninstalled. Change the pref from
a string (of the url) to a dictionary with { entry: <url>, active: <bool> } to
allow for inactive, but retained, entries.

Also add regression tests.

BUG=571429

Review URL: https://codereview.chromium.org/1543923002

Cr-Commit-Position: refs/heads/master@{#367137}

chrome/browser/extensions/extension_message_bubble_controller_unittest.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/extensions/dev_mode_bubble_delegate.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_web_ui_override_registrar.h"
 #include "chrome/browser/extensions/ntp_overridden_bubble_delegate.h"
 #include "chrome/browser/extensions/proxy_overridden_bubble_delegate.h"
 #include "chrome/browser/extensions/settings_api_bubble_delegate.h"
@@ -45,6 +46,12 @@ const char kId1[] = "iccfkkhkfiphcjdakkmcjmkfboccmndk";
 const char kId2[] = "ajjhifimiemdpmophmkkkcijegphclbl";
 const char kId3[] = "ioibbbfddncmmabjmpokikkeiofalaek";
 
+scoped_ptr<KeyedService> BuildOverrideRegistrar(
+    content::BrowserContext* context) {
+  return make_scoped_ptr(
+      new extensions::ExtensionWebUIOverrideRegistrar(context));
+}
+
 }  // namespace
 
 namespace extensions {
@@ -289,6 +296,11 @@ class ExtensionMessageBubbleTest : public BrowserWithTestWindowTest {
                                  base::FilePath(), false);
     service_ = ExtensionSystem::Get(profile())->extension_service();
     service_->Init();
+
+    extensions::ExtensionWebUIOverrideRegistrar::GetFactoryInstance()->
+        SetTestingFactory(profile(), &BuildOverrideRegistrar);
+    extensions::ExtensionWebUIOverrideRegistrar::GetFactoryInstance()->Get(
+        profile());
   }
 
   ~ExtensionMessageBubbleTest() override {}

chrome/browser/extensions/extension_web_ui.h

@@ -48,16 +48,31 @@ class ExtensionWebUI : public content::WebUIController {
   static bool HandleChromeURLOverrideReverse(
       GURL* url, content::BrowserContext* browser_context);
 
-  // Register and unregister a dictionary of one or more overrides.
-  // Page names are the keys, and chrome-extension: URLs are the values.
-  // (e.g. { "newtab": "chrome-extension://<id>/my_new_tab.html" }
-  static void RegisterChromeURLOverrides(Profile* profile,
+  // Initialize the Chrome URL overrides. This must happen *before* any further
+  // calls for URL overrides!
+  static void InitializeChromeURLOverrides(Profile* profile);
+
+  // Validate the Chrome URL overrides, ensuring that each is valid and points
+  // to an existing extension. To be called once all extensions are loaded.
+  static void ValidateChromeURLOverrides(Profile* profile);
+
+  // Add new Chrome URL overrides. If an entry exists, it is marked as active.
+  // If one doesn't exist, it is added at the beginning of the list of
+  // overrides (meaning it has priority).
+  static void RegisterOrActivateChromeURLOverrides(
+      Profile* profile,
+      const extensions::URLOverrides::URLOverrideMap& overrides);
+
+  // Deactivate overrides without removing them from the list or modifying their
+  // positions in the list.
+  static void DeactivateChromeURLOverrides(
+      Profile* profile,
       const extensions::URLOverrides::URLOverrideMap& overrides);
-  static void UnregisterChromeURLOverrides(Profile* profile,
+
+  // Completely unregister overrides, removing them from the list.
+  static void UnregisterChromeURLOverrides(
+      Profile* profile,
       const extensions::URLOverrides::URLOverrideMap& overrides);
-  static void UnregisterChromeURLOverride(const std::string& page,
-                                          Profile* profile,
-                                          const base::Value* override);
 
   // Called from BrowserPrefs
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);

chrome/browser/extensions/extension_web_ui_override_registrar.cc

@@ -8,13 +8,23 @@
 #include "chrome/browser/extensions/extension_web_ui.h"
 #include "chrome/browser/profiles/profile.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/one_shot_event.h"
 
 namespace extensions {
 
 ExtensionWebUIOverrideRegistrar::ExtensionWebUIOverrideRegistrar(
     content::BrowserContext* context)
-    : extension_registry_observer_(this) {
+    : extension_registry_observer_(this),
+      weak_factory_(this) {
+  ExtensionWebUI::InitializeChromeURLOverrides(
+      Profile::FromBrowserContext(context));
   extension_registry_observer_.Add(ExtensionRegistry::Get(context));
+  ExtensionSystem::Get(context)->ready().Post(
+      FROM_HERE,
+      base::Bind(&ExtensionWebUIOverrideRegistrar::OnExtensionSystemReady,
+                 weak_factory_.GetWeakPtr(),
+                 context));
 }
 
 ExtensionWebUIOverrideRegistrar::~ExtensionWebUIOverrideRegistrar() {
@@ -23,7 +33,7 @@ ExtensionWebUIOverrideRegistrar::~ExtensionWebUIOverrideRegistrar() {
 void ExtensionWebUIOverrideRegistrar::OnExtensionLoaded(
     content::BrowserContext* browser_context,
     const Extension* extension) {
-  ExtensionWebUI::RegisterChromeURLOverrides(
+  ExtensionWebUI::RegisterOrActivateChromeURLOverrides(
       Profile::FromBrowserContext(browser_context),
       URLOverrides::GetChromeURLOverrides(extension));
 }
@@ -32,11 +42,26 @@ void ExtensionWebUIOverrideRegistrar::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
+  ExtensionWebUI::DeactivateChromeURLOverrides(
+      Profile::FromBrowserContext(browser_context),
+      URLOverrides::GetChromeURLOverrides(extension));
+}
+
+void ExtensionWebUIOverrideRegistrar::OnExtensionUninstalled(
+    content::BrowserContext* browser_context,
+    const Extension* extension,
+    UninstallReason reason) {
   ExtensionWebUI::UnregisterChromeURLOverrides(
       Profile::FromBrowserContext(browser_context),
       URLOverrides::GetChromeURLOverrides(extension));
 }
 
+void ExtensionWebUIOverrideRegistrar::OnExtensionSystemReady(
+    content::BrowserContext* context) {
+  ExtensionWebUI::ValidateChromeURLOverrides(
+      Profile::FromBrowserContext(context));
+}
+
 static base::LazyInstance<
     BrowserContextKeyedAPIFactory<ExtensionWebUIOverrideRegistrar> > g_factory =
     LAZY_INSTANCE_INITIALIZER;

chrome/browser/extensions/extension_web_ui_override_registrar.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_WEB_UI_OVERRIDE_REGISTRAR_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
 #include "extensions/browser/browser_context_keyed_api_factory.h"
 #include "extensions/browser/extension_registry_observer.h"
@@ -36,16 +37,24 @@ class ExtensionWebUIOverrideRegistrar : public BrowserContextKeyedAPI,
   void OnExtensionUnloaded(content::BrowserContext* browser_context,
                            const Extension* extension,
                            UnloadedExtensionInfo::Reason reason) override;
+  void OnExtensionUninstalled(content::BrowserContext* browser_context,
+                              const Extension* extension,
+                              UninstallReason reason) override;
+
+  void OnExtensionSystemReady(content::BrowserContext* context);
 
   // BrowserContextKeyedAPI implementation.
   static const char* service_name() {
     return "ExtensionWebUIOverrideRegistrar";
   }
+  static const bool kServiceIsNULLWhileTesting = true;
 
   // Listen to extension load, unloaded notifications.
   ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
       extension_registry_observer_;
 
+  base::WeakPtrFactory<ExtensionWebUIOverrideRegistrar> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionWebUIOverrideRegistrar);
 };
 

chrome/browser/extensions/extension_web_ui_unittest.cc

@@ -7,6 +7,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_web_ui.h"
+#include "chrome/browser/extensions/extension_web_ui_override_registrar.h"
 #include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread.h"
@@ -24,6 +25,15 @@
 
 namespace extensions {
 
+namespace {
+
+scoped_ptr<KeyedService> BuildOverrideRegistrar(
+    content::BrowserContext* context) {
+  return make_scoped_ptr(new ExtensionWebUIOverrideRegistrar(context));
+}
+
+}  // namespace
+
 class ExtensionWebUITest : public testing::Test {
  public:
   ExtensionWebUITest()
@@ -36,6 +46,9 @@ class ExtensionWebUITest : public testing::Test {
         static_cast<TestExtensionSystem*>(ExtensionSystem::Get(profile_.get()));
     extension_service_ = system->CreateExtensionService(
         base::CommandLine::ForCurrentProcess(), base::FilePath(), false);
+    ExtensionWebUIOverrideRegistrar::GetFactoryInstance()->SetTestingFactory(
+        profile_.get(), &BuildOverrideRegistrar);
+    ExtensionWebUIOverrideRegistrar::GetFactoryInstance()->Get(profile_.get());
   }
 
   void TearDown() override {
@@ -110,7 +123,7 @@ TEST_F(ExtensionWebUITest, ExtensionURLOverride) {
 
   // This time the non-component extension was registered more recently and
   // should still take precedence.
-  ExtensionWebUI::RegisterChromeURLOverrides(
+  ExtensionWebUI::RegisterOrActivateChromeURLOverrides(
       profile_.get(), URLOverrides::GetChromeURLOverrides(ext_unpacked.get()));
   url = GURL("chrome://bookmarks");
   EXPECT_TRUE(ExtensionWebUI::HandleChromeURLOverride(&url, profile_.get()));

chrome/test/data/extensions/api_test/override/newtab/manifest.json

 {
-  "name": "New tab override test",
+  "name": "New tab override test, number 1",
   "version": "0.1",
   "manifest_version": 2,
   "description": "Test chrome://newtab override",
-  "background": {
-    "scripts": ["background.js"]
-  },
-  "permissions": [ 
+  "permissions": [
     "tabs"
   ],
   "chrome_url_overrides": {

chrome/test/data/extensions/api_test/override/newtab/newtab.js

@@ -2,4 +2,4 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-chrome.test.notifyPass();
+chrome.test.sendMessage('controlled by first');

chrome/test/data/extensions/api_test/override/newtab2/manifest.json

+{
+  "name": "New tab override test, number 2",
+  "version": "0.1",
+  "manifest_version": 2,
+  "description": "Test chrome://newtab override",
+  "permissions": [
+    "tabs"
+  ],
+  "chrome_url_overrides": {
+    "newtab": "override.html"
+  }
+}

chrome/test/data/extensions/api_test/override/newtab2/override.html

+<!doctype html>
+<html>
+<script src="override.js"></script>
+This is a tab override.
+</html>

chrome/test/data/extensions/api_test/override/newtab/background.js → chrome/test/data/extensions/api_test/override/newtab2/override.js

@@ -2,4 +2,4 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-chrome.test.notifyPass();
+chrome.test.sendMessage('controlled by second');