Set proper referrer for css resource fetching

Set proper referrer for css resource fetching

Fetching css resources are initiated as follows:
  - Create FetchRequest
  - Call ResourceFetcher::fetchXXX()

git-grep shows me that CSSImageValue, CSSImageSetValue and
CSSFontFaceSrcValue are all of the resource fetch initiators
which live in css/. I think that setting referrer in these
classes should cover all css resource fetching.

Since resource fetching is delayed until the resource is actually
needed, we need to hold the referrer somewhere.
This CL modifies the css parser to set referrer to corresponding
CSSValue (CSSImageValue and CSSFontFaceSrcValue) when it sees url(...).

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172433

[bashi, 2014-04-16 05:28:30]

Hi abarth@,

Could you take a look? If this approach sounds reasonable, I'll add tests.

[abarth-chromium, 2014-04-17 17:44:58]

This approach looks much better, thanks!  I have a few detail-oriented
questions.

I'm kind of amazed we don't have tests for this already, but we should certainly
write some.  :)

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSFontFaceS...
File Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSFontFaceS...
Source/core/css/CSSFontFaceSrcValue.cpp:108: if (!m_referrer.isEmpty())
When would m_referrer be empty?  Can we change this to an ASSERT?

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSFontFaceS...
File Source/core/css/CSSFontFaceSrcValue.h (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSFontFaceS...
Source/core/css/CSSFontFaceSrcValue.h:93: String m_referrer;
Please group this with the other strings to improve structure packing.

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageSetV...
File Source/core/css/CSSImageSetValue.cpp (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageSetV...
Source/core/css/CSSImageSetValue.cpp:108: if (!image.referrer.isEmpty())
Ditto

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageValu...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageValu...
Source/core/css/CSSImageValue.cpp:65: if (!m_referrer.isEmpty())
ditto

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageValue.h
File Source/core/css/CSSImageValue.h (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageValu...
Source/core/css/CSSImageValue.h:81: String m_referrer;
Please move this up one line to improve packing.

https://codereview.chromium.org/239353007/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:369:
toCSSImageValue(imageValue.get())->setReferrer(m_context.baseURL().string());
What does m_context.baseURL() mean here?

Don't we need to call strippedForUseAsReferrer() instead of string() here?

[bashi, 2014-04-18 10:16:59]

Thank you for review. Added a test, updated the description(remove WIP). Could
you take another look?

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSFontFaceS...
File Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSFontFaceS...
Source/core/css/CSSFontFaceSrcValue.cpp:108: if (!m_referrer.isEmpty())
On 2014/04/17 17:44:58, abarth wrote:
> When would m_referrer be empty?  Can we change this to an ASSERT?

Looks like strippedForUseAsReferrer() could return an empty string so I think it
is safer not to make this an ASSERT?

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSFontFaceS...
File Source/core/css/CSSFontFaceSrcValue.h (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSFontFaceS...
Source/core/css/CSSFontFaceSrcValue.h:93: String m_referrer;
On 2014/04/17 17:44:58, abarth wrote:
> Please group this with the other strings to improve structure packing.

Done.

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageSetV...
File Source/core/css/CSSImageSetValue.cpp (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageSetV...
Source/core/css/CSSImageSetValue.cpp:108: if (!image.referrer.isEmpty())
On 2014/04/17 17:44:58, abarth wrote:
> Ditto

Same as CSSFontFaceSrcValue.

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageValu...
File Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageValu...
Source/core/css/CSSImageValue.cpp:65: if (!m_referrer.isEmpty())
On 2014/04/17 17:44:58, abarth wrote:
> ditto

Same as CSSFontFaceSrcValue.

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageValue.h
File Source/core/css/CSSImageValue.h (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/CSSImageValu...
Source/core/css/CSSImageValue.h:81: String m_referrer;
On 2014/04/17 17:44:58, abarth wrote:
> Please move this up one line to improve packing.

Done.

https://codereview.chromium.org/239353007/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/239353007/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:369:
toCSSImageValue(imageValue.get())->setReferrer(m_context.baseURL().string());
On 2014/04/17 17:44:58, abarth wrote:
> What does m_context.baseURL() mean here?

I think it's the URL of stylesheets which is being parsed.

> Don't we need to call strippedForUseAsReferrer() instead of string() here?

I was not aware that such method exists. Thank you for pointing this out. Done.

[abarth-chromium, 2014-04-18 16:36:49]

Please add tests to cover the srcdoc bug in this iteration of the CL.

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLBod...
File Source/core/html/HTMLBodyElement.cpp (right):

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLBod...
Source/core/html/HTMLBodyElement.cpp:73:
imageValue->setReferrer(document().url().strippedForUseAsReferrer());
This is not correct.  You need to call document().outgoingReferrer() instead.

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTableElement.cpp (right):

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableElement.cpp:309:
imageValue->setReferrer(document().url().strippedForUseAsReferrer());
document().outgoingReferrer()

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTablePartElement.cpp (right):

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLTab...
Source/core/html/HTMLTablePartElement.cpp:57:
imageValue->setReferrer(document().url().strippedForUseAsReferrer());
document().outgoingReferrer()

[abarth-chromium, 2014-04-18 16:39:23]

By the way, do other browsers have this behavior?

[bashi, 2014-04-22 01:22:55]

Sorry for late response. Added a test for srcdoc. I'm not sure what the srcdoc
bug is, so please let me know if this CL should contain more tests.

> By the way, do other browsers have this behavior?

Firefox has this behavior (but it seems Firefox strips referrer for srcdoc). IE
and Safari don't have this behavior. I think we should follow Firefox at least
for web fonts as the spec requires.

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLBod...
File Source/core/html/HTMLBodyElement.cpp (right):

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLBod...
Source/core/html/HTMLBodyElement.cpp:73:
imageValue->setReferrer(document().url().strippedForUseAsReferrer());
On 2014/04/18 16:36:49, abarth wrote:
> This is not correct.  You need to call document().outgoingReferrer() instead.

Done.

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTableElement.cpp (right):

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableElement.cpp:309:
imageValue->setReferrer(document().url().strippedForUseAsReferrer());
On 2014/04/18 16:36:49, abarth wrote:
> document().outgoingReferrer()

Done.

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTablePartElement.cpp (right):

https://codereview.chromium.org/239353007/diff/60001/Source/core/html/HTMLTab...
Source/core/html/HTMLTablePartElement.cpp:57:
imageValue->setReferrer(document().url().strippedForUseAsReferrer());
On 2014/04/18 16:36:49, abarth wrote:
> document().outgoingReferrer()

Done.

[abarth-chromium, 2014-04-22 16:11:10]

lgtm

[commit-bot: I haz the power, 2014-04-22 16:11:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bashi@chromium.org/239353007/80001

[commit-bot: I haz the power, 2014-04-22 16:44:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 16:44:01]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[eseidel, 2014-04-22 16:51:01]

We do this so that servers can block certain referrers?

Presumably we already send referrers on other sub-resources (like scripts)? 
Although since subresources can be cached/shared per-process, i could imagine
servers which change behavior based on the referrer would get odd client-side
behavior with cache hits.  I guess servers which do that would set vary:
referrer and be fine.

[abarth-chromium, 2014-04-22 23:30:06]

On 2014/04/22 16:51:01, eseidel wrote:
> We do this so that servers can block certain referrers?

It might also just be for logging so that servers know who is using their
resources.

> Presumably we already send referrers on other sub-resources (like scripts)?

Yes, those get the outgoingReferrer of the document in which they appear.  For
CSS resources loaded via stylesheets loaded via <link rel="stylesheet">, the
question is whether they should get the outgoingReferrer of the document that
started the whole shebang or whether they should get the URL of the stylesheet
itself as their referrer because that's the resource that actually referred to
them.  This CL makes us do the latter.

> Although since subresources can be cached/shared per-process, i could imagine
> servers which change behavior based on the referrer would get odd client-side
> behavior with cache hits.  I guess servers which do that would set vary:
> referrer and be fine.

Yeah, Referer makes no sense in theory.  :)

[bashi, 2014-04-22 23:33:10]

On 2014/04/22 16:51:01, eseidel wrote:
> We do this so that servers can block certain referrers?

I think it's one of the main reasons why spec requires that.

> 
> Presumably we already send referrers on other sub-resources (like scripts)? 
> Although since subresources can be cached/shared per-process, i could imagine
> servers which change behavior based on the referrer would get odd client-side
> behavior with cache hits.  I guess servers which do that would set vary:
> referrer and be fine.

I'm not sure our cache mechanism takes referrer into account. If not, odd
client-side behavior could happen. But we've been setting referrers on
sub-resources to the document's URL so we would have seen the problem in that
case. Am I missing something?

[bashi, 2014-04-22 23:45:43]

On 2014/04/22 23:30:06, abarth wrote:
> On 2014/04/22 16:51:01, eseidel wrote:
> > We do this so that servers can block certain referrers?
> 
> It might also just be for logging so that servers know who is using their
> resources.
> 
> > Presumably we already send referrers on other sub-resources (like scripts)?
> 
> Yes, those get the outgoingReferrer of the document in which they appear.  For
> CSS resources loaded via stylesheets loaded via <link rel="stylesheet">, the
> question is whether they should get the outgoingReferrer of the document that
> started the whole shebang or whether they should get the URL of the stylesheet
> itself as their referrer because that's the resource that actually referred to
> them.  This CL makes us do the latter.
> 
> > Although since subresources can be cached/shared per-process, i could
imagine
> > servers which change behavior based on the referrer would get odd
client-side
> > behavior with cache hits.  I guess servers which do that would set vary:
> > referrer and be fine.
> 
> Yeah, Referer makes no sense in theory.  :)

Thank you for detailed explanation!

@eseidel, please leave another comment if needed, I'll check CQ bit tomorrow.

[bashi, 2014-04-23 23:26:00]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-04-23 23:26:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bashi@chromium.org/239353007/80001

[commit-bot: I haz the power, 2014-04-24 00:09:23]

Change committed as 172433

[bashi, 2014-05-28 12:07:56]

A revert of this CL has been created in
https://codereview.chromium.org/304603006/ by bashi@chromium.org.

The reason for reverting is: Caused regression.