Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912593003:
[ServiceWorker] Fix bugs of wpt tests for Client.navigate (Closed)
|
Created:
3 years, 6 months ago by leonhsl(Using Gerrit) Modified:
3 years, 6 months ago CC:
blink-reviews, blink-reviews-w3ctests_chromium.org, chromium-reviews, falken+watch_chromium.org, haraken, horo+watch_chromium.org, jsbell+serviceworker_chromium.org, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, shimazu+serviceworker_chromium.org, tzik Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ServiceWorker] Fix bugs of wpt tests for Client.navigate
This CL
- modifies is_service_worker() in external/wpt/resources/testharness.js
so that frame.contentWindow.navigator.serviceWorker.controller CAN be
identified correctly as ServiceWorker object within main frame's
execution context, this ensures success of fetch_tests_from_worker,
which is called by several tests in client-navigate.https.html.
- throws TypeError if navigation failed for Client.navigate,
E.g. mixed-content violation.
BUG=658997
TEST=blink_tests
external/wpt/service-workers/service-worker/client-navigate.https.html
Review-Url: https://codereview.chromium.org/2912593003
Cr-Commit-Position: refs/heads/master@{#478561}
Committed: https://chromium.googlesource.com/chromium/src/+/3c433e7dca949e4df3ce288475d2cc5a6d134787
Patch Set 1 #Patch Set 2 : Fix other tests influnced by changes of testharness.js #
Total comments: 13
Patch Set 3 : Rebase only #Patch Set 4 : Catch navigation error #
Total comments: 8
Patch Set 5 : Address comments from falken@ #
Messages
Total messages: 46 (33 generated)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ServiceWorker] Fix wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError in case of mixed-content violation when calling Client.navigate. BUG=678905 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html ========== to ========== [ServiceWorker] Fix wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError in case of mixed-content violation when calling Client.navigate. BUG=678905 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ServiceWorker] Fix wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError in case of mixed-content violation when calling Client.navigate. BUG=678905 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html ========== to ========== [ServiceWorker] Fix bugs of wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError in case of mixed-content violation when calling Client.navigate. BUG=678905 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
leon.han@intel.com changed reviewers: + falken@chromium.org, shimazu@chromium.org
PTAL, Thanks. https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js:489: Object.prototype.toString.call(worker) == '[object ServiceWorker]'; Another optional solution without changing testharness.js here: Change client-navigate.https.html to pass reg.installing instead of frame.contentWindow.navigator.serviceWorker.controller as sw object when calling fetch_tests_from_worker(sw). I'd prefer to change is_service_worker() here.
falken@chromium.org changed reviewers: + mkwst@chromium.org
+mkwst for mixed content advice https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js:486: // The worker object may be from other execution context, s/other/another https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resources/testharness.js (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resources/testharness.js:486: // The worker object may be from other execution context, s/other/another https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:81: parsed_url)) { Can you point me to the test that this fixes? I assume mixed content means the service worker shouldn't navigate an iframe to an insecure URL if the iframe's parent is secure. This doesn't look like the correct way to detect mixed content. IsMixedContent() just checks if the first arg is an https url, and the second arg is secure. Since we're passing the service worker's origin as the first arg, I'd expect this to always reject a navigation to an http url. It also doesn't account for redirects or browser settings (I thnk you can choose to enable mixed content). I'm wondering if the W3C test is valid. Does the spec in fact require throwing a TypeError if Client.navigate() would create a mixed content violation?
https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js:489: Object.prototype.toString.call(worker) == '[object ServiceWorker]'; On 2017/05/28 at 09:12:59, leonhsl wrote: > Another optional solution without changing testharness.js here: > Change client-navigate.https.html to pass reg.installing instead of frame.contentWindow.navigator.serviceWorker.controller as sw object when calling fetch_tests_from_worker(sw). > > I'd prefer to change is_service_worker() here. I'm not sure we can do that via this update mechanism, both because it's in a separate repository, and because I'm not sure whether folks are on board with individual vendors changing central infrastructure without cross-org review. +foolip@ who will know whether we can upstream changes to `testharness.js` this way. https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:81: parsed_url)) { On 2017/05/29 at 01:07:04, falken wrote: > Can you point me to the test that this fixes? I assume mixed content means the service worker shouldn't navigate an iframe to an insecure URL if the iframe's parent is secure. > > This doesn't look like the correct way to detect mixed content. IsMixedContent() just checks if the first arg is an https url, and the second arg is secure. Since we're passing the service worker's origin as the first arg, I'd expect this to always reject a navigation to an http url. > > It also doesn't account for redirects or browser settings (I thnk you can choose to enable mixed content). > > I'm wondering if the W3C test is valid. Does the spec in fact require throwing a TypeError if Client.navigate() would create a mixed content violation? I don't have a strong opinion about what the test result should be (e.g. whether we should throw as a result of `navigate()`. I do believe that the navigation should fail, however, as it attempts to navigate an HTTPS page's child frame to an HTTP URL. As falken@ notes, however, this doesn't look like the right way to guarantee that. We have mixed content checks in `MixedContentNavigationThrottle` that should ensure the navigation fails. That seems like enough.
mkwst@chromium.org changed reviewers: + jeffcarp@chromium.org
+jeffcarp@ for the `testharness.js` question, as foolip@ is OOO.
Thanks a lot for review! https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js:486: // The worker object may be from other execution context, On 2017/05/29 01:07:04, falken wrote: > s/other/another Acknowledged. https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resources/testharness.js (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resources/testharness.js:486: // The worker object may be from other execution context, On 2017/05/29 01:07:04, falken wrote: > s/other/another Acknowledged. https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:81: parsed_url)) { On 2017/05/29 08:02:44, Mike West wrote: > On 2017/05/29 at 01:07:04, falken wrote: > > Can you point me to the test that this fixes? I assume mixed content means the > service worker shouldn't navigate an iframe to an insecure URL if the iframe's > parent is secure. > > > > This doesn't look like the correct way to detect mixed content. > IsMixedContent() just checks if the first arg is an https url, and the second > arg is secure. Since we're passing the service worker's origin as the first arg, > I'd expect this to always reject a navigation to an http url. > > > > It also doesn't account for redirects or browser settings (I thnk you can > choose to enable mixed content). > > > > I'm wondering if the W3C test is valid. Does the spec in fact require throwing > a TypeError if Client.navigate() would create a mixed content violation? > > I don't have a strong opinion about what the test result should be (e.g. whether > we should throw as a result of `navigate()`. I do believe that the navigation > should fail, however, as it attempts to navigate an HTTPS page's child frame to > an HTTP URL. As falken@ notes, however, this doesn't look like the right way to > guarantee that. We have mixed content checks in `MixedContentNavigationThrottle` > that should ensure the navigation fails. That seems like enough. Sorry now I realized that I've done a hurry&incorrect fix for this problem.. The corresponding test is at: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... The corresponding description in spec https://w3c.github.io/ServiceWorker/#client-navigate: 7.1 HandleNavigate: Navigate browsingContext to url with exceptions enabled. The source browsing context must be browsingContext. 7.2 If the algorithm steps invoked in the step labeled HandleNavigate throws an exception, set navigateFailed to true. 7.x XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX 8 XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX 9 If navigateFailed is true, reject promise with a TypeError and abort these steps. I think the wpt test itself is valid but I was somehow misunderstanding what mixed content means.. I agree with falken's all comments about mixed content. And just as Mike said, there is already some logic in navigation code ensuring failure for such case, and our wpt test fails just because SW impl is lack of logic catching the navigation failure and throwing TypeError in correspondence. Thank you all for all the explanations! I'll try the correct way later. And for sure I want to confirm with Mike about cases for redirects or browser settings, I suppose navigation code is already taking account of them? So SW impl has no need to detect them, but just catching potential navigation failure is OK. E.g. If user choosed to enable mixed content, then navigation code will navigate successfully(allow mixed content). Otherwise it will fail(Should be caught by our SW impl).
https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:81: parsed_url)) { On 2017/05/29 13:09:54, leonhsl (OOO until 09 June) wrote: > On 2017/05/29 08:02:44, Mike West wrote: > > On 2017/05/29 at 01:07:04, falken wrote: > > > Can you point me to the test that this fixes? I assume mixed content means > the > > service worker shouldn't navigate an iframe to an insecure URL if the iframe's > > parent is secure. > > > > > > This doesn't look like the correct way to detect mixed content. > > IsMixedContent() just checks if the first arg is an https url, and the second > > arg is secure. Since we're passing the service worker's origin as the first > arg, > > I'd expect this to always reject a navigation to an http url. > > > > > > It also doesn't account for redirects or browser settings (I thnk you can > > choose to enable mixed content). > > > > > > I'm wondering if the W3C test is valid. Does the spec in fact require > throwing > > a TypeError if Client.navigate() would create a mixed content violation? > > > > I don't have a strong opinion about what the test result should be (e.g. > whether > > we should throw as a result of `navigate()`. I do believe that the navigation > > should fail, however, as it attempts to navigate an HTTPS page's child frame > to > > an HTTP URL. As falken@ notes, however, this doesn't look like the right way > to > > guarantee that. We have mixed content checks in > `MixedContentNavigationThrottle` > > that should ensure the navigation fails. That seems like enough. > > Sorry now I realized that I've done a hurry&incorrect fix for this problem.. > The corresponding test is at: > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... > > The corresponding description in spec > https://w3c.github.io/ServiceWorker/#client-navigate: > 7.1 > HandleNavigate: Navigate browsingContext to url with exceptions enabled. The > source browsing context must be browsingContext. > 7.2 > If the algorithm steps invoked in the step labeled HandleNavigate throws an > exception, set navigateFailed to true. > 7.x > XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX > 8 > XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX > 9 > If navigateFailed is true, reject promise with a TypeError and abort these > steps. > > I think the wpt test itself is valid but I was somehow misunderstanding what > mixed content means.. I agree with falken's all comments about mixed content. > And just as Mike said, there is already some logic in navigation code ensuring > failure for such case, and our wpt test fails just because SW impl is lack of > logic catching the navigation failure and throwing TypeError in correspondence. > > Thank you all for all the explanations! I'll try the correct way later. > > And for sure I want to confirm with Mike about cases for redirects or browser > settings, I suppose navigation code is already taking account of them? So SW > impl has no need to detect them, but just catching potential navigation failure > is OK. > E.g. If user choosed to enable mixed content, then navigation code will navigate > successfully(allow mixed content). Otherwise it will fail(Should be caught by > our SW impl). Thanks for the investigation and links. Yes it seems to me Client.navigate() needs to detect errors during navigation and reject the promise. Apparently we're not doing that currently.
On 2017/05/29 at 14:29:35, falken wrote: > https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right): > > https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:81: parsed_url)) { > On 2017/05/29 13:09:54, leonhsl (OOO until 09 June) wrote: > > On 2017/05/29 08:02:44, Mike West wrote: > > > On 2017/05/29 at 01:07:04, falken wrote: > > > > Can you point me to the test that this fixes? I assume mixed content means > > the > > > service worker shouldn't navigate an iframe to an insecure URL if the iframe's > > > parent is secure. > > > > > > > > This doesn't look like the correct way to detect mixed content. > > > IsMixedContent() just checks if the first arg is an https url, and the second > > > arg is secure. Since we're passing the service worker's origin as the first > > arg, > > > I'd expect this to always reject a navigation to an http url. > > > > > > > > It also doesn't account for redirects or browser settings (I thnk you can > > > choose to enable mixed content). > > > > > > > > I'm wondering if the W3C test is valid. Does the spec in fact require > > throwing > > > a TypeError if Client.navigate() would create a mixed content violation? > > > > > > I don't have a strong opinion about what the test result should be (e.g. > > whether > > > we should throw as a result of `navigate()`. I do believe that the navigation > > > should fail, however, as it attempts to navigate an HTTPS page's child frame > > to > > > an HTTP URL. As falken@ notes, however, this doesn't look like the right way > > to > > > guarantee that. We have mixed content checks in > > `MixedContentNavigationThrottle` > > > that should ensure the navigation fails. That seems like enough. > > > > Sorry now I realized that I've done a hurry&incorrect fix for this problem.. > > The corresponding test is at: > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... > > > > The corresponding description in spec > > https://w3c.github.io/ServiceWorker/#client-navigate: > > 7.1 > > HandleNavigate: Navigate browsingContext to url with exceptions enabled. The > > source browsing context must be browsingContext. > > 7.2 > > If the algorithm steps invoked in the step labeled HandleNavigate throws an > > exception, set navigateFailed to true. > > 7.x > > XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX > > 8 > > XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX > > 9 > > If navigateFailed is true, reject promise with a TypeError and abort these > > steps. > > > > I think the wpt test itself is valid but I was somehow misunderstanding what > > mixed content means.. I agree with falken's all comments about mixed content. > > And just as Mike said, there is already some logic in navigation code ensuring > > failure for such case, and our wpt test fails just because SW impl is lack of > > logic catching the navigation failure and throwing TypeError in correspondence. > > > > Thank you all for all the explanations! I'll try the correct way later. > > > > And for sure I want to confirm with Mike about cases for redirects or browser > > settings, I suppose navigation code is already taking account of them? So SW > > impl has no need to detect them, but just catching potential navigation failure > > is OK. > > E.g. If user choosed to enable mixed content, then navigation code will navigate > > successfully(allow mixed content). Otherwise it will fail(Should be caught by > > our SW impl). > > Thanks for the investigation and links. Yes it seems to me Client.navigate() needs to detect errors during navigation and reject the promise. Apparently we're not doing that currently. I checked and I couldn't find any examples of previous exports changing testharness.js files. I can't say authoritatively whether it's ok, but it's easily revertible if you want to go ahead and land it then CC the relevant people on the GitHub PR.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAnL at ps#4, Thanks. https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/resources/testharness.js:486: // The worker object may be from other execution context, On 2017/05/29 13:09:54, leonhsl (OOO until 09 June) wrote: > On 2017/05/29 01:07:04, falken wrote: > > s/other/another > > Acknowledged. Done. https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resources/testharness.js (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resources/testharness.js:486: // The worker object may be from other execution context, On 2017/05/29 13:09:54, leonhsl (OOO until 09 June) wrote: > On 2017/05/29 01:07:04, falken wrote: > > s/other/another > > Acknowledged. Done. https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right): https://codereview.chromium.org/2912593003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:81: parsed_url)) { On 2017/05/29 14:29:35, falken wrote: > On 2017/05/29 13:09:54, leonhsl (OOO until 09 June) wrote: > > On 2017/05/29 08:02:44, Mike West wrote: > > > On 2017/05/29 at 01:07:04, falken wrote: > > > > Can you point me to the test that this fixes? I assume mixed content means > > the > > > service worker shouldn't navigate an iframe to an insecure URL if the > iframe's > > > parent is secure. > > > > > > > > This doesn't look like the correct way to detect mixed content. > > > IsMixedContent() just checks if the first arg is an https url, and the > second > > > arg is secure. Since we're passing the service worker's origin as the first > > arg, > > > I'd expect this to always reject a navigation to an http url. > > > > > > > > It also doesn't account for redirects or browser settings (I thnk you can > > > choose to enable mixed content). > > > > > > > > I'm wondering if the W3C test is valid. Does the spec in fact require > > throwing > > > a TypeError if Client.navigate() would create a mixed content violation? > > > > > > I don't have a strong opinion about what the test result should be (e.g. > > whether > > > we should throw as a result of `navigate()`. I do believe that the > navigation > > > should fail, however, as it attempts to navigate an HTTPS page's child frame > > to > > > an HTTP URL. As falken@ notes, however, this doesn't look like the right way > > to > > > guarantee that. We have mixed content checks in > > `MixedContentNavigationThrottle` > > > that should ensure the navigation fails. That seems like enough. > > > > Sorry now I realized that I've done a hurry&incorrect fix for this problem.. > > The corresponding test is at: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... > > > > The corresponding description in spec > > https://w3c.github.io/ServiceWorker/#client-navigate: > > 7.1 > > HandleNavigate: Navigate browsingContext to url with exceptions enabled. The > > source browsing context must be browsingContext. > > 7.2 > > If the algorithm steps invoked in the step labeled HandleNavigate throws an > > exception, set navigateFailed to true. > > 7.x > > XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX > > 8 > > XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX > > 9 > > If navigateFailed is true, reject promise with a TypeError and abort these > > steps. > > > > I think the wpt test itself is valid but I was somehow misunderstanding what > > mixed content means.. I agree with falken's all comments about mixed content. > > And just as Mike said, there is already some logic in navigation code ensuring > > failure for such case, and our wpt test fails just because SW impl is lack of > > logic catching the navigation failure and throwing TypeError in > correspondence. > > > > Thank you all for all the explanations! I'll try the correct way later. > > > > And for sure I want to confirm with Mike about cases for redirects or browser > > settings, I suppose navigation code is already taking account of them? So SW > > impl has no need to detect them, but just catching potential navigation > failure > > is OK. > > E.g. If user choosed to enable mixed content, then navigation code will > navigate > > successfully(allow mixed content). Otherwise it will fail(Should be caught by > > our SW impl). > > Thanks for the investigation and links. Yes it seems to me Client.navigate() > needs to detect errors during navigation and reject the promise. Apparently > we're not doing that currently. Done. https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:63: RunCallback(ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE); DidFinishNavigation() should definitely fire the callback even in error case, otherwise the callback will be pending. Based on comments at line48,49 of this file, pass (kInvalidUniqueID, MSG_ROUTING_NONE) back to indicate navigation error, this way will finally trigger Blink side to throw type error at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...
This lgtm, thanks for fixing this test. Please add 658997 to the BUG line since the CL removes a TestExpectation line that links to it. https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:62: if (!navigation_handle->HasCommitted()) { Add a comment like: // Return error. https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:63: RunCallback(ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE); On 2017/06/10 02:57:44, leonhsl wrote: > > DidFinishNavigation() should definitely fire the callback even in error case, > otherwise the callback will be pending. > > Based on comments at line48,49 of this file, pass (kInvalidUniqueID, > MSG_ROUTING_NONE) back to indicate navigation error, this way will finally > trigger Blink side to throw type error at > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic... According to HasCommitted() documentation, this also returns false if the navigation ended up being a download or 204/205 response. It also returns true if the navigation resulted in an error page. Do you know if we have test coverage for these kinds of navigations? I'm guessing our error handling for OpenWindow will need more rigor and testing but this looks OK as a step in the right direction. https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:67: if (navigation_handle->GetFrameTreeNodeId() != frame_tree_node_id_) { // Return error.
Description was changed from ========== [ServiceWorker] Fix bugs of wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError in case of mixed-content violation when calling Client.navigate. BUG=678905 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html ========== to ========== [ServiceWorker] Fix bugs of wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError in case of mixed-content violation when calling Client.navigate. BUG=658997 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html ==========
Description was changed from ========== [ServiceWorker] Fix bugs of wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError in case of mixed-content violation when calling Client.navigate. BUG=658997 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html ========== to ========== [ServiceWorker] Fix bugs of wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError if navigation failed for Client.navigate, E.g. mixed-content violation. BUG=658997 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html ==========
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, falken@, Thanks a lot for review! Uploaded ps#5 to address comments and updated CL description. And, about changing testharness.js, I'm not sure I fully understood what jeffcarp@ said, after landed this CL need/should I do some other additional work for testharness.js? https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:62: if (!navigation_handle->HasCommitted()) { On 2017/06/12 01:50:14, falken wrote: > Add a comment like: > // Return error. Done. https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:63: RunCallback(ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE); On 2017/06/12 01:50:14, falken wrote: > On 2017/06/10 02:57:44, leonhsl wrote: > > > > DidFinishNavigation() should definitely fire the callback even in error case, > > otherwise the callback will be pending. > > > > Based on comments at line48,49 of this file, pass (kInvalidUniqueID, > > MSG_ROUTING_NONE) back to indicate navigation error, this way will finally > > trigger Blink side to throw type error at > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic... > > According to HasCommitted() documentation, this also returns false if the > navigation ended up being a download or 204/205 response. It also returns true > if the navigation resulted in an error page. > > Do you know if we have test coverage for these kinds of navigations? > > I'm guessing our error handling for OpenWindow will need more rigor and testing > but this looks OK as a step in the right direction. I checked through but did not find any wpt tests against Client.navigate() for 'download or 204/205 response or displayed an error page', and seems there are't any wpts tests against Clients.openWindow(). For both Client.navigate() and Clients.openWindow(), I suppose the expected behavior should be as following? - download: throw type error, because no new page display, no Client will be returned back - 204/205 response: throw type error, because no new page display, no Client will be returned back - error page: success WDYT? Thanks. https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:67: if (navigation_handle->GetFrameTreeNodeId() != frame_tree_node_id_) { On 2017/06/12 01:50:14, falken wrote: > // Return error. Done.
I think it's OK to land this. The relevant people ought to be CC'd automatically on the GitHub PR. https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2912593003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:63: RunCallback(ChildProcessHost::kInvalidUniqueID, MSG_ROUTING_NONE); On 2017/06/12 07:18:12, leonhsl wrote: > On 2017/06/12 01:50:14, falken wrote: > > On 2017/06/10 02:57:44, leonhsl wrote: > > > > > > DidFinishNavigation() should definitely fire the callback even in error > case, > > > otherwise the callback will be pending. > > > > > > Based on comments at line48,49 of this file, pass (kInvalidUniqueID, > > > MSG_ROUTING_NONE) back to indicate navigation error, this way will finally > > > trigger Blink side to throw type error at > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic... > > > > According to HasCommitted() documentation, this also returns false if the > > navigation ended up being a download or 204/205 response. It also returns > true > > if the navigation resulted in an error page. > > > > Do you know if we have test coverage for these kinds of navigations? > > > > I'm guessing our error handling for OpenWindow will need more rigor and > testing > > but this looks OK as a step in the right direction. > > I checked through but did not find any wpt tests against Client.navigate() for > 'download or 204/205 response or displayed an error page', and seems there are't > any wpts tests against Clients.openWindow(). > > For both Client.navigate() and Clients.openWindow(), I suppose the expected > behavior should be as following? > - download: throw type error, because no new page display, no Client > will be returned back > - 204/205 response: throw type error, because no new page display, no Client > will be returned back > - error page: success > WDYT? Thanks. Not sure if error pages should be clients. But we could follow up on the spec and follow-up bugs.
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/2912593003/#ps80001 (title: "Address comments from falken@")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497253137554690,
"parent_rev": "387a6b29a641d6721774f860c25fe85c47f821c0", "commit_rev":
"3c433e7dca949e4df3ce288475d2cc5a6d134787"}
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Fix bugs of wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError if navigation failed for Client.navigate, E.g. mixed-content violation. BUG=658997 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html ========== to ========== [ServiceWorker] Fix bugs of wpt tests for Client.navigate This CL - modifies is_service_worker() in external/wpt/resources/testharness.js so that frame.contentWindow.navigator.serviceWorker.controller CAN be identified correctly as ServiceWorker object within main frame's execution context, this ensures success of fetch_tests_from_worker, which is called by several tests in client-navigate.https.html. - throws TypeError if navigation failed for Client.navigate, E.g. mixed-content violation. BUG=658997 TEST=blink_tests external/wpt/service-workers/service-worker/client-navigate.https.html Review-Url: https://codereview.chromium.org/2912593003 Cr-Commit-Position: refs/heads/master@{#478561} Committed: https://chromium.googlesource.com/chromium/src/+/3c433e7dca949e4df3ce288475d2... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3c433e7dca949e4df3ce288475d2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
