IDBDatabase and IDBObjectStore metadata is not recovered correctly when the setVersion transactions aborts.

[Andrei Popescu]

IDBDatabase and IDBObjectStore metadata is not recovered correctly when the setVersion transactions aborts.

[Andrei Popescu]

Created attachment 69883 [details]
Patch

[Jeremy Orlow]

Comment on attachment 69883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69883&action=review

Looks pretty good!

> LayoutTests/storage/indexeddb/objectstore-basics.html:73
> +function createIndex() {

{ on next line

> LayoutTests/storage/indexeddb/resources/shared.js:86
> +    verifyAbortEvent(event);

Verify complete event.

> WebCore/storage/IDBDatabaseBackendImpl.cpp:193
> +                                   createCallbackTask(&IDBDatabaseBackendImpl::removeObjectStoreFromMap, database, objectStore))) {

This should add it, right?  Can you add testing for that?

> WebCore/storage/IDBDatabaseBackendImpl.cpp:256
> +    database->m_objectStores.remove(objectStore->name());

What does remove do if it doesn't exist?  If it doesn't assert, maybe add one manually?

> WebCore/storage/IDBDatabaseBackendImpl.cpp:261
> +    database->m_objectStores.set(objectStore->name(), objectStore);

similarly here

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:414
> +    objectStore->m_indexes.remove(index->name());

and here

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:419
> +    objectStore->m_indexes.set(index->name(), index);

and here

> WebCore/storage/IDBTransactionBackendImpl.cpp:70
> +        m_abortTaskQueue.prepend(abortTask);

Is this expensive?  If so, maybe you should process the abort task queue in reverse?

And maybe have a layout test that adds removes adds removes and then aborts?

Also maybe add a layout test that double adds then aborts...and double removes then aborts?

> WebCore/storage/IDBTransactionBackendImpl.h:50
> +    virtual bool scheduleTask(PassOwnPtr<ScriptExecutionContext::Task> task, PassOwnPtr<ScriptExecutionContext::Task> abortTaskn);

abortTask minus the n

[Andrei Popescu]

Thanks for the review!

(In reply to comment #2)
> (From update of attachment 69883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69883&action=review
> 
> Looks pretty good!
> 
> > LayoutTests/storage/indexeddb/objectstore-basics.html:73
> > +function createIndex() {
> 
> { on next line
> 

Fixed.

> > LayoutTests/storage/indexeddb/resources/shared.js:86
> > +    verifyAbortEvent(event);
> 
> Verify complete event.
>

Fixed,
 
> > WebCore/storage/IDBDatabaseBackendImpl.cpp:193
> > +                                   createCallbackTask(&IDBDatabaseBackendImpl::removeObjectStoreFromMap, database, objectStore))) {
> 
> This should add it, right?  Can you add testing for that?
>

Fixed.
 
> > WebCore/storage/IDBDatabaseBackendImpl.cpp:256
> > +    database->m_objectStores.remove(objectStore->name());
> 
> What does remove do if it doesn't exist?  If it doesn't assert, maybe add one manually?
>

Added asserts.
 
> 
> > WebCore/storage/IDBTransactionBackendImpl.cpp:70
> > +        m_abortTaskQueue.prepend(abortTask);
> 
> Is this expensive?  If so, maybe you should process the abort task queue in reverse?
>

It's not expensive.
 
> And maybe have a layout test that adds removes adds removes and then aborts?
> 
> Also maybe add a layout test that double adds then aborts...and double removes then aborts?
> 

Added.

> > WebCore/storage/IDBTransactionBackendImpl.h:50
> > +    virtual bool scheduleTask(PassOwnPtr<ScriptExecutionContext::Task> task, PassOwnPtr<ScriptExecutionContext::Task> abortTaskn);
> 
> abortTask minus the n

Fixed.

[Andrei Popescu]

Created attachment 69997 [details]
Patch

[Jeremy Orlow]

Comment on attachment 69997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69997&action=review

r=me

> WebCore/storage/IDBDatabase.cpp:62
> +    if (!objectStore || !m_setVersionTransaction)

Do this before you call the backend function!!

> WebCore/storage/IDBDatabaseBackendImpl.cpp:143
> +                                   createCallbackTask(&IDBDatabaseBackendImpl::removeObjectStoreFromMap, database, objectStore)))

{}

> WebCore/storage/IDBDatabaseBackendImpl.cpp:217
> +                                   createCallbackTask(&IDBDatabaseBackendImpl::resetVersion, database, m_version)))

use {}

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:279
> +                                   createCallbackTask(&IDBObjectStoreBackendImpl::removeIndexFromMap, objectStore, index)))

use {}s

[Andrei Popescu]

Committed r69247: <http://trac.webkit.org/changeset/69247>

[WebKit Review Bot]

http://trac.webkit.org/changeset/69247 might have broken Chromium Win Release