Bug 150735

Summary: storage/indexeddb/modern/idbdatabase-deleteobjectstore-failures.html is flaky on mac-wk1
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, beidson, commit-queue, jsbell
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Mac   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149117    
Attachments:
Description Flags
WIP exploration
none
Patch v1
darin: review+
Patch for landing. none

Ryan Haddad
Reported 2015-10-30 14:19:34 PDT
storage/indexeddb/modern/idbdatabase-deleteobjectstore-failures.html failing on mac-wk1 Build log of first failure <https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/builds/9501> --- /Volumes/Data/slave/yosemite-release-tests-wk1/build/layout-test-results/storage/indexeddb/modern/idbdatabase-deleteobjectstore-failures-expected.txt +++ /Volumes/Data/slave/yosemite-release-tests-wk1/build/layout-test-results/storage/indexeddb/modern/idbdatabase-deleteobjectstore-failures-actual.txt @@ -5,7 +5,6 @@ ALERT: readwrite transaction complete ALERT: Second upgrade needed: Old version - 1 New version - 2 ALERT: Failed to deleteObjectStore with a non-existent objectstore - Error: NotFoundError: DOM IDBDatabase Exception 8 -ALERT: Failed to deleteObjectStore with an in-progress versionchange transaction that is inactive - Error: TransactionInactiveError: DOM IDBDatabase Exception 0 ALERT: Second version change transaction complete ALERT: Done This tests some obvious failures that can happen while calling IDBDatabase.deleteObjectStore()
Attachments
WIP exploration (4.82 KB, application/octet-stream)
2015-10-30 17:20 PDT, Brady Eidson
no flags
Patch v1 (9.34 KB, patch)
2015-10-31 10:27 PDT, Brady Eidson
darin: review+
Patch for landing. (9.28 KB, patch)
2015-10-31 15:32 PDT, Brady Eidson
no flags
Ryan Haddad
Comment 1 2015-10-30 14:24:50 PDT
Marked as flaky in <https://trac.webkit.org/r191813>
Brady Eidson
Comment 2 2015-10-30 16:21:49 PDT
I think this revealed a real issue and I have a lead on fixing it.
Brady Eidson
Comment 3 2015-10-30 16:21:55 PDT
I think this revealed a real issue and I have a lead on fixing it.
Brady Eidson
Comment 4 2015-10-30 17:20:10 PDT
Created attachment 264447 [details] WIP exploration Attaching my WIP exploration so I can get to it from home later
Brady Eidson
Comment 5 2015-10-31 10:27:29 PDT
Created attachment 264478 [details] Patch v1 Delay commit of a transaction if it has requests still waiting to deliver their events, as those events can keep the transaction runnable.
Darin Adler
Comment 6 2015-10-31 14:42:10 PDT
Comment on attachment 264478 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=264478&action=review > Source/WebCore/Modules/indexeddb/client/IDBRequestImpl.cpp:167 > + // FIXME: With reusable requests (for cursors) we won't always remove this request from its transaction. I don’t understand what that FIXME means. Does it mean that we will need to change this code to not always remove the request, or does it mean that this code, which looks like it’s removing the request all the time, will sometimes fail to do that? I think maybe you should reword to say "When we implement reusable requests (for cursors) it will be incorrect to always remove the request from the transaction". Or something similar that is correct, if I got some details wrong. > Source/WebCore/Modules/indexeddb/client/IDBRequestImpl.cpp:171 > m_hasPendingActivity = false; This seems dangerous. We just called removeRequest on the transaction. The transaction is holding a RefPtr in a HashSet. So that means we might have just deleted "this"; going on to set m_hasPendingActivity to false afterward seems super-dangerous. I see at least two possibilities: 1) The HashSet should be a set of raw pointers; the transaction does not need to keep the request alive because the requisition will keep the transaction alive. 2) We should set m_hasPendingActivity *before* calling removeRequest. Or maybe there’s something even more complex going on here. > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:527 > + auto takenOperation = m_transactionOperationMap.take(operation.identifier()); > + ASSERT_UNUSED(takenOperation, takenOperation == &operation); I suggest writing this instead: ASSERT(m_transactionOperationMap.get(operation.identifier()) == &operation); m_transactionOperationMap.remove(operation.identifier());
Brady Eidson
Comment 7 2015-10-31 15:29:19 PDT
(In reply to comment #6) > Comment on attachment 264478 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264478&action=review > > > Source/WebCore/Modules/indexeddb/client/IDBRequestImpl.cpp:167 > > + // FIXME: With reusable requests (for cursors) we won't always remove this request from its transaction. > > I don’t understand what that FIXME means. Does it mean that we will need to > change this code to not always remove the request, or does it mean that this > code, which looks like it’s removing the request all the time, will > sometimes fail to do that? > > I think maybe you should reword to say "When we implement reusable requests > (for cursors) it will be incorrect to always remove the request from the > transaction". Or something similar that is correct, if I got some details > wrong. Your rewording is the correct understanding, and I'll use it verbatim. > > > Source/WebCore/Modules/indexeddb/client/IDBRequestImpl.cpp:171 > > m_hasPendingActivity = false; > > This seems dangerous. We just called removeRequest on the transaction. The > transaction is holding a RefPtr in a HashSet. So that means we might have > just deleted "this"; going on to set m_hasPendingActivity to false afterward > seems super-dangerous. I see at least two possibilities: > > 1) The HashSet should be a set of raw pointers; the transaction does not > need to keep the request alive because the requisition will keep the > transaction alive. > > 2) We should set m_hasPendingActivity *before* calling removeRequest. > > Or maybe there’s something even more complex going on here. The long answer is that there *is* something even more complex going on here (the IDBRequest, by virtue of just having dispatched an event, is guaranteed to have a JSIDBRequest wrapper keeping it alive until GC runs). But the short answer is that I'll implement suggest #2 > > > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:527 > > + auto takenOperation = m_transactionOperationMap.take(operation.identifier()); > > + ASSERT_UNUSED(takenOperation, takenOperation == &operation); > > I suggest writing this instead: > > ASSERT(m_transactionOperationMap.get(operation.identifier()) == > &operation); > m_transactionOperationMap.remove(operation.identifier()); Yah... this way was a leftover vestige. Yours clearly seems right.
Brady Eidson
Comment 8 2015-10-31 15:32:45 PDT
Created attachment 264489 [details] Patch for landing.
WebKit Commit Bot
Comment 9 2015-10-31 16:18:35 PDT
Comment on attachment 264489 [details] Patch for landing. Clearing flags on attachment: 264489 Committed r191847: <http://trac.webkit.org/changeset/191847>
WebKit Commit Bot
Comment 10 2015-10-31 16:18:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.