Allow creating font data while FontResource is revalidating

Allow creating font data while FontResource is revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" state by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, existing RemoteFontFaceSource returns fontdata from
FontResource, even while it is being revalidated by other
RemoteFontFaceSource instances.

BUG=602968, 652974, 692574

[Kunihiko Sakamoto, 2016-12-09 06:06:30]

The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 06:06:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 07:23:10]

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

[commit-bot: I haz the power, 2016-12-09 07:23:11]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-02-22 08:57:45]

Description was changed from

==========
Test case for crbug.com/652974

BUG=652974
==========

to

==========
Allow creating font data while revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" status by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, RemoteFontFaceSource::createFontData() returns real
fontdata if FontResource have it, even while it is being revalidated by
other frames.

BUG=602968,652974,692574
==========

[Kunihiko Sakamoto, 2017-02-22 09:01:40]

Description was changed from

==========
Allow creating font data while revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" status by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, RemoteFontFaceSource::createFontData() returns real
fontdata if FontResource have it, even while it is being revalidated by
other frames.

BUG=602968,652974,692574
==========

to

==========
Allow creating font data while revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" status by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, existing RemoteFontFaceSource::createFontData()
returns fontdata from FontResource, even while it is being revalidated by
other RemoteFontFaceSources.

BUG=602968,652974,692574
==========

[Kunihiko Sakamoto, 2017-02-22 09:02:50]

Description was changed from

==========
Allow creating font data while revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" status by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, existing RemoteFontFaceSource::createFontData()
returns fontdata from FontResource, even while it is being revalidated by
other RemoteFontFaceSources.

BUG=602968,652974,692574
==========

to

==========
Allow creating font data while revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" status by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, existing RemoteFontFaceSource returns fontdata from
FontResource, even while it is being revalidated by other
RemoteFontFaceSource instances.

BUG=602968,652974,692574
==========

[Kunihiko Sakamoto, 2017-02-22 09:04:44]

Description was changed from

==========
Allow creating font data while revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" status by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, existing RemoteFontFaceSource returns fontdata from
FontResource, even while it is being revalidated by other
RemoteFontFaceSource instances.

BUG=602968,652974,692574
==========

to

==========
Allow creating font data while FontResource is revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" status by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, existing RemoteFontFaceSource returns fontdata from
FontResource, even while it is being revalidated by other
RemoteFontFaceSource instances.

BUG=602968,652974,692574
==========

[Kunihiko Sakamoto, 2017-02-22 09:05:54]

Description was changed from

==========
Allow creating font data while FontResource is revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" status by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, existing RemoteFontFaceSource returns fontdata from
FontResource, even while it is being revalidated by other
RemoteFontFaceSource instances.

BUG=602968,652974,692574
==========

to

==========
Allow creating font data while FontResource is revalidating

Before this patch, RemoteFontFaceSource::createFontData() returned a
blank fontdata ("loading fallback" fontdata) whenever
FontResource::isLoaded() is false. However, FontResource once loaded
can go back to "loading" state by resource revalidation. This caused a
bug where webfonts are not displayed when loaded in one frame and
immediately revalidated in another frame.

After this change, existing RemoteFontFaceSource returns fontdata from
FontResource, even while it is being revalidated by other
RemoteFontFaceSource instances.

BUG=602968,652974,692574
==========

[Kunihiko Sakamoto, 2017-02-22 09:08:27]

ksakamoto@chromium.org changed reviewers:
+ hiroshige@chromium.org, japhet@chromium.org

[Takashi Toyoshima, 2017-02-23 03:21:36]

toyoshim@chromium.org changed reviewers:
+ toyoshim@chromium.org

[Takashi Toyoshima, 2017-02-23 03:21:37]

https://codereview.chromium.org/2550663002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2550663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:209: if (!isLoaded()
&& !m_font->hasCustomFontData())
Should we still have the check of !isLoaded() here?
I think hascustomFontData() returning true exactly means that the notifyFinished
was already called, right?

