Bug 103960

Summary: IndexedDB: Abort transactions because of leveldb errors part 3
Product: WebKit Reporter: David Grogan <dgrogan>
Component: New BugsAssignee: David Grogan <dgrogan>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, jsbell, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 103782    
Bug Blocks: 103964    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

David Grogan
Reported 2012-12-03 19:14:59 PST
IndexedDB: Abort transactions because of leveldb errors part 3
Attachments
Patch (7.84 KB, patch)
2012-12-03 19:18 PST, David Grogan
no flags
Patch (7.84 KB, patch)
2012-12-04 16:52 PST, David Grogan
no flags
Patch (8.00 KB, patch)
2012-12-04 19:22 PST, David Grogan
no flags
Patch for landing (8.00 KB, patch)
2012-12-05 15:41 PST, David Grogan
no flags
David Grogan
Comment 1 2012-12-03 19:18:02 PST
David Grogan
Comment 2 2012-12-04 16:52:46 PST
David Grogan
Comment 3 2012-12-04 16:53:34 PST
Josh or Alec, could you take a look?
Joshua Bell
Comment 4 2012-12-04 17:26:48 PST
Comment on attachment 177606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177606&action=review > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:252 > + int64_t databaseId, int64_t objectStoreId, int64_t indexId, bool& canAddKeys, Just remove wrapping? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:253 > + const IDBKey* primaryKey = 0, String* errorMessage = 0) Add WARN_UNUSED_RETURN > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:282 > + int64_t databaseId, int64_t objectStoreId, int64_t indexId, Just remove wrapping? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:283 > + const IDBKey* indexKey, const IDBKey* primaryKey, bool& allowed) const Add WARN_UNUSED_RETURN > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:306 > +static bool makeIndexWriters(PassRefPtr<IDBTransactionBackendImpl> transaction, IDBObjectStoreBackendImpl* objectStore, PassRefPtr<IDBKey> primaryKey, bool keyWasGenerated, const Vector<int64_t>& indexIds, const Vector<IDBObjectStoreBackendInterface::IndexKeys>& indexKeys, Vector<OwnPtr<IndexWriter> >* indexWriters, String* errorMessage, bool& completed) Add WARN_UNUSED_RETURN > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:329 > + bool noLevelDBErrors = indexWriter->verifyIndexKeys(*objectStore->backingStore(), I'd avoid using the "leveldb" name here. um... backingStoreSuccess ? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:330 > + transaction->backingStoreTransaction(), objectStore->databaseId(), Just remove wrapping? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:368 > + bool completed = false; Maybe rename to consistencySuccess ? Or something that's more clearly the inverse of constraintError ? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:371 > + transaction->abort(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error in makeIndexWriters")); I wouldn't name the internal method explicitly here, just as it's one more thing to keep in sync. Maybe "Backing store error updating index keys." ? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:448 > + bool completed = false; Rename to constraintError? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:449 > + bool noLevelDBErrors = makeIndexWriters(transaction, objectStore.get(), key, keyWasGenerated, *indexIds, *indexKeys, &indexWriters, &errorMessage, completed); Ditto re: "leveldb" > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:451 > + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error in makeIndexWriters.")); Same as above.
David Grogan
Comment 5 2012-12-04 19:20:37 PST
Comment on attachment 177606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177606&action=review >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:252 >> + int64_t databaseId, int64_t objectStoreId, int64_t indexId, bool& canAddKeys, > > Just remove wrapping? check-webkit-style complained about the number of spaces. Do you know how to appease it? >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:253 >> + const IDBKey* primaryKey = 0, String* errorMessage = 0) > > Add WARN_UNUSED_RETURN Done >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:283 >> + const IDBKey* indexKey, const IDBKey* primaryKey, bool& allowed) const > > Add WARN_UNUSED_RETURN Done. >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:306 >> +static bool makeIndexWriters(PassRefPtr<IDBTransactionBackendImpl> transaction, IDBObjectStoreBackendImpl* objectStore, PassRefPtr<IDBKey> primaryKey, bool keyWasGenerated, const Vector<int64_t>& indexIds, const Vector<IDBObjectStoreBackendInterface::IndexKeys>& indexKeys, Vector<OwnPtr<IndexWriter> >* indexWriters, String* errorMessage, bool& completed) > > Add WARN_UNUSED_RETURN Done. Apparently it needs to go before the function here, maybe because of the static? >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:329 >> + bool noLevelDBErrors = indexWriter->verifyIndexKeys(*objectStore->backingStore(), > > I'd avoid using the "leveldb" name here. um... backingStoreSuccess ? Better, changed. >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:368 >> + bool completed = false; > > Maybe rename to consistencySuccess ? Or something that's more clearly the inverse of constraintError ? I went with obeysConstraints >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:371 >> + transaction->abort(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error in makeIndexWriters")); > > I wouldn't name the internal method explicitly here, just as it's one more thing to keep in sync. Maybe "Backing store error updating index keys." ? Done. >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:448 >> + bool completed = false; > > Rename to constraintError? went with obeysConstraints >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:449 >> + bool noLevelDBErrors = makeIndexWriters(transaction, objectStore.get(), key, keyWasGenerated, *indexIds, *indexKeys, &indexWriters, &errorMessage, completed); > > Ditto re: "leveldb" done. >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:451 >> + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error in makeIndexWriters.")); > > Same as above. Done.
David Grogan
Comment 6 2012-12-04 19:22:04 PST
David Grogan
Comment 7 2012-12-05 10:56:31 PST
Josh, I'm assuming the lack of lgtm means you want to review this again before I send it off to Tony. (Is that a generally accurate interpretation?)
Joshua Bell
Comment 8 2012-12-05 11:04:46 PST
(In reply to comment #7) > Josh, I'm assuming the lack of lgtm means you want to review this again before I send it off to Tony. (Is that a generally accurate interpretation?) Generally yeah but I just forgot. LGTM!
David Grogan
Comment 9 2012-12-05 11:05:44 PST
Tony, could you review this?
WebKit Review Bot
Comment 10 2012-12-05 12:51:06 PST
Comment on attachment 177651 [details] Patch Rejecting attachment 177651 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: set 108 lines). Hunk #2 succeeded at 387 (offset 108 lines). Hunk #3 succeeded at 411 (offset 108 lines). Hunk #4 succeeded at 433 (offset 108 lines). Hunk #5 succeeded at 473 (offset 108 lines). Hunk #6 FAILED at 445. 1 out of 6 hunks FAILED -- saving rejects to file Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tony Chang']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/15173025
David Grogan
Comment 11 2012-12-05 15:41:58 PST
Created attachment 177849 [details] Patch for landing
WebKit Review Bot
Comment 12 2012-12-05 16:09:50 PST
Comment on attachment 177849 [details] Patch for landing Clearing flags on attachment: 177849 Committed r136775: <http://trac.webkit.org/changeset/136775>
WebKit Review Bot
Comment 13 2012-12-05 16:09:53 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.