IndexedDB: Align abort behavior on uncaught exception with Gecko

IndexedDB: Align abort behavior on uncaught exception with Gecko

Indexed DB specifies that an uncaught exception in an event handler
should abort the transaction. The spec states that this should happen
after dispatch, and Gecko agrees. Blink was aborting after the
specific handler aborted.

This change aligns Blink's behavior with Gecko and the spec, and
adds test coverage to web-platform-tests.

BUG=698342
R=pwnall@chromium.org

Review-Url: https://codereview.chromium.org/2734533002
Cr-Commit-Position: refs/heads/master@{#454937}
Committed: https://chromium.googlesource.com/chromium/src/+/ed3d60a6f09642595391b6678d46f548b649e832

[jsbell, 2017-03-03 20:23:37]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 20:23:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2017-03-03 20:27:43]

pwnall@ - please take a look?

[commit-bot: I haz the power, 2017-03-03 22:03:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 22:03:08]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-03-04 00:27:33]

LGTM, because everything looks correct. Here are some refactoring suggestions
that can make it easier to check the correctness of repetitive blocks :)

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html
(right):

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html:4:
<script src=/resources/testharness.js></script>
Do you think it's worth adding help links, like below?

<link rel="help" href="https://w3c.github.io/IndexedDB/#fire-success-event">
<link rel="help" href="https://w3c.github.io/IndexedDB/#fire-error-event">

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html:17:
const tx = db.transaction('s', 'readwrite');
It seems to me that lines 17-21 and 25-28 repeat in all tests. Do you think it's
worth extracting them in a function?

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html:117:
assert_true(second_listener_called);
This shows up in a few places. I imagine you could overwrite the onabort handler
set up by extracted function once this happens, though it's also reasonable to
just have the onabort handler spelled out in each test case.

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html:336:
throw Error('hi there');
It's slightly odd that this error has a message, whereas the other ones thrown
in this file don't. Does it make debugging easier? If so, would it make sense to
have the message everywhere?

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-upgradeneeded-event-exception.html
(right):

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-upgradeneeded-event-exception.html:8:
setup({allow_uncaught_exception:true});
Cool, I didn't know about this. I may need to add it to my abort transaction
tests too.

(no response needed)

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-upgradeneeded-event-exception.html:66:
let store;
I don't think you use store outside the block in 70-71

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js (right):

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:150: // to let
the transaction finish.
double "to", and I don't think this comment is accurate

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:161: 'Active
check should either not throw, or throw ' +
"not throw anything" would make the structure parallel with "should throw ...",
and is a bit easier to parse for me.

[jsbell, 2017-03-06 18:24:53]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2017-03-06 18:24:53]

The patchset sent to the CQ was uploaded after l-g-t-m from pwnall@chromium.org
Link to the patchset: https://codereview.chromium.org/2734533002/#ps20001
(title: "Test refactors per review")

[jsbell, 2017-03-06 18:25:03]

Thanks for the excellent feedback!

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html
(right):

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html:4:
<script src=/resources/testharness.js></script>
On 2017/03/04 00:27:33, pwnall wrote:
> Do you think it's worth adding help links, like below?
> 
> <link rel="help" href="https://w3c.github.io/IndexedDB/#fire-success-event">
> <link rel="help" href="https://w3c.github.io/IndexedDB/#fire-error-event">

Done.

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html:17:
const tx = db.transaction('s', 'readwrite');
On 2017/03/04 00:27:33, pwnall wrote:
> It seems to me that lines 17-21 and 25-28 repeat in all tests. Do you think
it's
> worth extracting them in a function?

Factored out wrapper functions here and in the other files.

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html:117:
assert_true(second_listener_called);
On 2017/03/04 00:27:33, pwnall wrote:
> This shows up in a few places. I imagine you could overwrite the onabort
handler
> set up by extracted function once this happens, though it's also reasonable to
> just have the onabort handler spelled out in each test case.

I kept the onabort handler explicit in each test.

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-error-event-exception.html:336:
throw Error('hi there');
On 2017/03/04 00:27:33, pwnall wrote:
> It's slightly odd that this error has a message, whereas the other ones thrown
> in this file don't. Does it make debugging easier? If so, would it make sense
to
> have the message everywhere?

Oops, I left that in from some debugging. Will remove.

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-upgradeneeded-event-exception.html
(right):

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/fire-upgradeneeded-event-exception.html:66:
let store;
On 2017/03/04 00:27:33, pwnall wrote:
> I don't think you use store outside the block in 70-71

Thanks, left over from some refactoring. Removed.

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js (right):

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:150: // to let
the transaction finish.
On 2017/03/04 00:27:33, pwnall wrote:
> double "to", and I don't think this comment is accurate

Thanks. And whoah, not sure what happened there.

https://codereview.chromium.org/2734533002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:161: 'Active
check should either not throw, or throw ' +
On 2017/03/04 00:27:33, pwnall wrote:
> "not throw anything" would make the structure parallel with "should throw
...",
> and is a bit easier to parse for me.

Done.

[commit-bot: I haz the power, 2017-03-06 18:25:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-03-06 18:46:33]

On 2017/03/06 18:25:03, jsbell wrote:
> Thanks for the excellent feedback!

Thank you for taking the time to address the feedback! The tests look beautiful!

[commit-bot: I haz the power, 2017-03-06 20:23:32]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488824693274750,
"parent_rev": "aeec4d330e053262b28e1559f2c052206aad3aad", "commit_rev":
"ed3d60a6f09642595391b6678d46f548b649e832"}

[commit-bot: I haz the power, 2017-03-06 20:24:19]

Description was changed from

==========
IndexedDB: Align abort behavior on uncaught exception with Gecko

Indexed DB specifies that an uncaught exception in an event handler
should abort the transaction. The spec states that this should happen
after dispatch, and Gecko agrees. Blink was aborting after the
specific handler aborted.

This change aligns Blink's behavior with Gecko and the spec, and
adds test coverage to web-platform-tests.

BUG=698342
R=pwnall@chromium.org
==========

to

==========
IndexedDB: Align abort behavior on uncaught exception with Gecko

Indexed DB specifies that an uncaught exception in an event handler
should abort the transaction. The spec states that this should happen
after dispatch, and Gecko agrees. Blink was aborting after the
specific handler aborted.

This change aligns Blink's behavior with Gecko and the spec, and
adds test coverage to web-platform-tests.

BUG=698342
R=pwnall@chromium.org

Review-Url: https://codereview.chromium.org/2734533002
Cr-Commit-Position: refs/heads/master@{#454937}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed3d60a6f09642595391b6678d46...
==========

[commit-bot: I haz the power, 2017-03-06 20:24:20]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ed3d60a6f09642595391b6678d46...