Bug 150735

Marked as flaky in <https://trac.webkit.org/r191813>

I think this revealed a real issue and I have a lead on fixing it.

I think this revealed a real issue and I have a lead on fixing it.

Created attachment 264447 [details] WIP exploration Attaching my WIP exploration so I can get to it from home later

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.

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());

(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.

Created attachment 264489 [details] Patch for landing.

Comment on attachment 264489 [details] Patch for landing. Clearing flags on attachment: 264489 Committed r191847: <http://trac.webkit.org/changeset/191847>

All reviewed patches have been landed. Closing bug.