S13nSW: browsertest ServiceWorkerBrowserTest.Reload is failing |
|||
Issue descriptionThis is failing because FetchEvent#isReload isn't set for reload requests. FetchEvent#isReload is deprecated (issue 652994) but we still need to support it. ToWebServiceWorkerRequest() sets isReload to true when |transition_type| of network::ResourceRequest is PAGE_TRANSITION_RELOAD[1]. [1] https://cs.chromium.org/chromium/src/content/renderer/service_worker/service_worker_context_client.cc?rcl=9b11dbe5ef9b56788ff997ba9945a7b65842cd8c&l=229 However, with S13nServiceWorker, |transition_type| is always unset for some reason. At least, at the point a ResourceRequest being passed to ServiceWorkerControlleeRequestHandler::MaybeCreateLoader(), it's |transition_type| isn't set accordingly. Since ServiceWorkerNavigationLoader uses the ResourceRequest passed to ServiceWorkerControlleeRequestHandler::MaybeCreateLoader(), |transition_type| of the given ResourceRequest should be set or overridden to PAGE_TRANSITION_RELOAD when the request is a reload. Here is a quick hack to override |transition_type|[2], but I don't think this is a right approach. [2] https://chromium-review.googlesource.com/#/c/chromium/src/+/1068536 FYI, here are how non-S13nServiceWorker works: - A ResourceRequestInfoImpl is created in ResourceDispatcherHostImpl::BeginNavigationRequest(), which has |common_params.transition|. - The info is attached to the corresponding URLRequest. - ServiceWorkerURLRequestJob::CreateResourceRequest() retrieves the info via ResourceRequestInfo::ForRequest() then set |transition_type| to info's |common_params.transition|[3]. [3] https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_url_request_job.cc?l=582&rcl=27af8e0a6de92fc683661ae453ed7a2a1edc7c6e
,
May 22 2018
Hum, I'm not sure why ResourceRequest's transition_type isn't set in the NetworkService case. yhirano: do you understand this now as you've been looking at isReloadNavigation?
,
May 22 2018
I don't know, sorry.
,
May 22 2018
Talked to yhirano@ offline. There seems no easy solution so we may go with something similar to [2] in the description. I'll dig into this a bit later.
,
May 30 2018
I think I found a simpler solution: https://chromium-review.googlesource.com/c/chromium/src/+/1077880
,
May 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/61e41b3cb2aeb6f9b98d86b47d12a214cfa572ce commit 61e41b3cb2aeb6f9b98d86b47d12a214cfa572ce Author: Yutaka Hirano <yhirano@chromium.org> Date: Thu May 31 05:31:06 2018 Set ResourceRequest::transition_type for navigation requests ServiceWorker needs the information. Bug: 845408 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: I138d02f7bf3cf69b1949be785fb0e57c596d8c06 Reviewed-on: https://chromium-review.googlesource.com/1077880 Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Camille Lamy <clamy@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#563156} [modify] https://crrev.com/61e41b3cb2aeb6f9b98d86b47d12a214cfa572ce/content/browser/loader/navigation_url_loader_impl.cc [modify] https://crrev.com/61e41b3cb2aeb6f9b98d86b47d12a214cfa572ce/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter
,
May 31 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by bashi@chromium.org
, May 22 2018