Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(25)

Issue 8574045: Revert 110384 - Revert 110356 - Sync: Improve handling of database load failures (Closed)

Created:
9 years, 1 month ago by cpu_(ooo_6.6-7.5)
Modified:
9 years, 1 month ago
Reviewers:
cpu_(ooo_6.6-7.5)
CC:
chromium-reviews, Raghu Simha, ncarter (slow), akalin, tim (not reviewing), Paweł Hajdan Jr.
Visibility:
Public.

Description

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 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110388

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -72 lines) Patch
M chrome/browser/sync/glue/sync_backend_host.cc View 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/sync/internal_api/sync_manager.cc View 3 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.cc View 1 chunk +9 lines, -4 lines 0 comments Download
M chrome/browser/sync/profile_sync_service_unittest.cc View 6 chunks +36 lines, -8 lines 0 comments Download
M chrome/browser/sync/syncable/directory_backing_store.cc View 3 chunks +27 lines, -42 lines 0 comments Download
M chrome/browser/sync/syncable/directory_backing_store_unittest.cc View 1 chunk +1 line, -9 lines 0 comments Download
M chrome/browser/sync/util/sqlite_utils.cc View 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 1 (0 generated)
cpu_(ooo_6.6-7.5)
9 years, 1 month ago (2011-11-17 00:05:21 UTC) #1

          

Powered by Google App Engine
This is Rietveld 408576698