RemoteFontFaceSource should keep FontCustomPlatformData over FontResource revalidation

RemoteFontFaceSource should keep FontCustomPlatformData over FontResource revalidation

RemoteFontFaceSource assumed that FontResource::isLoaded() never returns
false after notifyFinished(), but FontResource 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.

To fix this bug, this CL does the following:
- Make FontCustomPlatformData RefCounted.
- Make RemoteFontFaceSource to get RefPtr<FontCustomPlatformData> and
  use it in RemoteFontFaceSource::createFontData().
- RemoteFontFaceSource never accesses FontResource after notifyFinished().

BUG=602968, 652974, 692574
Review-Url: https://codereview.chromium.org/2717123003
Cr-Commit-Position: refs/heads/master@{#454537}
Committed: https://chromium.googlesource.com/chromium/src/+/99cc846f07ae952c27c026f7a4ddc0dbfba0a83a

[Kunihiko Sakamoto, 2017-02-27 03:29:13]

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

[commit-bot: I haz the power, 2017-02-27 03:29:28]

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, 2017-02-27 05:10:18]

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

[commit-bot: I haz the power, 2017-02-27 05:10:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kunihiko Sakamoto, 2017-02-27 08:50:21]

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

[commit-bot: I haz the power, 2017-02-27 08:50:33]

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

[Kunihiko Sakamoto, 2017-02-27 09:46:43]

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

[commit-bot: I haz the power, 2017-02-27 09:46:54]

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

[Kunihiko Sakamoto, 2017-02-27 10:15:32]

Description was changed from

==========
WIP
==========

to

==========
RemoteFontFaceSource should keep FontCustomPlatformData over FontResource
revalidation

RemoteFontFaceSource assumed that FontResource::isLoaded() never returns
false after notifyFinished(), but FontResource 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.

To fix this bug, this CL does the following:
- Make FontCustomPlatformData RefCounted.
- Make RemoteFontFaceSource to get RefPtr<FontCustomPlatformData> and
  use it in RemoteFontFaceSource::createFontData().
- RemoteFontFaceSource never access FontResource after that point.

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

[Kunihiko Sakamoto, 2017-02-27 10:16:38]

Description was changed from

==========
RemoteFontFaceSource should keep FontCustomPlatformData over FontResource
revalidation

RemoteFontFaceSource assumed that FontResource::isLoaded() never returns
false after notifyFinished(), but FontResource 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.

To fix this bug, this CL does the following:
- Make FontCustomPlatformData RefCounted.
- Make RemoteFontFaceSource to get RefPtr<FontCustomPlatformData> and
  use it in RemoteFontFaceSource::createFontData().
- RemoteFontFaceSource never access FontResource after that point.

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

to

==========
RemoteFontFaceSource should keep FontCustomPlatformData over FontResource
revalidation

RemoteFontFaceSource assumed that FontResource::isLoaded() never returns
false after notifyFinished(), but FontResource 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.

To fix this bug, this CL does the following:
- Make FontCustomPlatformData RefCounted.
- Make RemoteFontFaceSource to get RefPtr<FontCustomPlatformData> and
  use it in RemoteFontFaceSource::createFontData().
- RemoteFontFaceSource never accesses FontResource after notifyFinished().

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

[Kunihiko Sakamoto, 2017-02-27 10:18:47]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2017-02-27 10:18:54]

Patchset #1 (id:20001) has been deleted

[Kunihiko Sakamoto, 2017-02-27 10:19:02]

Patchset #1 (id:40001) has been deleted

[Kunihiko Sakamoto, 2017-02-27 10:19:10]

Patchset #1 (id:60001) has been deleted

[Kunihiko Sakamoto, 2017-02-27 10:21:17]

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

[Kunihiko Sakamoto, 2017-02-27 10:21:18]

PTAL

This is based on the suggestion by hiroshige@ at
https://codereview.chromium.org/2550663002/#msg14 .

[commit-bot: I haz the power, 2017-02-27 11:19:54]

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

[commit-bot: I haz the power, 2017-02-27 11:19:55]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2017-02-27 12:56:02]

I can not find any problem on this change, but have some comments.

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

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:252: if (isLoaded())
Does this short-circuit affect our metrics?

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.h (right):

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:116:
Member<FontResource> m_font;
Can we start having comments on members?
Probably, it would be better to say m_font will be reset once loading is
finished. That explains your isLoaded() implementation in this class.

[hiroshige, 2017-02-28 00:18:28]

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

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:140: m_font =
nullptr;
I love clearing |m_font| here, but clearing |m_font| might cause FontResource to
be garbage collected (if anyone else has strong references to FontResource).
From the aspect of users,
"@font-face { font-family: ahem; src: url('foo.otf'); }
previously kept the Font 'foo.otf' alive on MemoryCache, while after this CL it
doesn't, thus calling addFontFaceRule() in the layout test after Oilpan GC might
result in a new FontResource and duplicated FontCustomPlatformData objects.

WDYT about this behavior change?

[Kunihiko Sakamoto, 2017-02-28 04:04:02]

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

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:140: m_font =
nullptr;
On 2017/02/28 00:18:28, hiroshige wrote:
> I love clearing |m_font| here, but clearing |m_font| might cause FontResource
to
> be garbage collected (if anyone else has strong references to FontResource).
> From the aspect of users,
> "@font-face { font-family: ahem; src: url('foo.otf'); }
> previously kept the Font 'foo.otf' alive on MemoryCache, while after this CL
it
> doesn't, thus calling addFontFaceRule() in the layout test after Oilpan GC
might
> result in a new FontResource and duplicated FontCustomPlatformData objects.
> 
> WDYT about this behavior change?

Good point.

