From 15caf87218526c1d6c3e4e3d06988ba504ca2e54 Mon Sep 17 00:00:00 2001 From: "cpu@chromium.org" Date: Thu, 17 Nov 2011 00:09:51 +0000 Subject: 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 --- chrome/browser/sync/util/sqlite_utils.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'chrome/browser/sync/util') 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(db); + LOG(ERROR) << "sqlite error " << error + << " " << sqlite3_errmsg(db) + << " db " << static_cast(db); return error; } }; -- cgit v1.1