Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1236583002:
Try to reset quota database which is suspected to be corrupted (Closed)
|
Created:
5 years, 5 months ago by alexanderk Modified:
5 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTry to reset quota database which is suspected to be corrupted
If we are able to open quota database file but cannot create a table
then we suspect it to be corrupted and trying to reset.
BUG=508916
Committed: https://crrev.com/aba1d19017e23be931303a3274b035861da3e4ae
Cr-Commit-Position: refs/heads/master@{#345966}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Reworked in more logical way. Added unittest #
Total comments: 1
Messages
Total messages: 31 (10 generated)
alexanderk@opera.com changed reviewers: + dgrogan@chromium.org, pilgrim@chromium.org
dgrogan@chromium.org changed reviewers: + michaeln@chromium.org
This seems fine to me, but could you dig up the CL where EnsureDatabaseVersion was added and see if this was addressed in any of the comments? Wait for a review from michaeln who knows this code a lot better than me.
Also, a test would be good.
thnx for the cl! https://codereview.chromium.org/1236583002/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_database.cc (right): https://codereview.chromium.org/1236583002/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_database.cc:456: opened = db_->Open(db_file_path_); It looks like the DLOG(FATAL) happens inside of the Open method but 'true' is returned since the result of PRAGMA journal_mode is ignored. https://codereview.chromium.org/1236583002/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_database.cc:461: if (!opened || !EnsureDatabaseVersion()) { It looks like there are other ways in which a borken db file could get be forever borken? I'd vote to move the retry logic higher up in the stack to catch more of those ways. For example, if open fails due to outright corruption noticed upon open, we probably should retry in that case too. Take a look at appcache_database.cc and its unittests for an example of more aggresive retry logic. if (!opened || !db_->QuickIntegrityCheck() || !EnsureDatabaseVersion()) ... delete and retry if not using an in-memory db ... https://codereview.chromium.org/1236583002/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_database.cc:486: return ResetSchema(); I was wondering how this fixed a crash? Its not really a crash in release builds. A corrupt DB runs afoul a DLOG(FATAL) statement when being opened and crashes debug builds. A new unittest quota_database_unittest.cc would be good. That file is in content\browser\quota. The crashing in debug builds behavior can probably be avoided with a ScopedErrorIgnorer. CorruptSizeInHeader() can help create corrupt dbs.
On 2015/07/23 19:54:18, michaeln wrote: > thnx for the cl! > > https://codereview.chromium.org/1236583002/diff/1/storage/browser/quota/quota... > File storage/browser/quota/quota_database.cc (right): > > https://codereview.chromium.org/1236583002/diff/1/storage/browser/quota/quota... > storage/browser/quota/quota_database.cc:456: opened = db_->Open(db_file_path_); > It looks like the DLOG(FATAL) happens inside of the Open method but 'true' is > returned since the result of PRAGMA journal_mode is ignored. > > https://codereview.chromium.org/1236583002/diff/1/storage/browser/quota/quota... > storage/browser/quota/quota_database.cc:461: if (!opened || > !EnsureDatabaseVersion()) { > It looks like there are other ways in which a borken db file could get be > forever borken? I'd vote to move the retry logic higher up in the stack to catch > more of those ways. For example, if open fails due to outright corruption > noticed upon open, we probably should retry in that case too. > Right, I have reworked it in more logical way. > Take a look at appcache_database.cc and its unittests for an example of more > aggresive retry logic. > > if (!opened || !db_->QuickIntegrityCheck() || !EnsureDatabaseVersion()) > ... delete and retry if not using an in-memory db ... Yes, QuotaDatabase::ResetSchema() is actually the same as AppCacheDatabase::DeleteExistingAndCreateNewDatabase() > https://codereview.chromium.org/1236583002/diff/1/storage/browser/quota/quota... > storage/browser/quota/quota_database.cc:486: return ResetSchema(); > I was wondering how this fixed a crash? Its not really a crash in release > builds. A corrupt DB runs afoul a DLOG(FATAL) statement when being opened and > crashes debug builds. It does not crash but fall into unrecoverable state, the only way is to delete DB file and create it again. Described well in AppCacheDatabase::LazyOpen(). > A new unittest quota_database_unittest.cc would be good. That file is in > content\browser\quota. The crashing in debug builds behavior can probably be > avoided with a ScopedErrorIgnorer. CorruptSizeInHeader() can help create corrupt > dbs. Added.
lgtm thnx! https://codereview.chromium.org/1236583002/diff/20001/content/browser/quota/q... File content/browser/quota/quota_database_unittest.cc (right): https://codereview.chromium.org/1236583002/diff/20001/content/browser/quota/q... content/browser/quota/quota_database_unittest.cc:51: void Reopen(const base::FilePath& kDbFile) { very funky name for an a param but i see its in keeping with the surrounding code
The CQ bit was checked by alexanderk@opera.com
The CQ bit was unchecked by alexanderk@opera.com
The CQ bit was checked by alexanderk@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236583002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1236583002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dgrogan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236583002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1236583002/20001
I'm going to try to make michaeln an OWNER for content/browser/quota in https://codereview.chromium.org/1320463005
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dgrogan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236583002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1236583002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/aba1d19017e23be931303a3274b035861da3e4ae Cr-Commit-Position: refs/heads/master@{#345966}
Message was sent while issue was closed.
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
Message was sent while issue was closed.
fyi, this seems to have caused the following failure: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1314523003/ by dewittj@chromium.org. The reason for reverting is: Broke at least the following: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2... https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%2....
Message was sent while issue was closed.
oooops, we forgot to use a sql::ScopedErrorIgnorer for the new test :(
Message was sent while issue was closed.
On 2015/08/27 21:18:41, michaeln wrote: > oooops, we forgot to use a sql::ScopedErrorIgnorer for the new test :( i'll re'up the cl with that addition
Message was sent while issue was closed.
On 2015/08/27 22:49:20, michaeln wrote: > On 2015/08/27 21:18:41, michaeln wrote: > > oooops, we forgot to use a sql::ScopedErrorIgnorer for the new test :( > > i'll re'up the cl with that addition Is it a bug that this wasn't caught by the try bots?
Message was sent while issue was closed.
> Is it a bug that this wasn't caught by the try bots? i think it wasn't tried, no dbg try bot ran content_unittests |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
