Upstream service worker indexedDB tests to WPT

Upstream service worker indexedDB tests to WPT

The Web Platform Tests project includes a version that is roughly
equivalent, but it does not explicitly extend the lifetime of the
"message" event handler. Introduce this logic in the upstream version to
discourage worker termination.

Introduce additional improvements not previously implemented by either version:

- Schedule promise rejection in response to failed IndexedDB operations in
  order to improve reporting under failure conditions.
- Consistently destroy database following test execution

Remove the Chromium-specific version of this test.

BUG=688116
R=mek@chromium.org

Review-Url: https://codereview.chromium.org/2867573002
Cr-Commit-Position: refs/heads/master@{#476082}
Committed: https://chromium.googlesource.com/chromium/src/+/35aa2825fa9b58a5f89b53e4a730bd2327cb108b

[mike3, 2017-05-05 22:53:41]

Hi Mek,

The change set is a little noisier than it needs to be due to indentation
changes. Here's an alternate version generated with git's `-w` flag:

https://gist.github.com/jugglinmike/ca0b018b79db5964ec45a2a8f5fc4579

[Marijn Kruisselbrink, 2017-05-06 00:12:55]

I know it's already a problem in the current test, but it would be nice if the
test could also be fixed to clean up after itself (i.e. delete the database it
created).

https://codereview.chromium.org/2867573002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/indexeddb-worker.js
(right):

https://codereview.chromium.org/2867573002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/indexeddb-worker.js:7:
}));
should probably also postMessage something back to the port when the promise
returned by doIndexedDBTest rejects (and then verify in the page that the right
message was received)?

[mike3, 2017-05-08 20:58:36]

Description was changed from

==========
Upstream service worker indexedDB tests to WPT

The Web Platform Tests project includes a version that is roughly
equivalent, but it does not explicitly extend the lifetime of the
"message" event handler. Introduce this logic in the upstream version to
discourage worker termination.

Additionally, schedule promise rejection in response to failed IndexedDB
operations in order to improve reporting under failure conditions.

Remove the Chromium-specific version of this test.

BUG=688116
R=mek@chromium.org
==========

to

==========
Upstream service worker indexedDB tests to WPT

The Web Platform Tests project includes a version that is roughly
equivalent, but it does not explicitly extend the lifetime of the
"message" event handler. Introduce this logic in the upstream version to
discourage worker termination.

Introduce additional improvements not previously implemented by either version:

- Schedule promise rejection in response to failed IndexedDB operations in
  order to improve reporting under failure conditions.
- Consistently destroy database following test execution

Remove the Chromium-specific version of this test.

BUG=688116
R=mek@chromium.org
==========

[mike3, 2017-05-08 21:02:48]

> I know it's already a problem in the current test, but it would be nice if
> the test could also be fixed to clean up after itself (i.e. delete the
> database it created).

Sure. I tried to limit change by using the existing test structure (honest!),
but handling all the potential paths via callback functions got to be too
distracting. I've refactored the test to use Promises and the "cleanup test"
approach we've been leaning on for the last few patches. (I still don't like
authoring inter-dependent tests like this, but I haven't had the time to extend
the harness with the functionality we need here. I will soon, hopefully.)

https://codereview.chromium.org/2867573002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/indexeddb-worker.js
(right):

https://codereview.chromium.org/2867573002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/indexeddb-worker.js:7:
}));
On 2017/05/06 00:12:54, Marijn Kruisselbrink wrote:
> should probably also postMessage something back to the port when the promise
> returned by doIndexedDBTest rejects (and then verify in the page that the
right
> message was received)?

Done.

[Marijn Kruisselbrink, 2017-05-16 18:17:44]

sorry for the delay in reviewing

https://codereview.chromium.org/2867573002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/indexeddb.https.html
(right):

https://codereview.chromium.org/2867573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/indexeddb.https.html:63:
return send(worker, 'cleanup');
Does this work? The registration.unregister() from above should have processed
by the time this code executes, which means (I believe) that the worker should
be marked as "redundant" by here. And in that case calling postMessage should
throw InvalidStateError rather than actually sending a message.

