Bug 54785 – When an IDBTransaction is aborted, all requests that have not yet fired should fire an ABORT_ERR

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

When an IDBTransaction is aborted, all requests that have not yet fired should fire an ABORT_ERR

[Jeremy Orlow]

Created attachment 83040 [details]
Patch

[Jeremy Orlow]

please review

[Hans Wennborg]

Comment on attachment 83040 [details]
Patch

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

> LayoutTests/storage/indexeddb/transaction-abort.html:55
> +    request.onerror = firstAdd;

Maybe set onsuccess = unexpectedSuccessFeedback?

> LayoutTests/storage/indexeddb/transaction-abort.html:66
> +{

Assert that it's an ABORT_ERR?

[David Grogan]

Bugzilla won't let me submit this comment on the patch ("You are not authorized to edit attachment 83040 [details]."), but:

> Source/WebCore/storage/IDBRequest.cpp:226
> +    m_transaction->registerRequest(this);

What does this do?
It seems to be here so that if the SetVersion transaction aborts, the error handler on the request for a setversion transaction will be called.  But for the transaction to abort, wouldn't the setversion success handler have to already be called?  What am I missing?

[Jeremy Orlow]

(In reply to comment #3)
> (From update of attachment 83040 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83040&action=review
> 
> > LayoutTests/storage/indexeddb/transaction-abort.html:55
> > +    request.onerror = firstAdd;
> 
> Maybe set onsuccess = unexpectedSuccessFeedback?

Good idea
 
> > LayoutTests/storage/indexeddb/transaction-abort.html:66
> > +{
> 
> Assert that it's an ABORT_ERR?

Good idea

(In reply to comment #4)
> Bugzilla won't let me submit this comment on the patch ("You are not authorized to edit attachment 83040 [details]."), but:
> 
> > Source/WebCore/storage/IDBRequest.cpp:226
> > +    m_transaction->registerRequest(this);
> 
> What does this do?
> It seems to be here so that if the SetVersion transaction aborts, the error handler on the request for a setversion transaction will be called.  But for the transaction to abort, wouldn't the setversion success handler have to already be called?  What am I missing?

Originally I asserted that if you unregistered something it had been registered before.  And we don't fire onError when aborting something that already fired an onSuccess.  So I believe it's OK as is, but probably a bit cleaner without.  Will change.

[Jeremy Orlow]

Created attachment 83406 [details]
Patch

[James Robinson]

Comment on attachment 83406 [details]
Patch

You have some spurious test_expectations.txt changes and conflicts that prevent EWS from running. Mind re-upping one without these?

[Jeremy Orlow]

I think the test_expectation change is a real issue...I couldn't run without it.  I can't upload a new one at the moment, but I will Monday.  The bots won't add much information though since this code only runs for Chromium.

[Jeremy Orlow]

Created attachment 84120 [details]
Patch

[Jeremy Orlow]

Nate's OOO and this is for m11, so if someone else could look, it'd be great.

[Steve Block]

Comment on attachment 84120 [details]
Patch

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

> Source/WebCore/storage/IDBRequest.h:62
> +        EarlyDeath = 3

Why do you use a different style for this member of the enum?

[Jeremy Orlow]

(In reply to comment #11)
> (From update of attachment 84120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84120&action=review
> 
> > Source/WebCore/storage/IDBRequest.h:62
> > +        EarlyDeath = 3
> 
> Why do you use a different style for this member of the enum?

Because its proper webkit style.  The others match the IDL, but this isn't in IDL.  I can make it match if you think that's better though.

Thanks!

[Steve Block]

Comment on attachment 84120 [details]
Patch

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

>>> Source/WebCore/storage/IDBRequest.h:62
>>> +        EarlyDeath = 3
>> 
>> Why do you use a different style for this member of the enum?
> 
> Because its proper webkit style.  The others match the IDL, but this isn't in IDL.  I can make it match if you think that's better though.
> 
> Thanks!

OK, makes sense. So while the request is in the EarlyDeath internal-only state, what state is reported to script? Or is this not relevant/possible?

[Jeremy Orlow]

(In reply to comment #13)
> (From update of attachment 84120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84120&action=review
> 
> >>> Source/WebCore/storage/IDBRequest.h:62
> >>> +        EarlyDeath = 3
> >> 
> >> Why do you use a different style for this member of the enum?
> > 
> > Because its proper webkit style.  The others match the IDL, but this isn't in IDL.  I can make it match if you think that's better though.
> > 
> > Thanks!
> 
> OK, makes sense. So while the request is in the EarlyDeath internal-only state, what state is reported to script? Or is this not relevant/possible?

Not possible.  That's why readyState() has this assert: ASSERT(m_readyState == LOADING || m_readyState == DONE);

[Steve Block]

Comment on attachment 84120 [details]
Patch

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

>>>>> Source/WebCore/storage/IDBRequest.h:62
>>>>> +        EarlyDeath = 3
>>>> 
>>>> Why do you use a different style for this member of the enum?
>>> 
>>> Because its proper webkit style.  The others match the IDL, but this isn't in IDL.  I can make it match if you think that's better though.
>>> 
>>> Thanks!
>> 
>> OK, makes sense. So while the request is in the EarlyDeath internal-only state, what state is reported to script? Or is this not relevant/possible?
> 
> Not possible.  That's why readyState() has this assert: ASSERT(m_readyState == LOADING || m_readyState == DONE);

I see, makes sense. Maybe add a comment in the enum that the EarlyDeath value is internal only.

[Jeremy Orlow]

Committed r80028: <http://trac.webkit.org/changeset/80028>

[WebKit Review Bot]

http://trac.webkit.org/changeset/80028 might have broken Qt Linux Release
The following tests are not passing:
editing/deleting/4916235-1.html
editing/deleting/5390681.html
editing/deleting/5546763.html
editing/deleting/delete-3775172-fix.html
editing/deleting/delete-at-paragraph-boundaries-007.html
editing/deleting/delete-before-block-image-1.html

[Jeremy Orlow]

Caused regressions...need to fix up.
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r80028%20(9911)/results.html
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r80028%20(9911)/editing/deleting/6026335-crash-log.txt

FAULTING_SOURCE_CODE:  
   100:     ASSERT_UNUSED(wasAdded, wasAdded); // It should not have already been in the list.
   101: 
   102:     while (!m_queuedEvents.isEmpty()) {
   103:         ListHashSet<RefPtr<Event> >::iterator iter = m_queuedEvents.begin();
>  104:         RefPtr<Event> event = *iter;
   105:         m_queuedEvents.remove(iter);
   106:         if (!event)
   107:             break;
   108:         dispatchEvent(event.get());
   109:     }

[Jeremy Orlow]

Committed r80055: <http://trac.webkit.org/changeset/80055>