Bug 103763 – IndexedDB: Correct database version after aborted upgrade

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

IndexedDB: Correct database version after aborted upgrade

[Joshua Bell]

Created attachment 177012 [details]
Patch

[Joshua Bell]

dgrogan@ - can you take a look?

[David Grogan]

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?

[Joshua Bell]

*** Bug 102412 has been marked as a duplicate of this bug. ***

[Joshua Bell]

(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.)

[Joshua Bell]

Created attachment 177059 [details]
Patch

[Joshua Bell]

dgrogan@ - can you take another look?

[David Grogan]

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.

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 177320 [details]
Patch

[Joshua Bell]

tony@ - r?

[WebKit Review Bot]

Comment on attachment 177320 [details]
Patch

Clearing flags on attachment: 177320

Committed r136475: <http://trac.webkit.org/changeset/136475>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joshua Bell]

*** Bug 100537 has been marked as a duplicate of this bug. ***

[David Grogan]

(In reply to comment #14)
> *** Bug 100537 has been marked as a duplicate of this bug. ***

I think you got the wrong bug