(apparently either my analysis of what is supposed to happen, or chromes
implementation are wrong, since I assume you ran the test and it didn't fail)

I think it would be cleaner to just have a single cleanup, which both cleans up
IDB and unregisters the service worker; or alternatively don't rely on the
service worker to do the IDB cleanup, since the document can do it just directly
here.

[mike3, 2017-05-17 22:07:40]

Figuring out why the original version worked took the better part of my day!
It's amazing how complex these operations can be.

Thanks for the review, and also thanks for helping me interpret the
specification. I think this is ready for another look.

https://codereview.chromium.org/2867573002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/indexeddb.https.html
(right):

https://codereview.chromium.org/2867573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/indexeddb.https.html:63:
return send(worker, 'cleanup');


On 2017/05/16 18:17:44, Marijn Kruisselbrink wrote:
> Does this work? The registration.unregister() from above should have processed
> by the time this code executes, which means (I believe) that the worker should
> be marked as "redundant" by here. And in that case calling postMessage should
> throw InvalidStateError rather than actually sending a message.
> 
> (apparently either my analysis of what is supposed to happen, or chromes
> implementation are wrong, since I assume you ran the test and it didn't fail)

It does indeed work, but I think the state we're discussing may be a result of
a spec bug. I'm going to file a report against the specification shortly. In
the mean time, we should certainly avoid entering this state.

> I think it would be cleaner to just have a single cleanup, which both cleans
up
> IDB and unregisters the service worker; or alternatively don't rely on the
> service worker to do the IDB cleanup, since the document can do it just
directly
> here.

I tried the former approach because I wanted to avoid duplicating database
deletion logic already available in the worker. Doing this correctly introduced
a surprising amount of complexity (the patch is attached if you're curious), so
I decided to bite the bullet and re-implement the 4-line function.

[Marijn Kruisselbrink, 2017-05-30 19:24:41]

sorry for not getting back to this sooner. This looks good, but I assume you
didn't mean to include your test for posting to a redundant worker in this CL?

https://codereview.chromium.org/2867573002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/postmessage.https.html
(right):

https://codereview.chromium.org/2867573002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/postmessage.https.html:60:

did you intend to have this as part of this change?

[mike3, 2017-05-30 21:22:32]

https://codereview.chromium.org/2867573002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/postmessage.https.html
(right):

https://codereview.chromium.org/2867573002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/postmessage.https.html:60:

On 2017/05/30 19:24:41, Marijn Kruisselbrink wrote:
> did you intend to have this as part of this change? 

No, I did not. This was part of my research regarding your previous review. (I
mentioned that I would file an issue against the specification, but I never
shared a link here. That issue is available at
https://github.com/w3c/ServiceWorker/issues/1146 .) It's not appropriate for
this change set, though, so I've removed it.

[Marijn Kruisselbrink, 2017-05-30 21:48:59]

lgtm

[mike3, 2017-05-30 21:51:03]

The CQ bit was checked by mike@mikepennisi.com

[commit-bot: I haz the power, 2017-05-30 21:52:02]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-30 21:55:00]

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

[commit-bot: I haz the power, 2017-05-30 21:55:01]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mike3, 2017-05-30 22:00:29]

The CQ bit was unchecked by mike@mikepennisi.com

[mike3, 2017-05-30 22:05:48]

Hi again Mek,

The previous patch was doomed to fail due to a merge conflict from a recent
change in `master`. Specifically, commit
dbbb4e564b1412421f3a7b70cd8fa267581b3dea removed the same line from
`third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG`
that this patch was removing. That conflict was easy to resolve and pretty
uncontroversial to boot. Still, I wanted to make you aware of this before I
initiated another attempt to merge. Can I start a new run?

[Marijn Kruisselbrink, 2017-05-30 22:10:05]

On 2017/05/30 at 22:05:48, mike wrote:
> Hi again Mek,
> 
> The previous patch was doomed to fail due to a merge conflict from a recent
> change in `master`. Specifically, commit
> dbbb4e564b1412421f3a7b70cd8fa267581b3dea removed the same line from
>
`third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG`
> that this patch was removing. That conflict was easy to resolve and pretty
> uncontroversial to boot. Still, I wanted to make you aware of this before I
> initiated another attempt to merge. Can I start a new run?

yes, still LGTM

[mike3, 2017-05-30 22:21:34]

The CQ bit was checked by mike@mikepennisi.com

[commit-bot: I haz the power, 2017-05-30 22:22:10]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-30 22:33:01]

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

[commit-bot: I haz the power, 2017-05-30 22:33:02]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mike3, 2017-05-31 16:26:31]

