Fix bug that caused us to have many duplicate registrations

recorded in extension prefs for override pages.

BUG=41442

Review URL: http://codereview.chromium.org/1518028

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

3 files changed, 96 insertions, 5 deletions

diff --git a/chrome/browser/extensions/extension_dom_ui.cc b/chrome/browser/extensions/extension_dom_ui.cc
index 40f11f1..5314078 100644
--- a/chrome/browser/extensions/extension_dom_ui.cc
+++ b/chrome/browser/extensions/extension_dom_ui.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/extensions/extension_dom_ui.h"
 
+#include <set>
+
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "net/base/file_stream.h"
@@ -23,8 +25,6 @@
 #include "chrome/common/url_constants.h"
 
 namespace {
-const wchar_t kExtensionURLOverrides[] = L"extensions.chrome_url_overrides";
-
 // Returns a piece of memory with the contents of the file |path|.
 RefCountedMemory* ReadFileData(const FilePath& path) {
   // TODO(arv): We currently read this on the UI thread since extension objects
@@ -45,8 +45,30 @@ RefCountedMemory* ReadFileData(const FilePath& path) {
   return result;
 }
 
+// De-dupes the items in |list|. Assumes the values are strings.
+void CleanUpDuplicates(ListValue* list) {
+  std::set<std::string> seen_values;
+
+  // Loop backwards as we may be removing items.
+  for (size_t i = list->GetSize() - 1; (i + 1) > 0; --i) {
+    std::string value;
+    if (!list->GetString(i, &value)) {
+      NOTREACHED();
+      continue;
+    }
+
+    if (seen_values.find(value) == seen_values.end())
+      seen_values.insert(value);
+    else
+      list->Remove(i, NULL);
+  }
+}
+
 }  // namespace
 
+const wchar_t ExtensionDOMUI::kExtensionURLOverrides[] =
+    L"extensions.chrome_url_overrides";
+
 ExtensionDOMUI::ExtensionDOMUI(TabContents* tab_contents)
     : DOMUI(tab_contents) {
   should_hide_url_ = true;
@@ -222,6 +244,8 @@ void ExtensionDOMUI::RegisterChromeURLOverrides(
       page_overrides = new ListValue();
       all_overrides->Set(key, page_overrides);
     } else {
+      CleanUpDuplicates(page_overrides);
+
       // Verify that the override isn't already in the list.
       ListValue::iterator i = page_overrides->begin();
       for (; i != page_overrides->end(); ++i) {
@@ -230,7 +254,7 @@ void ExtensionDOMUI::RegisterChromeURLOverrides(
           NOTREACHED();
           continue;
         }
-        if (override_val == (*iter).first)
+        if (override_val == (*iter).second.spec())
           break;
       }
       // This value is already in the list, leave it alone.
diff --git a/chrome/browser/extensions/extension_dom_ui.h b/chrome/browser/extensions/extension_dom_ui.h
index d14eb69..87f57fc 100644
--- a/chrome/browser/extensions/extension_dom_ui.h
+++ b/chrome/browser/extensions/extension_dom_ui.h
@@ -26,6 +26,8 @@ class ExtensionDOMUI
     : public DOMUI,
       public ExtensionFunctionDispatcher::Delegate {
  public:
+  static const wchar_t kExtensionURLOverrides[];
+
   explicit ExtensionDOMUI(TabContents* tab_contents);
 
   ExtensionFunctionDispatcher* extension_function_dispatcher() const {
diff --git a/chrome/browser/extensions/extension_override_apitest.cc b/chrome/browser/extensions/extension_override_apitest.cc
index fd52440..2afe199 100644
--- a/chrome/browser/extensions/extension_override_apitest.cc
+++ b/chrome/browser/extensions/extension_override_apitest.cc
@@ -2,10 +2,40 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/browser.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/extension_dom_ui.h"
 #include "chrome/test/ui_test_utils.h"
 
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, OverrideNewtab) {
+class ExtensionOverrideTest : public ExtensionApiTest {
+ protected:
+  bool CheckHistoryOverridesContainsNoDupes() {
+    // There should be no duplicate entries in the preferences.
+    const DictionaryValue* overrides =
+        browser()->profile()->GetPrefs()->GetDictionary(
+            ExtensionDOMUI::kExtensionURLOverrides);
+
+    ListValue* values = NULL;
+    if (!overrides->GetList(L"history", &values))
+      return false;
+
+    std::set<std::string> seen_overrides;
+    for (size_t i = 0; i < values->GetSize(); ++i) {
+      std::string value;
+      if (!values->GetString(i, &value))
+        return false;
+
+      if (seen_overrides.find(value) != seen_overrides.end())
+        return false;
+
+      seen_overrides.insert(value);
+    }
+
+    return true;
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, OverrideNewtab) {
   ASSERT_TRUE(RunExtensionTest("override/newtab")) << message_;
   {
     ResultCatcher catcher;
@@ -19,7 +49,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, OverrideNewtab) {
   // Verify behavior, then unload the first and verify behavior, etc.
 }
 
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, OverrideHistory) {
+IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, OverrideHistory) {
   ASSERT_TRUE(RunExtensionTest("override/history")) << message_;
   {
     ResultCatcher catcher;
@@ -29,3 +59,38 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, OverrideHistory) {
     ASSERT_TRUE(catcher.GetNextResult());
   }
 }
+
+// Regression test for http://crbug.com/41442.
+IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, ShouldNotCreateDuplicateEntries) {
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("override/history")));
+
+  // Simulate several LoadExtension() calls happening over the lifetime of
+  // a preferences file without corresponding UnloadExtension() calls.
+  for (size_t i = 0; i < 3; ++i) {
+    ExtensionDOMUI::RegisterChromeURLOverrides(
+        browser()->profile(),
+        browser()->profile()->GetExtensionsService()->extensions()->back()->
+            GetChromeURLOverrides());
+  }
+
+  ASSERT_TRUE(CheckHistoryOverridesContainsNoDupes());
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionOverrideTest, ShouldCleanUpDuplicateEntries) {
+  // Simulate several LoadExtension() calls happening over the lifetime of
+  // a preferences file without corresponding UnloadExtension() calls. This is
+  // the same as the above test, except for that it is testing the case where
+  // the file already contains dupes when an extension is loaded.
+  ListValue* list = new ListValue();
+  for (size_t i = 0; i < 3; ++i)
+    list->Append(Value::CreateStringValue("http://www.google.com/"));
+
+  browser()->profile()->GetPrefs()->GetMutableDictionary(
+      ExtensionDOMUI::kExtensionURLOverrides)->Set(L"history", list);
+
+  ASSERT_FALSE(CheckHistoryOverridesContainsNoDupes());
+
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("override/history")));
+
+  ASSERT_TRUE(CheckHistoryOverridesContainsNoDupes());
+}