Bug 96947 – IndexedDB: Assertion failure with open() within upgradeneeded

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Assertion failure with open() within upgradeneeded

[Joshua Bell]

Created attachment 164443 [details]
Patch

[Joshua Bell]

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]

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]

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]

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

Created attachment 170275 [details]
Patch

[Joshua Bell]

dgrogan@ - can you take a look?

[WebKit Review Bot]

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]

Created attachment 170411 [details]
Patch

[Joshua Bell]

Let's try that again with an expected.txt file

[Joshua Bell]

Created attachment 171311 [details]
Patch

[Alec Flett]

Comment on attachment 171311 [details]
Patch

why the switch to ThreadSafeRefCounted?

[Joshua Bell]

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

Created attachment 172825 [details]
Patch

[Joshua Bell]

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

dglazkov@ - can you r?

[Dimitri Glazkov (Google)]

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]

Created attachment 172826 [details]
Patch for landing

[Joshua Bell]

(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 on attachment 172826 [details]
Patch for landing

Clearing flags on attachment: 172826

Committed r133776: <http://trac.webkit.org/changeset/133776>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.