Webfont CORS-enabled fetching with fallback

Webfont CORS-enabled fetching with fallback

Use CORS-enabled fetch method for webfonts. If the CORS-enabled fetch
fails, make a second CORS-disabled request so that we won't break
existing webpages which don't honor the css3 font spec.

We also collect CORS failure counts.

BUG=286681
R=ksakamoto@chromium.org, japhet@chromium.org

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

[bashi, 2014-04-09 09:33:32]

Could you take a look?

Notes:
- I'm not sure how I can add a test for this kind of change which depends on
cross-origin request. No behavior change for local and same-origin request.
- This CL doesn't set referrer as the spec requires. Follow-up CL would come
after this CL.

[Kunihiko Sakamoto, 2014-04-09 10:24:18]

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

https://codereview.chromium.org/230383003/diff/1/Source/core/css/CSSFontFaceS...
Source/core/css/CSSFontFaceSrcValue.cpp:95: return
!securityOrigin->canRequest(resource);
I'm not familiar with this, but doesn't this return false for cross-origin
requests?

https://codereview.chromium.org/230383003/diff/1/Source/core/css/RemoteFontFa...
File Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/230383003/diff/1/Source/core/css/RemoteFontFa...
Source/core/css/RemoteFontFaceSource.cpp:96: m_font->removeClient(this);
CSSFontFaceSrcValue have a reference to this FontResource too, and we're not
updating it. I'm not sure if this may cause any problem or not.

