Fix deadlock by introducing a new transaction event.

This is still going through the trybots now.

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

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

2 files changed, 19 insertions, 2 deletions

diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index 7c6cc71..5516c59 100644
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -950,6 +950,8 @@ BaseTransaction::BaseTransaction(Directory* directory, const char* name,
   Lock();
 }
 
+BaseTransaction::~BaseTransaction() {}
+
 void BaseTransaction::UnlockAndLog(OriginalEntries* originals_arg) {
   dirkernel_->transaction_mutex.AssertAcquired();
 
@@ -972,8 +974,17 @@ void BaseTransaction::UnlockAndLog(OriginalEntries* originals_arg) {
                                  originals.get(), this, writer_ };
   dirkernel_->changes_channel.Notify(event);
 
+  // Necessary for reads to be performed prior to transaction mutex release.
+  // Allows the listener to use the current transaction to perform reads.
+  DirectoryChangeEvent ending_event =
+      { DirectoryChangeEvent::TRANSACTION_ENDING,
+        NULL, this, INVALID };
+  dirkernel_->changes_channel.Notify(ending_event);
+
   dirkernel_->transaction_mutex.Release();
 
+  // Directly after transaction mutex release, but lock on changes channel.
+  // You cannot be re-entrant to a transaction in this handler.
   DirectoryChangeEvent complete_event =
       { DirectoryChangeEvent::TRANSACTION_COMPLETE,
         NULL, NULL, INVALID };
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index 3793a2c..291e798 100644
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -586,6 +586,10 @@ struct DirectoryChangeEvent {
     // callbacks or attempt to lock anything because a
     // WriteTransaction is being held until the listener returns.
     CALCULATE_CHANGES,
+    // Means the WriteTransaction is ending, and this is the absolute
+    // last chance to perform any read operations in the current transaction.
+    // It is not recommended that the listener perform any writes.
+    TRANSACTION_ENDING,
     // Means the WriteTransaction has been released and the listener
     // can now take action on the changes it calculated.
     TRANSACTION_COMPLETE,
@@ -594,7 +598,7 @@ struct DirectoryChangeEvent {
   } todo;
   // These members are only valid for CALCULATE_CHANGES.
   const OriginalEntries* originals;
-  BaseTransaction* trans;
+  BaseTransaction* trans;  // This is valid also for TRANSACTION_ENDING
   WriterTag writer;
   typedef DirectoryChangeEvent EventType;
   static inline bool IsChannelShutdownEvent(const EventType& e) {
@@ -1044,6 +1048,8 @@ class BaseTransaction {
   inline Directory* directory() const { return directory_; }
   inline Id root_id() const { return Id(); }
 
+  virtual ~BaseTransaction();
+
  protected:
   BaseTransaction(Directory* directory, const char* name,
                   const char* source_file, int line, WriterTag writer);
@@ -1072,7 +1078,7 @@ class ReadTransaction : public BaseTransaction {
   ReadTransaction(const ScopedDirLookup& scoped_dir,
                   const char* source_file, int line);
 
-  ~ReadTransaction();
+  virtual ~ReadTransaction();
 
  protected:  // Don't allow creation on heap, except by sync API wrapper.
   friend class sync_api::ReadTransaction;