Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
197333009:
Handling LevelDB errors encountered after open. (Closed)
|
Created:
6 years, 9 months ago by cmumford Modified:
6 years, 8 months ago CC:
chromium-reviews, jam, alecflett, ericu+idb_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, dgrogan, jsbell+idb_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUn-revert Handling LevelDB errors encountered after open.
The previous IndexedDB implementation only checked for (and reported)
corrupted LevelDB's during open. If they are determined to be corrupted
during use then the database was simply closed. The page would be unaware of
this, and would likely reopen the database at a later time (which would work)
only to fail again during a read/write operation. There was no way for the
page to get out of that corrupted state - the user would have to delete all
origin data.
This change will record the fact that the backing store was corrupted, and
during open this would be read and, if present, result in the same dataLoss
state being reported to the page in the upgradeneeded event.
Also split IndexedDBBackingStore's REPORT_ERROR macro in two:
REPORT_ERROR_UNTESTED (which calls NOTREACHED) and REPORT_ERROR. REPORT_ERROR
is used for errors which are encountered during tests. This was done so that
runtime errors could still be caught during a debug build run.
The original change (r260147) had a buffer overrun in a unit test fixed
in this CL.
BUG=322707
R=isherman@chromium.org, jochen@chromium.org, jsbell@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260697
Patch Set 1 #
Total comments: 75
Patch Set 2 : Address Josh' comments from file set #1 #Patch Set 3 : Commented that DestroyBackingStore leaves non LevelDB files alone #
Total comments: 1
Patch Set 4 : Add description for histogram metric. #Patch Set 5 : Rebased to resolve conflict with trunk #Patch Set 6 : Resolved upstream conflict #2 #Patch Set 7 : Added FILE_PATH_LITERAL #Patch Set 8 : FILE_PATH_LITERAL #2 #Patch Set 9 : Fixed buffer overflow (woops) in original test #
Total comments: 1
Patch Set 10 : Tweaked comment. #Patch Set 11 : Reverted two unecessary changes #Messages
Total messages: 39 (0 generated)
jsbell@chromium.org: Please review changes in jochen@chromium.org: Please review changes in
https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:50: return ComputeFileName(origin_url).Append("corruption_info.json"); Doesn't this put the file in the directory, which will end up being deleted? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:111: // could be caused by data corruption. A macro is used instead of an Maybe add to the comment that anything that could be caused by data corruption should really be handled, i.e. we should eliminate all uses of REPORT_ERROR_UNTESTED over time. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:541: // Assumes caller has already closed the backing store. Move this comment to the header file? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:576: if (file_size == base::ReadPlatformFile(file, 0, &bytes[0], file_size)) { &bytes[0] is undefined behavior if the vector length is 0. You should probably guard against 0-length files anyway (i.e. no need to open them) https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:580: if (val) { Add: && val->GetType() == base::Value::TYPE_DICTIONARY https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:597: const IndexedDBDatabaseError& dberror) { Since this only serializes the message, and since ReadCorruptionInfo only returns the message, what do you think about having this method only take a string rather than an error object? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:616: if (file) { Early exit instead, which would let you eliminate 'success' ? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:684: INDEXED_DB_BACKING_STORE_OPEN_FAILED_IO_ERROR_CHECKING_SCHEMA, Add a new histogram value? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:691: bool ok = IsSchemaKnown(db.get(), &is_schema_known); Merge this into the else i.e. "else if (IsSchemaKnown(...))" - save a level of nesting (and minimize the diff) https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_backing_store.h (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.h:74: virtual void Flush(); Can you add a comment to explain what this does? It's not a flush in the traditional sense (i.e. everything should already be on disk). Also, if it's for testing only, consider making it protected (if e.g. it's called via a subclass) and/or rename with '..ForTesting' https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:366: static void FlushIndexedDBDatabase(scoped_refptr<IndexedDBContextImpl> context, 'Database' or 'BackingStore' ? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:377: backing_store->Flush(); All databases in an origin share a backing store, so this is redundant. Intentional? Paranoid? (add comment to explain?) https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:382: static void CorruptIndexedDBDatabase( Add comment about why it's async? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:397: int64 size(0); size_t? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:402: FILE* f = fopen(idb_file.value().c_str(), "w"); Why FILE/fopen/fwrite instead of base::PlatformFile abstractions? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:421: static scoped_ptr<net::test_server::HttpResponse> CorruptDbRequestHandler( Nit: Capitalize DB https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:433: // Remove the query string if present. Can you use a GURL to make this parsing easier? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:442: VLOG(0) << "Requested to currupt IndexedDB: " << request_query; Typo: corrupt https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:455: } else { No need for else block since if() early-exits. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:456: // A request for a test resource Do we need to handle this? Can't the registered handler's prefix be something only the test's XHR hits? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_factory.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_factory.cc:246: IndexedDBBackingStore::RecordCorruptionInfo( Doesn't this write the file into the directory... https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_factory.cc:249: if (!IndexedDBBackingStore::DestroyBackingStore(path_base, saved_origin_url) ... which this deletes? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_factory.h (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_factory.h:52: void HandleBackingStoreFailure(const GURL& origin_url); Maybe we should rename this to ...IOError ? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/l... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/l... content/browser/indexed_db/leveldb/leveldb_database.cc:464: void LevelDBDatabase::CompactAll() { db_->CompactRange(NULL, NULL); } Given https://code.google.com/p/leveldb/issues/detail?id=221 is one call enough? https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... File content/test/data/indexeddb/common.js (right): https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/common.js:151: if (typeof String.prototype.startsWith != 'function') { Maybe add a comment that this is a (partial) polyfill for an ES6 method. (Modifying prototypes of built-ins is frowned upon otherwise) https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/common.js:151: if (typeof String.prototype.startsWith != 'function') { Nit: !== https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/common.js:153: return this.indexOf(str) == 0; Nit: === https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... File content/test/data/indexeddb/corrupted_open_db_detection.html (right): https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:26: window.db = event.target.result; We should be consistent and either use 'window.XXX' everywhere (and have no top-level vars) or have a top-level 'var db' and drop the 'window.' https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:33: for (i = 0; i < len; i++) { Nit: pre-increment https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:37: for (i = 0; i < numObjectsWrittenToDb; i++) { Nit: pre-increment https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:46: numTransactions++; Nit: pre-increment https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:47: if (numTransactions == numObjectsWrittenToDb) { Nit: === https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:56: else { nit: } and else on same line https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:65: else { nit: } and else on same line https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:99: if (xmlhttp.readyState == 4) { Nit: === (and below)
https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:50: return ComputeFileName(origin_url).Append("corruption_info.json"); On 2014/03/26 18:16:58, jsbell wrote: > Doesn't this put the file in the directory, which will end up being deleted? No, we call DestroyDB when the db is determined to be corrupt. This LevelDB function only deletes the LevelDB files (*.ldb, etc.). It will call env::DeleteDir, but that will fail if other files (like this one) are present. Had that been the case then the corrupted_open_db_recovery.html test would fail. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:111: // could be caused by data corruption. A macro is used instead of an On 2014/03/26 18:16:58, jsbell wrote: > Maybe add to the comment that anything that could be caused by data corruption > should really be handled, i.e. we should eliminate all uses of > REPORT_ERROR_UNTESTED over time. Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:541: // Assumes caller has already closed the backing store. On 2014/03/26 18:16:58, jsbell wrote: > Move this comment to the header file? Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:576: if (file_size == base::ReadPlatformFile(file, 0, &bytes[0], file_size)) { On 2014/03/26 18:16:58, jsbell wrote: > &bytes[0] is undefined behavior if the vector length is 0. You should probably > guard against 0-length files anyway (i.e. no need to open them) Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:580: if (val) { On 2014/03/26 18:16:58, jsbell wrote: > Add: && val->GetType() == base::Value::TYPE_DICTIONARY Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:597: const IndexedDBDatabaseError& dberror) { On 2014/03/26 18:16:58, jsbell wrote: > Since this only serializes the message, and since ReadCorruptionInfo only > returns the message, what do you think about having this method only take a > string rather than an error object? OK. Either way those two functions should be symmetrical, and they are not. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:616: if (file) { On 2014/03/26 18:16:58, jsbell wrote: > Early exit instead, which would let you eliminate 'success' ? Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:684: INDEXED_DB_BACKING_STORE_OPEN_FAILED_IO_ERROR_CHECKING_SCHEMA, On 2014/03/26 18:16:58, jsbell wrote: > Add a new histogram value? Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_backing_store.h (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.h:74: virtual void Flush(); On 2014/03/26 18:16:58, jsbell wrote: > Can you add a comment to explain what this does? It's not a flush in the > traditional sense (i.e. everything should already be on disk). > > Also, if it's for testing only, consider making it protected (if e.g. it's > called via a subclass) and/or rename with '..ForTesting' Good point - it's not a flush as I originally thought. It is for a test so I'll make it private. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:366: static void FlushIndexedDBDatabase(scoped_refptr<IndexedDBContextImpl> context, On 2014/03/26 18:16:58, jsbell wrote: > 'Database' or 'BackingStore' ? Renamed to 'BackingStore' https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:377: backing_store->Flush(); On 2014/03/26 18:16:58, jsbell wrote: > All databases in an origin share a backing store, so this is redundant. > Intentional? Paranoid? (add comment to explain?) Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:382: static void CorruptIndexedDBDatabase( On 2014/03/26 18:16:58, jsbell wrote: > Add comment about why it's async? Actually it's effectively sync, but I didn't see a better alternative. Is there one? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:397: int64 size(0); On 2014/03/26 18:16:58, jsbell wrote: > size_t? Not according to base/file_util.h https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:402: FILE* f = fopen(idb_file.value().c_str(), "w"); On 2014/03/26 18:16:58, jsbell wrote: > Why FILE/fopen/fwrite instead of base::PlatformFile abstractions? I think I was unaware way back when I wrote this. Fixed. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:421: static scoped_ptr<net::test_server::HttpResponse> CorruptDbRequestHandler( On 2014/03/26 18:16:58, jsbell wrote: > Nit: Capitalize DB Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:433: // Remove the query string if present. On 2014/03/26 18:16:58, jsbell wrote: > Can you use a GURL to make this parsing easier? No, this is only the resource/query part of the URL - already processed. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:442: VLOG(0) << "Requested to currupt IndexedDB: " << request_query; On 2014/03/26 18:16:58, jsbell wrote: > Typo: corrupt Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:455: } else { On 2014/03/26 18:16:58, jsbell wrote: > No need for else block since if() early-exits. Done. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:456: // A request for a test resource On 2014/03/26 18:16:58, jsbell wrote: > Do we need to handle this? Can't the registered handler's prefix be something > only the test's XHR hits? From what I can tell this BasicHttpResponse object is used both to serve up the tests themselves (i.e. corrupted_open_db_detection.html) and handle the XHR call. I bet there is a cleaner more general way to do this, but the solution isn't obvious. I'd hoped to clean this up a bit, but was pressed for time. My goal is to improve this a bit and then to refactor our other PRE_, PRE_PRE_ tests to use XHR's. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_factory.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_factory.cc:249: if (!IndexedDBBackingStore::DestroyBackingStore(path_base, saved_origin_url) On 2014/03/26 18:16:58, jsbell wrote: > ... which this deletes? No, see second reply in this review. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_factory.h (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_factory.h:52: void HandleBackingStoreFailure(const GURL& origin_url); On 2014/03/26 18:16:58, jsbell wrote: > Maybe we should rename this to ...IOError ? Well a full disk is an IOError, but that won't result in this function being called. I think a Failure is a subset of IOError. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/l... File content/browser/indexed_db/leveldb/leveldb_database.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/l... content/browser/indexed_db/leveldb/leveldb_database.cc:464: void LevelDBDatabase::CompactAll() { db_->CompactRange(NULL, NULL); } On 2014/03/26 18:16:58, jsbell wrote: > Given https://code.google.com/p/leveldb/issues/detail?id=221 is one call enough? Yep, testing shows it to be sufficient. https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... File content/test/data/indexeddb/common.js (right): https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/common.js:151: if (typeof String.prototype.startsWith != 'function') { On 2014/03/26 18:16:58, jsbell wrote: > Maybe add a comment that this is a (partial) polyfill for an ES6 method. > > (Modifying prototypes of built-ins is frowned upon otherwise) Really? I'll look into that. I can redo this to not modify String if you want. https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... File content/test/data/indexeddb/corrupted_open_db_detection.html (right): https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:26: window.db = event.target.result; On 2014/03/26 18:16:58, jsbell wrote: > We should be consistent and either use 'window.XXX' everywhere (and have no > top-level vars) or have a top-level 'var db' and drop the 'window.' Done. https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:33: for (i = 0; i < len; i++) { On 2014/03/26 18:16:58, jsbell wrote: > Nit: pre-increment Done. https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:37: for (i = 0; i < numObjectsWrittenToDb; i++) { On 2014/03/26 18:16:58, jsbell wrote: > Nit: pre-increment Done. https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:46: numTransactions++; On 2014/03/26 18:16:58, jsbell wrote: > Nit: pre-increment Done, but just curious why? https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:47: if (numTransactions == numObjectsWrittenToDb) { On 2014/03/26 18:16:58, jsbell wrote: > Nit: === Done, but just curious why? https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:56: else { On 2014/03/26 18:16:58, jsbell wrote: > nit: } and else on same line Done. https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:65: else { On 2014/03/26 18:16:58, jsbell wrote: > nit: } and else on same line Done. https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:99: if (xmlhttp.readyState == 4) { On 2014/03/26 18:16:58, jsbell wrote: > Nit: === (and below) Done.
https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_backing_store.cc:597: const IndexedDBDatabaseError& dberror) { On 2014/03/26 21:40:36, cmumford wrote: > On 2014/03/26 18:16:58, jsbell wrote: > > Since this only serializes the message, and since ReadCorruptionInfo only > > returns the message, what do you think about having this method only take a > > string rather than an error object? > > OK. Either way those two functions should be symmetrical, and they are not. Yep - having it store all the error fields would also be fine. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:382: static void CorruptIndexedDBDatabase( On 2014/03/26 21:40:36, cmumford wrote: > On 2014/03/26 18:16:58, jsbell wrote: > > Add comment about why it's async? > > Actually it's effectively sync, but I didn't see a better alternative. Is there > one? Oops, yeah, I shouldn't have said "async". It wasn't obvious (until I dug further) that it was intended to be run "sync" but on another thread. On more reflection... yeah, no need for a comment. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:397: int64 size(0); On 2014/03/26 21:40:36, cmumford wrote: > On 2014/03/26 18:16:58, jsbell wrote: > > size_t? > > Not according to base/file_util.h Sorry. I swear I saw it as size_t. :( https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_browsertest.cc:456: // A request for a test resource On 2014/03/26 21:40:36, cmumford wrote: > On 2014/03/26 18:16:58, jsbell wrote: > > Do we need to handle this? Can't the registered handler's prefix be something > > only the test's XHR hits? > > From what I can tell this BasicHttpResponse object is used both to serve up the > tests themselves (i.e. corrupted_open_db_detection.html) and handle the XHR > call. I bet there is a cleaner more general way to do this, but the solution > isn't obvious. Again, sorry - I thought the RegisterRequestHandler call itself took a prefix path and was just registering to handle on a subset of requests. https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_factory.cc (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_factory.cc:249: if (!IndexedDBBackingStore::DestroyBackingStore(path_base, saved_origin_url) On 2014/03/26 21:40:36, cmumford wrote: > On 2014/03/26 18:16:58, jsbell wrote: > > ... which this deletes? > > No, see second reply in this review. Got it, thanks. Maybe add a comment here that the directory will still exist since DestroyBackingStore doesn't remove non-LDB files? https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... File content/browser/indexed_db/indexed_db_factory.h (right): https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i... content/browser/indexed_db/indexed_db_factory.h:52: void HandleBackingStoreFailure(const GURL& origin_url); On 2014/03/26 21:40:36, cmumford wrote: > On 2014/03/26 18:16:58, jsbell wrote: > > Maybe we should rename this to ...IOError ? > > Well a full disk is an IOError, but that won't result in this function being > called. I think a Failure is a subset of IOError. SGTM. https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... File content/test/data/indexeddb/common.js (right): https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/common.js:151: if (typeof String.prototype.startsWith != 'function') { On 2014/03/26 21:40:36, cmumford wrote: > On 2014/03/26 18:16:58, jsbell wrote: > > Maybe add a comment that this is a (partial) polyfill for an ES6 method. > > > > (Modifying prototypes of built-ins is frowned upon otherwise) > > Really? I'll look into that. I can redo this to not modify String if you want. These days it's "bad form" on the Web to extend built-in prototypes, since that means the language itself can't add such a method. E.g. if "gQuery" added String.prototype.startsWith() but computed the result by saying (new RegExp('^' + s)).test(this) then a sensible startsWith() might "break the web". Object.prototype is the worst case. But since this basically matches the ES6 semantics, it's fine. :) https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... File content/test/data/indexeddb/corrupted_open_db_detection.html (right): https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:46: numTransactions++; On 2014/03/26 21:40:36, cmumford wrote: > On 2014/03/26 18:16:58, jsbell wrote: > > Nit: pre-increment > > Done, but just curious why? It's just Chromium/Blink coding convention. Post-increment has more complex behavior, so only use it when you really mean it. In cases where it doesn't matter, prefer to pre-increment so that post-increment stands out (and train your fingers to type pre-increment-style) https://codereview.chromium.org/197333009/diff/1/content/test/data/indexeddb/... content/test/data/indexeddb/corrupted_open_db_detection.html:47: if (numTransactions == numObjectsWrittenToDb) { On 2014/03/26 21:40:36, cmumford wrote: > On 2014/03/26 18:16:58, jsbell wrote: > > Nit: === > > Done, but just curious why? double-equals is really busted in JS due to type coercion. "0" == 0, undefined == null, 1 == true, [] == 0, and so on. So prefer to use === and !==.
Josh: I added a comment to IndexedDBFactory::HandleBackingStoreCorruption. I think that was your only request from your last set of comments. Let me know if I missed anything.
lgtm
https://codereview.chromium.org/197333009/diff/140001/content/browser/indexed... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/197333009/diff/140001/content/browser/indexed... content/browser/indexed_db/indexed_db_backing_store.cc:468: INDEXED_DB_BACKING_STORE_OPEN_FAILED_PRIOR_CORRUPTION, er, not lgtm This needs to be before _MAX, and histograms.xml needs to be updated
rubberstamp lgtm
On 2014/03/27 16:04:49, jsbell wrote: > https://codereview.chromium.org/197333009/diff/140001/content/browser/indexed... > File content/browser/indexed_db/indexed_db_backing_store.cc (right): > > https://codereview.chromium.org/197333009/diff/140001/content/browser/indexed... > content/browser/indexed_db/indexed_db_backing_store.cc:468: > INDEXED_DB_BACKING_STORE_OPEN_FAILED_PRIOR_CORRUPTION, > er, not lgtm > > This needs to be before _MAX, and histograms.xml needs to be updated Crap, how did I miss that? thx!
Got this error with last git cl upload: Not uploading the base file for tools/metrics/histograms/histograms.xml because it's too large.
The diff is fine, just the side-by-side failed. lgtm isherman@ - histograms review?
histograms lgtm
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cmumford@chromium.org/197333009/160001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
content/browser/indexed_db/indexed_db_backing_store.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/browser/indexed_db/indexed_db_backing_store.cc
Hunk #1 FAILED at 5.
Hunk #2 succeeded at 41 (offset -1 lines).
Hunk #3 succeeded at 88 (offset -1 lines).
Hunk #4 succeeded at 102 (offset -1 lines).
Hunk #5 succeeded at 298 (offset -1 lines).
Hunk #6 succeeded at 325 (offset -1 lines).
Hunk #7 succeeded at 351 (offset -1 lines).
Hunk #8 succeeded at 368 (offset -1 lines).
Hunk #9 succeeded at 459 (offset -1 lines).
Hunk #10 succeeded at 536 (offset -1 lines).
Hunk #11 succeeded at 674 (offset -1 lines).
Hunk #12 succeeded at 806 (offset -1 lines).
Hunk #13 succeeded at 823 (offset -1 lines).
Hunk #14 succeeded at 835 (offset -1 lines).
Hunk #15 succeeded at 849 (offset -1 lines).
Hunk #16 succeeded at 863 (offset -1 lines).
Hunk #17 succeeded at 878 (offset -1 lines).
Hunk #18 succeeded at 921 (offset -1 lines).
Hunk #19 succeeded at 977 (offset -1 lines).
Hunk #20 succeeded at 1025 (offset -1 lines).
Hunk #21 succeeded at 1039 (offset -1 lines).
Hunk #22 succeeded at 1047 (offset -1 lines).
Hunk #23 succeeded at 1063 (offset -1 lines).
Hunk #24 succeeded at 1078 (offset -1 lines).
Hunk #25 succeeded at 1088 (offset -1 lines).
Hunk #26 succeeded at 1098 (offset -1 lines).
Hunk #27 succeeded at 1117 (offset -1 lines).
Hunk #28 succeeded at 1126 (offset -1 lines).
Hunk #29 succeeded at 1142 (offset -1 lines).
Hunk #30 succeeded at 1175 (offset -1 lines).
Hunk #31 succeeded at 1258 (offset -1 lines).
Hunk #32 succeeded at 1310 (offset -1 lines).
Hunk #33 succeeded at 1339 (offset -1 lines).
Hunk #34 succeeded at 1455 (offset -1 lines).
Hunk #35 succeeded at 1486 (offset -1 lines).
Hunk #36 succeeded at 1548 (offset -1 lines).
Hunk #37 succeeded at 1611 (offset -1 lines).
Hunk #38 succeeded at 1625 (offset -1 lines).
Hunk #39 succeeded at 1660 (offset -1 lines).
Hunk #40 succeeded at 1683 (offset -1 lines).
Hunk #41 succeeded at 1828 (offset -1 lines).
Hunk #42 succeeded at 1872 (offset -1 lines).
Hunk #43 succeeded at 1918 (offset -1 lines).
Hunk #44 succeeded at 1957 (offset -1 lines).
Hunk #45 succeeded at 2206 (offset -1 lines).
Hunk #46 succeeded at 2215 (offset -1 lines).
Hunk #47 succeeded at 2263 (offset -1 lines).
Hunk #48 succeeded at 2272 (offset -1 lines).
Hunk #49 succeeded at 2337 (offset -1 lines).
Hunk #50 succeeded at 2347 (offset -1 lines).
Hunk #51 succeeded at 2365 (offset -1 lines).
Hunk #52 succeeded at 2373 (offset -1 lines).
Hunk #53 succeeded at 2445 (offset -1 lines).
Hunk #54 succeeded at 2455 (offset -1 lines).
Hunk #55 succeeded at 2472 (offset -1 lines).
Hunk #56 succeeded at 2480 (offset -1 lines).
Hunk #57 succeeded at 2758 (offset -1 lines).
1 out of 57 hunks FAILED -- saving rejects to file
content/browser/indexed_db/indexed_db_backing_store.cc.rej
Patch: content/browser/indexed_db/indexed_db_backing_store.cc
Index: content/browser/indexed_db/indexed_db_backing_store.cc
diff --git a/content/browser/indexed_db/indexed_db_backing_store.cc
b/content/browser/indexed_db/indexed_db_backing_store.cc
index
2aebb3ea0ae2ab8b4cc7bcfbe92bfc0e67d0939e..43f85128aff1bb5df8a3eb97e562ba9b11a6fdf1
100644
--- a/content/browser/indexed_db/indexed_db_backing_store.cc
+++ b/content/browser/indexed_db/indexed_db_backing_store.cc
@@ -5,11 +5,15 @@
#include "content/browser/indexed_db/indexed_db_backing_store.h"
#include "base/file_util.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
#include "base/logging.h"
#include "base/metrics/histogram.h"
+#include "base/platform_file.h"
#include "base/strings/string_piece.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
+#include "content/browser/indexed_db/indexed_db_database_error.h"
#include "content/browser/indexed_db/indexed_db_leveldb_coding.h"
#include "content/browser/indexed_db/indexed_db_metadata.h"
#include "content/browser/indexed_db/indexed_db_tracing.h"
@@ -42,6 +46,10 @@ static base::FilePath ComputeFileName(const GURL& origin_url)
{
.AddExtension(FILE_PATH_LITERAL(".indexeddb.leveldb"));
}
+static base::FilePath ComputeCorruptionFileName(const GURL& origin_url) {
+ return ComputeFileName(origin_url).Append("corruption_info.json");
+}
+
} // namespace
static const int64 kKeyGeneratorInitialNumber =
@@ -85,13 +93,12 @@ static void RecordInternalError(const char* type,
->Add(location);
}
-// Use to signal conditions that usually indicate developer error, but
-// could be caused by data corruption. A macro is used instead of an
-// inline function so that the assert and log report the line number.
+// Use to signal conditions caused by data corruption.
+// A macro is used instead of an inline function so that the assert and log
+// report the line number.
#define REPORT_ERROR(type, location) \
do { \
LOG(ERROR) << "IndexedDB " type " Error: " #location; \
- NOTREACHED(); \
RecordInternalError(type, location); \
} while (0)
@@ -100,6 +107,25 @@ static void RecordInternalError(const char* type,
REPORT_ERROR("Consistency", location)
#define INTERNAL_WRITE_ERROR(location) REPORT_ERROR("Write", location)
+// Use to signal conditions that usually indicate developer error, but
+// could be caused by data corruption. A macro is used instead of an
+// inline function so that the assert and log report the line number.
+// TODO: Improve test coverage so that all error conditions are "tested" and
+// then delete this macro.
+#define REPORT_ERROR_UNTESTED(type, location) \
+ do { \
+ LOG(ERROR) << "IndexedDB " type " Error: " #location; \
+ NOTREACHED(); \
+ RecordInternalError(type, location); \
+ } while (0)
+
+#define INTERNAL_READ_ERROR_UNTESTED(location) \
+ REPORT_ERROR_UNTESTED("Read", location)
+#define INTERNAL_CONSISTENCY_ERROR_UNTESTED(location) \
+ REPORT_ERROR_UNTESTED("Consistency", location)
+#define INTERNAL_WRITE_ERROR_UNTESTED(location) \
+ REPORT_ERROR_UNTESTED("Write", location)
+
static void PutBool(LevelDBTransaction* transaction,
const StringPiece& key,
bool value) {
@@ -277,7 +303,7 @@ WARN_UNUSED_RESULT static bool SetUpMetadata(
leveldb::Status s =
GetInt(transaction.get(), schema_version_key, &db_schema_version,
&found);
if (!s.ok()) {
- INTERNAL_READ_ERROR(SET_UP_METADATA);
+ INTERNAL_READ_ERROR_UNTESTED(SET_UP_METADATA);
return false;
}
if (!found) {
@@ -304,11 +330,11 @@ WARN_UNUSED_RESULT static bool SetUpMetadata(
found = false;
s = GetInt(transaction.get(), it->Key(), &database_id, &found);
if (!s.ok()) {
- INTERNAL_READ_ERROR(SET_UP_METADATA);
+ INTERNAL_READ_ERROR_UNTESTED(SET_UP_METADATA);
return false;
}
if (!found) {
- INTERNAL_CONSISTENCY_ERROR(SET_UP_METADATA);
+ INTERNAL_CONSISTENCY_ERROR_UNTESTED(SET_UP_METADATA);
return false;
}
std::string int_version_key = DatabaseMetaDataKey::Encode(
@@ -330,11 +356,11 @@ WARN_UNUSED_RESULT static bool SetUpMetadata(
found = false;
s = GetInt(transaction.get(), data_version_key, &db_data_version, &found);
if (!s.ok()) {
- INTERNAL_READ_ERROR(SET_UP_METADATA);
+ INTERNAL_READ_ERROR_UNTESTED(SET_UP_METADATA);
return false;
}
if (!found) {
- INTERNAL_CONSISTENCY_ERROR(SET_UP_METADATA);
+ INTERNAL_CONSISTENCY_ERROR_UNTESTED(SET_UP_METADATA);
return false;
}
if (db_data_version < latest_known_data_version) {
@@ -347,7 +373,7 @@ WARN_UNUSED_RESULT static bool SetUpMetadata(
s = transaction->Commit();
if (!s.ok()) {
- INTERNAL_WRITE_ERROR(SET_UP_METADATA);
+ INTERNAL_WRITE_ERROR_UNTESTED(SET_UP_METADATA);
return false;
}
return true;
@@ -438,6 +464,7 @@ enum IndexedDBBackingStoreOpenResult {
INDEXED_DB_BACKING_STORE_OPEN_DISK_FULL_DEPRECATED,
INDEXED_DB_BACKING_STORE_OPEN_ORIGIN_TOO_LONG,
INDEXED_DB_BACKING_STORE_OPEN_NO_RECOVERY,
+ INDEXED_DB_BACKING_STORE_OPEN_FAILED_PRIOR_CORRUPTION,
INDEXED_DB_BACKING_STORE_OPEN_MAX,
};
@@ -514,6 +541,91 @@ static bool IsPathTooLong(const base::FilePath&
leveldb_dir) {
return false;
}
+leveldb::Status IndexedDBBackingStore::DestroyBackingStore(
+ const base::FilePath& path_base,
+ const GURL& origin_url) {
+ const base::FilePath file_path =
+ path_base.Append(ComputeFileName(origin_url));
+ DefaultLevelDBFactory leveldb_factory;
+ return leveldb_factory.DestroyLevelDB(file_path);
+}
+
+bool IndexedDBBackingStore::ReadCorruptionInfo(const base::FilePath& path_base,
+ const GURL& origin_url,
+ std::string& message) {
+
+ const base::FilePath info_path =
+ path_base.Append(ComputeCorruptionFileName(origin_url));
+
+ if (IsPathTooLong(info_path))
+ return false;
+
+ const int64 max_json_len = 4096;
+ int64 file_size(0);
+ if (!GetFileSize(info_path, &file_size) || file_size > max_json_len)
+ return false;
+ if (!file_size) {
+ NOTREACHED();
+ return false;
+ }
+
+ bool created(false);
+ base::PlatformFileError error(base::PLATFORM_FILE_OK);
+ base::PlatformFile file = base::CreatePlatformFile(
+ info_path,
+ base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ,
+ &created,
+ &error);
+ bool success = false;
+ if (file) {
+ std::vector<char> bytes(file_size);
+ if (file_size == base::ReadPlatformFile(file, 0, &bytes[…
(message too large)
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cmumford@chromium.org/197333009/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_clang_dbg
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cmumford@chromium.org/197333009/190001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_x64_rel for step(s) base_unittests, chrome_elf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cmumford@chromium.org/197333009/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cmumford@chromium.org/197333009/230001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Message was sent while issue was closed.
Committed patchset #8 manually as r260147 (presubmit successful).
Message was sent while issue was closed.
FYI, reverted due to Mac ASAN failures http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%20Tests%20%28...
Message was sent while issue was closed.
The previous bug (caught by Mac ASan), was with fwrite. I didn't realize that it would read size*nmemb bytes from ptr and wrote those into the file - I (incorrectly) thought it would write the same data nmemb tiles - doh!
Message was sent while issue was closed.
lgtm with nit we should send this through the bots again https://codereview.chromium.org/197333009/diff/250001/content/browser/indexed... File content/browser/indexed_db/indexed_db_browsertest.cc (right): https://codereview.chromium.org/197333009/diff/250001/content/browser/indexed... content/browser/indexed_db/indexed_db_browsertest.cc:412: // Was opened truncated, truncate back to the original file size to fill Nit: Can you tweak this to be "... opened truncated, expand back to the original file size and fill ..." ?
The bots missed this bug the first time. I did manually start the mac_asan test on this change. This one other builder failed (XP), but I'm not sure which bot this is. http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29
Josh: I manually ran these bots: win_x64_rel, linux_rel, android_dbg_triggered_tests, android_dbg, android_aosp, and mac_asan. Two failed: mac_asan and win_x64_rel. On mac_asan the new test (IndexedDBBrowserTest.CorruptedOpenDatabase - the one what failed previously on this bot) passed w/o error. The win_x64_rel all appear to be unrelated. Can you commit this on my behalf if you are satisfied?
Message was sent while issue was closed.
Committed patchset #11 manually as r260697 (presubmit successful). |
