Generate IDs for --load-extension by hashing the path instead

of incrementing a static counter.

BUG=21281
TEST=Load an unpacked extension from chrome://extensions/ that
does not have an ID in its manifest. The resulting ID should
be random-looking, not something like "aaaaaa".

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

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

11 files changed, 68 insertions, 88 deletions

diff --git a/chrome/browser/extensions/extension_browsertest.cc b/chrome/browser/extensions/extension_browsertest.cc
index c7e37fa..b7acc21 100644
--- a/chrome/browser/extensions/extension_browsertest.cc
+++ b/chrome/browser/extensions/extension_browsertest.cc
@@ -199,6 +199,7 @@ void ExtensionBrowserTest::Observe(NotificationType type,
                                    const NotificationDetails& details) {
   switch (type.value) {
     case NotificationType::EXTENSION_LOADED:
+      last_loaded_extension_id_ = Details<Extension>(details).ptr()->id();
       std::cout << "Got EXTENSION_LOADED notification.\n";
       MessageLoopForUI::current()->Quit();
       break;
diff --git a/chrome/browser/extensions/extension_browsertest.h b/chrome/browser/extensions/extension_browsertest.h
index 943bdd4..5e2b882 100644
--- a/chrome/browser/extensions/extension_browsertest.h
+++ b/chrome/browser/extensions/extension_browsertest.h
@@ -56,6 +56,7 @@ class ExtensionBrowserTest
   bool loaded_;
   bool installed_;
   FilePath test_data_dir_;
+  std::string last_loaded_extension_id_;
 
  private:
   bool InstallOrUpdateExtension(const std::string& id, const FilePath& path,
diff --git a/chrome/browser/extensions/extension_browsertests_misc.cc b/chrome/browser/extensions/extension_browsertests_misc.cc
index bdda3dd..bab50d1 100644
--- a/chrome/browser/extensions/extension_browsertests_misc.cc
+++ b/chrome/browser/extensions/extension_browsertests_misc.cc
@@ -27,9 +27,6 @@
 #include "chrome/test/ui_test_utils.h"
 #include "net/base/net_util.h"
 
-// ID assigned to the first unpacked extension loaded by LoadExtension().
-#define kDefaultExtensionID "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
-
 const std::wstring kSubscribePage =
     L"files/extensions/samples/subscribe_page_action/subscribe.html";
 const std::wstring kValidFeed0 = L"files/feeds/feed_script.xml";
@@ -250,7 +247,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, UnloadPageAction) {
   ui_test_utils::NavigateToURL(browser(), net::FilePathToFileURL(feed));
   ASSERT_TRUE(WaitForPageActionCountChangeTo(1));
 
-  UnloadExtension(kDefaultExtensionID);
+  UnloadExtension(last_loaded_extension_id_);
 
   // Make sure the page action goes away when it's unloaded.
   ASSERT_TRUE(WaitForPageActionCountChangeTo(0));
@@ -622,7 +619,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, WindowOpenExtension) {
 
   TabContents* newtab = WindowOpenHelper(
       browser(),
-      GURL("chrome-extension://" kDefaultExtensionID "/test.html"),
+      GURL(std::string("chrome-extension://") + last_loaded_extension_id_ +
+           "/test.html"),
       "newtab.html");
 
   bool result = false;
@@ -639,7 +637,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, WindowOpenInvalidExtension) {
 
   WindowOpenHelper(
       browser(),
-      GURL("chrome-extension://" kDefaultExtensionID "/test.html"),
+      GURL(std::string("chrome-extension://") + last_loaded_extension_id_ +
+           "/test.html"),
       "chrome-extension://thisissurelynotavalidextensionid/newtab.html");
 
   // If we got to this point, we didn't crash, so we're good.
@@ -655,7 +654,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, WindowOpenNoPrivileges) {
   TabContents* newtab = WindowOpenHelper(
       browser(),
       GURL("about:blank"),
-      "chrome-extension://" kDefaultExtensionID "/newtab.html");
+      std::string("chrome-extension://") + last_loaded_extension_id_ +
+          "/newtab.html");
 
   // Extension API should fail.
   bool result = false;
diff --git a/chrome/browser/extensions/extension_uitest.cc b/chrome/browser/extensions/extension_uitest.cc
index 1f9238a..8518dad 100644
--- a/chrome/browser/extensions/extension_uitest.cc
+++ b/chrome/browser/extensions/extension_uitest.cc
@@ -400,7 +400,7 @@ void BrowserEventAutomationProxy::HandleMessageFromChrome() {
     // TEST_F(BrowserEventExtensionTest, RunTest) to understand where the
     // extension Id comes from.
     tab_->HandleMessageFromExternalHost(
-        "{\"rqid\":0, \"extid\": \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\","
+        "{\"rqid\":0, \"extid\": \"ofoknjclcmghjfmbncljcnpjmfmldhno\","
         " \"connid\": 1}",
         keys::kAutomationOrigin,
         keys::kAutomationPortRequestTarget);
@@ -475,15 +475,8 @@ TEST_F(BrowserEventExtensionTest, RunTest) {
   // This test loads an HTML file that tries to add listeners to a bunch of
   // chrome.* events and upon adding a listener it posts the name of the event
   // to the automation layer, which we'll count to make sure the events work.
-  //
-  // The extension for this test does not specify a "key" property in its
-  // manifest file.  Therefore, the extension system will automatically assign
-  // it an Id.  To make this test consistent and non-flaky, the genetated Id
-  // counter is reset before the test so that we can hardcode the first Id
-  // that will be generated.
-  Extension::ResetGeneratedIdCounter();
   TestWithURL(GURL(
-      "chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/test.html"));
+      "chrome-extension://ofoknjclcmghjfmbncljcnpjmfmldhno/test.html"));
   BrowserEventAutomationProxy* proxy =
       static_cast<BrowserEventAutomationProxy*>(automation());
 
diff --git a/chrome/browser/extensions/extension_updater_unittest.cc b/chrome/browser/extensions/extension_updater_unittest.cc
index e9892a2..69f2460 100644
--- a/chrome/browser/extensions/extension_updater_unittest.cc
+++ b/chrome/browser/extensions/extension_updater_unittest.cc
@@ -79,8 +79,6 @@ class ScopedTempPrefService {
   scoped_ptr<PrefService> prefs_;
 };
 
-const char* kIdPrefix = "000000000000000000000000000000000000000";
-
 // Creates test extensions and inserts them into list. The name and
 // version are all based on their index. If |update_url| is non-null, it
 // will be used as the update_url for each extension.
@@ -88,7 +86,12 @@ void CreateTestExtensions(int count, ExtensionList *list,
                           const std::string* update_url) {
   for (int i = 1; i <= count; i++) {
     DictionaryValue input;
-    Extension* e = new Extension();
+#if defined(OS_WIN)
+    FilePath path(StringPrintf(L"c:\\extension%i", i));
+#else
+    FilePath path(StringPrintf("/extension%i", i));
+#endif
+    Extension* e = new Extension(path);
     input.SetString(extension_manifest_keys::kVersion,
                     StringPrintf("%d.0.0.0", i));
     input.SetString(extension_manifest_keys::kName,
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 04ff3d7..fa257e0 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -1197,7 +1197,6 @@ TEST_F(ExtensionsServiceTest, LoadExtension) {
 // --load-extension.
 TEST_F(ExtensionsServiceTest, GenerateID) {
   InitializeEmptyExtensionsService();
-  Extension::ResetGeneratedIdCounter();
 
   FilePath extensions_path;
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
@@ -1208,25 +1207,19 @@ TEST_F(ExtensionsServiceTest, GenerateID) {
   loop_.RunAllPending();
   EXPECT_EQ(0u, GetErrors().size());
   ASSERT_EQ(1u, loaded_.size());
-  std::string id1 = loaded_[0]->id();
-  EXPECT_EQ(all_zero, id1);
-  EXPECT_EQ("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/",
-            loaded_[0]->url().spec());
+  ASSERT_TRUE(Extension::IdIsValid(loaded_[0]->id()));
   EXPECT_EQ(loaded_[0]->location(), Extension::LOAD);
 
   // --load-extension doesn't add entries to prefs
   ValidatePrefKeyCount(0);
 
+  std::string previous_id = loaded_[0]->id();
+
+  // If we reload the same path, we should get the same extension ID.
   service_->LoadExtension(no_id_ext);
   loop_.RunAllPending();
-  std::string id2 = loaded_[1]->id();
-  EXPECT_EQ(zero_n_one, id2);
-  EXPECT_EQ("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab/",
-            loaded_[1]->url().spec());
-  EXPECT_EQ(loaded_[1]->location(), Extension::LOAD);
-
-  // --load-extension doesn't add entries to prefs
-  ValidatePrefKeyCount(0);
+  ASSERT_EQ(1u, loaded_.size());
+  ASSERT_EQ(previous_id, loaded_[0]->id());
 }
 
 // Tests the external installation feature
diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker.cc b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
index 9d043b7..8756450 100644
--- a/chrome/browser/extensions/sandboxed_extension_unpacker.cc
+++ b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
@@ -127,18 +127,13 @@ void SandboxedExtensionUnpacker::OnUnpackExtensionSucceeded(
     return;
   }
 
-  // Delete any images that may be used by the browser.  We're going to write
-  // out our own versions of the parsed images, and we want to make sure the
-  // originals are gone for good.
-  extension_.reset(new Extension);
-  std::string manifest_error;
-
-  // Update the path to refer to the temporary location. We do this because
-  // clients may want to use resources inside the extension before it is
-  // installed and they need the correct path. For example, the install UI shows
-  // one of the icons from the extension.
-  extension_->set_path(extension_root_);
+  // Create an extension object that refers to the temporary location the
+  // extension was unpacked to. We use this until the extension is finally
+  // installed. For example, the install UI shows images from inside the
+  // extension.
+  extension_.reset(new Extension(extension_root_));
 
+  std::string manifest_error;
   if (!extension_->InitFromValue(*final_manifest, true,  // require id
                                  &manifest_error)) {
     ReportFailure(std::string("Manifest is invalid: ") +
@@ -146,6 +141,9 @@ void SandboxedExtensionUnpacker::OnUnpackExtensionSucceeded(
     return;
   }
 
+  // Delete any images that may be used by the browser.  We're going to write
+  // out our own versions of the parsed images, and we want to make sure the
+  // originals are gone for good.
   std::set<FilePath> image_paths = extension_->GetBrowserImages();
   if (image_paths.size() != images.size()) {
     ReportFailure("Decoded images don't match what's in the manifest.");
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 2ac504e..f00db65 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -64,9 +64,6 @@ static bool IsAPIPermission(const std::string& str) {
 
 }  // namespace
 
-// static
-int Extension::id_counter_ = 0;
-
 const char Extension::kManifestFilename[] = "manifest.json";
 const char Extension::kLocaleFolder[] = "_locales";
 const char Extension::kMessagesFilename[] = "messages.json";
@@ -176,8 +173,7 @@ Extension::Location Extension::ExternalExtensionInstallType(
   return Extension::EXTERNAL_PREF;
 }
 
-bool Extension::GenerateIdFromPublicKey(const std::string& input,
-                                        std::string* output) {
+bool Extension::GenerateId(const std::string& input, std::string* output) {
   CHECK(output);
   if (input.length() == 0)
     return false;
@@ -607,7 +603,7 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     std::string public_key_bytes;
     if (!source.GetString(keys::kPublicKey, &public_key_) ||
       !ParsePEMKeyBytes(public_key_, &public_key_bytes) ||
-      !GenerateIdFromPublicKey(public_key_bytes, &id_)) {
+      !GenerateId(public_key_bytes, &id_)) {
         *error = errors::kInvalidKey;
         return false;
     }
@@ -615,14 +611,13 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     *error = errors::kInvalidKey;
     return false;
   } else {
-    // Generate a random ID
-    id_ = StringPrintf("%x", NextGeneratedId());
-
-    // pad the string out to kIdSize*2 chars with zeroes.
-    id_.insert(0, Extension::kIdSize*2 - id_.length(), '0');
-
-    // Convert to our mp-decimal.
-    ConvertHexadecimalToIDAlphabet(&id_);
+    // If there is a path, we generate the ID from it. This is useful for
+    // development mode, because it keeps the ID stable across restarts and
+    // reloading the extension.
+    if (!GenerateId(WideToUTF8(path_.ToWStringHack()), &id_)) {
+      NOTREACHED() << "Could not create ID from path.";
+      return false;
+    }
   }
 
   // Make a copy of the manifest so we can store it in prefs.
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index d77e5e27..c01d948 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -106,17 +106,9 @@ class Extension {
   // The mimetype used for extensions.
   static const char kMimeType[];
 
-  Extension()
-      : location_(INVALID), is_theme_(false),
-        background_page_ready_(false) {}
   explicit Extension(const FilePath& path);
   virtual ~Extension();
 
-  // Resets the id counter. This is only useful for unit tests.
-  static void ResetGeneratedIdCounter() {
-    id_counter_ = 0;
-  }
-
   // Checks to see if the extension has a valid ID.
   static bool IdIsValid(const std::string& id);
 
@@ -160,10 +152,9 @@ class Extension {
   // Does a simple base64 encoding of |input| into |output|.
   static bool ProducePEM(const std::string& input, std::string* output);
 
-  // Note: The result is coverted to lower-case because the browser enforces
-  // hosts to be lower-case in omni-bar.
-  static bool GenerateIdFromPublicKey(const std::string& input,
-                                      std::string* output);
+  // Generates an extension ID from arbitrary input. The same input string will
+  // always generate the same output ID.
+  static bool GenerateId(const std::string& input, std::string* output);
 
   // Expects base64 encoded |input| and formats into |output| including
   // the appropriate header & footer.
@@ -281,14 +272,6 @@ class Extension {
   void SetBackgroundPageReady();
 
  private:
-  // Counter used to assign ids to extensions that are loaded using
-  // --load-extension.
-  static int id_counter_;
-
-  // Returns the next counter id. Intentionally post-incrementing so that first
-  // value is 0.
-  static int NextGeneratedId() { return id_counter_++; }
-
   // Helper method that loads a UserScript object from a
   // dictionary in the content_script list of the manifest.
   bool LoadUserScriptHelper(const DictionaryValue* content_script,
diff --git a/chrome/common/extensions/extension_unittest.cc b/chrome/common/extensions/extension_unittest.cc
index fc024ab..466f27a 100644
--- a/chrome/common/extensions/extension_unittest.cc
+++ b/chrome/common/extensions/extension_unittest.cc
@@ -227,8 +227,6 @@ TEST(ExtensionTest, InitFromValueValid) {
 #elif defined(OS_POSIX)
   FilePath path(FILE_PATH_LITERAL("/foo"));
 #endif
-  Extension::ResetGeneratedIdCounter();
-
   Extension extension(path);
   std::string error;
   DictionaryValue input_value;
@@ -239,11 +237,10 @@ TEST(ExtensionTest, InitFromValueValid) {
 
   EXPECT_TRUE(extension.InitFromValue(input_value, false, &error));
   EXPECT_EQ("", error);
-  EXPECT_EQ("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", extension.id());
+  EXPECT_TRUE(Extension::IdIsValid(extension.id()));
   EXPECT_EQ("1.0.0.0", extension.VersionString());
   EXPECT_EQ("my extension", extension.name());
-  EXPECT_EQ("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/",
-            extension.url().spec());
+  EXPECT_EQ(extension.id(), extension.url().host());
   EXPECT_EQ(path.value(), extension.path().value());
 }
 
@@ -277,7 +274,12 @@ TEST(ExtensionTest, GetResourceURLAndPath) {
 }
 
 TEST(ExtensionTest, LoadPageActionHelper) {
-  Extension extension;
+#if defined(OS_WIN)
+    FilePath path(StringPrintf(L"c:\\extension"));
+#else
+    FilePath path(StringPrintf("/extension"));
+#endif
+  Extension extension(path);
   std::string error_msg;
   scoped_ptr<ExtensionAction> action;
   DictionaryValue input;
@@ -424,7 +426,7 @@ TEST(ExtensionTest, IdIsValid) {
   EXPECT_FALSE(Extension::IdIsValid("abcdefghijklmnopabcdefghijklmno0"));
 }
 
-TEST(ExtensionTest, GenerateIDFromPublicKey) {
+TEST(ExtensionTest, GenerateID) {
   const uint8 public_key_info[] = {
     0x30, 0x81, 0x9f, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7,
     0x0d, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03, 0x81, 0x8d, 0x00, 0x30, 0x81,
@@ -444,7 +446,7 @@ TEST(ExtensionTest, GenerateIDFromPublicKey) {
 
   std::string extension_id;
   EXPECT_TRUE(
-      Extension::GenerateIdFromPublicKey(
+      Extension::GenerateId(
           std::string(reinterpret_cast<const char*>(&public_key_info[0]),
                       arraysize(public_key_info)),
           &extension_id));
@@ -463,7 +465,12 @@ TEST(ExtensionTest, UpdateUrls) {
     EXPECT_TRUE(url.is_valid());
 
     DictionaryValue input_value;
-    Extension extension;
+#if defined(OS_WIN)
+    FilePath path(StringPrintf(L"c:\\extension%i", i));
+#else
+    FilePath path(StringPrintf("/extension%i", i));
+#endif
+    Extension extension(path);
     std::string error;
 
     input_value.SetString(keys::kVersion, "1.0");
@@ -480,7 +487,12 @@ TEST(ExtensionTest, UpdateUrls) {
   valid.push_back("http://test.com/update#whatever");
   for (size_t i = 0; i < invalid.size(); i++) {
     DictionaryValue input_value;
-    Extension extension;
+#if defined(OS_WIN)
+    FilePath path(StringPrintf(L"c:\\extension%i", i));
+#else
+    FilePath path(StringPrintf("/extension%i", i));
+#endif
+    Extension extension(path);
     std::string error;
     input_value.SetString(keys::kVersion, "1.0");
     input_value.SetString(keys::kName, "Test");
@@ -529,7 +541,7 @@ static Extension* LoadManifest(const std::string& dir,
     return NULL;
 
   std::string error;
-  scoped_ptr<Extension> extension(new Extension);
+  scoped_ptr<Extension> extension(new Extension(path.DirName()));
   extension->InitFromValue(*static_cast<DictionaryValue*>(result.get()),
                            false, &error);
 
diff --git a/chrome/test/data/extensions/uitest/event_sink/manifest.json b/chrome/test/data/extensions/uitest/event_sink/manifest.json
index 859aa8f..9e29271 100644
--- a/chrome/test/data/extensions/uitest/event_sink/manifest.json
+++ b/chrome/test/data/extensions/uitest/event_sink/manifest.json
@@ -1,5 +1,6 @@
 {
   "version": "1.0.0.0",
+  "key": "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC4RIIi/YbLZ6dRpdQnWfrQuo2vunkuPJpD9HNvZQ3J5aylSw7Y8ghzom793CbTJK1ZI254yZtkaWZJWOBhPKCaaRac+qfpRN1spl42vpyMn1OrGQm1pwZT6rDpCyIVIx/k2o4puMsQHNUIqxXPu3Oj+KSVdCIGOmabMhz765UjhwIDAQAB",
   "name": "Browser Event Sink Test Extension",
   "description": "An extension UITest for testing the sending of browser events.",
   "permissions": ["tabs", "bookmarks"]