https://codereview.chromium.org/230383003/diff/1/Source/core/fetch/FontResour...
File Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/230383003/diff/1/Source/core/fetch/FontResour...
Source/core/fetch/FontResource.cpp:190: if (errorOccurred() &&
m_options.corsEnabled == IsCORSEnabled) {
Can we retry only when the error is CORS violation?

[bashi, 2014-04-10 03:40:56]

Thank you for review!

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

https://codereview.chromium.org/230383003/diff/1/Source/core/css/CSSFontFaceS...
Source/core/css/CSSFontFaceSrcValue.cpp:95: return
!securityOrigin->canRequest(resource);
On 2014/04/09 10:24:18, Kunihiko Sakamoto wrote:
> I'm not familiar with this, but doesn't this return false for cross-origin
> requests?

In short, canRequest() returns true when |resource| is the same origin as
|securityOrigin|. I think we want to set "Origin" header when |resource| isn't
the same origin.

https://codereview.chromium.org/230383003/diff/1/Source/core/css/RemoteFontFa...
File Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/230383003/diff/1/Source/core/css/RemoteFontFa...
Source/core/css/RemoteFontFaceSource.cpp:96: m_font->removeClient(this);
On 2014/04/09 10:24:18, Kunihiko Sakamoto wrote:
> CSSFontFaceSrcValue have a reference to this FontResource too, and we're not
> updating it. I'm not sure if this may cause any problem or not.

Thank you for pointing this out. But it's difficult to update the reference
because RemoteFontFaceSource, which is the client of FontResource, can't reach
CSSFontFaceSrcValue when CORS failure occurs. Updating it will require
non-trivial change in css module and I'm not sure it is worth to do it for this
tentative logic.

I added an 'invalidator' which clears the reference in CSSFontFaceSrcValue so
that calling CSSFontFaceSrcValue::fetch() won't return invalid FontResource at
least. Does this make sense?

https://codereview.chromium.org/230383003/diff/1/Source/core/fetch/FontResour...
File Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/230383003/diff/1/Source/core/fetch/FontResour...
Source/core/fetch/FontResource.cpp:190: if (errorOccurred() &&
m_options.corsEnabled == IsCORSEnabled) {
On 2014/04/09 10:24:18, Kunihiko Sakamoto wrote:
> Can we retry only when the error is CORS violation?

Done.

[Kunihiko Sakamoto, 2014-04-10 05:49:23]

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

https://codereview.chromium.org/230383003/diff/1/Source/core/css/CSSFontFaceS...
Source/core/css/CSSFontFaceSrcValue.cpp:95: return
!securityOrigin->canRequest(resource);
On 2014/04/10 03:40:56, bashik wrote:
> On 2014/04/09 10:24:18, Kunihiko Sakamoto wrote:
> > I'm not familiar with this, but doesn't this return false for cross-origin
> > requests?
> 
> In short, canRequest() returns true when |resource| is the same origin as
> |securityOrigin|. I think we want to set "Origin" header when |resource| isn't
> the same origin.

Oh, I missed the ! operator. So we add Origin header only for cross-origin
request. Is that consistent with other CORS-subjected resources? FF seems to add
Origin header for same-origin font requests as well.

https://codereview.chromium.org/230383003/diff/20001/Source/core/css/CSSFontF...
File Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/230383003/diff/20001/Source/core/css/CSSFontF...
Source/core/css/CSSFontFaceSrcValue.cpp:46: m_resource->addClient(this);
Resource cancels in-flight request when all clients are removed.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

This change makes FontResource always have one client (invalidator), so
cancellation won't work for fonts.

Instead of using the invalidator, maybe we can just clear m_fetched in the
beginning of CSSFontFaceSrcValue::fetch() if m_corsFailed is true?

[Kunihiko Sakamoto, 2014-04-10 06:53:09]

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

https://codereview.chromium.org/230383003/diff/1/Source/core/css/CSSFontFaceS...
Source/core/css/CSSFontFaceSrcValue.cpp:95: return
!securityOrigin->canRequest(resource);
On 2014/04/10 05:49:24, Kunihiko Sakamoto wrote:
> On 2014/04/10 03:40:56, bashik wrote:
> > On 2014/04/09 10:24:18, Kunihiko Sakamoto wrote:
> > > I'm not familiar with this, but doesn't this return false for cross-origin
> > > requests?
> > 
> > In short, canRequest() returns true when |resource| is the same origin as
> > |securityOrigin|. I think we want to set "Origin" header when |resource|
isn't
> > the same origin.
> 
> Oh, I missed the ! operator. So we add Origin header only for cross-origin
> request. Is that consistent with other CORS-subjected resources? FF seems to
add
> Origin header for same-origin font requests as well.

Nevermind - FireFox does not add Origin header for same-origin requests. I was
looking at wrong log.

[bashi, 2014-04-10 07:05:50]

https://codereview.chromium.org/230383003/diff/20001/Source/core/css/CSSFontF...
File Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/230383003/diff/20001/Source/core/css/CSSFontF...
Source/core/css/CSSFontFaceSrcValue.cpp:46: m_resource->addClient(this);
On 2014/04/10 05:49:24, Kunihiko Sakamoto wrote:
> Resource cancels in-flight request when all clients are removed.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> This change makes FontResource always have one client (invalidator), so
> cancellation won't work for fonts.
> 
> Instead of using the invalidator, maybe we can just clear m_fetched in the
> beginning of CSSFontFaceSrcValue::fetch() if m_corsFailed is true?

Great suggestion! Done.

[Kunihiko Sakamoto, 2014-04-10 08:32:17]

non-owner lgtm

[bashi, 2014-04-10 09:16:31]

+dglazkov

[dglazkov, 2014-04-10 15:48:35]

lgtm, please stay focused on fixing that FIXME :)

[bashi, 2014-04-10 22:04:14]

The CQ bit was checked by bashi@chromium.org

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

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

[bashi, 2014-04-10 22:05:02]

On 2014/04/10 15:48:35, dglazkov wrote:
> lgtm, please stay focused on fixing that FIXME :)

Certainly.

[bashi, 2014-04-10 22:11:27]

The CQ bit was unchecked by bashi@chromium.org

[bashi, 2014-04-10 23:27:12]

The CQ bit was checked by bashi@chromium.org

[bashi, 2014-04-10 23:27:14]

The CQ bit was unchecked by bashi@chromium.org

[bashi, 2014-04-10 23:35:31]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-04-10 23:35:38]

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

[commit-bot: I haz the power, 2014-04-11 23:53:50]

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

[bashi, 2014-04-14 04:52:00]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-04-14 04:52:08]

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

[commit-bot: I haz the power, 2014-04-14 05:50:28]

Change committed as 171423