[ServiceWorker] Propagates ServiceWorker fetched response's URL and wasFetchedViaServiceWorker flag.

[ServiceWorker] Propagates ServiceWorker fetched response's URL and wasFetchedViaServiceWorker flag.

If the request was fetched via a ServiceWorker, the URL of the response could be different from the URL of the original request. 
The URL must be checked in the renderer process for CSP and CORS.

So in this patch I introduce was_fetched_via_service_worker flag and original_url_via_service_worker which are set in ServiceWorkerURLRequestJob and propagated to the renderer process.


This change depends on http://crrev.com/370733002 and http://crrev.com/378473002.

BUG=373120
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284627

[horo, 2014-07-07 06:18:54]

The values are propagated in the following ways.

--Browser process--
ServiceWorkerURLRequestJob::CommitResponseHeader()
  http_response_info_->was_fetched_via_service_worker = true;
  http_response_info_->original_url_via_service_worker = responce_url_;

ResourceLoader::CompleteResponseStarted()
  scoped_refptr<ResourceResponse> response(new ResourceResponse());
  PopulateResourceResponse(request_.get(), response.get());
  --> PopulateResourceResponse()
    response->head.was_fetched_via_service_worker =
        request->was_fetched_via_service_worker();
        --> URLRequest()::was_fetched_via_service_worker()
          return response_info_.was_fetched_via_service_worker;
    response->head.original_url_via_service_worker =
        request->original_url_via_service_worker();
        --> URLRequest()::original_url_via_service_worker()
          return response_info_.original_url_via_service_worker;

AsyncResourceHandler::OnResponseStarted()
  Send(new ResourceMsg_ReceivedResponse)
-------------------
  |
 IPC [ResourceMsg_ReceivedResponse]
  |
--Renderer process--
ResourceDispatcher::OnReceivedResponse()
  if (response_head.was_fetched_via_service_worker &&
      !response_head.original_url_via_service_worker.is_empty()) {
    request_info->response_url = response_head.original_url_via_service_worker;
  }
  ...
  request_info->peer->OnReceivedResponse(renderer_response_info);
  --> WebURLLoaderImpl::Context::OnReceivedResponse()
    if (info.was_fetched_via_service_worker &&
        !info.original_url_via_service_worker.is_empty()) {
      request_.setURL(info.original_url_via_service_worker);
    }
    PopulateURLResponse(request_.url(), info, &response);
    --> WebURLLoaderImpl::PopulateURLResponse
      response->setURL(url);
      ...
     
response->setWasFetchedViaServiceWorker(info.was_fetched_via_service_worker);
    ...
    m_resource->responseReceived(resourceResponse);
    --> RawResource::responseReceived()
      while (RawResourceClient* c = w.next())
        c->responseReceived(this, m_response);
        --> DocumentThreadableLoader::responseReceived()
          handleResponse(resource->identifier(), response);
          --> DocumentThreadableLoader::handleResponse()
            // FIXME: Check response.wasFetchedViaServiceWorker() and
response.url()
    ...
    client_->didReceiveResponse(loader_, response);
    --> ResourceLoader::didReceiveResponse()
      m_host->didReceiveResponse(m_resource, resourceResponse);
      --> ResourceFetcher::didReceiveResponse()
        // FIXME: Check response.wasFetchedViaServiceWorker() and response.url()

[horo, 2014-07-07 06:32:55]

tyoshino@
Could you please review this patch?

[tyoshino (SeeGerritForStatus), 2014-07-07 06:48:38]

https://codereview.chromium.org/375513002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/375513002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.h:124: GURL
responce_url_;
responce -> response

[horo, 2014-07-07 06:58:10]

https://codereview.chromium.org/375513002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/375513002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.h:124: GURL
responce_url_;
On 2014/07/07 06:48:37, tyoshino wrote:
> responce -> response

Done.

[dominicc (has gone to gerrit), 2014-07-08 02:08:31]

Nit: The description contains reqonse -> response

[horo, 2014-07-08 02:16:23]

On 2014/07/08 02:08:31, dominicc wrote:
> Nit: The description contains reqonse -> response

done. thank you.

[tyoshino (SeeGerritForStatus), 2014-07-08 14:27:49]

sorry. i'll take time to review this in depth tomorrow.

