[sync] Fix deadlock in SessionModelAssociator for Android.

chrome/browser/sync/glue/session_model_associator.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/glue/session_model_associator.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
@@ skipping 595 matching lines @@
   syncer::SyncError error;
 
   // Ensure that we disassociated properly, otherwise memory might leak.
   DCHECK(synced_session_tracker_.Empty());
   DCHECK_EQ(0U, tab_pool_.capacity());
 
   local_session_syncid_ = syncer::kInvalidId;
 
   scoped_ptr<DeviceInfo> local_device_info(sync_service_->GetLocalDeviceInfo());
 
+#if defined(OS_ANDROID)
+  std::string transaction_tag;
+#endif
   // Read any available foreign sessions and load any session data we may have.
   // If we don't have any local session data in the db, create a header node.
   {
     syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
 
     syncer::ReadNode root(&trans);
     if (root.InitByTagLookup(syncer::ModelTypeToRootTag(model_type())) !=
             syncer::BaseNode::INIT_OK) {
       return error_handler_->CreateAndUploadError(
           FROM_HERE,
@@ skipping 39 matching lines @@
       sync_pb::SessionSpecifics base_specifics;
       base_specifics.set_session_tag(current_machine_tag_);
       sync_pb::SessionHeader* header_s = base_specifics.mutable_header();
       header_s->set_client_name(current_session_name_);
       header_s->set_device_type(DeviceInfo::GetLocalDeviceType());
       write_node.SetSessionSpecifics(base_specifics);
 
       local_session_syncid_ = write_node.GetId();
     }
 #if defined(OS_ANDROID)
-    std::string transaction_tag = GetMachineTagFromTransaction(&trans);
-    if (current_machine_tag_.compare(transaction_tag) != 0)
-      DeleteForeignSession(transaction_tag);
+    transaction_tag = GetMachineTagFromTransaction(&trans);
 #endif
   }
+#if defined(OS_ANDROID)
+  if (current_machine_tag_.compare(transaction_tag) != 0)

[Yaron, 2013/02/13 21:01:47]

While I'm not excited about add a comment about implementation details, it does
seem worth calling out why this is done afterwards. Either that, or refactor the
code to re-use a transaction and/or a test.

Ok to do refactor/cleanup in a follow-up given the intent to merge to branch.

+    DeleteForeignSession(transaction_tag);
+#endif
 
   // Check if anything has changed on the client side.
   if (!UpdateSyncModelDataFromClient(&error)) {
     DCHECK(error.IsSet());
     return error;
   }
 
   DVLOG(1) << "Session models associated.";
   DCHECK(!error.IsSet());
   return error;
@@ skipping 555 matching lines @@
 
 bool SessionModelAssociator::CryptoReadyIfNecessary() {
   // We only access the cryptographer while holding a transaction.
   syncer::ReadTransaction trans(FROM_HERE, sync_service_->GetUserShare());
   const syncer::ModelTypeSet encrypted_types = trans.GetEncryptedTypes();
   return !encrypted_types.Has(SESSIONS) ||
          sync_service_->IsCryptographerReady(&trans);
 }
 
 }  // namespace browser_sync