Bookmark import from IE preserving the order.

This change is to read the order information of IE Favorites from registry and sort the imported bookmarks in that order. If it failed to read the information from registry, it falls back to the current implementation: alphabetical sorting.

BUG=4962
TEST=unit_tests --gtest_filter=*Importer*

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

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

5 files changed, 605 insertions, 151 deletions

diff --git a/chrome/browser/importer/firefox2_importer.cc b/chrome/browser/importer/firefox2_importer.cc
index 399895e..b59a561 100644
--- a/chrome/browser/importer/firefox2_importer.cc
+++ b/chrome/browser/importer/firefox2_importer.cc
@@ -158,7 +158,6 @@ void Firefox2Importer::ImportBookmarksFile(
   std::vector<std::string> lines;
   base::SplitString(content, '\n', &lines);
 
-  std::vector<ProfileWriter::BookmarkEntry> toolbar_bookmarks;
   string16 last_folder;
   bool last_folder_on_toolbar = false;
   bool last_folder_is_empty = true;
@@ -217,7 +216,6 @@ void Firefox2Importer::ImportBookmarksFile(
         // The toolbar folder should be at the top level.
         entry.in_toolbar = true;
         entry.path.assign(path.begin() + toolbar_folder - 1, path.end());
-        toolbar_bookmarks.push_back(entry);
       } else {
         // Add this bookmark to the list of |bookmarks|.
         if (!has_subfolder && !last_folder.empty()) {
@@ -225,8 +223,8 @@ void Firefox2Importer::ImportBookmarksFile(
           last_folder.clear();
         }
         entry.path.assign(path.begin(), path.end());
-        bookmarks->push_back(entry);
       }
+      bookmarks->push_back(entry);
 
       // Save the favicon. DataURLToFaviconUsage will handle the case where
       // there is no favicon.
@@ -275,7 +273,7 @@ void Firefox2Importer::ImportBookmarksFile(
           if (toolbar_folder <= path.size()) {
             entry.in_toolbar = true;
             entry.path.assign(path.begin() + toolbar_folder - 1, path.end());
-            toolbar_bookmarks.push_back(entry);
+            bookmarks->push_back(entry);
           }
         } else {
           // Add this folder to the list of |bookmarks|.
@@ -291,9 +289,6 @@ void Firefox2Importer::ImportBookmarksFile(
         toolbar_folder = 0;
     }
   }
-
-  bookmarks->insert(bookmarks->begin(), toolbar_bookmarks.begin(),
-                    toolbar_bookmarks.end());
 }
 
 void Firefox2Importer::ImportBookmarks() {
diff --git a/chrome/browser/importer/ie_importer.cc b/chrome/browser/importer/ie_importer.cc
index 1d34c6a..54c4093 100644
--- a/chrome/browser/importer/ie_importer.cc
+++ b/chrome/browser/importer/ie_importer.cc
@@ -43,6 +43,21 @@
 
 namespace {
 
+// Registry key paths from which we import IE settings.
+const char16 kStorage2Path[] =
+  L"Software\\Microsoft\\Internet Explorer\\IntelliForms\\Storage2";
+const char16 kSearchScopePath[] =
+  L"Software\\Microsoft\\Internet Explorer\\SearchScopes";
+const char16 kIESettingsMain[] =
+  L"Software\\Microsoft\\Internet Explorer\\Main";
+const char16 kIEFavoritesOrderKey[] =
+  L"Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\"
+  L"MenuOrder\\Favorites";
+const char16 kIEVersionKey[] =
+  L"Software\\Microsoft\\Internet Explorer";
+const char16 kIEToolbarKey[] =
+  L"Software\\Microsoft\\Internet Explorer\\Toolbar";
+
 // A struct that hosts the information of AutoComplete data in PStore.
 struct AutoCompleteInfo {
   string16 key;
@@ -64,6 +79,189 @@ base::Time GetFileCreationTime(const string16& file) {
   return creation_time;
 }
 
+// Safely read an object of type T from a raw sequence of bytes.
+template<typename T>
+bool BinaryRead(T* data, size_t offset, const std::vector<uint8>& blob) {
+  if (offset + sizeof(T) > blob.size())
+    return false;
+  memcpy(data, &blob[offset], sizeof(T));
+  return true;
+}
+
+// Safely read an ITEMIDLIST from a raw sequence of bytes.
+//
+// An ITEMIDLIST is a list of SHITEMIDs, terminated by a SHITEMID with
+// .cb = 0. Here, before simply casting &blob[offset] to LPITEMIDLIST,
+// we verify that the list structure is not overrunning the boundary of
+// the binary blob.
+LPCITEMIDLIST BinaryReadItemIDList(size_t offset, size_t idlist_size,
+                                   const std::vector<uint8>& blob) {
+  size_t head = 0;
+  while (true) {
+    SHITEMID id;
+    if (head >= idlist_size || !BinaryRead(&id, offset + head, blob))
+      return NULL;
+    if (id.cb == 0)
+      break;
+    head += id.cb;
+  }
+  return reinterpret_cast<LPCITEMIDLIST>(&blob[offset]);
+}
+
+// Compares the two bookmarks in the order of IE's Favorites menu.
+// Returns true if rhs should come later than lhs (lhs < rhs).
+struct IEOrderBookmarkComparator {
+  bool operator()(const ProfileWriter::BookmarkEntry& lhs,
+                  const ProfileWriter::BookmarkEntry& rhs) const {
+    static const uint32 kNotSorted = 0xfffffffb; // IE uses this magic value.
+    FilePath lhs_prefix;
+    FilePath rhs_prefix;
+    for (size_t i = 0; i <= lhs.path.size() && i <= rhs.path.size(); ++i) {
+      const FilePath::StringType lhs_i =
+        (i < lhs.path.size() ? lhs.path[i] : lhs.title + L".url");
+      const FilePath::StringType rhs_i =
+        (i < rhs.path.size() ? rhs.path[i] : rhs.title + L".url");
+      lhs_prefix = lhs_prefix.Append(lhs_i);
+      rhs_prefix = rhs_prefix.Append(rhs_i);
+      if (lhs_i == rhs_i)
+        continue;
+      // The first path element that differs between the two.
+      std::map<FilePath, uint32>::const_iterator lhs_iter =
+        sort_index_->find(lhs_prefix);
+      std::map<FilePath, uint32>::const_iterator rhs_iter =
+        sort_index_->find(rhs_prefix);
+      uint32 lhs_sort_index = (lhs_iter == sort_index_->end() ? kNotSorted
+        : lhs_iter->second);
+      uint32 rhs_sort_index = (rhs_iter == sort_index_->end() ? kNotSorted
+        : rhs_iter->second);
+      if (lhs_sort_index != rhs_sort_index)
+        return lhs_sort_index < rhs_sort_index;
+      // If they have the same sort order, sort alphabetically.
+      return lhs_i < rhs_i;
+    }
+    return lhs.path.size() < rhs.path.size();
+  }
+  const std::map<FilePath, uint32>* sort_index_;
+};
+
+// IE stores the order of the Favorites menu in registry under:
+// HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\MenuOrder\Favorites.
+// The folder hierarchy of Favorites menu is directly mapped to the key
+// hierarchy in the registry.
+//
+// If the order of the items in a folder is customized by user, the order is
+// recorded in the REG_BINARY value named "Order" of the corresponding key.
+// The content of the "Order" value is a raw binary dump of an array of the
+// following data structure
+//   struct {
+//     uint32 size;  // Note that ITEMIDLIST is variably-sized.
+//     uint32 sort_index;  // 0 means this is the first item, 1 the second, ...
+//     ITEMIDLIST item_id;
+//   };
+// where each item_id should correspond to a favorites link file (*.url) in
+// the current folder.
+bool ParseFavoritesOrderBlob(
+    const Importer* importer,
+    const std::vector<uint8>& blob,
+    const FilePath& path,
+    std::map<FilePath, uint32>* sort_index) WARN_UNUSED_RESULT {
+  static const int kItemCountOffset = 16;
+  static const int kItemListStartOffset = 20;
+
+  // Read the number of items.
+  uint32 item_count = 0;
+  if (!BinaryRead(&item_count, kItemCountOffset, blob))
+    return false;
+
+  // Traverse over the items.
+  size_t base_offset = kItemListStartOffset;
+  for (uint32 i = 0; i < item_count && !importer->cancelled(); ++i) {
+    static const int kSizeOffset = 0;
+    static const int kSortIndexOffset = 4;
+    static const int kItemIDListOffset = 8;
+
+    // Read the size (number of bytes) of the current item.
+    uint32 item_size = 0;
+    if (!BinaryRead(&item_size, base_offset + kSizeOffset, blob) ||
+        base_offset + item_size <= base_offset || // checking overflow
+        base_offset + item_size > blob.size())
+      return false;
+
+    // Read the sort index of the current item.
+    uint32 item_sort_index = 0;
+    if (!BinaryRead(&item_sort_index, base_offset + kSortIndexOffset, blob))
+      return false;
+
+    // Read the file name from the ITEMIDLIST structure.
+    LPCITEMIDLIST idlist = BinaryReadItemIDList(
+      base_offset + kItemIDListOffset, item_size - kItemIDListOffset, blob);
+    TCHAR item_filename[MAX_PATH];
+    if (!idlist || FAILED(SHGetPathFromIDList(idlist, item_filename)))
+      return false;
+    FilePath item_relative_path =
+      path.Append(FilePath(item_filename).BaseName());
+
+    // Record the retrieved information and go to the next item.
+    sort_index->insert(std::make_pair(item_relative_path, item_sort_index));
+    base_offset += item_size;
+  }
+  return true;
+}
+
+bool ParseFavoritesOrderRegistryTree(
+    const Importer* importer,
+    const base::win::RegKey& key,
+    const FilePath& path,
+    std::map<FilePath, uint32>* sort_index) WARN_UNUSED_RESULT {
+  // Parse the order information of the current folder.
+  DWORD blob_length = 0;
+  if (key.ReadValue(L"Order", NULL, &blob_length, NULL) == ERROR_SUCCESS) {
+    std::vector<uint8> blob(blob_length);
+    if (blob_length > 0 &&
+        key.ReadValue(L"Order", reinterpret_cast<DWORD*>(&blob[0]),
+                      &blob_length, NULL) == ERROR_SUCCESS) {
+      if (!ParseFavoritesOrderBlob(importer, blob, path, sort_index))
+        return false;
+    }
+  }
+
+  // Recursively parse subfolders.
+  for (base::win::RegistryKeyIterator child(key.Handle(), L"");
+       child.Valid() && !importer->cancelled();
+       ++child) {
+    base::win::RegKey subkey(key.Handle(), child.Name(), KEY_READ);
+    if (subkey.Valid()) {
+      FilePath subpath(path.Append(child.Name()));
+      if (!ParseFavoritesOrderRegistryTree(importer, subkey, subpath,
+                                           sort_index)) {
+        return false;
+      }
+    }
+  }
+  return true;
+}
+
+bool ParseFavoritesOrderInfo(
+    const Importer* importer,
+    std::map<FilePath, uint32>* sort_index) WARN_UNUSED_RESULT {
+  base::win::RegKey key(HKEY_CURRENT_USER, kIEFavoritesOrderKey, KEY_READ);
+  if (!key.Valid())
+    return false;
+  return ParseFavoritesOrderRegistryTree(importer, key, FilePath(), sort_index);
+}
+
+// Read the sort order from registry. If failed, we don't touch the list
+// and use the default (alphabetical) order.
+void SortBookmarksInIEOrder(
+    const Importer* importer,
+    std::vector<ProfileWriter::BookmarkEntry>* bookmarks) {
+  std::map<FilePath, uint32> sort_index;
+  if (!ParseFavoritesOrderInfo(importer, &sort_index))
+    return;
+  IEOrderBookmarkComparator compare = {&sort_index};
+  std::sort(bookmarks->begin(), bookmarks->end(), compare);
+}
+
 }  // namespace
 
 // static
@@ -322,9 +520,6 @@ void IEImporter::ImportPasswordsIE7() {
     return;
   }
 
-  const wchar_t* kStorage2Path =
-      L"Software\\Microsoft\\Internet Explorer\\IntelliForms\\Storage2";
-
   base::win::RegKey key(HKEY_CURRENT_USER, kStorage2Path, KEY_READ);
   base::win::RegistryValueIterator reg_iterator(HKEY_CURRENT_USER,
                                                 kStorage2Path);
@@ -356,9 +551,6 @@ void IEImporter::ImportSearchEngines() {
   // Each key represents a search engine. The URL value contains the URL and
   // the DisplayName the name.
   // The default key's name is contained under DefaultScope.
-  const wchar_t* kSearchScopePath =
-      L"Software\\Microsoft\\Internet Explorer\\SearchScopes";
-
   base::win::RegKey key(HKEY_CURRENT_USER, kSearchScopePath, KEY_READ);
   string16 default_search_engine_name;
   const TemplateURL* default_search_engine = NULL;
@@ -434,8 +626,6 @@ void IEImporter::ImportSearchEngines() {
 }
 
 void IEImporter::ImportHomepage() {
-  const wchar_t* kIESettingsMain =
-      L"Software\\Microsoft\\Internet Explorer\\Main";
   const wchar_t* kIEHomepage = L"Start Page";
   const wchar_t* kIEDefaultHomepage = L"Default_Page_URL";
 
@@ -510,8 +700,7 @@ bool IEImporter::GetFavoritesInfo(IEImporter::FavoritesInfo* info) {
   if (base::win::GetVersion() < base::win::VERSION_VISTA) {
     // The Link folder name is stored in the registry.
     DWORD buffer_length = sizeof(buffer);
-    base::win::RegKey reg_key(HKEY_CURRENT_USER,
-        L"Software\\Microsoft\\Internet Explorer\\Toolbar", KEY_READ);
+    base::win::RegKey reg_key(HKEY_CURRENT_USER, kIEToolbarKey, KEY_READ);
     if (reg_key.ReadValue(L"LinksFolderName", buffer,
                           &buffer_length, NULL) != ERROR_SUCCESS)
       return false;
@@ -525,7 +714,6 @@ bool IEImporter::GetFavoritesInfo(IEImporter::FavoritesInfo* info) {
 
 void IEImporter::ParseFavoritesFolder(const FavoritesInfo& info,
                                       BookmarkVector* bookmarks) {
-  BookmarkVector toolbar_bookmarks;
   FilePath file;
   std::vector<FilePath::StringType> file_list;
   FilePath favorites_path(info.path);
@@ -573,13 +761,12 @@ void IEImporter::ParseFavoritesFolder(const FavoritesInfo& info,
     if (!entry.path.empty() && entry.path[0] == info.links_folder) {
       // Bookmarks in the Link folder should be imported to the toolbar.
       entry.in_toolbar = true;
-      toolbar_bookmarks.push_back(entry);
-    } else {
-      bookmarks->push_back(entry);
     }
+    bookmarks->push_back(entry);
   }
-  bookmarks->insert(bookmarks->begin(), toolbar_bookmarks.begin(),
-                    toolbar_bookmarks.end());
+
+  // Reflect the menu order in IE.
+  SortBookmarksInIEOrder(this, bookmarks);
 }
 
 int IEImporter::CurrentIEVersion() const {
@@ -587,8 +774,7 @@ int IEImporter::CurrentIEVersion() const {
   if (version < 0) {
     wchar_t buffer[128];
     DWORD buffer_length = sizeof(buffer);
-    base::win::RegKey reg_key(HKEY_LOCAL_MACHINE,
-        L"Software\\Microsoft\\Internet Explorer", KEY_READ);
+    base::win::RegKey reg_key(HKEY_LOCAL_MACHINE, kIEVersionKey, KEY_READ);
     LONG result = reg_key.ReadValue(L"Version", buffer, &buffer_length, NULL);
     version = ((result == ERROR_SUCCESS)? _wtoi(buffer) : 0);
   }
diff --git a/chrome/browser/importer/importer_unittest.cc b/chrome/browser/importer/importer_unittest.cc
index eff6a6d..4c97adc 100644
--- a/chrome/browser/importer/importer_unittest.cc
+++ b/chrome/browser/importer/importer_unittest.cc
@@ -12,8 +12,10 @@
 #include <pstore.h>
 #include <shlguid.h>
 #include <urlhist.h>
+#include <shlobj.h>
 #endif
 
+#include <algorithm>
 #include <vector>
 
 #include "base/bind.h"
@@ -39,6 +41,7 @@
 #include "webkit/forms/password_form.h"
 
 #if defined(OS_WIN)
+#include "base/win/registry.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/windows_version.h"
 #include "chrome/browser/importer/ie_importer.h"
@@ -54,6 +57,202 @@
 
 using content::BrowserThread;
 
+namespace {
+
+const int kMaxPathSize = 5;
+
+struct BookmarkList {
+  const bool in_toolbar;
+  const size_t path_size;
+  const wchar_t* path[kMaxPathSize];
+  const wchar_t* title;
+  const char* url;
+};
+
+bool LexicographicCompareBookmark(const ProfileWriter::BookmarkEntry& lhs,
+                                  const ProfileWriter::BookmarkEntry& rhs) {
+  if (lhs.path == rhs.path)
+    return lhs.title < rhs.title;
+  return lhs.path < rhs.path;
+}
+
+// Returns true if the |entry| is equal to |expected|.
+bool EqualBookmarkEntry(const ProfileWriter::BookmarkEntry& entry,
+                        const BookmarkList& expected) {
+  if (expected.in_toolbar != entry.in_toolbar ||
+      expected.path_size != entry.path.size() ||
+      expected.url != entry.url.spec() ||
+      WideToUTF16Hack(expected.title) != entry.title)
+    return false;
+  for (size_t i = 0; i < expected.path_size; ++i) {
+    if (WideToUTF16Hack(expected.path[i]) != entry.path[i])
+      return false;
+  }
+  return true;
+}
+
+// Returns true if the |entry| is in the |list|.
+bool FindBookmarkEntry(const ProfileWriter::BookmarkEntry& entry,
+                       const BookmarkList* list, int list_size) {
+  for (int i = 0; i < list_size; ++i) {
+    if (EqualBookmarkEntry(entry, list[i]))
+      return true;
+  }
+  return false;
+}
+
+// TODO(kinaba): too many #ifdef's. Move tests for IE to another file.
+#if defined(OS_WIN)
+const char16 kUnitTestRegistrySubKey[] = L"SOFTWARE\\Chromium Unit Tests";
+const char16 kUnitTestUserOverrideSubKey[] =
+    L"SOFTWARE\\Chromium Unit Tests\\HKCU Override";
+
+static const BookmarkList kIEBookmarks[] = {
+  {true, 2, {L"Links", L"SubFolderOfLinks"},
+   L"SubLink",
+   "http://www.links-sublink.com/"},
+  {true, 1, {L"Links"},
+   L"TheLink",
+   "http://www.links-thelink.com/"},
+  {false, 0, {},
+   L"Google Home Page",
+   "http://www.google.com/"},
+  {false, 0, {},
+   L"TheLink",
+   "http://www.links-thelink.com/"},
+  {false, 1, {L"SubFolder"},
+   L"Title",
+   "http://www.link.com/"},
+  {false, 0, {},
+   L"WithPortAndQuery",
+   "http://host:8080/cgi?q=query"},
+  {false, 1, {L"a"},
+   L"\x4E2D\x6587",
+   "http://chinese-title-favorite/"},
+  {false, 0, {},
+   L"SubFolder",
+   "http://www.subfolder.com/"},
+};
+
+static const BookmarkList kIESortedBookmarks[] = {
+  {false, 0, {}, L"a", "http://www.google.com/0"},
+  {false, 1, {L"b"}, L"a", "http://www.google.com/1"},
+  {false, 1, {L"b"}, L"b", "http://www.google.com/2"},
+  {false, 0, {}, L"c", "http://www.google.com/3"},
+};
+
+static const wchar_t* kIEIdentifyUrl =
+    L"http://A79029D6-753E-4e27-B807-3D46AB1545DF.com:8080/path?key=value";
+static const wchar_t* kIEIdentifyTitle =
+    L"Unittest GUID";
+
+const wchar_t kIEFavoritesOrderKey[] =
+  L"Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\"
+  L"MenuOrder\\Favorites";
+
+bool CreateOrderBlob(const FilePath& favorites_folder,
+                     const string16& path,
+                     const std::vector<string16>& entries) {
+  if (entries.size() > 255)
+    return false;
+
+  // Create a binary sequence for setting a specific order of favorites.
+  // The format depends on the version of Shell32.dll, so we cannot embed
+  // a binary constant here.
+  std::vector<uint8> blob(20, 0);
+  blob[16] = static_cast<uint8>(entries.size());
+
+  for (size_t i = 0; i < entries.size(); ++i) {
+    ITEMIDLIST* id_list_full = ILCreateFromPath(
+        favorites_folder.Append(path).Append(entries[i]).value().c_str());
+    ITEMIDLIST* id_list = ILFindLastID(id_list_full);
+    size_t id_list_size = id_list->mkid.cb + sizeof(id_list->mkid);
+
+    blob.resize(blob.size() + 8);
+    uint32 total_size = id_list_size + 8;
+    memcpy(&blob[blob.size() - 8], &total_size, 4);
+    uint32 sort_index = i;
+    memcpy(&blob[blob.size() - 4], &sort_index, 4);
+    blob.resize(blob.size() + id_list_size);
+    memcpy(&blob[blob.size() - id_list_size], id_list, id_list_size);
+    ILFree(id_list_full);
+  }
+
+  string16 key_path = kIEFavoritesOrderKey;
+  if (!path.empty())
+    key_path += L"\\" + path;
+  base::win::RegKey key;
+  if (key.Create(HKEY_CURRENT_USER, key_path.c_str(), KEY_WRITE) !=
+      ERROR_SUCCESS) {
+    return false;
+  }
+  if (key.WriteValue(L"Order", &blob[0], blob.size(), REG_BINARY) !=
+      ERROR_SUCCESS) {
+    return false;
+  }
+  return true;
+}
+
+bool CreateUrlFile(const string16& file, const string16& url) {
+  base::win::ScopedComPtr<IUniformResourceLocator> locator;
+  HRESULT result = locator.CreateInstance(CLSID_InternetShortcut, NULL,
+                                          CLSCTX_INPROC_SERVER);
+  if (FAILED(result))
+    return false;
+  base::win::ScopedComPtr<IPersistFile> persist_file;
+  result = persist_file.QueryFrom(locator);
+  if (FAILED(result))
+    return false;
+  result = locator->SetURL(url.c_str(), 0);
+  if (FAILED(result))
+    return false;
+  result = persist_file->Save(file.c_str(), TRUE);
+  if (FAILED(result))
+    return false;
+  return true;
+}
+
+void ClearPStoreType(IPStore* pstore, const GUID* type, const GUID* subtype) {
+  base::win::ScopedComPtr<IEnumPStoreItems, NULL> item;
+  HRESULT result = pstore->EnumItems(0, type, subtype, 0, item.Receive());
+  if (result == PST_E_OK) {
+    wchar_t* item_name;
+    while (SUCCEEDED(item->Next(1, &item_name, 0))) {
+      pstore->DeleteItem(0, type, subtype, item_name, NULL, 0);
+      CoTaskMemFree(item_name);
+    }
+  }
+  pstore->DeleteSubtype(0, type, subtype, 0);
+  pstore->DeleteType(0, type, 0);
+}
+
+void WritePStore(IPStore* pstore, const GUID* type, const GUID* subtype) {
+  struct PStoreItem {
+    wchar_t* name;
+    int data_size;
+    char* data;
+  } items[] = {
+    {L"http://localhost:8080/security/index.htm#ref:StringData", 8,
+     "\x31\x00\x00\x00\x32\x00\x00\x00"},
+    {L"http://localhost:8080/security/index.htm#ref:StringIndex", 20,
+     "\x57\x49\x43\x4b\x18\x00\x00\x00\x02\x00"
+     "\x00\x00\x2f\x00\x74\x00\x01\x00\x00\x00"},
+    {L"user:StringData", 4,
+     "\x31\x00\x00\x00"},
+    {L"user:StringIndex", 20,
+     "\x57\x49\x43\x4b\x18\x00\x00\x00\x01\x00"
+     "\x00\x00\x2f\x00\x74\x00\x00\x00\x00\x00"},
+  };
+
+  for (int i = 0; i < arraysize(items); ++i) {
+    HRESULT res = pstore->WriteItem(0, type, subtype, items[i].name,
+        items[i].data_size, reinterpret_cast<BYTE*>(items[i].data),
+        NULL, 0, 0);
+    ASSERT_TRUE(res == PST_E_OK);
+  }
+}
+#endif  // defined(OS_WIN)
+
 class ImporterTest : public testing::Test {
  public:
   ImporterTest()
@@ -76,6 +275,34 @@ class ImporterTest : public testing::Test {
     profile_path_ = test_path.AppendASCII("profile");
     app_path_ = test_path.AppendASCII("app");
     file_util::CreateDirectory(app_path_);
+    StartRegistryOverride();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    EndRegistryOverride();
+  }
+
+  void StartRegistryOverride() {
+#if defined(OS_WIN)
+    EXPECT_EQ(ERROR_SUCCESS, RegOverridePredefKey(HKEY_CURRENT_USER, NULL));
+    temp_hkcu_hive_key_.Create(HKEY_CURRENT_USER,
+                               kUnitTestUserOverrideSubKey,
+                               KEY_ALL_ACCESS);
+    EXPECT_TRUE(temp_hkcu_hive_key_.Valid());
+    EXPECT_EQ(ERROR_SUCCESS,
+              RegOverridePredefKey(HKEY_CURRENT_USER,
+                                   temp_hkcu_hive_key_.Handle()));
+#endif
+  }
+
+  void EndRegistryOverride() {
+#if defined(OS_WIN)
+    EXPECT_EQ(ERROR_SUCCESS, RegOverridePredefKey(HKEY_CURRENT_USER, NULL));
+    temp_hkcu_hive_key_.Close();
+    base::win::RegKey key(HKEY_CURRENT_USER, kUnitTestRegistrySubKey,
+                          KEY_ALL_ACCESS);
+    key.DeleteKey(L"");
+#endif
   }
 
   void Firefox3xImporterTest(std::string profile_dir,
@@ -128,70 +355,12 @@ class ImporterTest : public testing::Test {
   FilePath profile_path_;
   FilePath app_path_;
   scoped_ptr<TestingProfile> profile_;
-};
-
-const int kMaxPathSize = 5;
-
-struct BookmarkList {
-  const bool in_toolbar;
-  const size_t path_size;
-  const wchar_t* path[kMaxPathSize];
-  const wchar_t* title;
-  const char* url;
-};
-
-// Returns true if the |entry| is in the |list|.
-bool FindBookmarkEntry(const ProfileWriter::BookmarkEntry& entry,
-                       const BookmarkList* list, int list_size) {
-  for (int i = 0; i < list_size; ++i) {
-    if (list[i].in_toolbar == entry.in_toolbar &&
-        list[i].path_size == entry.path.size() &&
-        list[i].url == entry.url.spec() &&
-        WideToUTF16Hack(list[i].title) == entry.title) {
-      bool equal = true;
-      for (size_t k = 0; k < list[i].path_size; ++k)
-        if (WideToUTF16Hack(list[i].path[k]) != entry.path[k]) {
-          equal = false;
-          break;
-        }
-
-      if (equal)
-        return true;
-    }
-  }
-  return false;
-}
-
 #if defined(OS_WIN)
-static const BookmarkList kIEBookmarks[] = {
-  {true, 1, {L"Links"},
-   L"TheLink",
-   "http://www.links-thelink.com/"},
-  {true, 2, {L"Links", L"SubFolderOfLinks"},
-   L"SubLink",
-   "http://www.links-sublink.com/"},
-  {false, 0, {},
-   L"Google Home Page",
-   "http://www.google.com/"},
-  {false, 0, {},
-   L"TheLink",
-   "http://www.links-thelink.com/"},
-  {false, 1, {L"SubFolder"},
-   L"Title",
-   "http://www.link.com/"},
-  {false, 0, {},
-   L"WithPortAndQuery",
-   "http://host:8080/cgi?q=query"},
-  {false, 1, {L"a"},
-   L"\x4E2D\x6587",
-   "http://chinese-title-favorite/"},
+  base::win::RegKey temp_hkcu_hive_key_;
+#endif
 };
 
-static const wchar_t* kIEIdentifyUrl =
-    L"http://A79029D6-753E-4e27-B807-3D46AB1545DF.com:8080/path?key=value";
-static const wchar_t* kIEIdentifyTitle =
-    L"Unittest GUID";
-
+#if defined(OS_WIN)
 class TestObserver : public ProfileWriter,
                      public importer::ImporterProgressObserver {
  public:
@@ -245,11 +414,13 @@ class TestObserver : public ProfileWriter,
 
   virtual void AddBookmarks(const std::vector<BookmarkEntry>& bookmarks,
                             const string16& top_level_folder_name) OVERRIDE {
-    // Importer should import the IE Favorites folder the same as the list.
+    ASSERT_LE(bookmark_count_ + bookmarks.size(), arraysize(kIEBookmarks));
+    // Importer should import the IE Favorites folder the same as the list,
+    // in the same order.
     for (size_t i = 0; i < bookmarks.size(); ++i) {
-      if (FindBookmarkEntry(bookmarks[i], kIEBookmarks,
-                            arraysize(kIEBookmarks)))
-        ++bookmark_count_;
+      EXPECT_TRUE(EqualBookmarkEntry(bookmarks[i],
+                                     kIEBookmarks[bookmark_count_]));
+      ++bookmark_count_;
     }
   }
 
@@ -269,65 +440,52 @@ class TestObserver : public ProfileWriter,
   size_t password_count_;
 };
 
-bool CreateUrlFile(const string16& file, const string16& url) {
-  base::win::ScopedComPtr<IUniformResourceLocator> locator;
-  HRESULT result = locator.CreateInstance(CLSID_InternetShortcut, NULL,
-                                          CLSCTX_INPROC_SERVER);
-  if (FAILED(result))
-    return false;
-  base::win::ScopedComPtr<IPersistFile> persist_file;
-  result = persist_file.QueryFrom(locator);
-  if (FAILED(result))
-    return false;
-  result = locator->SetURL(url.c_str(), 0);
-  if (FAILED(result))
-    return false;
-  result = persist_file->Save(file.c_str(), TRUE);
-  if (FAILED(result))
-    return false;
-  return true;
-}
+class MalformedFavoritesRegistryTestObserver
+    : public ProfileWriter,
+      public importer::ImporterProgressObserver {
+ public:
+  MalformedFavoritesRegistryTestObserver() : ProfileWriter(NULL) {
+    bookmark_count_ = 0;
+  }
 
-void ClearPStoreType(IPStore* pstore, const GUID* type, const GUID* subtype) {
-  base::win::ScopedComPtr<IEnumPStoreItems, NULL> item;
-  HRESULT result = pstore->EnumItems(0, type, subtype, 0, item.Receive());
-  if (result == PST_E_OK) {
-    wchar_t* item_name;
-    while (SUCCEEDED(item->Next(1, &item_name, 0))) {
-      pstore->DeleteItem(0, type, subtype, item_name, NULL, 0);
-      CoTaskMemFree(item_name);
-    }
+  // importer::ImporterProgressObserver:
+  virtual void ImportStarted() OVERRIDE {}
+  virtual void ImportItemStarted(importer::ImportItem item) OVERRIDE {}
+  virtual void ImportItemEnded(importer::ImportItem item) OVERRIDE {}
+  virtual void ImportEnded() OVERRIDE {
+    MessageLoop::current()->Quit();
+    EXPECT_EQ(arraysize(kIESortedBookmarks), bookmark_count_);
   }
-  pstore->DeleteSubtype(0, type, subtype, 0);
-  pstore->DeleteType(0, type, 0);
-}
 
-void WritePStore(IPStore* pstore, const GUID* type, const GUID* subtype) {
-  struct PStoreItem {
-    wchar_t* name;
-    int data_size;
-    char* data;
-  } items[] = {
-    {L"http://localhost:8080/security/index.htm#ref:StringData", 8,
-     "\x31\x00\x00\x00\x32\x00\x00\x00"},
-    {L"http://localhost:8080/security/index.htm#ref:StringIndex", 20,
-     "\x57\x49\x43\x4b\x18\x00\x00\x00\x02\x00"
-     "\x00\x00\x2f\x00\x74\x00\x01\x00\x00\x00"},
-    {L"user:StringData", 4,
-     "\x31\x00\x00\x00"},
-    {L"user:StringIndex", 20,
-     "\x57\x49\x43\x4b\x18\x00\x00\x00\x01\x00"
-     "\x00\x00\x2f\x00\x74\x00\x00\x00\x00\x00"},
-  };
+  virtual bool BookmarkModelIsLoaded() const { return true; }
+  virtual bool TemplateURLServiceIsLoaded() const { return true; }
 
-  for (int i = 0; i < arraysize(items); ++i) {
-    HRESULT res = pstore->WriteItem(0, type, subtype, items[i].name,
-        items[i].data_size, reinterpret_cast<BYTE*>(items[i].data),
-        NULL, 0, 0);
-    ASSERT_TRUE(res == PST_E_OK);
+  virtual void AddPasswordForm(const webkit::forms::PasswordForm& form) {}
+  virtual void AddHistoryPage(const std::vector<history::URLRow>& page,
+                              history::VisitSource visit_source) {}
+  virtual void AddKeyword(std::vector<TemplateURL*> template_url,
+                          int default_keyword_index) {}
+  virtual void AddBookmarks(const std::vector<BookmarkEntry>& bookmarks,
+                            const string16& top_level_folder_name) OVERRIDE {
+    ASSERT_LE(bookmark_count_ + bookmarks.size(),
+              arraysize(kIESortedBookmarks));
+    for (size_t i = 0; i < bookmarks.size(); ++i) {
+      EXPECT_TRUE(EqualBookmarkEntry(bookmarks[i],
+                                     kIESortedBookmarks[bookmark_count_]));
+      ++bookmark_count_;
+    }
   }
-}
 
+ private:
+  ~MalformedFavoritesRegistryTestObserver() {}
+
+  size_t bookmark_count_;
+};
+#endif  // defined(OS_WIN)
+
+}  // namespace
+
+#if defined(OS_WIN)
 TEST_F(ImporterTest, IEImporter) {
   // Sets up a favorites folder.
   base::win::ScopedCOMInitializer com_init;
@@ -341,6 +499,8 @@ TEST_F(ImporterTest, IEImporter) {
                             L"http://www.google.com/"));
   ASSERT_TRUE(CreateUrlFile(path + L"\\SubFolder\\Title.url",
                             L"http://www.link.com/"));
+  ASSERT_TRUE(CreateUrlFile(path + L"\\SubFolder.url",
+                            L"http://www.subfolder.com/"));
   ASSERT_TRUE(CreateUrlFile(path + L"\\TheLink.url",
                             L"http://www.links-thelink.com/"));
   ASSERT_TRUE(CreateUrlFile(path + L"\\WithPortAndQuery.url",
@@ -354,6 +514,19 @@ TEST_F(ImporterTest, IEImporter) {
   file_util::WriteFile(path + L"\\InvalidUrlFile.url", "x", 1);
   file_util::WriteFile(path + L"\\PlainTextFile.txt", "x", 1);
 
+  const wchar_t* root_links[] = {
+    L"Links",
+    L"Google Home Page.url",
+    L"TheLink.url",
+    L"SubFolder",
+    L"WithPortAndQuery.url",
+    L"a",
+    L"SubFolder.url",
+  };
+  ASSERT_TRUE(CreateOrderBlob(
+      FilePath(path), L"",
+      std::vector<string16>(root_links, root_links + arraysize(root_links))));
+
   HRESULT res;
 
   // Sets up a special history link.
@@ -374,6 +547,9 @@ TEST_F(ImporterTest, IEImporter) {
   source_profile.importer_type = importer::TYPE_IE;
   source_profile.source_path = temp_dir_.path();
 
+  // IUrlHistoryStg2::AddUrl seems to reset the override. Ensure it here.
+  StartRegistryOverride();
+
   loop->PostTask(FROM_HERE, base::Bind(
       &ImporterHost::StartImportSettings,
       host.get(),
@@ -389,6 +565,82 @@ TEST_F(ImporterTest, IEImporter) {
   url_history_stg2.Release();
 }
 
+TEST_F(ImporterTest, IEImporterMalformedFavoritesRegistry) {
+  // Sets up a favorites folder.
+  base::win::ScopedCOMInitializer com_init;
+  string16 path = temp_dir_.path().AppendASCII("Favorites").value();
+  CreateDirectory(path.c_str(), NULL);
+  CreateDirectory((path + L"\\b").c_str(), NULL);
+  ASSERT_TRUE(CreateUrlFile(path + L"\\a.url",
+                            L"http://www.google.com/0"));
+  ASSERT_TRUE(CreateUrlFile(path + L"\\b\\a.url",
+                            L"http://www.google.com/1"));
+  ASSERT_TRUE(CreateUrlFile(path + L"\\b\\b.url",
+                            L"http://www.google.com/2"));
+  ASSERT_TRUE(CreateUrlFile(path + L"\\c.url",
+                            L"http://www.google.com/3"));
+
+  struct BadBinaryData {
+    const char* data;
+    int length;
+  };
+  static const BadBinaryData kBadBinary[] = {
+    // number_of_items field is truncated
+    {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+     "\x00\xff\xff\xff", 17},
+    // number_of_items = 0xffff, but the byte sequence is too short.
+    {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+     "\xff\xff\x00\x00", 20},
+    // number_of_items = 1, size_of_item is too big.
+    {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+     "\x01\x00\x00\x00"
+     "\xff\xff\x00\x00\x00\x00\x00\x00"
+     "\x00\x00\x00\x00", 32},
+    // number_of_items = 1, size_of_item = 16, size_of_shid is too big.
+    {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+     "\x01\x00\x00\x00"
+     "\x10\x00\x00\x00\x00\x00\x00\x00"
+     "\xff\x7f\x00\x00" "\x00\x00\x00\x00", 36},
+    // number_of_items = 1, size_of_item = 16, size_of_shid is too big.
+    {"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+     "\x01\x00\x00\x00"
+     "\x10\x00\x00\x00\x00\x00\x00\x00"
+     "\x06\x00\x00\x00" "\x00\x00\x00\x00", 36},
+  };
+
+  // Verify malformed registry data are safely ignored and alphabetical
+  // sort is performed.
+  for (size_t i = 0; i < arraysize(kBadBinary); ++i) {
+    base::win::RegKey key;
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Create(HKEY_CURRENT_USER, kIEFavoritesOrderKey, KEY_WRITE));
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.WriteValue(L"Order", kBadBinary[i].data, kBadBinary[i].length,
+                             REG_BINARY));
+
+    // Starts to import the above settings.
+    MessageLoop* loop = MessageLoop::current();
+    scoped_refptr<ImporterHost> host(new ImporterHost);
+
+    MalformedFavoritesRegistryTestObserver* observer =
+        new MalformedFavoritesRegistryTestObserver();
+    host->SetObserver(observer);
+    importer::SourceProfile source_profile;
+    source_profile.importer_type = importer::TYPE_IE;
+    source_profile.source_path = temp_dir_.path();
+
+    loop->PostTask(FROM_HERE, base::Bind(
+        &ImporterHost::StartImportSettings,
+        host.get(),
+        source_profile,
+        profile_.get(),
+        importer::FAVORITES,
+        observer,
+        true));
+    loop->Run();
+  }
+}
+
 TEST_F(ImporterTest, IE7Importer) {
   // This is the unencrypted values of my keys under Storage2.
   // The passwords have been manually changed to abcdef... but the size remains
diff --git a/chrome/browser/importer/profile_writer.cc b/chrome/browser/importer/profile_writer.cc
index dabea62..9496359 100644
--- a/chrome/browser/importer/profile_writer.cc
+++ b/chrome/browser/importer/profile_writer.cc
@@ -71,6 +71,16 @@ ProfileWriter::BookmarkEntry::BookmarkEntry()
 
 ProfileWriter::BookmarkEntry::~BookmarkEntry() {}
 
+bool ProfileWriter::BookmarkEntry::operator==(
+    const ProfileWriter::BookmarkEntry& other) const {
+  return (in_toolbar == other.in_toolbar &&
+          is_folder == other.is_folder &&
+          url == other.url &&
+          path == other.path &&
+          title == other.title &&
+          creation_time == other.creation_time);
+}
+
 ProfileWriter::ProfileWriter(Profile* profile) : profile_(profile) {}
 
 bool ProfileWriter::BookmarkModelIsLoaded() const {
@@ -122,23 +132,33 @@ void ProfileWriter::AddBookmarks(const std::vector<BookmarkEntry>& bookmarks,
   const BookmarkNode* bookmark_bar = model->bookmark_bar_node();
   bool import_to_top_level = bookmark_bar->empty();
 
+  // Reorder bookmarks so that the toolbar entries come first.
+  std::vector<BookmarkEntry> toolbar_bookmarks;
+  std::vector<BookmarkEntry> reordered_bookmarks;
+  for (std::vector<BookmarkEntry>::const_iterator it = bookmarks.begin();
+       it != bookmarks.end(); ++it) {
+    if (it->in_toolbar)
+      toolbar_bookmarks.push_back(*it);
+    else
+      reordered_bookmarks.push_back(*it);
+  }
+  reordered_bookmarks.insert(reordered_bookmarks.begin(),
+                             toolbar_bookmarks.begin(),
+                             toolbar_bookmarks.end());
+
   // If the user currently has no bookmarks in the bookmark bar, make sure that
   // at least some of the imported bookmarks end up there.  Otherwise, we'll end
   // up with just a single folder containing the imported bookmarks, which makes
   // for unnecessary nesting.
-  bool add_all_to_top_level = import_to_top_level;
-  for (std::vector<BookmarkEntry>::const_iterator it = bookmarks.begin();
-       it != bookmarks.end() && add_all_to_top_level; ++it) {
-    if (it->in_toolbar)
-      add_all_to_top_level = false;
-  }
+  bool add_all_to_top_level = import_to_top_level && toolbar_bookmarks.empty();
 
   model->BeginImportMode();
 
   std::set<const BookmarkNode*> folders_added_to;
   const BookmarkNode* top_level_folder = NULL;
-  for (std::vector<BookmarkEntry>::const_iterator bookmark = bookmarks.begin();
-       bookmark != bookmarks.end(); ++bookmark) {
+  for (std::vector<BookmarkEntry>::const_iterator bookmark =
+         reordered_bookmarks.begin();
+       bookmark != reordered_bookmarks.end(); ++bookmark) {
     // Disregard any bookmarks with invalid urls.
     if (!bookmark->is_folder && !bookmark->url.is_valid())
       continue;
diff --git a/chrome/browser/importer/profile_writer.h b/chrome/browser/importer/profile_writer.h
index 871858e..a00ea97 100644
--- a/chrome/browser/importer/profile_writer.h
+++ b/chrome/browser/importer/profile_writer.h
@@ -36,6 +36,7 @@ class ProfileWriter : public base::RefCountedThreadSafe<ProfileWriter> {
   struct BookmarkEntry {
     BookmarkEntry();
     ~BookmarkEntry();
+    bool operator==(const BookmarkEntry& other) const;
 
     bool in_toolbar;
     bool is_folder;