https://codereview.chromium.org/375513002/diff/80001/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/375513002/diff/80001/content/child/resource_d...
content/child/resource_dispatcher.cc:361: // Updates the response_url if the
response was feched via a ServiceWorker,
feched -> fetched

https://codereview.chromium.org/375513002/diff/80001/content/child/resource_d...
content/child/resource_dispatcher.cc:362: //  and it was not generated inside
the SherviceWorker.
Shervice -> Service

https://codereview.chromium.org/375513002/diff/80001/content/child/resource_d...
content/child/resource_dispatcher.cc:362: //  and it was not generated inside
the SherviceWorker.
//  and
->
// and

https://codereview.chromium.org/375513002/diff/80001/content/child/web_url_lo...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/375513002/diff/80001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:534: // Updates the request url if the
response was feched via a ServiceWorker,
feched -> fetched

https://codereview.chromium.org/375513002/diff/80001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:535: // and it was not generated inside the
SherviceWorker.
Shervice -> Service

https://codereview.chromium.org/375513002/diff/80001/content/public/common/re...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/375513002/diff/80001/content/public/common/re...
content/public/common/resource_response_info.h:104: // The original URL of the
repsponse which was fetched via the ServiceWorker.
response

https://codereview.chromium.org/375513002/diff/80001/content/public/common/re...
content/public/common/resource_response_info.h:105: // This may be empty if the
repsponse was created inside the ServiceWorker.
response

[horo, 2014-07-09 03:15:01]

https://codereview.chromium.org/375513002/diff/80001/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/375513002/diff/80001/content/child/resource_d...
content/child/resource_dispatcher.cc:361: // Updates the response_url if the
response was feched via a ServiceWorker,
On 2014/07/08 14:27:49, tyoshino wrote:
> feched -> fetched

Done.

https://codereview.chromium.org/375513002/diff/80001/content/child/resource_d...
content/child/resource_dispatcher.cc:362: //  and it was not generated inside
the SherviceWorker.
On 2014/07/08 14:27:49, tyoshino wrote:
> Shervice -> Service

Done.

https://codereview.chromium.org/375513002/diff/80001/content/child/resource_d...
content/child/resource_dispatcher.cc:362: //  and it was not generated inside
the SherviceWorker.
On 2014/07/08 14:27:49, tyoshino wrote:
> //  and
> ->
> // and

Done.

https://codereview.chromium.org/375513002/diff/80001/content/child/web_url_lo...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/375513002/diff/80001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:534: // Updates the request url if the
response was feched via a ServiceWorker,
On 2014/07/08 14:27:49, tyoshino wrote:
> feched -> fetched

Done.

https://codereview.chromium.org/375513002/diff/80001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:535: // and it was not generated inside the
SherviceWorker.
On 2014/07/08 14:27:49, tyoshino wrote:
> Shervice -> Service

Done.

https://codereview.chromium.org/375513002/diff/80001/content/public/common/re...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/375513002/diff/80001/content/public/common/re...
content/public/common/resource_response_info.h:104: // The original URL of the
repsponse which was fetched via the ServiceWorker.
On 2014/07/08 14:27:49, tyoshino wrote:
> response

Done.

https://codereview.chromium.org/375513002/diff/80001/content/public/common/re...
content/public/common/resource_response_info.h:105: // This may be empty if the
repsponse was created inside the ServiceWorker.
On 2014/07/08 14:27:49, tyoshino wrote:
> response

Done.

[tyoshino (SeeGerritForStatus), 2014-07-09 06:35:26]

https://codereview.chromium.org/375513002/diff/100001/net/http/http_response_...
File net/http/http_response_info.cc (right):

