S13nSW: service-workers/service-worker/navigation-preload/broken-chunked-encoding.https.html is failing |
||||||
Issue descriptionThis is because we resolve the FetchEvent#preloadResponse promise first then tries to reject it later: Resolving the promise occurs as follows: ServiceWorkerNavigationLoader::StartResponse() -> URLLoaderClient::OnStartLoadingResponseBody() -> (mojo IPC) -> ServiceWorkerContextClient::NavigationPreloadRequest::OnStartLoadingResponseBody() -> ServiceWorkerGlobalScopeProxy::OnNavigationPreloadResponse() -> FetchEvent::OnNavigationPreloadResponse() -> Resolve preloadResponse At this point the net service doesn't read chunked body so decode errors aren't detected yet. After that, rejecting the promise occurs as follows: ServiceWorkerNavigationLoader::Cancel() -> URLLoaderClient::OnComplete(ERR_ABORTED) -> (mojo IPC) -> ServiceWorkerContextClient::NavigationPreloadRequest::OnComplete(ERR_ABORTED) -> ServiceWorkerGlobalScopeProxy::OnNavigationPreloadError() -> FetchEvent::OnNavigationPreloadError() I haven't took a look at who calls SWNavigationLoader::Cancel() but looks like a mojo connection error handler calls it. Since URLLoaderClient::OnStartLoadingResponseBody() is network service specific, this doesn't occur if S13nSW is disabled. I'm not sure what's a right way to fix this. Probably not resolving the promise in FetchEvent::OnNavigationPreloadResponse() ?
,
Apr 9 2018
A response promise is supposed to resolve once the headers resolve, so resolving it in OnNavigationPreloadResponse sounds right. Therefore... I suspect the test is wrong. I don't 100% know what "broken chunked encoding" is but that .asis file seems to have correct headers followed by presumably a broken body.
,
Apr 10 2018
,
Apr 10 2018
Thanks for the info. We've chatted offline and we are going to update the test. This test will be passing with S13nSW but not w/o S13nSW.
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f4b08ee3d87eaee323d5571d89046d4baee5a8d4 commit f4b08ee3d87eaee323d5571d89046d4baee5a8d4 Author: Kenichi Ishibashi <bashi@chromium.org> Date: Thu Apr 12 07:05:00 2018 S13nSW: Update wpt/service-workers/service-worker/navigation-preload/broken-chunked-encoding.https.html Before this CL, the test expected that preloadResponse is rejected when decoding a chunked body is failed. However preloadResponse is supposed to be resolved once the headers are arrived. It should be resolved instead of rejected. Update the test to expect that preloadResponse is resolved even for broken chunked body. With S13nSW enabled, this test will start passing but it wouldn't pass w/o S13nSW. This CL also adds a test case where broken chunked body is transfered with delays. Bug: 830472 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: Ibc2909dfc4537f209417a13cff63898ad39229a7 Reviewed-on: https://chromium-review.googlesource.com/1004562 Commit-Queue: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#550061} [modify] https://crrev.com/f4b08ee3d87eaee323d5571d89046d4baee5a8d4/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService [modify] https://crrev.com/f4b08ee3d87eaee323d5571d89046d4baee5a8d4/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/f4b08ee3d87eaee323d5571d89046d4baee5a8d4/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/broken-chunked-encoding.https.html [modify] https://crrev.com/f4b08ee3d87eaee323d5571d89046d4baee5a8d4/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/broken-chunked-encoding-worker.js [modify] https://crrev.com/f4b08ee3d87eaee323d5571d89046d4baee5a8d4/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/chunked-encoding-scope.py
,
Apr 16 2018
,
May 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf482513cc7e9bf3a3e1352ae54188e280d5a19c commit bf482513cc7e9bf3a3e1352ae54188e280d5a19c Author: Matt Falkenhagen <falken@chromium.org> Date: Mon May 28 04:15:49 2018 Gardening: NetS13nServiceWorker: broken-chunked-encoding is passing. This passes under NetworkService and under S13nServiceWorker. Bug: 830472 Change-Id: I3d912df5d4818b35c702220b863c8d68b8ac0d88 TBR: shimazu Reviewed-on: https://chromium-review.googlesource.com/1074783 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#562181} [modify] https://crrev.com/bf482513cc7e9bf3a3e1352ae54188e280d5a19c/third_party/WebKit/LayoutTests/TestExpectations
,
Jul 11
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=webkit_layout_tests&tests=broken-chunked-encoding.https.html seems to indicate this test is still failing, and it is still listed as an exception in TestExpectations Could you update the expectation to a correct bug and close this one, or re-triage?
,
Jul 12
I see how that's confusing. Created issue 862886 and will update TestExpectations.
,
Jul 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/349d297d20d841791815fd8b86b87f02e755bd22 commit 349d297d20d841791815fd8b86b87f02e755bd22 Author: Matt Falkenhagen <falken@chromium.org> Date: Thu Jul 12 04:20:56 2018 Gardening: Revise bug # for broken-chunked-encoding.https.html. NOTRY=true Bug: 830472 , 862886 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: Ia6b50e1730d1f5dcb4f84b5c18d47003565fd838 TBR: bashi Reviewed-on: https://chromium-review.googlesource.com/1134627 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#574485} [modify] https://crrev.com/349d297d20d841791815fd8b86b87f02e755bd22/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService [modify] https://crrev.com/349d297d20d841791815fd8b86b87f02e755bd22/third_party/WebKit/LayoutTests/TestExpectations |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bashi@chromium.org
, Apr 9 2018