Hi Mek,

Can you believe it? Yesterday, in the time between rebasing (where I was
*removing* a change to an "expectations" file) and submitting, another patch
landed to `master` with conflicts, meaning I needed to rebase again (this
time *adding* a change to an "expectations" file). I know this is fairly
routine, but I've set a precedent for verifying these conflict resolutions
with you, so would you mind verifying?

[Marijn Kruisselbrink, 2017-05-31 18:00:25]

On 2017/05/31 at 16:26:31, mike wrote:
> Hi Mek,
> 
> Can you believe it? Yesterday, in the time between rebasing (where I was
> *removing* a change to an "expectations" file) and submitting, another patch
> landed to `master` with conflicts, meaning I needed to rebase again (this
> time *adding* a change to an "expectations" file). I know this is fairly
> routine, but I've set a precedent for verifying these conflict resolutions
> with you, so would you mind verifying?

Hah, still LGTM. Not sure if the wpt version of this test should also be listed
in FlagExpectations for that flag, but even if they should I think it's fine to
not do so in this CL.

[mike3, 2017-05-31 18:38:50]

The CQ bit was checked by mike@mikepennisi.com

[commit-bot: I haz the power, 2017-05-31 18:39:47]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-31 21:36:33]

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

[commit-bot: I haz the power, 2017-05-31 21:36:34]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mike3, 2017-05-31 21:49:10]

The CQ bit was checked by mike@mikepennisi.com

[commit-bot: I haz the power, 2017-05-31 21:50:14]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-31 23:14:42]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496267350009670,
"parent_rev": "cb36afa5925c2971e27b9653802a74264d76fa2b", "commit_rev":
"35aa2825fa9b58a5f89b53e4a730bd2327cb108b"}

[commit-bot: I haz the power, 2017-05-31 23:14:52]

Description was changed from

==========
Upstream service worker indexedDB tests to WPT

The Web Platform Tests project includes a version that is roughly
equivalent, but it does not explicitly extend the lifetime of the
"message" event handler. Introduce this logic in the upstream version to
discourage worker termination.

Introduce additional improvements not previously implemented by either version:

- Schedule promise rejection in response to failed IndexedDB operations in
  order to improve reporting under failure conditions.
- Consistently destroy database following test execution

Remove the Chromium-specific version of this test.

BUG=688116
R=mek@chromium.org
==========

to

==========
Upstream service worker indexedDB tests to WPT

The Web Platform Tests project includes a version that is roughly
equivalent, but it does not explicitly extend the lifetime of the
"message" event handler. Introduce this logic in the upstream version to
discourage worker termination.

Introduce additional improvements not previously implemented by either version:

- Schedule promise rejection in response to failed IndexedDB operations in
  order to improve reporting under failure conditions.
- Consistently destroy database following test execution

Remove the Chromium-specific version of this test.

BUG=688116
R=mek@chromium.org

Review-Url: https://codereview.chromium.org/2867573002
Cr-Commit-Position: refs/heads/master@{#476082}
Committed:
https://chromium.googlesource.com/chromium/src/+/35aa2825fa9b58a5f89b53e4a730...
==========

[commit-bot: I haz the power, 2017-05-31 23:14:54]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/35aa2825fa9b58a5f89b53e4a730...

[mike3, 2017-05-31 23:25:14]

On 2017/05/31 23:14:54, commit-bot: I haz the power wrote:
> Committed patchset #7 (id:120001) as
>
https://chromium.googlesource.com/chromium/src/+/35aa2825fa9b58a5f89b53e4a730...

Thanks for your patience, Mek!