RESOLVED FIXED 96947
IndexedDB: Assertion failure with open() within upgradeneeded
https://bugs.webkit.org/show_bug.cgi?id=96947
Summary IndexedDB: Assertion failure with open() within upgradeneeded
Joshua Bell
Reported 2012-09-17 13:00:26 PDT
IndexedDB: Assertion failure with open() within upgradeneeded
Attachments
Patch (3.39 KB, patch)
2012-09-17 13:00 PDT, Joshua Bell
no flags
Patch (9.69 KB, patch)
2012-10-23 17:29 PDT, Joshua Bell
no flags
Patch (10.87 KB, patch)
2012-10-24 09:11 PDT, Joshua Bell
no flags
Patch (10.98 KB, patch)
2012-10-29 14:13 PDT, Joshua Bell
no flags
Patch (11.01 KB, patch)
2012-11-07 09:48 PST, Joshua Bell
no flags
Patch for landing (11.04 KB, patch)
2012-11-07 10:03 PST, Joshua Bell
no flags
Joshua Bell
Comment 1 2012-09-17 13:00:51 PDT
Joshua Bell
Comment 2 2012-09-17 13:04:28 PDT
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]
David Grogan
Comment 3 2012-09-20 18:12:41 PDT
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.
Joshua Bell
Comment 4 2012-10-23 13:58:39 PDT
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.
Joshua Bell
Comment 5 2012-10-23 15:42:00 PDT
(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.
Joshua Bell
Comment 6 2012-10-23 17:29:49 PDT
Joshua Bell
Comment 7 2012-10-23 17:30:14 PDT
dgrogan@ - can you take a look?
WebKit Review Bot
Comment 8 2012-10-23 21:43:13 PDT
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
Joshua Bell
Comment 9 2012-10-24 09:11:20 PDT
Joshua Bell
Comment 10 2012-10-24 09:11:36 PDT
Let's try that again with an expected.txt file
Joshua Bell
Comment 11 2012-10-29 14:13:18 PDT
Alec Flett
Comment 12 2012-11-05 16:44:06 PST
Comment on attachment 171311 [details] Patch why the switch to ThreadSafeRefCounted?
Joshua Bell
Comment 13 2012-11-05 16:46:57 PST
(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.
David Grogan
Comment 14 2012-11-06 19:07:33 PST
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.
David Grogan
Comment 15 2012-11-06 19:13:37 PST
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.
Joshua Bell
Comment 16 2012-11-07 09:48:49 PST
Joshua Bell
Comment 17 2012-11-07 09:50:09 PST
(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.
Joshua Bell
Comment 18 2012-11-07 09:50:23 PST
dglazkov@ - can you r?
Dimitri Glazkov (Google)
Comment 19 2012-11-07 09:57:27 PST
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?
Joshua Bell
Comment 20 2012-11-07 10:03:44 PST
Created attachment 172826 [details] Patch for landing
Joshua Bell
Comment 21 2012-11-07 10:04:19 PST
(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
WebKit Review Bot
Comment 22 2012-11-07 10:26:47 PST
Comment on attachment 172826 [details] Patch for landing Clearing flags on attachment: 172826 Committed r133776: <http://trac.webkit.org/changeset/133776>
WebKit Review Bot
Comment 23 2012-11-07 10:26:51 PST
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.