https://codereview.chromium.org/2550663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:212: if
(!m_font->ensureCustomFontData() || m_period == FailurePeriod)
I'm not sure these checks are still reasonable.

Could you merge checks 209 through 213 just like

if (!m_font->ensureCustomFontData())
  return createLoadingFallbackFontData(fontDescription);

Also, how returning nullptr makes difference from returning fallback font data?

[hiroshige, 2017-02-23 18:35:27]

[1] I feel like |FontResource::m_fontData| is not updated on revalidation and
thus revalidation on FontResource has virtually no effect.

[2] If we fix [1], then |m_fontData| is null during revalidation (after
responseReceived() and before finish(), or when revalidation response is error),
and thus this CL doen't work in such cases.
In the case of FontResource, if we are not interested in updates/revalidations
after notifyFinished(), then we should receive the decoded data from
FontResource in notifyFinished() and never access FontResource after that point,
i.e.,
- make FontCustomPlatformData RefCounted,
- make RemoteFontFaceSource to get RefPtr<FontCustomPlatformData> and
- use it in RemoteFontFaceSource::createFontData().

FYI. ScriptResource might have a similar? issue under investigation (see
https://codereview.chromium.org/2706303002/).

[hiroshige, 2017-02-23 18:41:13]

On 2017/02/23 18:35:27, hiroshige wrote:
> [1] I feel like |FontResource::m_fontData| is not updated on revalidation and
> thus revalidation on FontResource has virtually no effect.
> 
> [2] If we fix [1], then |m_fontData| is null during revalidation (after
> responseReceived() and before finish(), or when revalidation response is
error),
> and thus this CL doen't work in such cases.
> In the case of FontResource, if we are not interested in updates/revalidations
> after notifyFinished(), then we should receive the decoded data from
> FontResource in notifyFinished() and never access FontResource after that
point,
> i.e.,
> - make FontCustomPlatformData RefCounted,
> - make RemoteFontFaceSource to get RefPtr<FontCustomPlatformData> and
> - use it in RemoteFontFaceSource::createFontData().
> 
> FYI. ScriptResource might have a similar? issue under investigation (see
> https://codereview.chromium.org/2706303002/).

If we want to have a quick and mergeable fix, then the direction of this current
CL looks good.
[1] is already broken, so this CL just allows use of |FontResource::m_fontData|
for an additional short period of time (between responseReceived() and
finish()), which doesn't weaken security fundamentally.

[Kunihiko Sakamoto, 2017-02-24 08:31:57]

On 2017/02/23 18:41:13, hiroshige wrote:
> On 2017/02/23 18:35:27, hiroshige wrote:
> > [1] I feel like |FontResource::m_fontData| is not updated on revalidation
and
> > thus revalidation on FontResource has virtually no effect.
> > 
> > [2] If we fix [1], then |m_fontData| is null during revalidation (after
> > responseReceived() and before finish(), or when revalidation response is
> error),
> > and thus this CL doen't work in such cases.
> > In the case of FontResource, if we are not interested in
updates/revalidations
> > after notifyFinished(), then we should receive the decoded data from
> > FontResource in notifyFinished() and never access FontResource after that
> point,
> > i.e.,
> > - make FontCustomPlatformData RefCounted,
> > - make RemoteFontFaceSource to get RefPtr<FontCustomPlatformData> and
> > - use it in RemoteFontFaceSource::createFontData().
> > 
> > FYI. ScriptResource might have a similar? issue under investigation (see
> > https://codereview.chromium.org/2706303002/).
> 
> If we want to have a quick and mergeable fix, then the direction of this
current
> CL looks good.
> [1] is already broken, so this CL just allows use of
|FontResource::m_fontData|
> for an additional short period of time (between responseReceived() and
> finish()), which doesn't weaken security fundamentally.

Your suggestion [2] sounds like the right way to fix this.
I didn't intend to merge this, so let me close this and create a new CL for [2].

Thanks!