ResourceFetcher::requestPreload should set its requestContext

ResourceFetcher::requestPreload should set its requestContext

This CL ensures ResourceFetcher::requestPreload to request with
an appropriate context, RequestContextPrefetch.
This CL fixes the issue that ASSERT in
MixedContentChecker::contextTypeFromContext is hit when preloader
is invoked.

This is to prepare for CL http://crrev.com/673603002 , which makes
preloader to be invoked more often.

TESTS=http/tests/security/mixedContent
BUG=421300
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185169

[kouhei (in TOK), 2014-11-11 01:59:08]

kouhei@chromium.org changed reviewers:
+ haraken@chromium.org, japhet@chromium.org, tyoshino@chromium.org

[kouhei (in TOK), 2014-11-11 01:59:18]

Would you take a look?

[tyoshino (SeeGerritForStatus), 2014-11-11 07:57:26]

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1257:
request.mutableResourceRequest().setRequestContext(WebURLRequest::RequestContextPrefetch);
This looked the "context" defined in the Fetch Standard or something corresponds
to it.
https://fetch.spec.whatwg.org/#concept-request-context-frame-type
The comment on the enum definition says so. But after some study, it's similar
but it's not (yet).

Introduced to WebKit on this revision. Was Chromium specific before that.
https://chromium.googlesource.com/chromium/blink/+/ea0fe8306fdc519755ec85027d...

mkwst@ has renamed it to "context" as they're similar concepts.
https://chromium.googlesource.com/chromium/blink/+/6741e56bdbab3c9acebfe3caa0...

If we're making them closer in the future, we should investigate if it's
appropriate use of RequestContextPrefetch.

The "prefetch" context defined in the Fetch Standard is used for a link element
when its rel attribute is set to "prefetch". Blink does use it for this purpose,
but there're some other code using this in Chromium.

Hmm, I'd recommend that you call determineRequestContext() as
ResourceFetcher::createResourceForLoading() does unless you're sure you need to
use RequestContextPreftch context here.

[tyoshino (SeeGerritForStatus), 2014-11-11 08:00:18]

tyoshino@chromium.org changed reviewers:
+ mkwst@chromium.org

[tyoshino (SeeGerritForStatus), 2014-11-11 08:00:26]

+mkwst

[Mike West, 2014-11-11 08:08:04]

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1257:
request.mutableResourceRequest().setRequestContext(WebURLRequest::RequestContextPrefetch);
On 2014/11/11 07:57:26, tyoshino wrote:
> This looked the "context" defined in the Fetch Standard or something
corresponds
> to it.
> https://fetch.spec.whatwg.org/#concept-request-context-frame-type
> The comment on the enum definition says so. But after some study, it's similar
> but it's not (yet).

I don't understand this comment? The `prefetch` context is part of Fetch, which
is why it's part of the RequestContext enum.

> mkwst@ has renamed it to "context" as they're similar concepts.

