Corrupted backing stores not always deleted |
||||||
Issue descriptionIndexed DB should immediately delete corrupted databases immediately when corruption is detected. This is done in in IndexedDBFactoryImpl::HandleBackingStoreCorruption() which calls IndexedDBBackingStore::DestroyBackingStore() eventually calling leveldb::DestroyDB(). The problem is that the IndexedDBBackingStore instance can be shared between multiple IndexedDBDatabase so with the current implementation they all must release their reference to their backing store before leveldb::DestroyDB() can succeed. This doesn't currently happen, so if the error is detected, only one reference is deleted and the others prevent the deletion. More information at b/65922888
,
Nov 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/538fe6816c231c7c91ce15a4983834f9e1a55811 commit 538fe6816c231c7c91ce15a4983834f9e1a55811 Author: Chris Mumford <cmumford@chromium.org> Date: Fri Nov 10 16:52:16 2017 IndexedDB: Added UMA value for destruction of corrupt leveldb. Added WebCore.IndexedDB.DestroyCorruptBackingStoreStatus to record the result of an attempt to destroy a corrupt (leveldb) backing store used by Indexed DB. Bug: 782972 Change-Id: I13e9d64531c72d1d4f5193e2d682671d799f59a5 Reviewed-on: https://chromium-review.googlesource.com/759616 Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Commit-Queue: Chris Mumford <cmumford@chromium.org> Cr-Commit-Position: refs/heads/master@{#515574} [modify] https://crrev.com/538fe6816c231c7c91ce15a4983834f9e1a55811/content/browser/indexed_db/indexed_db_factory_impl.cc [modify] https://crrev.com/538fe6816c231c7c91ce15a4983834f9e1a55811/tools/metrics/histograms/histograms.xml
,
Nov 17 2017
The implementation today already attempts to close all connections/databases before deleting the backing store. I spent about 6-8 hours trying to find a fault in the implementation, and was unable to do so. The fact that the user in b/65922888 was able to reproduce with reliably indicates there is a problem - just not an obvious one. The above change to add an UMA metric landed in 64.0.3265.0. I am going to request merge to M63.
,
Nov 17 2017
Requesting merge of https://chromium-review.googlesource.com/759616 to M63. It has been in M64 since 64.0.3265.0. The above change only adds a single UMA value: WebCore.IndexedDB.DestroyCorruptBackingStoreStatus. It has already recorded three events since landing, and there have been no reports of any adverse effects. The reason for the request is to assist in determining the cause of the failure to destroy a corrupted database as encountered in this bug. Hopefully the failure code will provide insight to the cause.
,
Nov 17 2017
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 17 2017
Please add affected OSs.
,
Nov 18 2017
,
Nov 20 2017
,
Nov 20 2017
Issue 781482 has been merged into this issue.
,
Nov 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e756ca94d4602c6ad727c78e4725936292e29fcc commit e756ca94d4602c6ad727c78e4725936292e29fcc Author: Chris Mumford <cmumford@chromium.org> Date: Mon Nov 20 21:45:04 2017 IndexedDB: Added UMA value for destruction of corrupt leveldb. Added WebCore.IndexedDB.DestroyCorruptBackingStoreStatus to record the result of an attempt to destroy a corrupt (leveldb) backing store used by Indexed DB. TBR=cmumford@chromium.org (cherry picked from commit 538fe6816c231c7c91ce15a4983834f9e1a55811) Bug: 782972 Change-Id: I13e9d64531c72d1d4f5193e2d682671d799f59a5 Reviewed-on: https://chromium-review.googlesource.com/759616 Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Commit-Queue: Chris Mumford <cmumford@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#515574} Reviewed-on: https://chromium-review.googlesource.com/780199 Reviewed-by: Chris Mumford <cmumford@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#551} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/e756ca94d4602c6ad727c78e4725936292e29fcc/content/browser/indexed_db/indexed_db_factory_impl.cc [modify] https://crrev.com/e756ca94d4602c6ad727c78e4725936292e29fcc/tools/metrics/histograms/histograms.xml
,
Jan 8 2018
Looking at the UMA stats for WebCore.IndexedDB.DestroyCorruptBackingStoreStatus 99.83% of all delete attempts in IndexedDBFactoryImpl::HandleBackingStoreCorruption() in M63 were successful. Unfortunately we don't have enough UMA coverage to know if all detected corruptions resulted in a db delete and I'm not sure it would be worth the effort (and time) to add enough UMA coverage to figure this out. The database mentioned in b/65922888 is quite large (> 200MB) so I doubt it is being deleted and recreated every time. I still suspect there is a way for the database to be corrupted w/o being deleted, but how that is eludes me. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by cmumford@chromium.org
, Nov 9 2017