Should add “script type” and “update via cache” when check two jobs are equivalent. #1358

d0iasm · 2018-10-04T05:45:39Z

In the current spec, we enqueue a new job to jobQueue if it is not equivalent to the last job of a jobQueue. Spec says ( Schedule Job 6. ):

Let lastJob be the element at the back of jobQueue.
2 .If job is equivalent to lastJob and lastJob’s job promise has not settled, append job to lastJob’s list of equivalent jobs.

Else, set job’s containing job queue to jobQueue, and enqueue job to jobQueue.

Also equivalent means in the spec ( dfn-job-equivalent ):

Two jobs are equivalent when their job type is the same and:

For register and update jobs, both their scope url and the script url are the same.

For unregister jobs, their scope url is the same.

I think script type and update via cache should also be the same for equivalent register and update jobs. Because we might ignore a new job which has different script type/update via cache if we do not add this check.

mfalken · 2018-10-05T00:30:47Z

Yes this also looks like an oversight. Did you have WPT for this also?

To sheriff: Please re-disable the test if it's still flaky. In this test file, some tests start re-registering the second service worker before the first service worker gets activated. In my theory, this sometimes squashes the second registration job into the first registration job, and results in test flakiness. This CL makes sure the second register runs after the first service worker gets activated. Note that it would be the correct behavior that the second registration job isn't squashed when the script type is changed. There is a spec issue about this: w3c/ServiceWorker#1358 Bug: 901317 Change-Id: I7ce379071a35ef9aeb98e4492d651ee6fc4714ec Reviewed-on: https://chromium-review.googlesource.com/c/1328546 Reviewed-by: Makoto Shimazu <[email protected]> Commit-Queue: Hiroki Nakagawa <[email protected]> Cr-Commit-Position: refs/heads/master@{#607494}

… sure to update a registration with different script type and identical script content., a=testonly Automatic update from web-platform-testsServiceWorker: Add new WPT tests to make sure to update a registration with different script type and identical script content. These tests check that a registration is updated correctly with different script type. At first Service Worker is registered as classic script type, then it is re-registered as module script type, and vice versa. A main script is identical. Bug: 824647 Change-Id: I2a3f87da1013f84c6e9495f362899dfe6ab97b45 Reviewed-on: https://chromium-review.googlesource.com/c/1298822 Commit-Queue: Asami Doi <[email protected]> Reviewed-by: Hiroki Nakagawa <[email protected]> Reviewed-by: Matt Falkenhagen <[email protected]> Cr-Commit-Position: refs/heads/master@{#604551} -- ServiceWorker: Deflake update-with-script-type.https.html To sheriff: Please re-disable the test if it's still flaky. In this test file, some tests start re-registering the second service worker before the first service worker gets activated. In my theory, this sometimes squashes the second registration job into the first registration job, and results in test flakiness. This CL makes sure the second register runs after the first service worker gets activated. Note that it would be the correct behavior that the second registration job isn't squashed when the script type is changed. There is a spec issue about this: w3c/ServiceWorker#1358 Bug: 901317 Change-Id: I7ce379071a35ef9aeb98e4492d651ee6fc4714ec Reviewed-on: https://chromium-review.googlesource.com/c/1328546 Reviewed-by: Makoto Shimazu <[email protected]> Commit-Queue: Hiroki Nakagawa <[email protected]> Cr-Commit-Position: refs/heads/master@{#607494} -- wpt-commits: 5abc7eaa765521804395067e8348e829ac2cb130, 3571d11d7c5201b454c2f3365fae04931ff6c03a wpt-pr: 13755

This makes the following changes: * updateViaCache is a live property. This is covered by the WPT registration-updateviacache.https.html. However we are missing tests for a register() that rejects with a new updateViaCache property. The desired behavior is to only update the property if register() resolves. * Changing worker type bypasses the byte-for-byte update check. This is covered by WPT update-registration-with-type.https.html. * Adds Worker type and updateViaCache to the job equivalence check. This probably can have a WPT for successive register() calls that vary these properties don’t get coalesced into a single job. Addresses w3c#1189, w3c#1408, w3c#1359, and w3c#1358.

…1411) This makes the following changes: * updateViaCache is a live property. This is mostly covered by the WPT registration-updateviacache.https.html. A test that the updateViaCache change is only committed if the register() call resolves is added in web-platform-tests/wpt#17131. * Changing worker type bypasses the byte-for-byte update check. This is covered by WPT update-registration-with-type.https.html. * Adds Worker type and updateViaCache to the job equivalence check. A test for this is added in web-platform-tests/wpt#17131. Addresses #1189, #1408, #1359, and #1358.

mfalken · 2019-06-03T08:29:54Z

Addressed in #1411

… sure to update a registration with different script type and identical script content., a=testonly Automatic update from web-platform-testsServiceWorker: Add new WPT tests to make sure to update a registration with different script type and identical script content. These tests check that a registration is updated correctly with different script type. At first Service Worker is registered as classic script type, then it is re-registered as module script type, and vice versa. A main script is identical. Bug: 824647 Change-Id: I2a3f87da1013f84c6e9495f362899dfe6ab97b45 Reviewed-on: https://chromium-review.googlesource.com/c/1298822 Commit-Queue: Asami Doi <asamidoigoogle.com> Reviewed-by: Hiroki Nakagawa <nhirokichromium.org> Reviewed-by: Matt Falkenhagen <falkenchromium.org> Cr-Commit-Position: refs/heads/master{#604551} -- ServiceWorker: Deflake update-with-script-type.https.html To sheriff: Please re-disable the test if it's still flaky. In this test file, some tests start re-registering the second service worker before the first service worker gets activated. In my theory, this sometimes squashes the second registration job into the first registration job, and results in test flakiness. This CL makes sure the second register runs after the first service worker gets activated. Note that it would be the correct behavior that the second registration job isn't squashed when the script type is changed. There is a spec issue about this: w3c/ServiceWorker#1358 Bug: 901317 Change-Id: I7ce379071a35ef9aeb98e4492d651ee6fc4714ec Reviewed-on: https://chromium-review.googlesource.com/c/1328546 Reviewed-by: Makoto Shimazu <shimazuchromium.org> Commit-Queue: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#607494} -- wpt-commits: 5abc7eaa765521804395067e8348e829ac2cb130, 3571d11d7c5201b454c2f3365fae04931ff6c03a wpt-pr: 13755 UltraBlame original commit: a02d1c15aaed438c907ba519efe7c9f5f44345cb

mfalken self-assigned this Jun 3, 2019

mfalken mentioned this issue Jun 3, 2019

Improve "update via cache" and "worker type" in register algorithms. #1411

Merged

mfalken closed this as completed Jun 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Should add “script type” and “update via cache” when check two jobs are equivalent. #1358

Should add “script type” and “update via cache” when check two jobs are equivalent. #1358

d0iasm commented Oct 4, 2018

mfalken commented Oct 5, 2018

Uh oh!

mfalken commented Jun 3, 2019

Uh oh!

Should add “script type” and “update via cache” when check two jobs are equivalent. #1358

Should add “script type” and “update via cache” when check two jobs are equivalent. #1358

Comments

d0iasm commented Oct 4, 2018

mfalken commented Oct 5, 2018

Uh oh!

mfalken commented Jun 3, 2019

Uh oh!