Bug 144242 – Refactor and streamline IDBDatabase

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Darin Adler]

Refactor IDBDatabase to eliminate crash where it is destroyed in the middle of closeConnection

[Darin Adler]

Created attachment 251715 [details]
Patch

[Darin Adler]

<rdar://problem/19816412>

[Darin Adler]

I uploaded this patch after I got it to compile but before I ran the tests.

[Ryosuke Niwa]

Comment on attachment 251715 [details]
Patch

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

> Source/WebCore/ChangeLog:28
> +        (WebCore::IDBDatabase::createObjectStore): Use nullptr instead of 0. Use autoincrement for
> +        the store id. Use add instead of set for the HahsSet. Changed the return type. Added a check
> +        so we throw an exception if this is called while closing.

Is it possible to add a test for that?

> Source/WebCore/ChangeLog:30
> +        (WebCore::IDBDatabase::deleteObjectStore): Added a check so we throw an exception if this is
> +        called while closing.

Ditto.

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:176
> -    int64_t objectStoreId = m_metadata.maxObjectStoreId + 1;
> +    int64_t objectStoreId = m_metadata.maxObjectStoreId++;
>      m_backend->createObjectStore(m_versionChangeTransaction->id(), objectStoreId, name, keyPath, autoIncrement);

Can we split this into a refactoring & the fix? It's really hard to see the actual logic change with all these refactoring merged in.
e.g. here, we're updating m_metadata.maxObjectStoreId earlier than we used to.

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:187
> -    if (!m_versionChangeTransaction) {
> +    if (!m_versionChangeTransaction || m_isClosing) {

And we're checking m_isClosing here.

[Build Bot]

Comment on attachment 251715 [details]
Patch

Attachment 251715 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5413859552657408

New failing tests:
storage/indexeddb/objectstore-removeobjectstore.html
storage/indexeddb/mozilla/remove-objectstore.html

[Build Bot]

Created attachment 251718 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

Comment on attachment 251715 [details]
Patch

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

> Source/WebCore/ChangeLog:28
> +        (WebCore::IDBDatabase::createObjectStore): Use nullptr instead of 0. Use autoincrement for
> +        the store id. Use add instead of set for the HahsSet. Changed the return type. Added a check
> +        so we throw an exception if this is called while closing.

No way to create a test for this behavior?

> Source/WebCore/ChangeLog:30
> +        (WebCore::IDBDatabase::deleteObjectStore): Added a check so we throw an exception if this is
> +        called while closing.

And this?

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:250
> +    // If so, then this code wonât work, trying to iterate a map while it's being modified.

Are we using non-ASCII characters in comments now?

Comment on attachment 251715 [details]
Patch

r- because the test failures look legitimate

[Darin Adler]

I’ll let Brady tackle this during his project to improve indexed database implementation. Not sure this patch has any value at this point.

[Brady Eidson]

(In reply to comment #9)
> I’ll let Brady tackle this during his project to improve indexed database
> implementation. Not sure this patch has any value at this point.

The ideas in this patch definitely have value.

Some things - like s/0/nullptr/ - are being done as subtasks of 149117

Other things - like Ref instead of RefPtr where possible - are being done as part of 149117, but not at the IDBDatabase level *quite yet*

The reason is IDBDatabase is abstract and shared between both the Modern IDB implementation and the still-in-use Legacy IDB implementation.

Once Modern is done and Legacy removed then we can finish implementing these ideas at IDBDatabase.

But I agree this bug is not useful - That's going to be the final subtask of 149117