Fix ProfileSyncService unit test.

Create TestHttpBridge for use by ProfileSyncServiceUnittest, so it
doesn't crash.

DoInitialize has been changed to take a HttpPostProviderFactory
before the core initialization.  The HttpBridge is now created
on the UI thread instead of the core thread.

Create fake tagged nodes in the order that they are requested.

Make the request order consistent and in visual order since this is the
most natural way to request them.

TEST=Ran the affected unit tests on Windows, tested basic sync functionality with two concurrent browsers.
BUG=25021

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

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

5 files changed, 64 insertions, 79 deletions

diff --git a/chrome/browser/sync/glue/http_bridge.h b/chrome/browser/sync/glue/http_bridge.h
index 6ddfcd3..ab491a0 100644
--- a/chrome/browser/sync/glue/http_bridge.h
+++ b/chrome/browser/sync/glue/http_bridge.h
@@ -68,7 +68,7 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
 
    private:
     std::string user_agent_;
-    scoped_refptr<URLRequestContext> baseline_context_;
+    URLRequestContext* baseline_context_;
 
     DISALLOW_COPY_AND_ASSIGN(RequestContext);
   };
diff --git a/chrome/browser/sync/glue/model_associator.cc b/chrome/browser/sync/glue/model_associator.cc
index 8fc1411..5256267 100644
--- a/chrome/browser/sync/glue/model_associator.cc
+++ b/chrome/browser/sync/glue/model_associator.cc
@@ -121,7 +121,7 @@ ModelAssociator::ModelAssociator(ProfileSyncService* sync_service)
 void ModelAssociator::ClearAll() {
   id_map_.clear();
   id_map_inverse_.clear();
-  dirty_assocations_sync_ids_.clear();
+  dirty_associations_sync_ids_.clear();
 }
 
 int64 ModelAssociator::GetSyncIdFromBookmarkId(int64 node_id) const {
@@ -165,7 +165,7 @@ void ModelAssociator::AssociateIds(int64 node_id, int64 sync_id) {
   DCHECK(id_map_inverse_.find(sync_id) == id_map_inverse_.end());
   id_map_[node_id] = sync_id;
   id_map_inverse_[sync_id] = node_id;
-  dirty_assocations_sync_ids_.insert(sync_id);
+  dirty_associations_sync_ids_.insert(sync_id);
   PostPersistAssociationsTask();
 }
 
@@ -175,7 +175,7 @@ void ModelAssociator::DisassociateIds(int64 sync_id) {
     return;
   id_map_.erase(iter->second);
   id_map_inverse_.erase(iter);
-  dirty_assocations_sync_ids_.erase(sync_id);
+  dirty_associations_sync_ids_.erase(sync_id);
 }
 
 bool ModelAssociator::BookmarkModelHasUserCreatedNodes() const {
@@ -269,12 +269,12 @@ bool ModelAssociator::AssociateModels() {
 
   ClearAll();
 
-  // We couldn't load model assocations from persisted assocations. So build
+  // We couldn't load model associations from persisted associations. So build
   // them.
-  return BuildAssocations();
+  return BuildAssociations();
 }
 
-bool ModelAssociator::BuildAssocations() {
+bool ModelAssociator::BuildAssociations() {
   // Algorithm description:
   // Match up the roots and recursively do the following:
   // * For each sync node for the current sync parent node, find the best
@@ -403,10 +403,10 @@ void ModelAssociator::PersistAssociations() {
   DCHECK(task_pending_);
   task_pending_ = false;
 
-  // If there are no dirty assocations we have nothing to do. We handle this
+  // If there are no dirty associations we have nothing to do. We handle this
   // explicity instead of letting the for loop do it to avoid creating a write
   // transaction in this case.
-  if (dirty_assocations_sync_ids_.empty()) {
+  if (dirty_associations_sync_ids_.empty()) {
     DCHECK(id_map_.empty());
     DCHECK(id_map_inverse_.empty());
     return;
@@ -414,9 +414,9 @@ void ModelAssociator::PersistAssociations() {
 
   sync_api::WriteTransaction trans(
       sync_service_->backend()->GetUserShareHandle());
-  DirtyAssocationsSyncIds::iterator iter;
-  for (iter = dirty_assocations_sync_ids_.begin();
-       iter != dirty_assocations_sync_ids_.end();
+  DirtyAssociationsSyncIds::iterator iter;
+  for (iter = dirty_associations_sync_ids_.begin();
+       iter != dirty_associations_sync_ids_.end();
        ++iter) {
     int64 sync_id = *iter;
     sync_api::WriteNode sync_node(&trans);
@@ -430,33 +430,34 @@ void ModelAssociator::PersistAssociations() {
     else
       NOTREACHED();
   }
-  dirty_assocations_sync_ids_.clear();
+  dirty_associations_sync_ids_.clear();
 }
 
 bool ModelAssociator::LoadAssociations() {
   BookmarkModel* model = sync_service_->profile()->GetBookmarkModel();
   DCHECK(model->IsLoaded());
-  // If the bookmarks changed externally, our previous assocations may not be
+  // If the bookmarks changed externally, our previous associations may not be
   // valid; so return false.
   if (model->file_changed())
     return false;
 
-  // Our persisted assocations should be valid. Try to populate id assocation
-  // maps using persisted assocations.
-
-  int64 other_bookmarks_id;
-  if (!GetSyncIdForTaggedNode(WideToUTF16(kOtherBookmarksTag),
-                              &other_bookmarks_id)) {
-    // We should always be able to find the permanent nodes.
-    sync_service_->OnUnrecoverableError();
-    return false;
-  }
+  // Our persisted associations should be valid. Try to populate id association
+  // maps using persisted associations.  Note that the unit tests will
+  // create the tagged nodes on demand, and the order in which we probe for
+  // them here will impact their positional ordering in that case.
   int64 bookmark_bar_id;
   if (!GetSyncIdForTaggedNode(WideToUTF16(kBookmarkBarTag), &bookmark_bar_id)) {
     // We should always be able to find the permanent nodes.
     sync_service_->OnUnrecoverableError();
     return false;
   }
+  int64 other_bookmarks_id;
+  if (!GetSyncIdForTaggedNode(WideToUTF16(kOtherBookmarksTag),
+    &other_bookmarks_id)) {
+      // We should always be able to find the permanent nodes.
+      sync_service_->OnUnrecoverableError();
+      return false;
+  }
 
   std::stack<int64> dfs_stack;
   dfs_stack.push(other_bookmarks_id);
diff --git a/chrome/browser/sync/glue/model_associator.h b/chrome/browser/sync/glue/model_associator.h
index d0db911..dc6aaba 100644
--- a/chrome/browser/sync/glue/model_associator.h
+++ b/chrome/browser/sync/glue/model_associator.h
@@ -4,8 +4,8 @@
 
 #if defined(BROWSER_SYNC)
 
-#ifndef CHROME_BROWSER_SYNC_GLUE_MODEL_ASSOCATOR_H_
-#define CHROME_BROWSER_SYNC_GLUE_MODEL_ASSOCATOR_H_
+#ifndef CHROME_BROWSER_SYNC_GLUE_MODEL_ASSOCIATOR_H_
+#define CHROME_BROWSER_SYNC_GLUE_MODEL_ASSOCIATOR_H_
 
 #include <map>
 #include <set>
@@ -30,17 +30,17 @@ namespace browser_sync {
 
 class ChangeProcessor;
 
-// Contains all model assocation related logic:
+// Contains all model association related logic:
 // * Algorithm to associate bookmark model and sync model.
 // * Methods to get a bookmark node for a given sync node and vice versa.
-// * Persisting model assocations and loading them back.
+// * Persisting model associations and loading them back.
 class ModelAssociator
     : public base::RefCountedThreadSafe<ModelAssociator> {
  public:
   explicit ModelAssociator(ProfileSyncService* sync_service);
   virtual ~ModelAssociator() { }
 
-  // Clears all assocations.
+  // Clears all associations.
   void ClearAll();
 
   // Returns sync id for the given bookmark node id.
@@ -96,21 +96,21 @@ class ModelAssociator
  private:
   typedef std::map<int64, int64> BookmarkIdToSyncIdMap;
   typedef std::map<int64, int64> SyncIdToBookmarkIdMap;
-  typedef std::set<int64> DirtyAssocationsSyncIds;
+  typedef std::set<int64> DirtyAssociationsSyncIds;
 
-  // Posts a task to persist dirty assocations.
+  // Posts a task to persist dirty associations.
   void PostPersistAssociationsTask();
-  // Persists all dirty assocations.
+  // Persists all dirty associations.
   void PersistAssociations();
 
-  // Loads the persisted assocations into in-memory maps.
+  // Loads the persisted associations into in-memory maps.
   // If the persisted associations are out-of-date due to some reason, returns
-  // false; otehrwise returns true.
+  // false; otherwise returns true.
   bool LoadAssociations();
 
   // Matches up the bookmark model and the sync model to build model
-  // assocations.
-  bool BuildAssocations();
+  // associations.
+  bool BuildAssociations();
 
   // Associate a top-level node of the bookmark model with a permanent node in
   // the sync domain.  Such permanent nodes are identified by a tag that is
@@ -128,7 +128,7 @@ class ModelAssociator
   BookmarkIdToSyncIdMap id_map_;
   SyncIdToBookmarkIdMap id_map_inverse_;
   // Stores sync ids for dirty associations.
-  DirtyAssocationsSyncIds dirty_assocations_sync_ids_;
+  DirtyAssociationsSyncIds dirty_associations_sync_ids_;
 
   // Indicates whether there is already a pending task to persist dirty model
   // associations.
@@ -139,5 +139,5 @@ class ModelAssociator
 
 }  // namespace browser_sync
 
-#endif  // CHROME_BROWSER_SYNC_GLUE_MODEL_ASSOCATOR_H_
+#endif  // CHROME_BROWSER_SYNC_GLUE_MODEL_ASSOCIATOR_H_
 #endif  // defined(BROWSER_SYNC)
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 0e24c80..8432381 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -45,10 +45,12 @@ void SyncBackendHost::Initialize(const GURL& sync_service_url,
   if (!core_thread_.Start())
     return;
   bookmark_model_worker_ = new BookmarkModelWorker(frontend_loop_);
-  core_.get()->SetBaseRequestContext(baseline_context);
+
   core_thread_.message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(core_.get(), &SyncBackendHost::Core::DoInitialize,
-                        sync_service_url, bookmark_model_worker_, true));
+                        sync_service_url, bookmark_model_worker_, true,
+                        new HttpBridgeFactory(baseline_context),
+                        new HttpBridgeFactory(baseline_context)));
 }
 
 void SyncBackendHost::Authenticate(const std::string& username,
@@ -127,28 +129,11 @@ AuthErrorState SyncBackendHost::GetAuthErrorState() const {
   return last_auth_error_;
 }
 
-void SyncBackendHost::Core::SetBaseRequestContext(
-    URLRequestContext* request_context) {
-  DCHECK(base_request_context_ == NULL);
-  base_request_context_ = request_context;
-  // This ref is removed on the IO thread after the core thread is over.
-  base_request_context_->AddRef();
-}
-
 SyncBackendHost::Core::Core(SyncBackendHost* backend)
     : host_(backend),
-      base_request_context_(NULL),
       syncapi_(new sync_api::SyncManager()) {
 }
 
-SyncBackendHost::Core::~Core() {
-  if (base_request_context_) {
-    ChromeThread::GetMessageLoop(ChromeThread::IO)->ReleaseSoon(FROM_HERE,
-        base_request_context_);
-    base_request_context_ = NULL;
-  }
-}
-
 // Helper to construct a user agent string (ASCII) suitable for use by
 // the syncapi for any HTTP communication. This string is used by the sync
 // backend for classifying client types when calculating statistics.
@@ -179,7 +164,9 @@ std::string MakeUserAgentForSyncapi() {
 void SyncBackendHost::Core::DoInitialize(
     const GURL& service_url,
     BookmarkModelWorker* bookmark_model_worker,
-    bool attempt_last_user_authentication) {
+    bool attempt_last_user_authentication,
+    sync_api::HttpPostProviderFactory* http_provider_factory,
+    sync_api::HttpPostProviderFactory* auth_http_provider_factory) {
   DCHECK(MessageLoop::current() == host_->core_thread_.message_loop());
 
   // Make sure that the directory exists before initializing the backend.
@@ -200,8 +187,8 @@ void SyncBackendHost::Core::DoInitialize(
       kGaiaServiceId,
       kGaiaSourceForChrome,
       service_url.SchemeIsSecure(),
-      new HttpBridgeFactory(base_request_context_),
-      new HttpBridgeFactory(base_request_context_),
+      http_provider_factory,
+      auth_http_provider_factory,
       bookmark_model_worker,
       attempt_last_user_authentication,
       MakeUserAgentForSyncapi().c_str());
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index b5473f0..3fad2c7 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -117,15 +117,20 @@ class SyncBackendHost {
 #if defined(UNIT_TEST)
   // Called from unit test to bypass authentication and initialize the syncapi
   // to a state suitable for testing but not production.
-  void InitializeForTestMode(const std::wstring& test_user) {
+  void InitializeForTestMode(const std::wstring& test_user,
+                             sync_api::HttpPostProviderFactory* factory,
+                             sync_api::HttpPostProviderFactory* auth_factory) {
     if (!core_thread_.Start())
       return;
     bookmark_model_worker_ = new BookmarkModelWorker(frontend_loop_);
+
     core_thread_.message_loop()->PostTask(FROM_HERE,
         NewRunnableMethod(core_.get(),
         &SyncBackendHost::Core::DoInitializeForTest,
         bookmark_model_worker_,
-        test_user));
+        test_user,
+        factory,
+        auth_factory));
   }
 #endif
 
@@ -136,12 +141,6 @@ class SyncBackendHost {
    public:
     explicit Core(SyncBackendHost* backend);
 
-    // Note: This destructor should *always* be called from the thread that
-    // created it, and *always* after core_thread_ has exited. The syncapi
-    // watches thread exit events and keeps pointers to objects this dtor will
-    // destroy, so this ordering is important.
-    ~Core();
-
     // SyncManager::Observer implementation.  The Core just acts like an air
     // traffic controller here, forwarding incoming messages to appropriate
     // landing threads.
@@ -163,8 +162,10 @@ class SyncBackendHost {
     // Called on the SyncBackendHost core_thread_ to perform initialization
     // of the syncapi on behalf of SyncBackendHost::Initialize.
     void DoInitialize(const GURL& service_url,
-                      BookmarkModelWorker* bookmark_model_worker,
-                      bool attempt_last_user_authentication);
+        BookmarkModelWorker* bookmark_model_worker,
+        bool attempt_last_user_authentication,
+        sync_api::HttpPostProviderFactory* http_bridge_factory,
+        sync_api::HttpPostProviderFactory* auth_http_bridge_factory);
 
     // Called on our SyncBackendHost's core_thread_ to perform authentication
     // on behalf of SyncBackendHost::Authenticate.
@@ -185,9 +186,6 @@ class SyncBackendHost {
     // Set the base request context to use when making HTTP calls.
     // This method will add a reference to the context to persist it
     // on the IO thread. Must be removed from IO thread.
-    // TODO(chron): After HttpNetworkSession reorganization happens, try
-    //              getting HttpBridgeFactory to initialize earlier.
-    void SetBaseRequestContext(URLRequestContext* request_context);
 
     sync_api::SyncManager* syncapi() { return syncapi_.get(); }
 
@@ -196,8 +194,11 @@ class SyncBackendHost {
     // last known user (since it will fail in test mode) and does some extra
     // setup to nudge the syncapi into a useable state.
     void DoInitializeForTest(BookmarkModelWorker* bookmark_model_worker,
-                             const std::wstring& test_user) {
-        DoInitialize(GURL(), bookmark_model_worker, false);
+                             const std::wstring& test_user,
+                             sync_api::HttpPostProviderFactory* factory,
+                             sync_api::HttpPostProviderFactory* auth_factory) {
+        DoInitialize(GURL(), bookmark_model_worker, false, factory,
+                     auth_factory);
         syncapi_->SetupForTestMode(WideToUTF16(test_user).c_str());
     }
 #endif
@@ -241,10 +242,6 @@ class SyncBackendHost {
     // Our parent SyncBackendHost
     SyncBackendHost* host_;
 
-    // Our request context that we have to keep a ref to.
-    // Contains session data.
-    URLRequestContext* base_request_context_;
-
     // The timer used to periodically call SaveChanges.
     base::RepeatingTimer<Core> save_changes_timer_;