IndexedDB: Assertion failure with open() within upgradeneeded
Created attachment 164443 [details] Patch
I implemented a layout test to try and exercise an edge case I noticed where an aborted version change that should see "old" metadata instead sees the "new" metadata of a subsequent/unblocked version change. I'm not sure the test will reveal that, but as written (attached) it triggers an assertion failure: STDERR: ASSERTION FAILED: pendingOpenWithVersionCall->version() == m_intVersion STDERR: ../../third_party/WebKit/Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp(363) : void WebCore::IDBDatabaseBackendImpl::processPendingCalls() Partial stack: STDERR: WebCore::IDBDatabaseBackendImpl::processPendingCalls() [0x24ca019] STDERR: WebCore::IDBDatabaseBackendImpl::transactionFinishedAndCompleteFired() [0x24ca769] STDERR: WebCore::IDBTransactionBackendImpl::commit() [0x24f6422] STDERR: WebCore::IDBTransactionBackendImpl::taskEventTimerFired() [0x24f5388]
I see the crash when running this test in DRT. Curiously it doesn't crash in content_shell. I haven't dug much deeper than that.
Event/call sequence: IDBDatabaseBackendImpl::openConnectionWithVersion(2) IDBDatabaseBackendImpl::runIntVersionChangeTransaction IDBDatabaseBackendImpl::setIntVersionInternal(2) IDBDatabaseBackendImpl::openConnectionWithVersion(3) IDBDatabaseBackendImpl::close IDBDatabaseBackendImpl::processPendingCalls > pendingOpenWithVersionCall IDBDatabaseBackendImpl::openConnectionWithVersion(3) IDBDatabaseBackendImpl::runIntVersionChangeTransaction IDBDatabaseBackendImpl::processPendingCalls > m_pendingSecondHalfOpenWithVersion Assertion is: m_pendingSecondHalfOpenWithVersion->version() == m_metadata.intVersion Values are 3 and 2 Relevant stack is: WebCore::IDBDatabaseBackendImpl::processPendingCalls() [0x25b76aa] WebCore::IDBDatabaseBackendImpl::transactionFinishedAndCompleteFired() [0x25b7e49] WebCore::IDBTransactionBackendImpl::commit() [0x25e558e] WebCore::IDBTransactionBackendImpl::taskEventTimerFired() [0x25e4458] So... what's going wrong: * first connection starts its upgrade needed transaction * close is called * second connection's runIntVersionChangeTransaction runs - there's only one connection so it and schedules a setIntVersionInternal call and adds a PendingOpenWithVersionCall * first connections's transaction completes * transactionFinishedAndCompleteFired runs and calling processPendingCalls assuming that it will "unblock" the pendingSecondHalf... but the setIntVersionInternal hasn't even run yet.
(In reply to comment #4) > Event/call sequence: > > IDBDatabaseBackendImpl::openConnectionWithVersion(2) > IDBDatabaseBackendImpl::runIntVersionChangeTransaction > IDBDatabaseBackendImpl::setIntVersionInternal(2) > IDBDatabaseBackendImpl::openConnectionWithVersion(3) > IDBDatabaseBackendImpl::close > IDBDatabaseBackendImpl::processPendingCalls > > pendingOpenWithVersionCall > IDBDatabaseBackendImpl::openConnectionWithVersion(3) > IDBDatabaseBackendImpl::runIntVersionChangeTransaction > IDBDatabaseBackendImpl::processPendingCalls > > m_pendingSecondHalfOpenWithVersion And since it isn't obvious... everything from the IDBDatabaseBackendImpl::close() call down is occurring within the IDBTransactionBackendImpl::commit of the versionchange transaction.
Created attachment 170275 [details] Patch
dgrogan@ - can you take a look?
Comment on attachment 170275 [details] Patch Attachment 170275 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14543073 New failing tests: storage/indexeddb/unblocked-version-changes.html
Created attachment 170411 [details] Patch
Let's try that again with an expected.txt file
Created attachment 171311 [details] Patch
Comment on attachment 171311 [details] Patch why the switch to ThreadSafeRefCounted?
(In reply to comment #12) > (From update of attachment 171311 [details]) > why the switch to ThreadSafeRefCounted? Compiler error (or compile-time assertion? - its been a while) without it, due to storing the pointer in a Task. IDBCallbacks has the same requirement. We don't actually execute them on separate threads, though.
Comment on attachment 171311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171311&action=review LGTM Thanks for digging into this. > LayoutTests/storage/indexeddb/resources/unblocked-version-changes.js:35 > + request.onblocked = unexpectedBlockedCallback; This request gets a blocked event in multi process, you may want to omit this line.
Comment on attachment 171311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171311&action=review > Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacks.h:37 > +class IDBDatabaseCallbacks : public ThreadSafeRefCounted<IDBDatabaseCallbacks> { I think that by marking this ThreadSafeRefCounted we are certifying that all the derived classes are thread-safe but I don't think that's true. Maybe a comment is in order? Similar to what you and Alec were talking about the other day, if we weren't re-appropriating ScriptExecutionContext::Task we wouldn't need to lie about class's thread-safety.
Created attachment 172825 [details] Patch
(In reply to comment #14) > > LayoutTests/storage/indexeddb/resources/unblocked-version-changes.js:35 > > + request.onblocked = unexpectedBlockedCallback; > > This request gets a blocked event in multi process, you may want to omit this line. Thanks, removed. (In reply to comment #15) > I think that by marking this ThreadSafeRefCounted we are certifying that all the derived classes are thread-safe but I don't think that's true. Maybe a comment is in order? Added a FIXME hinting that we should move away from Task.
dglazkov@ - can you r?
Comment on attachment 172825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172825&action=review > Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacks.h:38 > +// FIXME: Uses ThreadSafeRefCounted for storage in ScriptExecutionContext::Task but > +// it is never actually used on multiple threads. ew. Btw, what's the actionable comment here? Maybe file a bug to track progress and refer to it here?
Created attachment 172826 [details] Patch for landing
(In reply to comment #19) > ew. Btw, what's the actionable comment here? Maybe file a bug to track progress and refer to it here? Filed and referenced: https://bugs.webkit.org/show_bug.cgi?id=101483
Comment on attachment 172826 [details] Patch for landing Clearing flags on attachment: 172826 Committed r133776: <http://trac.webkit.org/changeset/133776>
All reviewed patches have been landed. Closing bug.