IndexedDB: Correct database version after aborted upgrade
Created attachment 177012 [details] Patch
dgrogan@ - can you take a look?
Comment on attachment 177012 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177012&action=review LGTM It's a little weird that the "previous" data for objectstores is stored in IDBTransaction but the "previous" data for the database is stored in IDBDatabase. But this does seem like the best option. > Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:121 > + idbDatabase = IDBDatabase::create(scriptExecutionContext(), backend, m_databaseCallbacks, IDBDatabaseMetadata::DefaultIntVersion); The default is ok here because this IDBDatabase object will never have a versionchange transaction, correct? > LayoutTests/storage/indexeddb/mozilla/versionchange-abort-expected.txt:16 > FIXME: Fails because of http://wkb.ug/102412 This line can be deleted and that bug duped against this one. > LayoutTests/storage/indexeddb/resources/unblocked-version-changes.js:39 > + evalAndLog("transaction.abort()"); This test would still pass without this line, correct?
*** Bug 102412 has been marked as a duplicate of this bug. ***
(In reply to comment #3) > (From update of attachment 177012 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177012&action=review > > LGTM > > It's a little weird that the "previous" data for objectstores is stored in IDBTransaction but the "previous" data for the database is stored in IDBDatabase. But this does seem like the best option. I agree that the transaction would be a better place for it. I'll see if I can make that work w/o too much plumbing. > > Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:121 > > + idbDatabase = IDBDatabase::create(scriptExecutionContext(), backend, m_databaseCallbacks, IDBDatabaseMetadata::DefaultIntVersion); > > The default is ok here because this IDBDatabase object will never have a versionchange transaction, correct? Correct. > > LayoutTests/storage/indexeddb/mozilla/versionchange-abort-expected.txt:16 > > FIXME: Fails because of http://wkb.ug/102412 > > This line can be deleted and that bug duped against this one. Thanks, updated in next patch upload. > > LayoutTests/storage/indexeddb/resources/unblocked-version-changes.js:39 > > + evalAndLog("transaction.abort()"); > > This test would still pass without this line, correct? No, it's the important bit of the test - that an aborted version change gets rolled back. The spec appears to read that closing the connection should have no effect on the version change itself (although the explicit close shouldn't be necessary with the abort.)
Created attachment 177059 [details] Patch
dgrogan@ - can you take another look?
Comment on attachment 177059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177059&action=review LGTM > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:98 > +void IDBDatabase::transactionFinished(IDBTransaction* transaction, bool completed) You don't need this anymore. > Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:87 > + IDBDatabaseMetadata metadata = databaseBackend->metadata(); We could eliminate this sync ipc call if the new metadata were passed in the upgradeneeded message, right? Could you add a fixme to that effect? > Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:119 > + // FIXME: Not safe to use backend here as it is only set if upgradeneeded didn't fire. This is due to the proxy not passing a duplicate This is fixed with the changes to WebIDBCallbacksImpl, right? > Source/WebCore/Modules/indexeddb/IDBTransaction.h:61 > + static PassRefPtr<IDBTransaction> create(ScriptExecutionContext*, int64_t, PassRefPtr<IDBTransactionBackendInterface>, IDBDatabase*, IDBOpenDBRequest*, const IDBDatabaseMetadata&); Could you label the new parameter oldMetadata or oldMetadataForRollback or something similar? Or just previousMetadata like m_previousMetadata.
Comment on attachment 177059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177059&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:98 >> +void IDBDatabase::transactionFinished(IDBTransaction* transaction, bool completed) > > You don't need this anymore. Hooray for another set of eyes! >> Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:87 >> + IDBDatabaseMetadata metadata = databaseBackend->metadata(); > > We could eliminate this sync ipc call if the new metadata were passed in the upgradeneeded message, right? Could you add a fixme to that effect? Done. >> Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:119 >> + // FIXME: Not safe to use backend here as it is only set if upgradeneeded didn't fire. This is due to the proxy not passing a duplicate > > This is fixed with the changes to WebIDBCallbacksImpl, right? Oops, yes. Removed. >> Source/WebCore/Modules/indexeddb/IDBTransaction.h:61 >> + static PassRefPtr<IDBTransaction> create(ScriptExecutionContext*, int64_t, PassRefPtr<IDBTransactionBackendInterface>, IDBDatabase*, IDBOpenDBRequest*, const IDBDatabaseMetadata&); > > Could you label the new parameter oldMetadata or oldMetadataForRollback or something similar? Or just previousMetadata like m_previousMetadata. Will do (just "previousMetadata" for consistency). I also pondered moving the storage into the IDBOpenDBRequest but keeping it in IDBTransaction for now, since that's where the object store rollback metadata lives at the moment.
Created attachment 177320 [details] Patch
tony@ - r?
Comment on attachment 177320 [details] Patch Clearing flags on attachment: 177320 Committed r136475: <http://trac.webkit.org/changeset/136475>
All reviewed patches have been landed. Closing bug.
*** Bug 100537 has been marked as a duplicate of this bug. ***
(In reply to comment #14) > *** Bug 100537 has been marked as a duplicate of this bug. *** I think you got the wrong bug