New issue
Advanced search Search tips

Issue 845408 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

S13nSW: browsertest ServiceWorkerBrowserTest.Reload is failing

Project Member Reported by bashi@chromium.org, May 22 2018

Issue description

This 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

 

Comment 1 by bashi@chromium.org, May 22 2018

Cc: falken@chromium.org kinuko@chromium.org
falken@, kinuko@: Any ideas about how to set |transition_type| of network::ResourceRequest accordingly with S13nServiceWorker/NetworkService?

Comment 2 by falken@chromium.org, May 22 2018

Cc: yhirano@chromium.org
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?
I don't know, sorry.

Comment 4 by bashi@chromium.org, 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.
I think I found a simpler solution: https://chromium-review.googlesource.com/c/chromium/src/+/1077880
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Owner: yhirano@chromium.org
Status: Fixed (was: Started)

Sign in to add a comment