https://codereview.chromium.org/375513002/diff/100001/net/http/http_response_...
net/http/http_response_info.cc:162: bool HttpResponseInfo::InitFromPickle(const
Pickle& pickle,
is it ok to skip serializer/deserializer implementation for the new fields?

https://codereview.chromium.org/375513002/diff/100001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/375513002/diff/100001/net/url_request/url_req...
net/url_request/url_request.h:507: // The original URL of the repsponse which
was fetched via the ServiceWorker.
response

https://codereview.chromium.org/375513002/diff/100001/net/url_request/url_req...
net/url_request/url_request.h:508: // This may be empty if the repsponse was
created inside the ServiceWorker.
response

[tyoshino (SeeGerritForStatus), 2014-07-09 07:09:01]

almost LG

https://codereview.chromium.org/375513002/diff/100001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/375513002/diff/100001/content/child/resource_...
content/child/resource_dispatcher.cc:365: request_info->response_url =
response_head.original_url_via_service_worker;
please make sure that one who knows what's expected for response_url agree on
this. I.e. owner of SiteIsolationPolicy?

https://codereview.chromium.org/375513002/diff/100001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/375513002/diff/100001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:538:
request_.setURL(info.original_url_via_service_worker);
I guess it's ok to also update request_ like this. For the redirect case, it's
also updated (request_ is replaced with new_request with the new URL). Just
curious if it breaks if we don't update the URL on request_.

[horo, 2014-07-09 11:14:03]

https://codereview.chromium.org/375513002/diff/100001/net/http/http_response_...
File net/http/http_response_info.cc (right):

https://codereview.chromium.org/375513002/diff/100001/net/http/http_response_...
net/http/http_response_info.cc:162: bool HttpResponseInfo::InitFromPickle(const
Pickle& pickle,
On 2014/07/09 06:35:25, tyoshino wrote:
> is it ok to skip serializer/deserializer implementation for the new fields?

Added serializer/deserializer of was_fetched_via_service_worker and
original_url_via_service_worker.

https://codereview.chromium.org/375513002/diff/100001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/375513002/diff/100001/net/url_request/url_req...
net/url_request/url_request.h:507: // The original URL of the repsponse which
was fetched via the ServiceWorker.
On 2014/07/09 06:35:25, tyoshino wrote:
> response

Done.

https://codereview.chromium.org/375513002/diff/100001/net/url_request/url_req...
net/url_request/url_request.h:508: // This may be empty if the repsponse was
created inside the ServiceWorker.
On 2014/07/09 06:35:25, tyoshino wrote:
> response

Done.

[horo, 2014-07-09 11:17:39]

mkwst@
Could you please review this?

[Mike West, 2014-07-09 12:06:56]

Happy to take a look, but you'll need a content/ and net/ OWNER as well.

[Mike West, 2014-07-09 12:14:46]

Thanks! A few comments/questions inline.

https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:261:
response_url_ = response.url;
What's the lifetime of the RequestJob? Do we need to worry about resetting this
value after the request completes, or do we just destroy the object?

https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
content/child/resource_dispatcher.cc:365: request_info->response_url =
response_head.original_url_via_service_worker;
Is overwriting the response url the right thing to do (here and in
WebURLLoaderImpl::Context::OnReceivedResponse)?

If I request `https://example.com/yay` on a site whose CSP only allows
`example.com`, don't we want to be able to reject resources from
`not-example.com` if the Service Worker delivers them in response?

Likewise, when we do CORS checks, don't we need to know that the response should
be treated as opaque?

Perhaps this would be clearer if you spell out in another CL exactly what the
Blink-side checks will look like.

[horo, 2014-07-10 06:17:56]

Thank you for your review!

https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:261:
response_url_ = response.url;
On 2014/07/09 12:14:46, Mike West wrote:
> What's the lifetime of the RequestJob? Do we need to worry about resetting
this
> value after the request completes, or do we just destroy the object?

ServiceWorkerURLRequestJob is created per a request in
ServiceWorkerControlleeRequestHandler::MaybeCreateJob().
So we don't need to worry about resetting the value.

https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
content/child/resource_dispatcher.cc:365: request_info->response_url =
response_head.original_url_via_service_worker;
On 2014/07/09 12:14:46, Mike West wrote:
> Is overwriting the response url the right thing to do (here and in
> WebURLLoaderImpl::Context::OnReceivedResponse)?
> 
> If I request `https://example.com/yay` on a site whose CSP only allows
> `example.com`, don't we want to be able to reject resources from
> `not-example.com` if the Service Worker delivers them in response?
> 
> Likewise, when we do CORS checks, don't we need to know that the response
should
> be treated as opaque?
> 
> Perhaps this would be clearer if you spell out in another CL exactly what the
> Blink-side checks will look like.

I think it's ok to overwrite the response url here, because it is also
overwritten in ResourceDispatcher::OnReceivedRedirect()
But I'm not sure it is really correct.
Do you know who to ask?


I'm planning to do CSP and CORS checks in ResourceFetcher::didReceiveResponse()
and DocumentThreadableLoader::handleResponse() in blink side.

The following are WIP patch for the check.
https://codereview.chromium.org/333423004/diff/1700001/Source/core/fetch/Reso...
https://codereview.chromium.org/333423004/diff/1700001/Source/core/loader/Doc...

[horo, 2014-07-10 06:17:57]

Thank you for your review!

https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:261:
response_url_ = response.url;
On 2014/07/09 12:14:46, Mike West wrote:
> What's the lifetime of the RequestJob? Do we need to worry about resetting
this
> value after the request completes, or do we just destroy the object?

ServiceWorkerURLRequestJob is created per a request in
ServiceWorkerControlleeRequestHandler::MaybeCreateJob().
So we don't need to worry about resetting the value.

https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
content/child/resource_dispatcher.cc:365: request_info->response_url =
response_head.original_url_via_service_worker;
On 2014/07/09 12:14:46, Mike West wrote:
> Is overwriting the response url the right thing to do (here and in
> WebURLLoaderImpl::Context::OnReceivedResponse)?
> 
> If I request `https://example.com/yay` on a site whose CSP only allows
> `example.com`, don't we want to be able to reject resources from
> `not-example.com` if the Service Worker delivers them in response?
> 
> Likewise, when we do CORS checks, don't we need to know that the response
should
> be treated as opaque?
> 
> Perhaps this would be clearer if you spell out in another CL exactly what the
> Blink-side checks will look like.

I think it's ok to overwrite the response url here, because it is also
overwritten in ResourceDispatcher::OnReceivedRedirect()
But I'm not sure it is really correct.
Do you know who to ask?


I'm planning to do CSP and CORS checks in ResourceFetcher::didReceiveResponse()
and DocumentThreadableLoader::handleResponse() in blink side.

The following are WIP patch for the check.
https://codereview.chromium.org/333423004/diff/1700001/Source/core/fetch/Reso...
https://codereview.chromium.org/333423004/diff/1700001/Source/core/loader/Doc...

[Mike West, 2014-07-10 08:48:37]

On 2014/07/10 06:17:57, horo wrote:
> Thank you for your review!
> 
>
https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
> File content/browser/service_worker/service_worker_url_request_job.cc (right):
> 
>
https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
> content/browser/service_worker/service_worker_url_request_job.cc:261:
> response_url_ = response.url;
> On 2014/07/09 12:14:46, Mike West wrote:
> > What's the lifetime of the RequestJob? Do we need to worry about resetting
> this
> > value after the request completes, or do we just destroy the object?
> 
> ServiceWorkerURLRequestJob is created per a request in
> ServiceWorkerControlleeRequestHandler::MaybeCreateJob().
> So we don't need to worry about resetting the value.
> 
>
https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
> File content/child/resource_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
> content/child/resource_dispatcher.cc:365: request_info->response_url =
> response_head.original_url_via_service_worker;
> On 2014/07/09 12:14:46, Mike West wrote:
> > Is overwriting the response url the right thing to do (here and in
> > WebURLLoaderImpl::Context::OnReceivedResponse)?
> > 
> > If I request `https://example.com/yay` on a site whose CSP only allows
> > `example.com`, don't we want to be able to reject resources from
> > `not-example.com` if the Service Worker delivers them in response?
> > 
> > Likewise, when we do CORS checks, don't we need to know that the response
> should
> > be treated as opaque?
> > 
> > Perhaps this would be clearer if you spell out in another CL exactly what
the
> > Blink-side checks will look like.
> 
> I think it's ok to overwrite the response url here, because it is also
> overwritten in ResourceDispatcher::OnReceivedRedirect()
> But I'm not sure it is really correct.
> Do you know who to ask?
> 
> 
> I'm planning to do CSP and CORS checks in
ResourceFetcher::didReceiveResponse()
> and DocumentThreadableLoader::handleResponse() in blink side.
> 
> The following are WIP patch for the check.
>
https://codereview.chromium.org/333423004/diff/1700001/Source/core/fetch/Reso...
>
https://codereview.chromium.org/333423004/diff/1700001/Source/core/loader/Doc...

If we overwrite the URL, then what is the difference between those checks, and
the checks that happen on the request's way out of the page, up into the SW?

[horo, 2014-07-11 07:27:42]

On 2014/07/10 08:48:37, Mike West wrote:
> On 2014/07/10 06:17:57, horo wrote:
> > Thank you for your review!
> > 
> >
>
https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
> > File content/browser/service_worker/service_worker_url_request_job.cc
(right):
> > 
> >
>
https://codereview.chromium.org/375513002/diff/120001/content/browser/service...
> > content/browser/service_worker/service_worker_url_request_job.cc:261:
> > response_url_ = response.url;
> > On 2014/07/09 12:14:46, Mike West wrote:
> > > What's the lifetime of the RequestJob? Do we need to worry about resetting
> > this
> > > value after the request completes, or do we just destroy the object?
> > 
> > ServiceWorkerURLRequestJob is created per a request in
> > ServiceWorkerControlleeRequestHandler::MaybeCreateJob().
> > So we don't need to worry about resetting the value.
> > 
> >
>
https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
> > File content/child/resource_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/375513002/diff/120001/content/child/resource_...
> > content/child/resource_dispatcher.cc:365: request_info->response_url =
> > response_head.original_url_via_service_worker;
> > On 2014/07/09 12:14:46, Mike West wrote:
> > > Is overwriting the response url the right thing to do (here and in
> > > WebURLLoaderImpl::Context::OnReceivedResponse)?
> > > 
> > > If I request `https://example.com/yay` on a site whose CSP only allows
> > > `example.com`, don't we want to be able to reject resources from
> > > `not-example.com` if the Service Worker delivers them in response?
> > > 
> > > Likewise, when we do CORS checks, don't we need to know that the response
> > should
> > > be treated as opaque?
> > > 
> > > Perhaps this would be clearer if you spell out in another CL exactly what
> the
> > > Blink-side checks will look like.
> > 
> > I think it's ok to overwrite the response url here, because it is also
> > overwritten in ResourceDispatcher::OnReceivedRedirect()
> > But I'm not sure it is really correct.
> > Do you know who to ask?
> > 
> > 
> > I'm planning to do CSP and CORS checks in
> ResourceFetcher::didReceiveResponse()
> > and DocumentThreadableLoader::handleResponse() in blink side.
> > 
> > The following are WIP patch for the check.
> >
>
https://codereview.chromium.org/333423004/diff/1700001/Source/core/fetch/Reso...
> >
>
https://codereview.chromium.org/333423004/diff/1700001/Source/core/loader/Doc...
> 
> If we overwrite the URL, then what is the difference between those checks, and
> the checks that happen on the request's way out of the page, up into the SW?

In ResourceDispatcher::OnReceivedResponse() we update request_info->response_url
so we can set request_info->site_isolation_metadata correctly and we can block
the illegal cross-site document in ResourceDispatcher::OnReceivedData().

In WebURLLoaderImpl::Context::OnReceivedResponse() we update request_'s url and
call PopulateURLResponse() and set ResourceResponseInfo's URL and
WasFetchedViaServiceWorker flag.

ResourceResponseInfo's URL will be used in ImageResource::isAccessAllowed() for
the image tainting (HTMLImageElement::wouldTaintOrigin).
It will avoid getting cross site images using a ServiceWorker.
Example: https://example.com/index.html
  var canvas = document.createElement('canvas');
  var ctx = canvas.getContext('2d);
  var img = new Image;
  img.src = 'https://example.com/sw-scope/FetchImageInOtherSiteInNoCorsMode";
  // In ServiceWorker
  //   self.addEventListener('fetch', function(event) {
  //     event.respondWith(
  //         new Request('http://othersite/data.jpg', {mode: 'no-cors'});)
  //   }
  ctx.drawImage( img1, 0, 0 );
  canvas.toDataURL();  // -> SecurityError!


In DocumentThreadableLoader::handleResponse() we must CORS check using
ResourceResponse's URL and WasFetchedViaServiceWorker flag to avoid getting
cross site document using XMLHttpRequest and a ServiceWorker.
A similar check is done in DocumentThreadableLoader::redirectReceived() for
HTTP redirect before sending the redirected request.
Example: https://example.com/index.html
  var xhr = new XMLHttpRequest();
  xhr.onload = function(e) {
    console.log(new Uint8Array(this.response);
  }
  xhr.responseType = 'arraybuffer';
  xhr.open('GET',
           'https://example.com/sw-scope/FetchDataInOtherSiteInNoCorsMode',
           true);
  xhr.send();
  // In ServiceWorker
  //   self.addEventListener('fetch', function(event) {
  //     event.respondWith(
  //         new Request('http://othersite/data.txt', {mode: 'no-cors'});)
  //   }

In ResourceFetcher::didReceiveResponse() we must CSP and CORS check using
ResourceResponse's URL and WasFetchedViaServiceWorker flag to avoid showing
other site's document.
A similar check is done in ResourceFetcher::canAccessRedirect() for HTTP
redirect before sending the redirected request.
Example: https://example.com/index.html
  <meta http-equiv="Content-Security-Policy"
      content="img-src https://www.example.com">
  <img src="https://www.example.com/sw-scope/FetchImageInOtherSite">

[Mike West, 2014-07-11 07:55:42]

Ok, it's entirely possible I've misunderstood what
"original_url_via_service_worker" represents.

Given the following scenario:

1. Website requests `https://example.com/image.png`.
2. Service Worker intercepts request from #1.
3. Service Worker synthesizes a response to #1 using a cached copy of
`https://not-example.com/not-image.png`.
4. Website receives the response synthesized in #3.

I think you're saying that the URL of the response in #4 would be
`https://not-example.com/not-image.png`. If that's the case, then this change is
wonderful and I support it.

If, on the other hand, the URL of the response in #4 would be
`https://example.com/image.png`, then I'm confused. :)

[horo, 2014-07-11 08:08:46]

> I think you're saying that the URL of the response in #4 would be
> `https://not-example.com/not-image.png`. If that's the case, then this change
is
> wonderful and I support it.

Oh yes that's right!
Sorry for not explaining enough.

Ah, to say it correctly
> 3. Service Worker synthesizes a response to #1 using a cached copy of
> `https://not-example.com/not-image.png`.
should be 
> 3. Service Worker synthesizes a response to #1 using a cached copy of
> `https://not-example.com/not-image.png` or the result of fetching
> `https://not-example.com/not-image.png`.

[Mike West, 2014-07-11 08:16:20]

In that case, LGTM.

[Mike West, 2014-07-11 08:19:28]

On 2014/07/11 at 08:16:20, Mike West wrote:
> In that case, LGTM.

(Though I'd perhaps suggest changing the variable name from "original_url" to
"actual_url" or something similar. That can obviously happen in a subsequent CL)

[horo, 2014-07-11 08:20:25]

gavinp@
Could you please review this change?
Thank you.

[tyoshino (SeeGerritForStatus), 2014-07-14 07:00:42]

lgtm

https://codereview.chromium.org/375513002/diff/140001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/375513002/diff/140001/content/public/common/r...
content/public/common/resource_response_info.h:104: // The original URL of the
response which was fetched via the ServiceWorker.
via -> by
would be better? I think the response can be a result of fetch() initiated by
the ServiceWorker but not for any FetchEvent coming from a page under the SW's
scope. For such a case, "via" doesn't fit I think.

https://codereview.chromium.org/375513002/diff/140001/net/http/http_response_...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/375513002/diff/140001/net/http/http_response_...
net/http/http_response_info.h:99: // The original URL of the repsponse which was
fetched via the ServiceWorker.
ditto

https://codereview.chromium.org/375513002/diff/140001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/375513002/diff/140001/net/url_request/url_req...
net/url_request/url_request.h:507: // The original URL of the response which was
fetched via the ServiceWorker.
ditto

[horo, 2014-07-15 00:36:18]

https://codereview.chromium.org/375513002/diff/140001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/375513002/diff/140001/content/public/common/r...
content/public/common/resource_response_info.h:104: // The original URL of the
response which was fetched via the ServiceWorker.
On 2014/07/14 07:00:42, tyoshino wrote:
> via -> by
> would be better? I think the response can be a result of fetch() initiated by
> the ServiceWorker but not for any FetchEvent coming from a page under the SW's
> scope. For such a case, "via" doesn't fit I think.

Done.

https://codereview.chromium.org/375513002/diff/140001/net/http/http_response_...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/375513002/diff/140001/net/http/http_response_...
net/http/http_response_info.h:99: // The original URL of the repsponse which was
fetched via the ServiceWorker.
On 2014/07/14 07:00:42, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/375513002/diff/140001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/375513002/diff/140001/net/url_request/url_req...
net/url_request/url_request.h:507: // The original URL of the response which was
fetched via the ServiceWorker.
On 2014/07/14 07:00:42, tyoshino wrote:
> ditto

Done.

[michaeln, 2014-07-15 23:16:38]

https://codereview.chromium.org/375513002/diff/180001/net/http/http_response_...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/375513002/diff/180001/net/http/http_response_...
net/http/http_response_info.h:101: GURL original_url_via_service_worker;
These service_worker specific data members probably don't belong in the lower
level networking library.

The content library associates extra data with each URLRequest, that extra data
is an instance of ServiceWorkerRequestHandler. These values could be added to
that set of extra data and retrieved from there when sending a ResourceResponse
back to a renderer.

Some pointers...

See ServiceWorkerRequestHandler::GetHandler(request).

See PopulateResourceResponse(...) in resource_loader.cc and notice how
AppCacheInterceptor::GetExtraResponseInfo() is used in that function.

I think we could similarly use a method like
ServiceWorkerRequestHandler::GetHandler()->GetExtraResponseInfo(...) for the
serviceworker's extra data.

[horo, 2014-07-16 02:24:39]

On 2014/07/15 23:16:38, michaeln wrote:
>
https://codereview.chromium.org/375513002/diff/180001/net/http/http_response_...
> File net/http/http_response_info.h (right):
> 
>
https://codereview.chromium.org/375513002/diff/180001/net/http/http_response_...
> net/http/http_response_info.h:101: GURL original_url_via_service_worker;
> These service_worker specific data members probably don't belong in the lower
> level networking library.
> 
> The content library associates extra data with each URLRequest, that extra
data
> is an instance of ServiceWorkerRequestHandler. These values could be added to
> that set of extra data and retrieved from there when sending a
ResourceResponse
> back to a renderer.
> 
> Some pointers...
> 
> See ServiceWorkerRequestHandler::GetHandler(request).
> 
> See PopulateResourceResponse(...) in resource_loader.cc and notice how
> AppCacheInterceptor::GetExtraResponseInfo() is used in that function.
> 
> I think we could similarly use a method like
> ServiceWorkerRequestHandler::GetHandler()->GetExtraResponseInfo(...) for the
> serviceworker's extra data.

Thank you for your review!
Done.
I added ServiceWorkerRequestHandler::GetExtraResponseInfo().

[michaeln, 2014-07-17 19:33:09]

lgtm

> I added ServiceWorkerRequestHandler::GetExtraResponseInfo().

Thnx! I'll expect we'll want to add to this set of info eventually to better
inform downstream caching layers.

[michaeln, 2014-07-17 19:34:20]

forgot to send this comment before

https://codereview.chromium.org/375513002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/375513002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.cc:78:
*was_fetched_via_service_worker = false;
maybe set both out params instead of trusting that the url is empty (here and in
the other two similar methods)

[horo, 2014-07-18 04:17:39]

https://codereview.chromium.org/375513002/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/375513002/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.cc:78:
*was_fetched_via_service_worker = false;
On 2014/07/17 19:34:20, michaeln wrote:
> maybe set both out params instead of trusting that the url is empty (here and
in
> the other two similar methods)

Done.

[horo, 2014-07-18 04:19:41]

jochen@
Could you please review this?
Thank you.

[jochen (gone - plz use gerrit), 2014-07-18 09:04:19]

lgtm

you'll need a security review for the ipc changes

[horo, 2014-07-18 09:19:05]

nasko@
Could you please review content/common/resource_messages.h and
content/common/service_worker/service_worker_messages.h?
Thank you.

[nasko, 2014-07-21 13:06:09]

IPC LGTM

[horo, 2014-07-22 01:00:42]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-07-22 01:05:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/375513002/260001

[commit-bot: I haz the power, 2014-07-22 03:54:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-07-22 04:39:20]

Change committed as 284627