DCHECK() fails when SW of the inner url doesn't call respondWith(). |
|||||||||
Issue descriptionChrome Version: 71.0.3578.0 (Developer Build) built with dcheck_always_on=true What steps will reproduce the problem? (1) Enable chrome://flags/#allow-sxg-certs-without-extension (2) Enable chrome://flags/#enable-service-worker-servicification or chrome://flags/#network-service (3) Go https://sxg-demo.horo.jp/amptest/sw.html (4) Click "register SW with no-respond-with" (5) Go https://htxg-b1.appspot.com/sxg/amptest.sxg What is the expected result? The inner response of the SXG should be shown. What happens instead? Crashes at "DCHECK(!signed_exchange_loader_);" in SignedExchangeRequestHandler::MaybeCreateLoaderForResponse()
,
Oct 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0fd45315adaa2d4a801d27af02e04aa257751d51 commit 0fd45315adaa2d4a801d27af02e04aa257751d51 Author: Tsuyoshi Horo <horo@chromium.org> Date: Tue Oct 16 01:45:20 2018 Skip other interceptors after SignedExchangeRequestHandler started handling the request Currently when both service-worker-servicification and network-service are not enabled, the internally redirected request doesn't go to the service worker. But when service-worker-servicification or network-service is enabled, the internally redirected request goes to the service worker. We may support service worker integration, like FetchEvent's preloadResponse in future. But we don't have plan to support service worker integration without service-worker-servicification. To make this behavior consistent, this CL makes NavigationURLLoaderImpl::URLLoaderRequestController skip sending the redirected request of signed exchange to the service worker. Bug: 894755 Change-Id: I6f22819655169b4d3c30de0aa269911111c45586 Reviewed-on: https://chromium-review.googlesource.com/c/1278433 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/heads/master@{#599815} [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/appcache/appcache_request_handler.cc [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/appcache/appcache_request_handler.h [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/loader/navigation_loader_interceptor.cc [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/loader/navigation_loader_interceptor.h [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/loader/navigation_url_loader_impl.cc [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/loader/navigation_url_loader_impl_unittest.cc [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/service_worker/embedded_worker_instance.cc [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/service_worker/embedded_worker_instance.h [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/shared_worker/shared_worker_script_loader.cc [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/web_package/signed_exchange_request_handler.cc [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/web_package/signed_exchange_request_handler.h [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/browser/web_package/signed_exchange_request_handler_browsertest.cc [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/public/test/url_loader_interceptor.cc [modify] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/public/test/url_loader_interceptor.h [add] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/test/data/sxg/publisher-service-worker.html [add] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/test/data/sxg/publisher-service-worker.js [add] https://crrev.com/0fd45315adaa2d4a801d27af02e04aa257751d51/content/test/data/sxg/publisher-service-worker.js.mock-http-headers
,
Oct 16
,
Oct 16
I want to merge the change 0fd45315adaa2d4a801d27af02e04aa257751d51 to M71 after baking for a while on Canary.
,
Oct 17
0fd45315adaa2d4a801d27af02e04aa257751d51 landed in 72.0.3582.0. I verified that the internally redirected request doesn't go to the service worker for each cases. - NetworkService: off, ServiceWorkerServicification: off - NetworkService: off, ServiceWorkerServicification: on - NetworkService: on, ServiceWorkerServicification: on 1. Go https://sxg-demo.horo.jp/amptest/sw.html 2. Click "register return-generated-response SW" 3. Open https://htxg-b1.appspot.com/sxg/amptest.sxg?v=1b2 4. Redirected to https://sxg-demo.horo.jp/amptest/amptest.html and "Beautiful autumn view (sxg)" is shown. 5. Check that the SW's fetch event handler doesn't executed.
,
Oct 17
,
Oct 17
govind@ I don't know why but sheriffbot@chromium.org doesn't detect this issue's Merge-Request label. Is it OK to merge 0fd45315adaa2d4a801d27af02e04aa257751d51 to M71?
,
Oct 17
Approving merge for 0fd45315adaa2d4a801d27af02e04aa257751d51 to M71 branch 3578 based on comment #5. Pls merge. Thank you.
,
Oct 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0 commit 789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0 Author: Tsuyoshi Horo <horo@chromium.org> Date: Wed Oct 17 05:48:57 2018 [Merge to M71] Skip other interceptors after SignedExchangeRequestHandler started handling the request Currently when both service-worker-servicification and network-service are not enabled, the internally redirected request doesn't go to the service worker. But when service-worker-servicification or network-service is enabled, the internally redirected request goes to the service worker. We may support service worker integration, like FetchEvent's preloadResponse in future. But we don't have plan to support service worker integration without service-worker-servicification. To make this behavior consistent, this CL makes NavigationURLLoaderImpl::URLLoaderRequestController skip sending the redirected request of signed exchange to the service worker. Bug: 894755 Change-Id: I6f22819655169b4d3c30de0aa269911111c45586 Reviewed-on: https://chromium-review.googlesource.com/c/1278433 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#599815}(cherry picked from commit 0fd45315adaa2d4a801d27af02e04aa257751d51) Reviewed-on: https://chromium-review.googlesource.com/c/1286010 Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#83} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/appcache/appcache_request_handler.cc [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/appcache/appcache_request_handler.h [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/loader/navigation_loader_interceptor.cc [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/loader/navigation_loader_interceptor.h [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/loader/navigation_url_loader_impl.cc [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/loader/navigation_url_loader_impl_unittest.cc [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/service_worker/embedded_worker_instance.cc [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/service_worker/embedded_worker_instance.h [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/shared_worker/shared_worker_script_loader.cc [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/web_package/signed_exchange_request_handler.cc [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/web_package/signed_exchange_request_handler.h [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/browser/web_package/signed_exchange_request_handler_browsertest.cc [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/public/test/url_loader_interceptor.cc [modify] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/public/test/url_loader_interceptor.h [add] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/test/data/sxg/publisher-service-worker.html [add] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/test/data/sxg/publisher-service-worker.js [add] https://crrev.com/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0/content/test/data/sxg/publisher-service-worker.js.mock-http-headers
,
Oct 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0 Commit: 789d1f3823ae0a4ec8ab5f544a4449cedfa7f5e0 Author: horo@chromium.org Commiter: horo@chromium.org Date: 2018-10-17 05:48:57 +0000 UTC [Merge to M71] Skip other interceptors after SignedExchangeRequestHandler started handling the request Currently when both service-worker-servicification and network-service are not enabled, the internally redirected request doesn't go to the service worker. But when service-worker-servicification or network-service is enabled, the internally redirected request goes to the service worker. We may support service worker integration, like FetchEvent's preloadResponse in future. But we don't have plan to support service worker integration without service-worker-servicification. To make this behavior consistent, this CL makes NavigationURLLoaderImpl::URLLoaderRequestController skip sending the redirected request of signed exchange to the service worker. Bug: 894755 Change-Id: I6f22819655169b4d3c30de0aa269911111c45586 Reviewed-on: https://chromium-review.googlesource.com/c/1278433 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#599815}(cherry picked from commit 0fd45315adaa2d4a801d27af02e04aa257751d51) Reviewed-on: https://chromium-review.googlesource.com/c/1286010 Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#83} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Nov 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9ecea5f760faf209a56fd8efc017028d323e7053 commit 9ecea5f760faf209a56fd8efc017028d323e7053 Author: Tsuyoshi Horo <horo@chromium.org> Date: Thu Nov 22 11:36:07 2018 Reset the state of SWProviderHost when SXGRequestHandler handles the response. I introduced skip_other_interceptors flag in crrev.com/c/1278433 to skip the service worker handling of the synthesized redirection of signed exchange. But this was wrong. We need to call ServiceWorkerProviderHost::SetDocumentUrl() and SetTopmostFrameUrl() even after SignedExchangeRequestHandler started handling the response to reject the service worker API calls from the page in the signed exchange correctly. And also we need to call SetControllerRegistration(null) to reset the controller state. Bug: 894755 ,907712 Change-Id: Iace762636982429c23ec24d9af6a466468e4b9fb Reviewed-on: https://chromium-review.googlesource.com/c/1347954 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/heads/master@{#610381} [modify] https://crrev.com/9ecea5f760faf209a56fd8efc017028d323e7053/content/browser/loader/navigation_url_loader_impl.cc [modify] https://crrev.com/9ecea5f760faf209a56fd8efc017028d323e7053/content/browser/service_worker/service_worker_request_handler.cc [modify] https://crrev.com/9ecea5f760faf209a56fd8efc017028d323e7053/content/browser/service_worker/service_worker_request_handler.h [modify] https://crrev.com/9ecea5f760faf209a56fd8efc017028d323e7053/content/browser/service_worker/service_worker_request_handler_unittest.cc [modify] https://crrev.com/9ecea5f760faf209a56fd8efc017028d323e7053/content/browser/web_package/signed_exchange_request_handler_browsertest.cc [add] https://crrev.com/9ecea5f760faf209a56fd8efc017028d323e7053/content/test/data/sxg/no-respond-with-service-worker.html [add] https://crrev.com/9ecea5f760faf209a56fd8efc017028d323e7053/content/test/data/sxg/no-respond-with-service-worker.js [add] https://crrev.com/9ecea5f760faf209a56fd8efc017028d323e7053/content/test/data/sxg/no-respond-with-service-worker.js.mock-http-headers
,
Nov 22
Is CL listed at #11 need a merge to M71?
,
Nov 22
Yes. #11 is the same patch as crbug.com/907712#c5.
,
Nov 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a33240fa58c073b568483dd2747bcc3b79604779 commit a33240fa58c073b568483dd2747bcc3b79604779 Author: Tsuyoshi Horo <horo@chromium.org> Date: Mon Nov 26 01:56:14 2018 [Merge to M71] Reset the state of SWProviderHost when SXGRequestHandler handles the response. I introduced skip_other_interceptors flag in crrev.com/c/1278433 to skip the service worker handling of the synthesized redirection of signed exchange. But this was wrong. We need to call ServiceWorkerProviderHost::SetDocumentUrl() and SetTopmostFrameUrl() even after SignedExchangeRequestHandler started handling the response to reject the service worker API calls from the page in the signed exchange correctly. And also we need to call SetControllerRegistration(null) to reset the controller state. TBR=kinuko@chromium.org (cherry picked from commit 9ecea5f760faf209a56fd8efc017028d323e7053) Bug: 894755 ,907712 Change-Id: Iace762636982429c23ec24d9af6a466468e4b9fb Reviewed-on: https://chromium-review.googlesource.com/c/1347954 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#610381} Reviewed-on: https://chromium-review.googlesource.com/c/1350371 Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#805} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/a33240fa58c073b568483dd2747bcc3b79604779/content/browser/loader/navigation_url_loader_impl.cc [modify] https://crrev.com/a33240fa58c073b568483dd2747bcc3b79604779/content/browser/service_worker/service_worker_request_handler.cc [modify] https://crrev.com/a33240fa58c073b568483dd2747bcc3b79604779/content/browser/service_worker/service_worker_request_handler.h [modify] https://crrev.com/a33240fa58c073b568483dd2747bcc3b79604779/content/browser/service_worker/service_worker_request_handler_unittest.cc [modify] https://crrev.com/a33240fa58c073b568483dd2747bcc3b79604779/content/browser/web_package/signed_exchange_request_handler_browsertest.cc [add] https://crrev.com/a33240fa58c073b568483dd2747bcc3b79604779/content/test/data/sxg/no-respond-with-service-worker.html [add] https://crrev.com/a33240fa58c073b568483dd2747bcc3b79604779/content/test/data/sxg/no-respond-with-service-worker.js [add] https://crrev.com/a33240fa58c073b568483dd2747bcc3b79604779/content/test/data/sxg/no-respond-with-service-worker.js.mock-http-headers
,
Nov 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a33240fa58c073b568483dd2747bcc3b79604779 Commit: a33240fa58c073b568483dd2747bcc3b79604779 Author: horo@chromium.org Commiter: horo@chromium.org Date: 2018-11-26 01:56:14 +0000 UTC [Merge to M71] Reset the state of SWProviderHost when SXGRequestHandler handles the response. I introduced skip_other_interceptors flag in crrev.com/c/1278433 to skip the service worker handling of the synthesized redirection of signed exchange. But this was wrong. We need to call ServiceWorkerProviderHost::SetDocumentUrl() and SetTopmostFrameUrl() even after SignedExchangeRequestHandler started handling the response to reject the service worker API calls from the page in the signed exchange correctly. And also we need to call SetControllerRegistration(null) to reset the controller state. TBR=kinuko@chromium.org (cherry picked from commit 9ecea5f760faf209a56fd8efc017028d323e7053) Bug: 894755 ,907712 Change-Id: Iace762636982429c23ec24d9af6a466468e4b9fb Reviewed-on: https://chromium-review.googlesource.com/c/1347954 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#610381} Reviewed-on: https://chromium-review.googlesource.com/c/1350371 Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#805} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by horo@chromium.org
, Oct 12