Bug 150735 – storage/indexeddb/modern/idbdatabase-deleteobjectstore-failures.html is flaky on mac-wk1

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ryan Haddad]

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

[Ryan Haddad]

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

[Brady Eidson]

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

[Brady Eidson]

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

[Brady Eidson]

Created attachment 264447 [details]
WIP exploration

Attaching my WIP exploration so I can get to it from home later

[Brady Eidson]

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 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]

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

Created attachment 264489 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 264489 [details]
Patch for landing.

Clearing flags on attachment: 264489

Committed r191847: <http://trac.webkit.org/changeset/191847>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.