Replace Read/Write lock with a plain old chrome lock.

BUG=19895

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

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

3 files changed, 68 insertions, 261 deletions

diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc
index 8897480..8bd4819 100644
--- a/chrome/browser/sync/engine/syncer_util.cc
+++ b/chrome/browser/sync/engine/syncer_util.cc
@@ -370,7 +370,7 @@ void SyncerUtil::UpdateServerFieldsFromUpdate(
 void SyncerUtil::ApplyExtendedAttributes(
     syncable::MutableEntry* local_entry,
     const SyncEntity& server_entry) {
-  local_entry->DeleteAllExtendedAttributes(local_entry->trans());
+  local_entry->DeleteAllExtendedAttributes(local_entry->write_transaction());
   if (server_entry.has_extended_attributes()) {
     const sync_pb::ExtendedAttributes & extended_attributes =
       server_entry.extended_attributes();
@@ -379,7 +379,7 @@ void SyncerUtil::ApplyExtendedAttributes(
       AppendUTF8ToPathString(
           extended_attributes.extendedattribute(i).key(), &pathstring_key);
       ExtendedAttributeKey key(local_entry->Get(META_HANDLE), pathstring_key);
-      MutableExtendedAttribute local_attribute(local_entry->trans(),
+      MutableExtendedAttribute local_attribute(local_entry->write_transaction(),
           CREATE, key);
       SyncerProtoUtil::CopyProtoBytesIntoBlob(
           extended_attributes.extendedattribute(i).value(),
diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index 87a3037..e8373a88 100644
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -181,13 +181,6 @@ Name Name::FromEntryKernel(EntryKernel* kernel) {
 static const DirectoryChangeEvent kShutdownChangesEvent =
     { DirectoryChangeEvent::SHUTDOWN, 0, 0 };
 
-void DestroyThreadNodeKey(void* vnode) {
-  ThreadNode* const node = reinterpret_cast<ThreadNode*>(vnode);
-  CHECK(!node->in_list)
-    << "\nThread exited while holding the transaction mutex!\n" << *node;
-  delete node;
-}
-
 Directory::Kernel::Kernel(const PathString& db_path,
                           const PathString& name,
                           const KernelLoadInfo& info)
@@ -210,7 +203,6 @@ Directory::Kernel::Kernel(const PathString& db_path,
   next_id(info.kernel_info.next_id) {
   info_status_ = Directory::KERNEL_SHARE_INFO_VALID;
   CHECK(0 == pthread_mutex_init(&mutex, NULL));
-  CHECK(0 == pthread_key_create(&thread_node_key, &DestroyThreadNodeKey));
 }
 
 inline void DeleteEntry(EntryKernel* kernel) {
@@ -231,7 +223,6 @@ Directory::Kernel::~Kernel() {
   delete channel;
   delete changes_channel;
   CHECK(0 == pthread_mutex_destroy(&mutex));
-  pthread_key_delete(thread_node_key);
   delete unsynced_metahandles;
   delete unapplied_update_metahandles;
   delete extended_attributes;
@@ -1139,68 +1130,13 @@ static const bool kLoggingInfo = true;
 static const bool kLoggingInfo = false;
 #endif
 
-ThreadNode* BaseTransaction::MakeThreadNode() {
-  ThreadNode* node = reinterpret_cast<ThreadNode*>
-    (pthread_getspecific(dirkernel_->thread_node_key));
-  if (NULL == node) {
-    node = new ThreadNode;
-    node->id = GetCurrentThreadId();
-    pthread_setspecific(dirkernel_->thread_node_key, node);
-  } else if (node->in_list) {
-    logging::LogMessage(source_file_, line_, logging::LOG_FATAL).stream()
-      << " Recursive Lock attempt by thread id " << node->id << "." << std::endl
-      << "Already entered transaction at " << node->file << ":" << node->line;
-  }
-  node->file = source_file_;
-  node->line = line_;
-  node->wait_started = base::TimeTicks::Now();
-  return node;
-}
-
-void BaseTransaction::Lock(ThreadCounts* const thread_counts,
-                           ThreadNode* node, TransactionClass tclass) {
-  ScopedTransactionLock lock(&dirkernel_->transaction_mutex);
-  // Increment the waiters count.
-  node->tclass = tclass;
-  thread_counts->waiting += 1;
-  node->Insert(&thread_counts->waiting_headtail);
-
-  // Block until we can own the reader/writer lock
-  bool ready = 1 == thread_counts->waiting;
-  while (true) {
-    if (ready) {
-      if (0 == thread_counts->active) {
-        // We can take the lock because there is no contention.
-        break;
-      } else if (READ == tclass
-                 && READ == thread_counts->active_headtail.next->tclass) {
-        // We can take the lock because reads can run simultaneously.
-        break;
-      }
-    }
-    // Wait to be woken up and check again.
-    node->wake_up = false;
-    do {
-      CHECK(0 == pthread_cond_wait(&node->condvar.condvar_,
-                                   &dirkernel_->transaction_mutex.mutex_));
-    } while (!node->wake_up);
-    ready = true;
-  }
+void BaseTransaction::Lock() {
+  base::TimeTicks start_time = base::TimeTicks::Now();
 
-  // Move from the list of waiters to the list of active.
-  thread_counts->waiting -= 1;
-  thread_counts->active += 1;
-  CHECK(WRITE != tclass || 1 == thread_counts->active);
-  node->Remove();
-  node->Insert(&thread_counts->active_headtail);
-  if (WRITE == tclass)
-    node->current_write_trans = static_cast<WriteTransaction*>(this);
-}
+  dirkernel_->transaction_mutex.Acquire();
 
-void BaseTransaction::AfterLock(ThreadNode* node) {
   time_acquired_ = base::TimeTicks::Now();
-
-  const base::TimeDelta elapsed = time_acquired_ - node->wait_started;
+  const base::TimeDelta elapsed = time_acquired_ - start_time;
   if (kLoggingInfo && elapsed.InMilliseconds() > 200) {
     logging::LogMessage(source_file_, line_, logging::LOG_INFO).stream()
       << name_ << " transaction waited "
@@ -1208,21 +1144,16 @@ void BaseTransaction::AfterLock(ThreadNode* node) {
   }
 }
 
-void BaseTransaction::Init(ThreadCounts* const thread_counts,
-                           TransactionClass tclass) {
-  ThreadNode* const node = MakeThreadNode();
-  Lock(thread_counts, node, tclass);
-  AfterLock(node);
-}
-
 BaseTransaction::BaseTransaction(Directory* directory, const char* name,
-                 const char* source_file, int line)
+                 const char* source_file, int line, WriterTag writer)
   : directory_(directory), dirkernel_(directory->kernel_), name_(name),
-    source_file_(source_file), line_(line) {
+    source_file_(source_file), line_(line), writer_(writer) {
+  Lock();
 }
 
-void BaseTransaction::UnlockAndLog(ThreadCounts* const thread_counts,
-                                   OriginalEntries* originals_arg) {
+void BaseTransaction::UnlockAndLog(OriginalEntries* originals_arg) {
+  dirkernel_->transaction_mutex.AssertAcquired();
+
   scoped_ptr<OriginalEntries> originals(originals_arg);
   const base::TimeDelta elapsed = base::TimeTicks::Now() - time_acquired_;
   if (kLoggingInfo && elapsed.InMilliseconds() > 50) {
@@ -1231,91 +1162,50 @@ void BaseTransaction::UnlockAndLog(ThreadCounts* const thread_counts,
         << " seconds.";
   }
 
-  {
-    ScopedTransactionLock lock(&dirkernel_->transaction_mutex);
-    // Let go of the reader/writer lock
-    thread_counts->active -= 1;
-    ThreadNode* const node = reinterpret_cast<ThreadNode*>
-      (pthread_getspecific(dirkernel_->thread_node_key));
-    CHECK(node != NULL);
-    node->Remove();
-    node->current_write_trans = NULL;
-    if (0 == thread_counts->active) {
-      // Wake up a waiting thread, FIFO
-      if (dirkernel_->thread_counts.waiting > 0) {
-        ThreadNode* const headtail =
-          &dirkernel_->thread_counts.waiting_headtail;
-        ThreadNode* node = headtail->next;
-        node->wake_up = true;
-        CHECK(0 == pthread_cond_signal(&node->condvar.condvar_));
-        if (READ == node->tclass) do {
-          // Wake up all consecutive readers.
-          node = node->next;
-          if (node == headtail)
-            break;
-          if (READ != node->tclass)
-            break;
-          node->wake_up = true;
-          CHECK(0 == pthread_cond_signal(&node->condvar.condvar_));
-        } while (true);
-      }
-    }
-    if (NULL == originals.get() || originals->empty())
-      return;
-    dirkernel_->changes_channel_mutex.Lock();
-    // Tell listeners to calculate changes while we still have the mutex.
-    DirectoryChangeEvent event = { DirectoryChangeEvent::CALCULATE_CHANGES,
-                                   originals.get(), this, writer_ };
-    dirkernel_->changes_channel->NotifyListeners(event);
-  }
-  DirectoryChangeEvent event = { DirectoryChangeEvent::TRANSACTION_COMPLETE,
-                                 NULL, NULL, INVALID };
+  if (NULL == originals.get() || originals->empty()) {
+    dirkernel_->transaction_mutex.Release();
+    return;
+  }
+
+  dirkernel_->changes_channel_mutex.Lock();
+  // Tell listeners to calculate changes while we still have the mutex.
+  DirectoryChangeEvent event = { DirectoryChangeEvent::CALCULATE_CHANGES,
+                                 originals.get(), this, writer_ };
   dirkernel_->changes_channel->NotifyListeners(event);
+
+  dirkernel_->transaction_mutex.Release();
+
+  DirectoryChangeEvent complete_event =
+      { DirectoryChangeEvent::TRANSACTION_COMPLETE,
+        NULL, NULL, INVALID };
+  dirkernel_->changes_channel->NotifyListeners(complete_event);
   dirkernel_->changes_channel_mutex.Unlock();
 }
 
 ReadTransaction::ReadTransaction(Directory* directory, const char* file,
                                  int line)
-  : BaseTransaction(directory, "Read", file, line) {
-  Init(&dirkernel_->thread_counts, READ);
-  writer_ = INVALID;
+  : BaseTransaction(directory, "Read", file, line, INVALID) {
 }
 
 ReadTransaction::ReadTransaction(const ScopedDirLookup& scoped_dir,
                                  const char* file, int line)
-  : BaseTransaction(scoped_dir.operator -> (), "Read", file, line) {
-  Init(&dirkernel_->thread_counts, READ);
-  writer_ = INVALID;
+  : BaseTransaction(scoped_dir.operator -> (), "Read", file, line, INVALID) {
 }
 
 ReadTransaction::~ReadTransaction() {
-  UnlockAndLog(&dirkernel_->thread_counts, NULL);
+  UnlockAndLog(NULL);
 }
 
 WriteTransaction::WriteTransaction(Directory* directory, WriterTag writer,
                                    const char* file, int line)
-  : BaseTransaction(directory, "Write", file, line), skip_destructor_(false),
+  : BaseTransaction(directory, "Write", file, line, writer),
     originals_(new OriginalEntries) {
-  Init(&dirkernel_->thread_counts, WRITE);
-  writer_ = writer;
 }
 
 WriteTransaction::WriteTransaction(const ScopedDirLookup& scoped_dir,
                                    WriterTag writer, const char* file, int line)
-  : BaseTransaction(scoped_dir.operator -> (), "Write", file, line),
-    skip_destructor_(false), originals_(new OriginalEntries) {
-  Init(&dirkernel_->thread_counts, WRITE);
-  writer_ = writer;
-}
-
-WriteTransaction::WriteTransaction(Directory* directory, const char* name,
-                                   WriterTag writer,
-                                   const char* file, int line,
-                                   bool skip_destructor,
-                                   OriginalEntries* originals)
-  : BaseTransaction(directory, name, file, line),
-    skip_destructor_(skip_destructor), originals_(originals) {
-  writer_ = writer;
+  : BaseTransaction(scoped_dir.operator -> (), "Write", file, line, writer),
+    originals_(new OriginalEntries) {
 }
 
 void WriteTransaction::SaveOriginal(EntryKernel* entry) {
@@ -1329,8 +1219,6 @@ void WriteTransaction::SaveOriginal(EntryKernel* entry) {
 }
 
 WriteTransaction::~WriteTransaction() {
-  if (skip_destructor_)
-    return;
   if (OFF != kInvariantCheckLevel) {
     const bool full_scan = (FULL_DB_VERIFICATION == kInvariantCheckLevel);
     if (full_scan)
@@ -1338,7 +1226,7 @@ WriteTransaction::~WriteTransaction() {
     else
       directory()->CheckTreeInvariants(this, originals_);
   }
-  UnlockAndLog(&dirkernel_->thread_counts, originals_);
+  UnlockAndLog(originals_);
 }
 
 ///////////////////////////////////////////////////////////////////////////
@@ -1405,7 +1293,7 @@ void Entry::DeleteAllExtendedAttributes(WriteTransaction *trans) {
 
 MutableEntry::MutableEntry(WriteTransaction* trans, Create,
                            const Id& parent_id, const PathString& name)
-  : Entry(trans) {
+  : Entry(trans), write_transaction_(trans) {
   if (NULL != trans->directory()->GetChildWithName(parent_id, name)) {
     kernel_ = NULL;  // would have duplicated an existing entry.
     return;
@@ -1445,7 +1333,7 @@ void MutableEntry::Init(WriteTransaction* trans, const Id& parent_id,
 
 MutableEntry::MutableEntry(WriteTransaction* trans, CreateNewUpdateItem,
                            const Id& id)
-  : Entry(trans) {
+  : Entry(trans), write_transaction_(trans) {
   Entry same_id(trans, GET_BY_ID, id);
   if (same_id.good()) {
     kernel_ = NULL;  // already have an item with this ID.
@@ -1467,31 +1355,33 @@ MutableEntry::MutableEntry(WriteTransaction* trans, CreateNewUpdateItem,
 }
 
 MutableEntry::MutableEntry(WriteTransaction* trans, GetById, const Id& id)
-  : Entry(trans, GET_BY_ID, id) {
+  : Entry(trans, GET_BY_ID, id), write_transaction_(trans) {
   trans->SaveOriginal(kernel_);
 }
 
 MutableEntry::MutableEntry(WriteTransaction* trans, GetByHandle,
                            int64 metahandle)
-  : Entry(trans, GET_BY_HANDLE, metahandle) {
+  : Entry(trans, GET_BY_HANDLE, metahandle), write_transaction_(trans) {
   trans->SaveOriginal(kernel_);
 }
 
 MutableEntry::MutableEntry(WriteTransaction* trans, GetByPath,
                            const PathString& path)
-  : Entry(trans, GET_BY_PATH, path) {
+  : Entry(trans, GET_BY_PATH, path), write_transaction_(trans) {
   trans->SaveOriginal(kernel_);
 }
 
 MutableEntry::MutableEntry(WriteTransaction* trans, GetByParentIdAndName,
                            const Id& parentid, const PathString& name)
-  : Entry(trans, GET_BY_PARENTID_AND_NAME, parentid, name) {
+  : Entry(trans, GET_BY_PARENTID_AND_NAME, parentid, name),
+    write_transaction_(trans) {
   trans->SaveOriginal(kernel_);
 }
 
 MutableEntry::MutableEntry(WriteTransaction* trans, GetByParentIdAndDBName,
                            const Id& parentid, const PathString& name)
-  : Entry(trans, GET_BY_PARENTID_AND_DBNAME, parentid, name) {
+  : Entry(trans, GET_BY_PARENTID_AND_DBNAME, parentid, name),
+    write_transaction_(trans) {
   trans->SaveOriginal(kernel_);
 }
 
@@ -1538,14 +1428,6 @@ bool MutableEntry::Put(IdField field, const Id& value) {
   return true;
 }
 
-WriteTransaction* MutableEntry::trans() const {
-  // We are in a mutable entry, so we must be in a write transaction.
-  // Maybe we could keep a pointer to the transaction in MutableEntry.
-  ThreadNode* node = reinterpret_cast<ThreadNode*>
-    (pthread_getspecific(dir()->kernel_->thread_node_key));
-  return node->current_write_trans;
-}
-
 bool MutableEntry::Put(BaseVersion field, int64 value) {
   DCHECK(kernel_);
   if (kernel_->ref(field) != value) {
@@ -1635,13 +1517,13 @@ void MutableEntry::UnlinkFromOrder() {
       CHECK((old_next == Get(ID)) || !old_next.ServerKnows());
       return;  // Done if we were already self-looped (hence unlinked).
     }
-    MutableEntry previous_entry(trans(), GET_BY_ID, old_previous);
+    MutableEntry previous_entry(write_transaction(), GET_BY_ID, old_previous);
     CHECK(previous_entry.good());
     previous_entry.Put(NEXT_ID, old_next);
   }
 
   if (!old_next.IsRoot()) {
-    MutableEntry next_entry(trans(), GET_BY_ID, old_next);
+    MutableEntry next_entry(write_transaction(), GET_BY_ID, old_next);
     CHECK(next_entry.good());
     next_entry.Put(PREV_ID, old_previous);
   }
@@ -1662,7 +1544,7 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
   // job interview.  An "IsRoot" Id signifies the head or tail.
   Id successor_id;
   if (!predecessor_id.IsRoot()) {
-    MutableEntry predecessor(trans(), GET_BY_ID, predecessor_id);
+    MutableEntry predecessor(write_transaction(), GET_BY_ID, predecessor_id);
     CHECK(predecessor.good());
     if (predecessor.Get(PARENT_ID) != Get(PARENT_ID))
       return false;
@@ -1673,7 +1555,7 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
     successor_id = dir->GetFirstChildId(trans(), Get(PARENT_ID));
   }
   if (!successor_id.IsRoot()) {
-    MutableEntry successor(trans(), GET_BY_ID, successor_id);
+    MutableEntry successor(write_transaction(), GET_BY_ID, successor_id);
     CHECK(successor.good());
     if (successor.Get(PARENT_ID) != Get(PARENT_ID))
       return false;
@@ -1988,12 +1870,4 @@ FastDump& operator<<(FastDump& dump, const syncable::Blob& blob) {
   string buffer(HexEncode(&blob[0], blob.size()));
   dump.out_->sputn(buffer.c_str(), buffer.size());
   return dump;
-}
-
-std::ostream& operator<<(std::ostream& s, const syncable::ThreadNode& node) {
-  s << "thread id: " << std::hex << node.id << "\n"
-    << "file: " << node.file << "\n"
-    << "line: " << std::dec << node.line << "\n"
-    << "wait_started: " << node.wait_started.ToInternalValue();
-  return s;
-}
+}
\ No newline at end of file
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index 1d343f9..01767ab 100644
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -15,6 +15,7 @@
 
 #include "base/atomicops.h"
 #include "base/basictypes.h"
+#include "base/lock.h"
 #include "base/time.h"
 #include "chrome/browser/sync/syncable/blob.h"
 #include "chrome/browser/sync/syncable/dir_open_result.h"
@@ -652,7 +653,9 @@ class MutableEntry : public Entry {
   MutableEntry(WriteTransaction* trans, GetByParentIdAndDBName,
     const Id& parentid, const PathString& name);
 
-  WriteTransaction* trans() const;
+  inline WriteTransaction* write_transaction() const {
+    return write_transaction_;
+  }
 
   // Field Accessors.  Some of them trigger the re-indexing of the entry.
   // Return true on success, return false on failure, which means
@@ -726,6 +729,11 @@ class MutableEntry : public Entry {
   // refer to this entry.
   void UnlinkFromOrder();
 
+  // Kind of redundant. We should reduce the number of pointers
+  // floating around if at all possible. Could we store this in Directory?
+  // Scope: Set on construction, never changed after that.
+  WriteTransaction* const write_transaction_;
+
  protected:
   MutableEntry();
 
@@ -802,59 +810,6 @@ struct ExtendedAttributeValue {
 typedef std::map<ExtendedAttributeKey, ExtendedAttributeValue>
     ExtendedAttributes;
 
-// Used to maintain our per-thread transaction state and to enforce
-// our transaction invariants (e.g. no recursive transactions).
-// Each time a thread enters a transaction by constructing a Read or a
-// WriteTransaction object, a ThreadNode object is pulled from thread
-// local storage, or created and stored in thread-local storage if it
-// doesn't yet exist.
-struct ThreadNode {
-  const char* file;
-  int line;
-  base::TimeTicks wait_started;
-  ThreadId id;
-  ThreadNode* next;
-  ThreadNode* prev;
-
-  // True when this node is in a linked list.  Only accessed from owner thread
-  // so no locking necessary.
-  bool in_list;
-  WriteTransaction* current_write_trans;
-  PThreadCondVar condvar;  // Mutex is the kernel's transaction mutex.
-  bool wake_up;  // flag for condvar.
-  int tclass;  // Really a BaseTransaction::TClass, but no forward enums.
-
-  // Linked list operations.
-  inline ThreadNode() : in_list(false), current_write_trans(NULL),
-       wake_up(false) {
-    next = prev = this;
-  }
-  inline ThreadNode* Remove() {
-    in_list = false;
-    prev->next = next;
-    next->prev = prev;
-    return next = prev = this;
-  }
-  inline void Insert(ThreadNode* node) {
-    in_list = true;
-    prev = node->prev;
-    next = node;
-    next->prev = prev->next = this;
-  }
-};
-
-struct ThreadCounts {
-  int waiting;
-  int active;
-  // Also keep a linked list of thread information.
-  ThreadNode waiting_headtail;
-  ThreadNode active_headtail;
-
-  ThreadCounts() : waiting(0), active(0) { }
-
-  DISALLOW_COPY_AND_ASSIGN(ThreadCounts);
-};
-
 typedef PThreadScopedLock<PThreadMutex> ScopedTransactionLock;
 typedef std::set<int64> MetahandleSet;
 
@@ -1159,10 +1114,8 @@ class Directory {
     void AddRef();  // For convenience.
     void Release();
 
-    // Next 3 members implement the reader/writer lock.
-    PThreadMutex transaction_mutex;  // Protects next member.
-    ThreadCounts thread_counts;
-    pthread_key_t thread_node_key;
+    // Implements ReadTransaction / WriteTransaction using a simple lock.
+    Lock transaction_mutex;
 
     // The name of this directory, used as a key into open_files_;
     PathString const name_;
@@ -1174,7 +1127,7 @@ class Directory {
     //
     // Never hold the mutex and do anything with the database or any
     // other buffered IO.  Violating this rule will result in deadlock.
-    pthread_mutex_t mutex;
+    pthread_mutex_t mutex;  // TODO(chron): Swap this out for Chrome Lock
     MetahandlesIndex* metahandles_index;  // Entries indexed by metahandle
     IdsIndex* ids_index;  // Entries indexed by id
     ParentIdAndNamesIndex* parent_id_and_names_index;
@@ -1251,25 +1204,15 @@ class ScopedKernelUnlock {
 class BaseTransaction {
   friend class Entry;
  public:
-  enum TransactionClass { READ, WRITE };
+  inline Directory* directory() const { return directory_; }
+  inline Id root_id() const { return Id(); }
 
  protected:
   BaseTransaction(Directory* directory, const char* name,
-                  const char* source_file, int line);
-
-  // The members below are optionally called by descendants.
-  void Lock(ThreadCounts* const thread_counts, ThreadNode* thread_node,
-            TransactionClass tclass);
-  void AfterLock(ThreadNode* thread_node);
-  void UnlockAndLog(ThreadCounts* const thread_counts, OriginalEntries*);
-  void Init(ThreadCounts* const thread_counts, TransactionClass tclass);
-  ThreadNode* MakeThreadNode();
+                  const char* source_file, int line, WriterTag writer);
 
- public:
-  inline Directory* directory() const { return directory_; }
-  inline Id root_id() const { return Id(); }
+  void UnlockAndLog(OriginalEntries* entries);
 
- protected:
   Directory* const directory_;
   Directory::Kernel* const dirkernel_;  // for brevity
   const char* const name_;
@@ -1278,6 +1221,9 @@ class BaseTransaction {
   const int line_;
   WriterTag writer_;
 
+ private:
+  void Lock();
+
   DISALLOW_COPY_AND_ASSIGN(BaseTransaction);
 };
 
@@ -1312,17 +1258,6 @@ class WriteTransaction : public BaseTransaction {
   void SaveOriginal(EntryKernel* entry);
 
  protected:
-  // If I had had the foresight to create a BaseWriteTransactionClass,
-  // I would not have needed this pass-through constructor and the
-  // skip_destructor flag.
-  explicit WriteTransaction(Directory* directory,
-                            const char* name, WriterTag writer,
-                            const char* source_file,
-                            int line, bool skip_destructor,
-                            OriginalEntries* originals);
-
-  const bool skip_destructor_;
-
   // Before an entry gets modified, we copy the original into a list
   // so that we can issue change notifications when the transaction
   // is done.
@@ -1411,6 +1346,4 @@ std::ostream& operator <<(std::ostream&, const syncable::Blob&);
 browser_sync::FastDump& operator <<
   (browser_sync::FastDump&, const syncable::Blob&);
 
-std::ostream& operator <<(std::ostream&, const syncable::ThreadNode&);
-
 #endif  // CHROME_BROWSER_SYNC_SYNCABLE_SYNCABLE_H_