Fix URL override priority.

A tiny bug in ExtensionWebUI::HandleChromeURLOverride could cause
component extensions to take precedence over previously registered
non-component extensions.

Also the mere act of the test calling Add[Component]Extension resulted in an
extension's URL overrides being registered, so by the time any direct calls
to ExtensionWebUI::RegisterChromeURLOverrides were being made to establish the
(non-component -> component) expected ordering, the overrides had already been
registered in the reverse (component -> non-component) order, thus masking
the bug.

This fixes the bug and the test.

BUG=338068

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@257681 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 71 insertions, 73 deletions

diff --git a/chrome/browser/extensions/extension_web_ui.cc b/chrome/browser/extensions/extension_web_ui.cc
index 9e346ae..4f89a1f 100644
--- a/chrome/browser/extensions/extension_web_ui.cc
+++ b/chrome/browser/extensions/extension_web_ui.cc
@@ -30,6 +30,7 @@
 #include "content/public/browser/web_ui.h"
 #include "content/public/common/bindings_policy.h"
 #include "content/public/common/page_transition_types.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_resource.h"
 #include "extensions/common/manifest_handlers/incognito_info.h"
@@ -120,6 +121,30 @@ void RunFaviconCallbackAsync(
                  base::Owned(favicon_bitmap_results)));
 }
 
+bool ValidateOverrideURL(const base::Value* override_url_value,
+                         const GURL& source_url,
+                         const extensions::ExtensionSet& extensions,
+                         GURL* override_url,
+                         const Extension** extension) {
+  std::string override;
+  if (!override_url_value || !override_url_value->GetAsString(&override)) {
+    return false;
+  }
+  if (!source_url.query().empty())
+    override += "?" + source_url.query();
+  if (!source_url.ref().empty())
+    override += "#" + source_url.ref();
+  *override_url = GURL(override);
+  if (!override_url->is_valid()) {
+    return false;
+  }
+  *extension = extensions.GetByID(override_url->host());
+  if (!*extension) {
+    return false;
+  }
+  return true;
+}
+
 }  // namespace
 
 const char ExtensionWebUI::kExtensionURLOverrides[] =
