Prevent FontResource load limit timers from restarting during loading

Prevent FontResource load limit timers from restarting during loading

FontResource::startLoadLimitTimersIfNeeded() was called from
RemoteFontFaceSource::beginLoadIfNeeded() on each style recalc, and bypasses
checks if called when FontResource is still loading and long limit timer has
fired. This may cause RemoteFontFaceSource callbacks to be called more than once
and in turn corrupt its internal state.
Now we start timers only after actual load of FontResource is started.

BUG=655076
Committed: https://crrev.com/6b2d64014d739051f120dc38df46ac73a469270d
Cr-Commit-Position: refs/heads/master@{#425010}

[Shao-Chuan Lee, 2016-10-13 05:09:12]

Description was changed from

==========
Prevent FontResource load limit timers to restart during loading

FontResource::startLoadLimitTimersIfNeeded() was called from
RemoteFontFaceSource::beginLoadIfNeeded() on each style recalc, and bypasses
checks if called when FontResource is still loading and long limit timer has
fired. This may cause RemoteFontFaceSource callbacks to be called more than once
and in turn corrupt its internal state.
Now we start timers only after actual load of FontResource is started.

BUG=655076
==========

to

==========
Prevent FontResource load limit timers from restarting during loading

FontResource::startLoadLimitTimersIfNeeded() was called from
RemoteFontFaceSource::beginLoadIfNeeded() on each style recalc, and bypasses
checks if called when FontResource is still loading and long limit timer has
fired. This may cause RemoteFontFaceSource callbacks to be called more than once
and in turn corrupt its internal state.
Now we start timers only after actual load of FontResource is started.

BUG=655076
==========

[Shao-Chuan Lee, 2016-10-13 05:10:38]

shaochuan@chromium.org changed reviewers:
+ kouhei@chromium.org, ksakamoto@chromium.org

[Shao-Chuan Lee, 2016-10-13 05:10:39]

PTAL. Is it OK to have regression LayoutTests like this?

[Kunihiko Sakamoto, 2016-10-13 05:55:20]

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/webfont/crbug-655076.html
(right):

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/webfont/crbug-655076.html:12: },
5000);
5000ms is the same as slow-ahem-loading.cgi delay below. Does this work
reliably?

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/FontResource.cpp:114:
DCHECK(!m_isLoadLimitTimersStarted);
Can you add DCHECK(isLoading()) here?

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/FontResource.cpp:175:
m_isLoadLimitTimersStarted = false;
Does this mean startLoadLimitTimers() can be called again?

[Shao-Chuan Lee, 2016-10-13 06:12:58]

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/webfont/crbug-655076.html
(right):

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/webfont/crbug-655076.html:12: },
5000);
On 2016/10/13 05:55:19, Kunihiko Sakamoto wrote:
> 5000ms is the same as slow-ahem-loading.cgi delay below. Does this work
> reliably?

It always crashes when the second span is rendered at 4000ms, I guess this
doesn't matter.
Maybe it's better to move notifyDone() right after the second makeSpan() call to
make this test faster.

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/FontResource.cpp:114:
DCHECK(!m_isLoadLimitTimersStarted);
On 2016/10/13 05:55:19, Kunihiko Sakamoto wrote:
> Can you add DCHECK(isLoading()) here?

Done.

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/FontResource.cpp:175:
m_isLoadLimitTimersStarted = false;
On 2016/10/13 05:55:19, Kunihiko Sakamoto wrote:
> Does this mean startLoadLimitTimers() can be called again?

Yes, in revalidation cases.

[Kunihiko Sakamoto, 2016-10-13 06:36:19]

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/webfont/crbug-655076.html
(right):

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/webfont/crbug-655076.html:12: },
5000);
On 2016/10/13 06:12:58, Shao-Chuan Lee wrote:
> On 2016/10/13 05:55:19, Kunihiko Sakamoto wrote:
> > 5000ms is the same as slow-ahem-loading.cgi delay below. Does this work
> > reliably?
> 
> It always crashes when the second span is rendered at 4000ms, I guess this
> doesn't matter.

But resulting document may be different depending on whether the font is loaded
or not, and may not match with the expected.html.

> Maybe it's better to move notifyDone() right after the second makeSpan() call
to
> make this test faster.

This looks good.

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/FontResource.cpp:175:
m_isLoadLimitTimersStarted = false;
On 2016/10/13 06:12:58, Shao-Chuan Lee wrote:
> On 2016/10/13 05:55:19, Kunihiko Sakamoto wrote:
> > Does this mean startLoadLimitTimers() can be called again?
> 
> Yes, in revalidation cases.

