Infra: remove old service worker code from testharness.js #21162

annevk · 2020-01-14T14:34:57Z

No description provided.

jgraham

I don't know if there's a spec change here, but having one codepath rather than two seems strictly better.

stephenmcgruer · 2020-03-02T15:46:05Z

So it looks like this was added in 821d133, with no details or Chrome bug link as to why, but the comment clearly suggests that there was a Blink bug that made this necessary.

@annevk do we know that any such bug has been fixed? Did we run a trigger run of this commit to see what changes it makes to the WPT results on Chrome? It would have been nice to be able to provide some of that information in the commit message, to help future-us not have to perform code archaeology* years down the line to understand why a change was made.

Like the archaeology I just had to perform for that comment, since it didn't link any form of bug to track this claimed Blink problem.

annevk · 2020-03-02T15:54:23Z

I thought I checked with someone if that comment was still true for Chrome, but you're right that I didn't really do a good job of documenting that, including for myself. So I guess this is all we have and if that's not enough, we should revert.

stephenmcgruer · 2020-03-02T15:55:31Z

We can check the wpt.fyi results before and after (I will take the action item to do that), which should be sufficient.

annevk · 2020-03-02T15:58:18Z

Thanks @stephenmcgruer! Won't happen again.

stephenmcgruer · 2020-03-02T17:03:38Z

Diff is https://wpt.fyi/results/?diff&filter=ADC&run_id=451860003&run_id=423660009

There are a handful of TIMEOUT differences in service-worker/:

https://wpt.fyi/results/service-workers/service-worker/client-navigate.https.html?diff&filter=ADC&run_id=451860003&run_id=423660009
https://wpt.fyi/results/service-workers/service-worker/windowclient-navigate.https.html?diff&filter=ADC&run_id=451860003&run_id=423660009

But I'm not sure if these are real or flake. Might be worth running locally with/without the change and seeing if we can reproduce the diff, or we can just wait and see if they go away in subsequent runs.

stephenmcgruer · 2020-03-02T18:29:55Z

Looks like the Chromium import of this change also marked a few service-worker tests as TIMEOUT now - https://chromium-review.googlesource.com/c/chromium/src/+/2083053/4/third_party/blink/web_tests/TestExpectations

This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] #21162 [2] #22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7

This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] #21162 [2] #22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org> Cr-Commit-Position: refs/heads/master@{#760974}

This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] web-platform-tests/wpt#21162 [2] web-platform-tests/wpt#22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org> Cr-Commit-Position: refs/heads/master@{#760974}

This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] #21162 [2] #22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org> Cr-Commit-Position: refs/heads/master@{#760974}

…igate.https.html, a=testonly Automatic update from web-platform-tests ServiceWorker: Fix timeout on client-navigate.https.html This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] web-platform-tests/wpt#21162 [2] web-platform-tests/wpt#22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org> Cr-Commit-Position: refs/heads/master@{#760974} -- wpt-commits: 0a9ee5f5458c922aab15c25339db79e93df5c707 wpt-pr: 23105

…igate.https.html, a=testonly Automatic update from web-platform-tests ServiceWorker: Fix timeout on client-navigate.https.html This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] web-platform-tests/wpt#21162 [2] web-platform-tests/wpt#22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazuchromium.org> Commit-Queue: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#760974} -- wpt-commits: 0a9ee5f5458c922aab15c25339db79e93df5c707 wpt-pr: 23105 UltraBlame original commit: a1a73fefe890d9fd377e15fcac599508ea28e1c1

…igate.https.html, a=testonly Automatic update from web-platform-tests ServiceWorker: Fix timeout on client-navigate.https.html This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] web-platform-tests/wpt#21162 [2] web-platform-tests/wpt#22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazuchromium.org> Commit-Queue: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#760974} -- wpt-commits: 0a9ee5f5458c922aab15c25339db79e93df5c707 wpt-pr: 23105 UltraBlame original commit: e4f2dd72c208f41d4cccb483eeead7523e60e0fc

…igate.https.html, a=testonly Automatic update from web-platform-tests ServiceWorker: Fix timeout on client-navigate.https.html This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] web-platform-tests/wpt#21162 [2] web-platform-tests/wpt#22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazuchromium.org> Commit-Queue: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#760974} -- wpt-commits: 0a9ee5f5458c922aab15c25339db79e93df5c707 wpt-pr: 23105 UltraBlame original commit: a1a73fefe890d9fd377e15fcac599508ea28e1c1