@@ -178,52 +203,33 @@ bool ExtensionWebUI::HandleChromeURLOverride(
   Profile* profile = Profile::FromBrowserContext(browser_context);
   const base::DictionaryValue* overrides =
       profile->GetPrefs()->GetDictionary(kExtensionURLOverrides);
-  std::string page = url->host();
+
+  std::string url_host = url->host();
   const base::ListValue* url_list = NULL;
-  if (!overrides || !overrides->GetList(page, &url_list))
+  if (!overrides || !overrides->GetList(url_host, &url_list))
     return false;
 
-  ExtensionService* service = profile->GetExtensionService();
+  extensions::ExtensionRegistry* registry =
+      extensions::ExtensionRegistry::Get(browser_context);
+  const extensions::ExtensionSet& extensions = registry->enabled_extensions();
 
-  // Use the first non-component override we find. Otherwise, use the first
-  // component override we find.
   GURL component_url;
   bool found_component_override = false;
 
-  size_t i = 0;
-  while (i < url_list->GetSize()) {
+  // Iterate over the URL list looking for a suitable override. If a
+  // valid non-component override is encountered it is chosen immediately.
+  for (size_t i = 0; i < url_list->GetSize(); ++i) {
     const base::Value* val = NULL;
     url_list->Get(i, &val);
 
-    // Verify that the override value is good.  If not, unregister it and find
-    // the next one.
-    std::string override;
-    if (!val->GetAsString(&override)) {
-      NOTREACHED();
-      UnregisterChromeURLOverride(page, profile, val);
-      continue;
-    }
-
-    if (!url->query().empty())
-      override += "?" + url->query();
-    if (!url->ref().empty())
-      override += "#" + url->ref();
-    GURL override_url(override);
-    if (!override_url.is_valid()) {
-      NOTREACHED();
-      UnregisterChromeURLOverride(page, profile, val);
-      continue;
-    }
-
-    // Verify that the extension that's being referred to actually exists.
-    const Extension* extension =
-        service->extensions()->GetByID(override_url.host());
-    if (!extension) {
-      // This can currently happen if you use --load-extension one run, and
-      // then don't use it the next.  It could also happen if an extension
-      // were deleted directly from the filesystem, etc.
-      LOG(WARNING) << "chrome URL override present for non-existant extension";
-      UnregisterChromeURLOverride(page, profile, val);
+    GURL override_url;
+    const Extension* extension;
+    if (!ValidateOverrideURL(
+            val, *url, extensions, &override_url, &extension)) {
+      LOG(WARNING) << "Invalid chrome URL override";
+      UnregisterChromeURLOverride(url_host, profile, val);
+      // The above Unregister call will remove this item from url_list.
+      --i;
       continue;
     }
 
@@ -233,25 +239,22 @@ bool ExtensionWebUI::HandleChromeURLOverride(
         extensions::IncognitoInfo::IsSplitMode(extension) &&
         extensions::util::IsIncognitoEnabled(extension->id(), profile);
     if (profile->IsOffTheRecord() && !incognito_override_allowed) {
-      ++i;
       continue;
     }
 
-    // If this is a component extension, record its url.
-    // Only use a component extension override if there are no non-component
-    // extension overrides.
-    if (!found_component_override &&
-        extensions::Manifest::IsComponentLocation(extension->location())) {
+    if (!extensions::Manifest::IsComponentLocation(extension->location())) {
+      *url = override_url;
+      return true;
+    }
+
+    if (!found_component_override) {
       found_component_override = true;
       component_url = override_url;
-      continue;
     }
-
-    *url = override_url;
-    return true;
   }
 
-  // If no other extension overrides were found, use the component override.
+  // If no other non-component overrides were found, use the first known
+  // component override, if any.
   if (found_component_override) {
     *url = component_url;
     return true;
diff --git a/chrome/browser/extensions/extension_web_ui_unittest.cc b/chrome/browser/extensions/extension_web_ui_unittest.cc
index b93877a..f912016 100644
--- a/chrome/browser/extensions/extension_web_ui_unittest.cc
+++ b/chrome/browser/extensions/extension_web_ui_unittest.cc
@@ -56,54 +56,49 @@ class ExtensionWebUITest : public testing::Test {
 // Test that component extension url overrides have lower priority than
 // non-component extension url overrides.
 TEST_F(ExtensionWebUITest, ExtensionURLOverride) {
-  // Make a component extension.
+  // Register a non-component extension.
   extensions::DictionaryBuilder manifest;
   manifest.Set(manifest_keys::kName, "ext1")
       .Set(manifest_keys::kVersion, "0.1")
       .Set(std::string(manifest_keys::kChromeURLOverrides),
            extensions::DictionaryBuilder().Set("bookmarks", "1.html"));
-  scoped_refptr<Extension> ext_component(
+  scoped_refptr<Extension> ext_unpacked(
       extensions::ExtensionBuilder()
           .SetManifest(manifest)
-          .SetLocation(Manifest::COMPONENT)
+          .SetLocation(Manifest::UNPACKED)
           .SetID("abcdefghijabcdefghijabcdefghijaa")
           .Build());
-  profile_->GetExtensionService()->AddComponentExtension(ext_component.get());
+  profile_->GetExtensionService()->AddExtension(ext_unpacked.get());
 
-  // Make a non-component extension.
+  GURL expected_unpacked_override_url(std::string(ext_unpacked->url().spec()) +
+                                      "1.html");
+  GURL url("chrome://bookmarks");
+  EXPECT_TRUE(ExtensionWebUI::HandleChromeURLOverride(&url, profile_.get()));
+  EXPECT_EQ(url, expected_unpacked_override_url);
+
+  // Register a component extension
   extensions::DictionaryBuilder manifest2;
   manifest2.Set(manifest_keys::kName, "ext2")
       .Set(manifest_keys::kVersion, "0.1")
       .Set(std::string(manifest_keys::kChromeURLOverrides),
            extensions::DictionaryBuilder().Set("bookmarks", "2.html"));
-  scoped_refptr<Extension> ext_unpacked(
+  scoped_refptr<Extension> ext_component(
       extensions::ExtensionBuilder()
           .SetManifest(manifest2)
-          .SetLocation(Manifest::UNPACKED)
+          .SetLocation(Manifest::COMPONENT)
           .SetID("bbabcdefghijabcdefghijabcdefghij")
           .Build());
-  profile_->GetExtensionService()->AddExtension(ext_unpacked.get());
-
-  GURL expected_component_override_url(
-      std::string(ext_component->url().spec()) + "1.html");
-  GURL expected_unpacked_override_url(std::string(ext_unpacked->url().spec()) +
-                                      "2.html");
-
-  // Register non-component extension.
-  ExtensionWebUI::RegisterChromeURLOverrides(
-      profile_.get(), URLOverrides::GetChromeURLOverrides(ext_unpacked.get()));
-  GURL url("chrome://bookmarks");
-  EXPECT_TRUE(ExtensionWebUI::HandleChromeURLOverride(&url, profile_.get()));
-  EXPECT_EQ(url, expected_unpacked_override_url);
+  profile_->GetExtensionService()->AddComponentExtension(ext_component.get());
 
-  // Register component extension. Despite being registered more recently, the
-  // non-component extension should still have precedence.
-  ExtensionWebUI::RegisterChromeURLOverrides(
-      profile_.get(), URLOverrides::GetChromeURLOverrides(ext_component.get()));
+  // Despite being registered more recently, the component extension should
+  // not take precendence over the non-component extension.
   url = GURL("chrome://bookmarks");
   EXPECT_TRUE(ExtensionWebUI::HandleChromeURLOverride(&url, profile_.get()));
   EXPECT_EQ(url, expected_unpacked_override_url);
 
+  GURL expected_component_override_url(
+      std::string(ext_component->url().spec()) + "2.html");
+
   // Unregister non-component extension. Only component extension remaining.
   ExtensionWebUI::UnregisterChromeURLOverrides(
       profile_.get(), URLOverrides::GetChromeURLOverrides(ext_unpacked.get()));
@@ -111,8 +106,8 @@ TEST_F(ExtensionWebUITest, ExtensionURLOverride) {
   EXPECT_TRUE(ExtensionWebUI::HandleChromeURLOverride(&url, profile_.get()));
   EXPECT_EQ(url, expected_component_override_url);
 
-  // This time the non-component extension was registered more recently.
-  // The non-component extension should still have precedence.
+  // This time the non-component extension was registered more recently and
+  // should still take precedence.
   ExtensionWebUI::RegisterChromeURLOverrides(
       profile_.get(), URLOverrides::GetChromeURLOverrides(ext_unpacked.get()));
   url = GURL("chrome://bookmarks");