Bug 103763 - IndexedDB: Correct database version after aborted upgrade
Summary: IndexedDB: Correct database version after aborted upgrade
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
: 102412 (view as bug list)
Depends on:
Blocks: 103920
  Show dependency treegraph
 
Reported: 2012-11-30 12:31 PST by Joshua Bell
Modified: 2012-12-04 12:26 PST (History)
4 users (show)

See Also:


Attachments
Patch (13.99 KB, patch)
2012-11-30 12:37 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (22.14 KB, patch)
2012-11-30 17:17 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (21.59 KB, patch)
2012-12-03 13:46 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-11-30 12:31:36 PST
IndexedDB: Correct database version after aborted upgrade
Comment 1 Joshua Bell 2012-11-30 12:37:52 PST
Created attachment 177012 [details]
Patch
Comment 2 Joshua Bell 2012-11-30 12:38:19 PST
dgrogan@ - can you take a look?
Comment 3 David Grogan 2012-11-30 14:03:25 PST
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?
Comment 4 Joshua Bell 2012-11-30 15:00:28 PST
*** Bug 102412 has been marked as a duplicate of this bug. ***
Comment 5 Joshua Bell 2012-11-30 15:40:01 PST
(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.)
Comment 6 Joshua Bell 2012-11-30 17:17:06 PST
Created attachment 177059 [details]
Patch
Comment 7 Joshua Bell 2012-11-30 17:17:19 PST
dgrogan@ - can you take another look?
Comment 8 David Grogan 2012-12-03 13:17:46 PST
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 9 Joshua Bell 2012-12-03 13:41:33 PST
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.
Comment 10 Joshua Bell 2012-12-03 13:46:03 PST
Created attachment 177320 [details]
Patch
Comment 11 Joshua Bell 2012-12-03 13:56:50 PST
tony@ - r?
Comment 12 WebKit Review Bot 2012-12-03 23:17:59 PST
Comment on attachment 177320 [details]
Patch

Clearing flags on attachment: 177320

Committed r136475: <http://trac.webkit.org/changeset/136475>
Comment 13 WebKit Review Bot 2012-12-03 23:18:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Joshua Bell 2012-12-04 10:53:37 PST
*** Bug 100537 has been marked as a duplicate of this bug. ***
Comment 15 David Grogan 2012-12-04 12:26:17 PST
(In reply to comment #14)
> *** Bug 100537 has been marked as a duplicate of this bug. ***

I think you got the wrong bug