For @font-face fonts, CSSFontFaceSrcValue also holds a reference to
FontResource, so it won't be GC-ed as long as stylesheet is alive. But that can
happen to font faces created by scripts, using new FontFace(...).

We are monitoring memory cache hit rate for webfonts. I would like to go ahead
with this change, and see if it affects the cache hit rate. (I doubt it as usage
of new FontFace() is low)

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:252: if (isLoaded())
On 2017/02/27 12:56:02, Takashi Toyoshima wrote:
> Does this short-circuit affect our metrics?

You mean m_histograms.loadStarted() at line 268? That will not be affected
because m_font->stillNeedsLoad() at line 256 cannot be true.

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.h (right):

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:116:
Member<FontResource> m_font;
On 2017/02/27 12:56:02, Takashi Toyoshima wrote:
> Can we start having comments on members?
> Probably, it would be better to say m_font will be reset once loading is
> finished. That explains your isLoaded() implementation in this class.

Done.

[Takashi Toyoshima, 2017-03-01 06:44:07]

lgtm, defer to hiroshige.

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

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:140: m_font =
nullptr;
https://www.chromestatus.com/metrics/feature/popularity#CSSAtRuleFontFace
https://www.chromestatus.com/metrics/feature/popularity#FontFaceConstructor

@font-face is used in 7.7%, and FontFace is in 0.6%.

If this could affect, we may want to consider having a reference in FontFaceSet
when FontFace is added?

But, I agreed that we can go as is now and see the metrics changes.

https://codereview.chromium.org/2717123003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:252: if (isLoaded())
I see. Thanks!

[hiroshige, 2017-03-01 20:30:45]

lgtm.

(I feel I'd like to further refactor things around
FontResource::getCustomFontData(), but it will be a separate issue)

https://codereview.chromium.org/2717123003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/webfont/font-face-revalidation.html
(right):

https://codereview.chromium.org/2717123003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/webfont/font-face-revalidation.html:24:
You should see a black square below.
Could you describe somewhere in this file what this is testing?
(I'm not sure, but isn't this sentence here obsolete?)

[hiroshige, 2017-03-01 22:17:05]

> If this could affect, we may want to consider having a
> reference in FontFaceSet when FontFace is added?
Creating FontResourceContent that holds references to FontCustomPlatformData and
FontResource might be another option (which is consistent with
ImageResourceContent lifetime management). Anyway I'll draft a CL to see what it
would looks like when needed.

[Kunihiko Sakamoto, 2017-03-03 02:17:24]

+kinuko for core/ and platform/ OWNERS review

https://codereview.chromium.org/2717123003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/webfont/font-face-revalidation.html
(right):

https://codereview.chromium.org/2717123003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/webfont/font-face-revalidation.html:24:
You should see a black square below.
On 2017/03/01 20:30:45, hiroshige wrote:
> Could you describe somewhere in this file what this is testing?
> (I'm not sure, but isn't this sentence here obsolete?)

Done. Actually this sentence is correct, "test" should be rendered as a black
square when "Ahem" font is used.

[Kunihiko Sakamoto, 2017-03-03 02:17:47]

ksakamoto@chromium.org changed reviewers:
+ kinuko@chromium.org

[Kunihiko Sakamoto, 2017-03-03 02:17:48]

+kinuko really

[Kunihiko Sakamoto, 2017-03-03 02:18:32]

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

[commit-bot: I haz the power, 2017-03-03 02:18:59]

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, 2017-03-03 03:39:21]

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

[commit-bot: I haz the power, 2017-03-03 03:39:22]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-03-03 07:13:52]

lgtm

[Kunihiko Sakamoto, 2017-03-03 07:18:52]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2017-03-03 07:18:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, toyoshim@chromium.org
Link to the patchset: https://codereview.chromium.org/2717123003/#ps120001
(title: "test description")

[commit-bot: I haz the power, 2017-03-03 07:19:09]

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, 2017-03-03 07:24:14]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488525532087620,
"parent_rev": "40a050ad605fa870a7f7c5da8c3b0f78bfb4d58f", "commit_rev":
"99cc846f07ae952c27c026f7a4ddc0dbfba0a83a"}

[commit-bot: I haz the power, 2017-03-03 07:25:12]

Description was changed from

==========
RemoteFontFaceSource should keep FontCustomPlatformData over FontResource
revalidation

RemoteFontFaceSource assumed that FontResource::isLoaded() never returns
false after notifyFinished(), but FontResource 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.

To fix this bug, this CL does the following:
- Make FontCustomPlatformData RefCounted.
- Make RemoteFontFaceSource to get RefPtr<FontCustomPlatformData> and
  use it in RemoteFontFaceSource::createFontData().
- RemoteFontFaceSource never accesses FontResource after notifyFinished().

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

to

==========
RemoteFontFaceSource should keep FontCustomPlatformData over FontResource
revalidation

RemoteFontFaceSource assumed that FontResource::isLoaded() never returns
false after notifyFinished(), but FontResource 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.

To fix this bug, this CL does the following:
- Make FontCustomPlatformData RefCounted.
- Make RemoteFontFaceSource to get RefPtr<FontCustomPlatformData> and
  use it in RemoteFontFaceSource::createFontData().
- RemoteFontFaceSource never accesses FontResource after notifyFinished().

BUG=602968, 652974, 692574

Review-Url: https://codereview.chromium.org/2717123003
Cr-Commit-Position: refs/heads/master@{#454537}
Committed:
https://chromium.googlesource.com/chromium/src/+/99cc846f07ae952c27c026f7a4dd...
==========

[commit-bot: I haz the power, 2017-03-03 07:25:14]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/99cc846f07ae952c27c026f7a4dd...