Bug 94402 - IndexedDB: Fire error at request when abort is called in upgradeneeded
Summary: IndexedDB: Fire error at request when abort is called in upgradeneeded
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-17 17:38 PDT by David Grogan
Modified: 2012-08-21 18:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.83 KB, patch)
2012-08-17 18:06 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (11.95 KB, patch)
2012-08-17 18:08 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (11.73 KB, patch)
2012-08-17 18:22 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (11.77 KB, patch)
2012-08-21 14:31 PDT, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-08-17 17:38:40 PDT
IndexedDB: Fire error at request when abort is called in upgradeneeded
Comment 1 David Grogan 2012-08-17 18:06:59 PDT
Created attachment 159250 [details]
Patch
Comment 2 David Grogan 2012-08-17 18:08:24 PDT
Created attachment 159251 [details]
Patch
Comment 3 David Grogan 2012-08-17 18:09:33 PDT
Josh, could you look at this?

The first of many addenda to the integer version stuff.
Comment 4 David Grogan 2012-08-17 18:22:31 PDT
Created attachment 159253 [details]
Patch
Comment 5 Peter Beverloo (cr-android ews) 2012-08-17 18:42:56 PDT
Comment on attachment 159253 [details]
Patch

Attachment 159253 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13535059
Comment 6 Joshua Bell 2012-08-21 12:22:06 PDT
Comment on attachment 159253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159253&action=review

lgtm

> Source/WebCore/ChangeLog:8
> +        Tests - updated expected.txt

Give test path/name here too.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:325
>      RefPtr<IDBTransactionBackendImpl> transaction = prpTransaction;

Add an ASSERT(m_state == Finished) here?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:340
> +    RefPtr<IDBTransactionBackendImpl> transaction = prpTransaction;

And here?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:500
> +    ASSERT_WITH_MESSAGE(!m_pendingSecondHalfOpenWithVersionCalls.size(), "m_pendingSecondHalfOpenWithVersionCalls.size = %lu", m_pendingSecondHalfOpenWithVersionCalls.size());

Based on EWS output, Android port's compiler requires a size_t->long long cast here, apparently.
Comment 7 David Grogan 2012-08-21 14:24:45 PDT
Comment on attachment 159253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159253&action=review

>> Source/WebCore/ChangeLog:8
>> +        Tests - updated expected.txt
> 
> Give test path/name here too.

done

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:325
>>      RefPtr<IDBTransactionBackendImpl> transaction = prpTransaction;
> 
> Add an ASSERT(m_state == Finished) here?

Are you suggesting adding an accessor for IDBTransactionBackendImpl::m_state? I would guess you didn't realize that'd be necessary and that it's not worth it. But if you did anticipate that and think it's a good idea, I'll add it.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:500
>> +    ASSERT_WITH_MESSAGE(!m_pendingSecondHalfOpenWithVersionCalls.size(), "m_pendingSecondHalfOpenWithVersionCalls.size = %lu", m_pendingSecondHalfOpenWithVersionCalls.size());
> 
> Based on EWS output, Android port's compiler requires a size_t->long long cast here, apparently.

We'll see if zu works.
Comment 8 Joshua Bell 2012-08-21 14:25:49 PDT
> > Add an ASSERT(m_state == Finished) here?
> 
> Are you suggesting adding an accessor for IDBTransactionBackendImpl::m_state? I would guess you didn't realize that'd be necessary and that it's not worth it. But if you did anticipate that and think it's a good idea, I'll add it.

Oops - sorry, my brain wasn't back from vacation here. I was thinking we were still in IDBTransactionBackendImpl.

No, keep state internal, no ASSERT.
Comment 9 David Grogan 2012-08-21 14:31:21 PDT
Created attachment 159770 [details]
Patch
Comment 10 David Grogan 2012-08-21 15:37:36 PDT
%zu worked on the android try job, it'll probably pass on cr-android.
http://build.chromium.org/p/tryserver.chromium/builders/android/builds/30549
Comment 11 David Grogan 2012-08-21 15:38:22 PDT
Tony, could you review this when you get a chance?
Comment 12 Tony Chang 2012-08-21 15:56:36 PDT
Comment on attachment 159770 [details]
Patch

OK
Comment 13 WebKit Review Bot 2012-08-21 18:08:23 PDT
Comment on attachment 159770 [details]
Patch

Clearing flags on attachment: 159770

Committed r126239: <http://trac.webkit.org/changeset/126239>
Comment 14 WebKit Review Bot 2012-08-21 18:08:26 PDT
All reviewed patches have been landed.  Closing bug.