Hm, then we have to reset m_loadLimitState too?
Can we remove m_isLoadLimitTimersStarted flag by introducing a new state to
LoadLimitState?

[Shao-Chuan Lee, 2016-10-13 07:14:06]

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/webfont/crbug-655076.html
(right):

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/webfont/crbug-655076.html:12: },
5000);
On 2016/10/13 06:36:18, Kunihiko Sakamoto wrote:
> On 2016/10/13 06:12:58, Shao-Chuan Lee wrote:
> > On 2016/10/13 05:55:19, Kunihiko Sakamoto wrote:
> > > 5000ms is the same as slow-ahem-loading.cgi delay below. Does this work
> > > reliably?
> > 
> > It always crashes when the second span is rendered at 4000ms, I guess this
> > doesn't matter.
> 
> But resulting document may be different depending on whether the font is
loaded
> or not, and may not match with the expected.html.

I see. I think it's better to make this test text-only.

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2419753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/FontResource.cpp:175:
m_isLoadLimitTimersStarted = false;
On 2016/10/13 06:36:18, Kunihiko Sakamoto wrote:
> On 2016/10/13 06:12:58, Shao-Chuan Lee wrote:
> > On 2016/10/13 05:55:19, Kunihiko Sakamoto wrote:
> > > Does this mean startLoadLimitTimers() can be called again?
> > 
> > Yes, in revalidation cases.
> 
> Hm, then we have to reset m_loadLimitState too?
> Can we remove m_isLoadLimitTimersStarted flag by introducing a new state to
> LoadLimitState?

Adding new LoadLimitState to indicate that timers are not yet started. This
looks more consistent, thanks.

[Kunihiko Sakamoto, 2016-10-13 07:21:55]

lgtm, thanks.

[Shao-Chuan Lee, 2016-10-13 07:28:34]

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

[commit-bot: I haz the power, 2016-10-13 07:28:45]

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-10-13 08:40:46]

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

[commit-bot: I haz the power, 2016-10-13 08:40:47]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2016-10-13 11:41:59]

lgtm

[Shao-Chuan Lee, 2016-10-13 12:58:05]

The CQ bit was checked by shaochuan@chromium.org

[commit-bot: I haz the power, 2016-10-13 12:58:11]

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-10-13 13:03:50]

Description was changed from

==========
Prevent FontResource load limit timers from restarting during loading

FontResource::startLoadLimitTimersIfNeeded() was called from
RemoteFontFaceSource::beginLoadIfNeeded() on each style recalc, and bypasses
checks if called when FontResource is still loading and long limit timer has
fired. This may cause RemoteFontFaceSource callbacks to be called more than once
and in turn corrupt its internal state.
Now we start timers only after actual load of FontResource is started.

BUG=655076
==========

to

==========
Prevent FontResource load limit timers from restarting during loading

FontResource::startLoadLimitTimersIfNeeded() was called from
RemoteFontFaceSource::beginLoadIfNeeded() on each style recalc, and bypasses
checks if called when FontResource is still loading and long limit timer has
fired. This may cause RemoteFontFaceSource callbacks to be called more than once
and in turn corrupt its internal state.
Now we start timers only after actual load of FontResource is started.

BUG=655076
==========

[commit-bot: I haz the power, 2016-10-13 13:03:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-13 13:05:30]

Description was changed from

==========
Prevent FontResource load limit timers from restarting during loading

FontResource::startLoadLimitTimersIfNeeded() was called from
RemoteFontFaceSource::beginLoadIfNeeded() on each style recalc, and bypasses
checks if called when FontResource is still loading and long limit timer has
fired. This may cause RemoteFontFaceSource callbacks to be called more than once
and in turn corrupt its internal state.
Now we start timers only after actual load of FontResource is started.

BUG=655076
==========

to

==========
Prevent FontResource load limit timers from restarting during loading

FontResource::startLoadLimitTimersIfNeeded() was called from
RemoteFontFaceSource::beginLoadIfNeeded() on each style recalc, and bypasses
checks if called when FontResource is still loading and long limit timer has
fired. This may cause RemoteFontFaceSource callbacks to be called more than once
and in turn corrupt its internal state.
Now we start timers only after actual load of FontResource is started.

BUG=655076

Committed: https://crrev.com/6b2d64014d739051f120dc38df46ac73a469270d
Cr-Commit-Position: refs/heads/master@{#425010}
==========

[commit-bot: I haz the power, 2016-10-13 13:05:31]

Patchset 4 (id:??) landed as
https://crrev.com/6b2d64014d739051f120dc38df46ac73a469270d
Cr-Commit-Position: refs/heads/master@{#425010}