…igate.https.html, a=testonly Automatic update from web-platform-tests ServiceWorker: Fix timeout on client-navigate.https.html This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] web-platform-tests/wpt#21162 [2] web-platform-tests/wpt#22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazuchromium.org> Commit-Queue: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#760974} -- wpt-commits: 0a9ee5f5458c922aab15c25339db79e93df5c707 wpt-pr: 23105 UltraBlame original commit: e4f2dd72c208f41d4cccb483eeead7523e60e0fc

…igate.https.html, a=testonly Automatic update from web-platform-tests ServiceWorker: Fix timeout on client-navigate.https.html This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] web-platform-tests/wpt#21162 [2] web-platform-tests/wpt#22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazuchromium.org> Commit-Queue: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#760974} -- wpt-commits: 0a9ee5f5458c922aab15c25339db79e93df5c707 wpt-pr: 23105 UltraBlame original commit: a1a73fefe890d9fd377e15fcac599508ea28e1c1

…igate.https.html, a=testonly Automatic update from web-platform-tests ServiceWorker: Fix timeout on client-navigate.https.html This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] web-platform-tests/wpt#21162 [2] web-platform-tests/wpt#22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazuchromium.org> Commit-Queue: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#760974} -- wpt-commits: 0a9ee5f5458c922aab15c25339db79e93df5c707 wpt-pr: 23105 UltraBlame original commit: e4f2dd72c208f41d4cccb483eeead7523e60e0fc

This CL fixes timeout on client-navigate.https.html, and removes unused code in testharness.js. This change is a follow-up for: [1] web-platform-tests/wpt#21162 [2] web-platform-tests/wpt#22086 After the change [1], fetch_tests_from_workers() helper uses ExtendableMessageEvent.source instead of MessageChannel to communicate between a window and a service worker. This change works well for most of cases, but fails some tests. The fetch_tests_from_workers() helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well as long as the given worker object is associated with `self`. However, these failing tests pass the worker object associated with the inner frame to the helper. In this case, the helper uses an inner frame's service worker object for postMessage(), while the helper waits on the main frame's navigator.serviceWorker.onmessage. Consequently event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout. To fix this, this CL makes the main frame pass its own service worker object instead of the inner frame's service worker object. Those objects should be equal other than associated contexts, and this change doesn't affect what the tests verify. Bug: 1057682 Change-Id: I0f30f1fe9c54c3de1006276f3445c5e2b92ea5a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155760 Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org> Cr-Commit-Position: refs/heads/master@{#760974} Former-commit-id: 502e37975b51c973355b1a62f14c076fa793bd95

Infra: remove old service worker code from testharness.js

d7032c0

wpt-pr-bot added infra testharness.js labels Jan 14, 2020

wpt-pr-bot assigned gsnedders Jan 14, 2020

wpt-pr-bot requested review from gsnedders and jgraham January 14, 2020 14:35

gsnedders closed this Jan 24, 2020

gsnedders deleted the annevk/remove-old-code branch January 24, 2020 18:01

gsnedders restored the annevk/remove-old-code branch January 24, 2020 18:42

Hexcles reopened this Jan 24, 2020

gsnedders removed their assignment Jan 31, 2020

annevk requested a review from wanderview February 19, 2020 14:59

jgraham approved these changes Mar 2, 2020

View reviewed changes

wpt-pr-bot deployed to wpt-preview-21162 March 2, 2020 15:17 View deployment

annevk merged commit ed0c310 into master Mar 2, 2020

annevk deleted the annevk/remove-old-code branch March 2, 2020 15:20

stephenmcgruer mentioned this pull request Apr 8, 2020

[WIP] Revert "Infra: remove old service worker code from testharness.js" #22086

Closed

chromium-wpt-export-bot mentioned this pull request Apr 20, 2020

ServiceWorker: Fix timeout on client-navigate.https.html #23105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infra: remove old service worker code from testharness.js #21162

Infra: remove old service worker code from testharness.js #21162

annevk commented Jan 14, 2020

jgraham left a comment

stephenmcgruer commented Mar 2, 2020

annevk commented Mar 2, 2020

stephenmcgruer commented Mar 2, 2020 •

edited

annevk commented Mar 2, 2020

stephenmcgruer commented Mar 2, 2020

stephenmcgruer commented Mar 2, 2020

Infra: remove old service worker code from testharness.js #21162

Infra: remove old service worker code from testharness.js #21162

Conversation

annevk commented Jan 14, 2020

jgraham left a comment

Choose a reason for hiding this comment

stephenmcgruer commented Mar 2, 2020

annevk commented Mar 2, 2020

stephenmcgruer commented Mar 2, 2020 • edited

annevk commented Mar 2, 2020

stephenmcgruer commented Mar 2, 2020

stephenmcgruer commented Mar 2, 2020

stephenmcgruer commented Mar 2, 2020 •

edited