Bug 164932

In testing and fixing up this patch, it became apparently you can't *ALWAYS* flush *EVERYTHING* to the server. Retitling: IndexedDB 2.0: The client's transaction operation queue should flush as much to the server as possible

Created attachment 295206 [details] Patch

Created attachment 295214 [details] Patch

Comment on attachment 295214 [details] Patch Still working on last minute cleanups

Created attachment 295218 [details] Patch

Comment on attachment 295218 [details] Patch Attachment 295218 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2539727 New failing tests: imported/blink/storage/indexeddb/blob-valid-before-commit.html

Created attachment 295238 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 295218 [details] Patch Attachment 295218 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2539925 New failing tests: imported/blink/storage/indexeddb/blob-valid-before-commit.html

Created attachment 295239 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Nice catch with that test - a very special kind of operation wasn't accounted for which is a put/add operation *with* blobs. *sigh*

Created attachment 295248 [details] Patch

This is for landing. I will cq+ once the previous failing tests are clear.

Comment on attachment 295248 [details] Patch Attachment 295248 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2540876 New failing tests: imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-event_order.html

Created attachment 295251 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Interested set of failings. Digging in on imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-event_order.html first: Can confirm it's a flaky failure locally but, bizarrely, only in WK2 (Private browsing works in-memory, in-process on both WK1 and WK2)

(In reply to comment #15) > Interested set of failings. > > Digging in on > imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7- > event_order.html first: Can confirm it's a flaky failure locally but, > bizarrely, only in WK2 > > (Private browsing works in-memory, in-process on both WK1 and WK2) Never mind. `run-webkit-tests -1 imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-event_order.html --repeat-each=10` got it to fail ~5 times.

So far, absolutely cannot repro in a debug build, which severely limits my logging. *Sigh*

rq_add2 causes a constraint error, so the rq_add3 should not succeed, but sometimes it does. Two possible solutions: 1 - rq_add3 is already in flight in the server when the client receives the constraint error. If the server tracked "this is aborted due to the constraint error" then it could also abort rq_add3 2 - Alternately, the client remembers all in-flight requests and *Should* abort then when it aborts the transaction for a constraint error. #2 is preferred. And my guess why it's failing is because it schedules the abort with a spin of the runloop but, before that happens, the success for rq_add3 comes in from the server. Aborts due to contraints/failed requests need to be synchronous, maybe.

(In reply to comment #18) > rq_add2 causes a constraint error, so the rq_add3 should not succeed, but > sometimes it does. > > Two possible solutions: > 1 - rq_add3 is already in flight in the server when the client receives the > constraint error. If the server tracked "this is aborted due to the > constraint error" then it could also abort rq_add3 > > 2 - Alternately, the client remembers all in-flight requests and *Should* > abort then when it aborts the transaction for a constraint error. > > #2 is preferred. And my guess why it's failing is because it schedules the > abort with a spin of the runloop but, before that happens, the success for > rq_add3 comes in from the server. > > Aborts due to contraints/failed requests need to be synchronous, maybe. It has to be #2, and it's semi-nefarious. The 2nd put comes back with a failure, and the event dispatch is scheduled. Then the 3rd comes back with a success, and the event dispatch is scheduled. Then the event for the 2nd fires, and since it's not handled we schedule an abort transaction. Then the success event for the 3rd fires. Then the transaction is aborted. Gotta break the event for the 3rd firing.

(In reply to comment #19) > (In reply to comment #18) > > rq_add2 causes a constraint error, so the rq_add3 should not succeed, but > > sometimes it does. > > > > Two possible solutions: > > 1 - rq_add3 is already in flight in the server when the client receives the > > constraint error. If the server tracked "this is aborted due to the > > constraint error" then it could also abort rq_add3 > > > > 2 - Alternately, the client remembers all in-flight requests and *Should* > > abort then when it aborts the transaction for a constraint error. > > > > #2 is preferred. And my guess why it's failing is because it schedules the > > abort with a spin of the runloop but, before that happens, the success for > > rq_add3 comes in from the server. > > > > Aborts due to contraints/failed requests need to be synchronous, maybe. > > It has to be #2, and it's semi-nefarious. > > The 2nd put comes back with a failure, and the event dispatch is scheduled. > Then the 3rd comes back with a success, and the event dispatch is scheduled. > Then the event for the 2nd fires, and since it's not handled we schedule an > abort transaction. > Then the success event for the 3rd fires. > Then the transaction is aborted. > > Gotta break the event for the 3rd firing. While working on a patch where IDBRequests remember the events they've queued up in the ScriptExecutionContext and know how to cancel them later, I realize there's still edge cases that will come up. New strategy I'm considering: Operations that come back complete go into a "CompleteOperationQueue", and we call the complete handler on each one-by-one. The idea being that if an operation completing results in event dispatch, we'd first wait on the event to dispatch.

Resolving this will require a refactor which I'll do first over in https://bugs.webkit.org/show_bug.cgi?id=165000

Comment on attachment 295248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295248&action=review imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-event_order.html failure? > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:256 > + Vector<RefPtr<IDBClient::TransactionOperation>> inProgressAbortVector; Ref instead of RefPtr? > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:261 > + while (!m_transactionOperationsInProgressQueue.isEmpty()) { > + auto operation = m_transactionOperationsInProgressQueue.takeFirst(); > + inProgressAbortVector.uncheckedAppend(operation); > + } Would read better without the local variable, I think.

(In reply to comment #22) > Comment on attachment 295248 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295248&action=review > > imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7- > event_order.html failure? > > > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:256 > > + Vector<RefPtr<IDBClient::TransactionOperation>> inProgressAbortVector; > > Ref instead of RefPtr? > > > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:261 > > + while (!m_transactionOperationsInProgressQueue.isEmpty()) { > > + auto operation = m_transactionOperationsInProgressQueue.takeFirst(); > > + inProgressAbortVector.uncheckedAppend(operation); > > + } > > Would read better without the local variable, I think. Will make both of those changes before landing this (but still working on a pre-requisite patch over in https://webkit.org/b/165000)

Created attachment 295603 [details] Patch

Looks solid locally. Giving EWS a run before landing.

Bots are seeing a crash in imported/blink/storage/indexeddb/blob-valid-before-commit.html and I can repro locally. Exploring.

(Debug bots assert, that's why release bots went green)

Comment on attachment 295603 [details] Patch Attachment 295603 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2589791 New failing tests: imported/blink/storage/indexeddb/blob-valid-before-commit.html

Created attachment 295610 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 295612 [details] Patch

Comment on attachment 295612 [details] Patch Clearing flags on attachment: 295612 Committed r209086: <http://trac.webkit.org/changeset/209086>

All reviewed patches have been landed. Closing bug.