(Because that's what Fetch calls it :) ).
> The "prefetch" context defined in the Fetch Standard is used for a link
element
> when its rel attribute is set to "prefetch". Blink does use it for this
purpose,
> but there're some other code using this in Chromium.

The `<link prefetch>` comment in Fetch is an example, not an exhaustive list of
the points in the platform where the `prefetch` context is appropriate.

> 
> Hmm, I'd recommend that you call determineRequestContext() as
> ResourceFetcher::createResourceForLoading() does unless you're sure you need
to
> use RequestContextPreftch context here.

The practical question is whether requests that go through this path should be
checked for Mixed Content violations or not. So far, we've allowed prefetch
requests. If we change our minds on that (to match the spec), then we'll need to
know that this is a prefetch request.

[tyoshino (SeeGerritForStatus), 2014-11-11 08:20:36]

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1257:
request.mutableResourceRequest().setRequestContext(WebURLRequest::RequestContextPrefetch);
On 2014/11/11 08:08:04, Mike West wrote:
> On 2014/11/11 07:57:26, tyoshino wrote:
> > This looked the "context" defined in the Fetch Standard or something
> corresponds
> > to it.
> > https://fetch.spec.whatwg.org/#concept-request-context-frame-type
> > The comment on the enum definition says so. But after some study, it's
similar
> > but it's not (yet).
> 
> I don't understand this comment? The `prefetch` context is part of Fetch,
which
> is why it's part of the RequestContext enum.

I wrote "not yet" since you've renamed it in
https://chromium.googlesource.com/chromium/blink/+/6741e56bdbab3c9acebfe3caa0...
and said "it's the first step towards a higher fidelity implementation of
Fetch's semantics".

I compared the current definition of the enum and the list of contexts defined
in the Fetch spec again. Yeah, they're same now. Sorry, my bad.

> > mkwst@ has renamed it to "context" as they're similar concepts.
> 
> (Because that's what Fetch calls it :) ).
> > The "prefetch" context defined in the Fetch Standard is used for a link
> element
> > when its rel attribute is set to "prefetch". Blink does use it for this
> purpose,
> > but there're some other code using this in Chromium.
> 
> The `<link prefetch>` comment in Fetch is an example, not an exhaustive list
of
> the points in the platform where the `prefetch` context is appropriate.
> 

Oh, ok.

[tyoshino (SeeGerritForStatus), 2014-11-11 08:24:42]

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1257:
request.mutableResourceRequest().setRequestContext(WebURLRequest::RequestContextPrefetch);
On 2014/11/11 08:08:04, Mike West wrote:
> The practical question is whether requests that go through this path should be
> checked for Mixed Content violations or not. So far, we've allowed prefetch
> requests. If we change our minds on that (to match the spec), then we'll need
to
> know that this is a prefetch request.

OK.

Regarding mixed content, just issuing a request over the network bypassing mixed
content check is bad. Right? Then, we should have it set to appropriate context
even for preload.

Regarding CSP, it's ok to issue a request over the network bypassing CSP is ok
(though waste of bandwidth) but it's not fine to deliver the result bypassing
CSP. Right?

[tyoshino (SeeGerritForStatus), 2014-11-11 08:30:44]

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/711213002/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1257:
request.mutableResourceRequest().setRequestContext(WebURLRequest::RequestContextPrefetch);
On 2014/11/11 08:24:41, tyoshino wrote:
> On 2014/11/11 08:08:04, Mike West wrote:
> > The practical question is whether requests that go through this path should
be
> > checked for Mixed Content violations or not. So far, we've allowed prefetch
> > requests. If we change our minds on that (to match the spec), then we'll
need
> to
> > know that this is a prefetch request.
> 
> OK.
> 
> Regarding mixed content, just issuing a request over the network bypassing
mixed
> content check is bad. Right? Then, we should have it set to appropriate
context
> even for preload.
> 
> Regarding CSP, it's ok to issue a request over the network bypassing CSP is ok
> (though waste of bandwidth) but it's not fine to deliver the result bypassing
> CSP. Right?

By "deliver", I mean having it evaluated, displayed, etc.

[Mike West, 2014-11-11 09:15:27]

Ideally, we wouldn't make the request.

As you note, this is more important for mixed content than for CSP.

-mike

[kouhei (in TOK), 2014-11-11 13:32:39]

PTAL. I updated the CL to follow tyoshino-san's suggestion. The requestContext
is set as same as it would be requested directly from <script> and <link
rel='stylesheet'> elements.

[Mike West, 2014-11-11 13:34:18]

LGTM, assuming the ASSERT holds true.

https://codereview.chromium.org/711213002/diff/40001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/711213002/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1245: ASSERT(type == Resource::Script ||
type == Resource::CSSStyleSheet || type == Resource::Image);
Is this an exhaustive list of what goes through this method? We preload a lot of
things...

[kouhei (in TOK), 2014-11-11 13:43:30]

On 2014/11/11 13:34:18, Mike West wrote:
> LGTM, assuming the ASSERT holds true.
> 
>
https://codereview.chromium.org/711213002/diff/40001/Source/core/fetch/Resour...
> File Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/711213002/diff/40001/Source/core/fetch/Resour...
> Source/core/fetch/ResourceFetcher.cpp:1245: ASSERT(type == Resource::Script ||
> type == Resource::CSSStyleSheet || type == Resource::Image);
> Is this an exhaustive list of what goes through this method? We preload a lot
of
> things...

Thanks!
HTMLPreloadScanner generates preload request for only those 3 types. I couldn't
find any usage of ResourceFetcher::preload outside from HTMLPreloadScanner /
HTMLResoucePrefetcher.

[kouhei (in TOK), 2014-11-12 01:36:27]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-11-12 01:37:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/711213002/40001

[commit-bot: I haz the power, 2014-11-12 02:28:00]

Committed patchset #3 (id:40001) as 185169

[tyoshino (SeeGerritForStatus), 2014-11-12 04:09:17]

lgtm