WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(9.69 KB, patch)
2012-10-23 17:29 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch
(10.87 KB, patch)
2012-10-24 09:11 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch
(10.98 KB, patch)
2012-10-29 14:13 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch
(11.01 KB, patch)
2012-11-07 09:48 PST
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Patch for landing
(11.04 KB, patch)
2012-11-07 10:03 PST
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Joshua Bell
Comment 1
2012-09-17 13:00:51 PDT
Created
attachment 164443
[details]
Patch
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
Created
attachment 170275
[details]
Patch
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
Created
attachment 170411
[details]
Patch
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
Created
attachment 171311
[details]
Patch
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
Created
attachment 172825
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug