Bug 46883 – IDBDatabase::createObjectStore/removeObjectStore and IDBObjectStore::createIndex/removeIndex should be synchronous.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andrei Popescu]

IDBDatabase::createObjectStore/removeObjectStore and IDBObjectStore::createIndex/removeIndex should be synchronous.

[Andrei Popescu]

Created attachment 69321 [details]
Patch

[WebKit Review Bot]

Attachment 69321 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4177027

[Andrei Popescu]

Created attachment 69323 [details]
Patch

[Jeremy Orlow]

Comment on attachment 69321 [details]
Patch

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

> WebCore/storage/IDBDatabase.cpp:68
> +    m_backend->removeObjectStore(name);

this needs the transaction

> WebCore/storage/IDBDatabaseBackendImpl.cpp:172
> +void IDBDatabaseBackendImpl::removeObjectStore(const String& name)

need the transaction passed in

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:92
> +    , m_id(0),

maybe make an invalid id constant?

[Jeremy Orlow]

couple comments

[Jeremy Orlow]

Comment on attachment 69323 [details]
Patch

besides previous comment, lgtm

[Andrei Popescu]

Created attachment 69498 [details]
Patch

[Andrei Popescu]

I rebased my patch after http://trac.webkit.org/changeset/68795 and the conflict resolving was non-trivial. Also modified the layout tests and updated the expected tests output. I would therefore be grateful if I could get another review. Thanks!

[Jeremy Orlow]

Comment on attachment 69498 [details]
Patch


close


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

> LayoutTests/storage/indexeddb/objectstore-cursor.html:67
> +    if (window.nextToAdd > 0)

hu?

> LayoutTests/storage/indexeddb/resources/shared.js:82
> +    onfinished();

ugh!  I'm so stupid...I just made this change...should have realized it's worthless (and not made the change in my last patch).  Maybe put in a FIXME to make this function sync and not do a callback like this.

> WebCore/storage/IDBDatabaseBackendImpl.cpp:191
> +    RefPtr<IDBTransactionBackendInterface> rpTransaction = transaction;

transactionPtr...here and elsewhere

> WebCore/storage/IDBIndexBackendImpl.cpp:55
> +    , m_id(0)

use a constant instead of 0 maybe?

> WebCore/storage/IDBIndexBackendImpl.h:52
> +    int64_t id() const { return m_id; }

ditto to all comments below

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:335
> +    transaction->didCompleteTaskEvents();

Wait...why do we need this?  Isn't this implicit whenever a task's events finish?

> WebCore/storage/IDBObjectStoreBackendImpl.h:55
>      int64_t id() const { return m_id; }

Assert the id is valid maybe?  ...here and everywhere that id is used?

Please put in a comment here describing the subtleties of this..and how stuff using id should only be queued asyncly.

We should put a fixme or actual code into the transaction coordinator to make sure that it blocks any transactions on this object store on this current transaction finishing....

> WebCore/storage/IDBTransactionBackendImpl.cpp:146
> +void IDBTransactionBackendImpl::taskEventTimerFired(Timer<IDBTransactionBackendImpl>*)

Hm...this still seems a bit fragile...but I guess it's good enough for now.

[Andrei Popescu]

(In reply to comment #9)
> (From update of attachment 69498 [details])
> 
> close
> 
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=69498&action=review
> 
> > LayoutTests/storage/indexeddb/objectstore-cursor.html:67
> > +    if (window.nextToAdd > 0)
> 
> hu?
> 

It's the same pattern used elsewhere for loading data into an object store.

> > LayoutTests/storage/indexeddb/resources/shared.js:82
> > +    onfinished();
> 
> ugh!  I'm so stupid...I just made this change...should have realized it's worthless (and not made the change in my last patch).  Maybe put in a FIXME to make this function sync and not do a callback like this.
> 

Added.

> > WebCore/storage/IDBDatabaseBackendImpl.cpp:191
> > +    RefPtr<IDBTransactionBackendInterface> rpTransaction = transaction;
> 
> transactionPtr...here and elsewhere
> 
> > WebCore/storage/IDBIndexBackendImpl.cpp:55
> > +    , m_id(0)
> 
> use a constant instead of 0 maybe?
> 
> > WebCore/storage/IDBIndexBackendImpl.h:52
> > +    int64_t id() const { return m_id; }
> 
> ditto to all comments below
>

You mean above? Done.
 
> > WebCore/storage/IDBObjectStoreBackendImpl.cpp:335
> > +    transaction->didCompleteTaskEvents();
> 
> Wait...why do we need this?  Isn't this implicit whenever a task's events finish?
> 

It isn't. These tasks don't have any events.

> > WebCore/storage/IDBObjectStoreBackendImpl.h:55
> >      int64_t id() const { return m_id; }
> 
> Assert the id is valid maybe?  ...here and everywhere that id is used?
> 

Asserting.

> Please put in a comment here describing the subtleties of this..and how stuff using id should only be queued asyncly.
> 
> We should put a fixme or actual code into the transaction coordinator to make sure that it blocks any transactions on this object store on this current transaction finishing....
>


Yep, that should happen once we have a thread per transaction and we will implement locking. The object store will be locked until the setVersion transaction commits, thereby guaranteeing that no other transaction can use its backend while it has the invalid ID.

 
> > WebCore/storage/IDBTransactionBackendImpl.cpp:146
> > +void IDBTransactionBackendImpl::taskEventTimerFired(Timer<IDBTransactionBackendImpl>*)
> 
> Hm...this still seems a bit fragile...but I guess it's good enough for now.

[Andrei Popescu]

Created attachment 69715 [details]
Patch

[Andrei Popescu]

Created attachment 69728 [details]
Patch

[Jeremy Orlow]

Comment on attachment 69728 [details]
Patch

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

> WebCore/storage/IDBIndexBackendImpl.h:54
> +        ASSERT(m_id != InvalidId);

ditto

> WebCore/storage/IDBObjectStoreBackendImpl.h:60
> +    void setId(int64_t id) { m_id = id; }

assert it's not valid to begin with?

also, i wonder if these should be un-inlined


r=me

[Jeremy Orlow]

Comment on attachment 69728 [details]
Patch

sitll looks fine

[Andrei Popescu]

Committed r69121.