Fix deadlock by introducing a new transaction event.

chrome/browser/sync/engine/syncapi.cc

 // Copyright (c) 2010 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/engine/syncapi.h"
 
 #include "build/build_config.h"
 
 #include <iomanip>
 #include <list>
@@ skipping 752 matching lines @@
   LOG_IF(WARNING, model_type == syncable::UNSPECIFIED ||
                   model_type == syncable::TOP_LEVEL_FOLDER)
       << "SyncAPI InitByTagLookup referencing unusually typed object.";
   return DecryptIfNecessary(entry_);
 }
 
 //////////////////////////////////////////////////////////////////////////
 // ReadTransaction member definitions
 ReadTransaction::ReadTransaction(UserShare* share)
     : BaseTransaction(share),
-      transaction_(NULL) {
+      transaction_(NULL),
+      close_transaction_(true) {
   transaction_ = new syncable::ReadTransaction(GetLookup(), __FILE__, __LINE__);
 }
 
+ReadTransaction::ReadTransaction(UserShare* share,
+                                 syncable::BaseTransaction* trans)
+    : BaseTransaction(share),
+      transaction_(trans),
+      close_transaction_(false) {}
+
 ReadTransaction::~ReadTransaction() {
-  delete transaction_;
+  if (close_transaction_) {
+    delete transaction_;
+  }
 }
 
 syncable::BaseTransaction* ReadTransaction::GetWrappedTrans() const {
   return transaction_;
 }
 
 //////////////////////////////////////////////////////////////////////////
 // WriteTransaction member definitions
 WriteTransaction::WriteTransaction(UserShare* share)
     : BaseTransaction(share),
@@ skipping 127 matching lines @@
   void SetPassphrase(const std::string& passphrase);
 
   // Call periodically from a database-safe thread to persist recent changes
   // to the syncapi model.
   void SaveChanges();
 
   // This listener is called upon completion of a syncable transaction, and
   // builds the list of sync-engine initiated changes that will be forwarded to
   // the SyncManager's Observers.
   virtual void HandleChannelEvent(const syncable::DirectoryChangeEvent& event);
-  void HandleTransactionCompleteChangeEvent(
+  void HandleTransactionEndingChangeEvent(
       const syncable::DirectoryChangeEvent& event);
   void HandleCalculateChangesChangeEventFromSyncApi(
       const syncable::DirectoryChangeEvent& event);
   void HandleCalculateChangesChangeEventFromSyncer(
       const syncable::DirectoryChangeEvent& event);
 
   // This listener is called by the syncer channel for all syncer events.
   virtual void HandleChannelEvent(const SyncerEvent& event);
 
   // We have a direct hookup to the authwatcher to be notified for auth failures
@@ skipping 199 matching lines @@
   // AuthWatcher kicks off the authentication process and follows it through
   // phase 1 (GAIA) to phase 2 (sync engine). As part of this work it determines
   // the initial connectivity and causes the server connection event to be
   // broadcast, which signals the syncer thread to start syncing.
   // It has a heavy duty constructor requiring boilerplate so we heap allocate.
   scoped_refptr<AuthWatcher> auth_watcher_;
 
   // Each element of this array is a store of change records produced by
   // HandleChangeEvent during the CALCULATE_CHANGES step.  The changes are
   // segregated by model type, and are stored here to be processed and
-  // forwarded to the observer slightly later, at the TRANSACTION_COMPLETE
-  // step by HandleTransactionCompleteChangeEvent.
+  // forwarded to the observer slightly later, at the TRANSACTION_ENDING
+  // step by HandleTransactionEndingChangeEvent.
   ChangeReorderBuffer change_buffers_[syncable::MODEL_TYPE_COUNT];
 
   // The event listener hookup that is registered for HandleChangeEvent.
   scoped_ptr<browser_sync::ChannelHookup<syncable::DirectoryChangeEvent> >
       dir_change_hookup_;
 
   // The event listener hookup registered for HandleSyncerEvent.
   scoped_ptr<browser_sync::ChannelHookup<SyncerEvent> > syncer_event_;
 
   // The event listener hookup registered for HandleAuthWatcherEvent.
@@ skipping 470 matching lines @@
      DCHECK(!initialized()) << "Should only happen once";
      MarkAndNotifyInitializationComplete();
   }
 }
 
 // Listen to model changes, filter out ones initiated by the sync API, and
 // saves the rest (hopefully just backend Syncer changes resulting from
 // ApplyUpdates) to data_->changelist.
 void SyncManager::SyncInternal::HandleChannelEvent(
     const syncable::DirectoryChangeEvent& event) {
-  if (event.todo == syncable::DirectoryChangeEvent::TRANSACTION_COMPLETE) {
-    HandleTransactionCompleteChangeEvent(event);
+  if (event.todo == syncable::DirectoryChangeEvent::TRANSACTION_ENDING) {

[ncarter (slow), 2010/07/21 03:02:14]

Do we even need the new TRANSACTION_ENDING phase, or could we simply call this
during the CALCULATE_CHANGES step, after the existing work we do?

I'm fine either way, just curious if there's something subtle at work (in e.g.
another observer).

+    HandleTransactionEndingChangeEvent(event);
     return;
   } else if (event.todo == syncable::DirectoryChangeEvent::CALCULATE_CHANGES) {
     if (event.writer == syncable::SYNCAPI) {
       HandleCalculateChangesChangeEventFromSyncApi(event);
       return;
     }
     HandleCalculateChangesChangeEventFromSyncer(event);
     return;
   } else if (event.todo == syncable::DirectoryChangeEvent::SHUTDOWN) {
     dir_change_hookup_.reset();
   }
 }
 
-void SyncManager::SyncInternal::HandleTransactionCompleteChangeEvent(
+void SyncManager::SyncInternal::HandleTransactionEndingChangeEvent(
     const syncable::DirectoryChangeEvent& event) {
-  // This notification happens immediately after a syncable WriteTransaction
-  // falls out of scope.
-  DCHECK_EQ(event.todo, syncable::DirectoryChangeEvent::TRANSACTION_COMPLETE);
+  // This notification happens immediately before a syncable WriteTransaction
+  // falls out of scope. It happens while the channel mutex is still held,
+  // and while the transaction mutex is held, so it cannot be re-entrant.
+  DCHECK_EQ(event.todo, syncable::DirectoryChangeEvent::TRANSACTION_ENDING);
   if (!observer_ || ChangeBuffersAreEmpty())
     return;
 
-  ReadTransaction trans(GetUserShare());
+  // This will continue the WriteTransaction using a read only wrapper.
+  // This is the last chance for read to occur in the WriteTransaction 
+  // that's closing. This special ReadTransaction will not close the
+  // underlying transaction.
+  ReadTransaction trans(GetUserShare(), event.trans);
+
   for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) {
     if (change_buffers_[i].IsEmpty())
       continue;
 
     vector<ChangeRecord> ordered_changes;
     change_buffers_[i].GetAllChangesInTreeOrder(&trans, &ordered_changes);
     if (!ordered_changes.empty()) {
       observer_->OnChangesApplied(syncable::ModelTypeFromInt(i), &trans,
                                   &ordered_changes[0], ordered_changes.size());
     }
@@ skipping 446 matching lines @@
 BaseTransaction::~BaseTransaction() {
   delete lookup_;
 }
 
 UserShare* SyncManager::GetUserShare() const {
   DCHECK(data_->initialized()) << "GetUserShare requires initialization!";
   return data_->GetUserShare();
 }
 
 }  // namespace sync_api