This is the third patch in the series.

This will make the transactions unrecoverable error aware.

On the destructor before releasing the lock they will call OnUnrecoverableError if an unrecoverable error has been detected during the course of the transaction.


BUG=100444
TEST=sync_integration_tests.exe, unit_tests.exe, sync_unit_tests.exe, manual tests.


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

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

3 files changed, 93 insertions, 12 deletions

diff --git a/chrome/browser/sync/internal_api/includes/unrecoverable_error_handler_mock.cc b/chrome/browser/sync/internal_api/includes/unrecoverable_error_handler_mock.cc
index 9c144e25..8ff9a1b 100644
--- a/chrome/browser/sync/internal_api/includes/unrecoverable_error_handler_mock.cc
+++ b/chrome/browser/sync/internal_api/includes/unrecoverable_error_handler_mock.cc
@@ -8,3 +8,4 @@ namespace browser_sync {
   MockUnrecoverableErrorHandler::MockUnrecoverableErrorHandler() {}
   MockUnrecoverableErrorHandler::~MockUnrecoverableErrorHandler() {}
 }  // namespace browser_sync
+
diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index 64b6030..4676a02 100644
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -486,7 +486,8 @@ Directory::Kernel::~Kernel() {
 Directory::Directory(UnrecoverableErrorHandler* unrecoverable_error_handler)
     : kernel_(NULL),
       store_(NULL),
-      unrecoverable_error_handler_(unrecoverable_error_handler) {
+      unrecoverable_error_handler_(unrecoverable_error_handler),
+      unrecoverable_error_set_(false) {
 }
 
 Directory::~Directory() {
@@ -571,6 +572,16 @@ void Directory::Close() {
   }
 }
 
+void Directory::OnUnrecoverableError(const BaseTransaction* trans,
+                                     const tracked_objects::Location& location,
+                                     const std::string & message) {
+  DCHECK(trans != NULL);
+  unrecoverable_error_set_ = true;
+  unrecoverable_error_handler_->OnUnrecoverableError(location,
+                                                     message);
+}
+
+
 EntryKernel* Directory::GetEntryById(const Id& id) {
   ScopedKernelLock lock(this);
   return GetEntryById(id, &lock);
@@ -711,8 +722,9 @@ void Directory::ReindexParentId(EntryKernel* const entry,
   }
 }
 
-UnrecoverableErrorHandler* Directory::unrecoverable_error_handler() {
-  return unrecoverable_error_handler_;
+bool Directory::unrecoverable_error_set(const BaseTransaction* trans) const {
+  DCHECK(trans != NULL);
+  return unrecoverable_error_set_;
 }
 
 void Directory::ClearDirtyMetahandles() {
@@ -740,9 +752,13 @@ bool Directory::SafeToPurgeFromMemory(const EntryKernel* const entry) const {
 void Directory::TakeSnapshotForSaveChanges(SaveChangesSnapshot* snapshot) {
   ReadTransaction trans(FROM_HERE, this);
   ScopedKernelLock lock(this);
+
+  // If there is an unrecoverable error then just bail out.
+  if (unrecoverable_error_set(&trans))
+    return;
+
   // Deep copy dirty entries from kernel_->metahandles_index into snapshot and
   // clear dirty flags.
-
   for (MetahandleSet::const_iterator i = kernel_->dirty_metahandles->begin();
        i != kernel_->dirty_metahandles->end(); ++i) {
     EntryKernel* entry = GetEntryByHandle(*i, &lock);
@@ -1232,12 +1248,36 @@ void BaseTransaction::Unlock() {
   dirkernel_->transaction_mutex.Release();
 }
 
+void BaseTransaction::OnUnrecoverableError(
+    const tracked_objects::Location& location,
+    const std::string& message) {
+  unrecoverable_error_set_ = true;
+  unrecoverable_error_location_ = location;
+  unrecoverable_error_msg_ = message;
+
+  // Note: We dont call the Directory's OnUnrecoverableError method right
+  // away. Instead we wait to unwind the stack and in the destructor of the
+  // transaction we would call the OnUnrecoverableError method.
+}
+
+void BaseTransaction::HandleUnrecoverableErrorIfSet() {
+  if (unrecoverable_error_set_) {
+    directory()->OnUnrecoverableError(this,
+        unrecoverable_error_location_,
+        unrecoverable_error_msg_);
+  }
+}
+
 BaseTransaction::BaseTransaction(const tracked_objects::Location& from_here,
                                  const char* name,
                                  WriterTag writer,
                                  Directory* directory)
     : from_here_(from_here), name_(name), writer_(writer),
-      directory_(directory), dirkernel_(directory->kernel_) {
+      directory_(directory), dirkernel_(directory->kernel_),
+      unrecoverable_error_set_(false) {
+  // TODO(lipalani): Don't issue a good transaction if the directory has
+  // unrecoverable error set. And the callers have to check trans.good before
+  // proceeding.
   TRACE_EVENT_BEGIN2("sync", name_,
                      "src_file", from_here_.file_name(),
                      "src_func", from_here_.function_name());
@@ -1261,6 +1301,7 @@ ReadTransaction::ReadTransaction(const tracked_objects::Location& location,
 }
 
 ReadTransaction::~ReadTransaction() {
+  HandleUnrecoverableErrorIfSet();
   Unlock();
 }
 
@@ -1369,12 +1410,21 @@ void WriteTransaction::NotifyTransactionComplete(
 WriteTransaction::~WriteTransaction() {
   const ImmutableEntryKernelMutationMap& mutations = RecordMutations();
 
-  if (OFF != kInvariantCheckLevel) {
-    const bool full_scan = (FULL_DB_VERIFICATION == kInvariantCheckLevel);
-    if (full_scan)
-      directory()->CheckTreeInvariants(this, full_scan);
-    else
-      directory()->CheckTreeInvariants(this, mutations.Get());
+  if (!unrecoverable_error_set_) {
+    if (OFF != kInvariantCheckLevel) {
+      const bool full_scan = (FULL_DB_VERIFICATION == kInvariantCheckLevel);
+      if (full_scan)
+        directory()->CheckTreeInvariants(this, full_scan);
+      else
+        directory()->CheckTreeInvariants(this, mutations.Get());
+    }
+  }
+
+  // |CheckTreeInvariants| could have thrown an unrecoverable error.
+  if (unrecoverable_error_set_) {
+    HandleUnrecoverableErrorIfSet();
+    Unlock();
+    return;
   }
 
   UnlockAndNotify(mutations);
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index bf517fe..6b945c8 100644
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -870,7 +870,17 @@ class Directory {
   // Unique to each account / client pair.
   std::string cache_guid() const;
 
-  browser_sync::UnrecoverableErrorHandler* unrecoverable_error_handler();
+  // Returns true if the directory had encountered an unrecoverable error.
+  // Note: Any function in |Directory| that can be called without holding a
+  // transaction need to check if the Directory already has an unrecoverable
+  // error on it.
+  bool unrecoverable_error_set(const BaseTransaction* trans) const;
+
+  // Called to set the unrecoverable error on the directory and to propagate
+  // the error to upper layers.
+  void OnUnrecoverableError(const BaseTransaction* trans,
+                            const tracked_objects::Location& location,
+                            const std::string & message);
 
  protected:  // for friends, mainly used by Entry constructors
   virtual EntryKernel* GetEntryByHandle(int64 handle);
@@ -1193,6 +1203,7 @@ class Directory {
   DirectoryBackingStore* store_;
 
   browser_sync::UnrecoverableErrorHandler* unrecoverable_error_handler_;
+  bool unrecoverable_error_set_;
 };
 
 class ScopedKernelLock {
@@ -1214,6 +1225,14 @@ class BaseTransaction {
 
   virtual ~BaseTransaction();
 
+  // This should be called when a database corruption is detected and there is
+  // no way for us to recover short of wiping the database clean. When this is
+  // called we set a bool in the transaction. The caller has to unwind the
+  // stack. When the destructor for the transaction is called it acts upon the
+  // bool and calls the Directory to handle the unrecoverable error.
+  void OnUnrecoverableError(const tracked_objects::Location& location,
+                            const std::string& message);
+
  protected:
   BaseTransaction(const tracked_objects::Location& from_here,
                   const char* name,
@@ -1223,12 +1242,23 @@ class BaseTransaction {
   void Lock();
   void Unlock();
 
+  // This should be called before unlocking because it calls the Direcotry's
+  // OnUnrecoverableError method which is not protected by locks and could
+  // be called from any thread. Holding the transaction lock ensures only one
+  // thread could call the method at a time.
+  void HandleUnrecoverableErrorIfSet();
+
   const tracked_objects::Location from_here_;
   const char* const name_;
   WriterTag writer_;
   Directory* const directory_;
   Directory::Kernel* const dirkernel_;  // for brevity
 
+  // Error information.
+  bool unrecoverable_error_set_;
+  tracked_objects::Location unrecoverable_error_location_;
+  std::string unrecoverable_error_msg_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(BaseTransaction);
 };