Refactor IDBDatabase to eliminate crash where it is destroyed in the middle of closeConnection
Created attachment 251715 [details] Patch
<rdar://problem/19816412>
I uploaded this patch after I got it to compile but before I ran the tests.
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.
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
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
I’ll let Brady tackle this during his project to improve indexed database implementation. Not sure this patch has any value at this point.
(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