Revert 110384 - Revert 110356 - Sync: Improve handling of database load failures

This failure path has not received a lot of testing until now. Here are
the issues addressed by this patch:

- We usually check the return value of step() calls. However, we do not
check the return value of prepare() calls, which are more likely to fail.
If they do fail, we will DCHECK() or go on to dereference an invalid
pointer in step(). This patch checks the return value of one particular
prepare statement, the one in CheckIntegrity().

- Disable DCHECKs on sqlite errors, DirectoryManager open failure,
and SyncManager initialization failure. This will allow us to test these
error paths.

- Be careful in ShutdownOnSyncThread(). The directory will not be fully
intialized during shutdown if the database load failed.

- Add a ProfileSyncService unit test that simulates a load from an
unreadable database. The harness had to be modified slightly to make
this possible.

- Remove a setup_for_test_mode_ flag in SyncManager::SyncInternal::Init.
I don't know what the original intent of this flag was. However, I do
know that it prevents me from properly simulating a database load failure
and removing it seems to have no ill effects.

- Do not delete the database from DirectoryBackingStore. If this code
were to get executed it would put us into an inconsistent state. See
issue 103824. However, it's unlikely this code would get executed. If
the database were actually corrupt, we would DCHECK or de-reference an
invalid pointer on our way to this code because we don't check the
return value of the attempt to prepare an SQL statement in
DirectoryBackingStore::CheckIntegrity().

- Modify the DirectoryBackingStoreTest.Corruption unit test to expect the
new behaviour.

- Disable sync when backend initialize fails. Such a failure could
be due to bad local state. We don't know the actual cause because the
information is not available from the ProfileSyncService callback. The
safe course of action is to clear our local sync state and try again
later. It's the easiest way to get back to the most well travelled sync
initialization path.

- Fix error handling logic in OpenAndConfigureHandleHelper.  It used
to rely on a specially-crafted scoped_ptr to close the database if we
had to leave the function unsuccessfully.  This was wrong in two ways.
First, it did not reset the handle to NULL, which meant that the
DirectoryBackingStore would attempt to free the handle again when it
was destructed.  Second, it failed to clean up the handle when the
return value was not SQLITE_OK.  (Though I suppose it would have been
cleaned up by the DirectoryBackingStore destructor, thanks to the
previous issue).

BUG=103307, 103824
TEST=DirectoryBackingStoreTest.Corruption, ProfileSyncServiceTest.CorruptDatabase


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

TBR=rlarocque@chromium.org
Review URL: http://codereview.chromium.org/8586004

TBR=cpu@chromium.org
Review URL: http://codereview.chromium.org/8574045

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

1 files changed, 3 insertions, 3 deletions

diff --git a/chrome/browser/sync/util/sqlite_utils.cc b/chrome/browser/sync/util/sqlite_utils.cc
index 4cb6c4e..7b75f0e 100644
--- a/chrome/browser/sync/util/sqlite_utils.cc
+++ b/chrome/browser/sync/util/sqlite_utils.cc
@@ -33,9 +33,9 @@ class DebugSQLErrorHandler: public VanillaSQLErrorHandler {
  public:
   virtual int HandleError(int error, sqlite3* db) {
     error_ = error;
-    NOTREACHED() << "sqlite error " << error
-                 << " " << sqlite3_errmsg(db)
-                 << " db " << static_cast<void*>(db);
+    LOG(ERROR) << "sqlite error " << error
+               << " " << sqlite3_errmsg(db)
+               << " db " << static_cast<void*>(db);
     return error;
   }
 };