New issue
Advanced search Search tips

Issue 670638 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 670626

Blocking:
issue 570205



Sign in to add a comment

WebFont: FontResource::startLoadLimitTimers() is called when isLoading() returns false

Project Member Reported by toyoshim@chromium.org, Dec 2 2016

Issue description

FontResource::startLoadLimitTimers() has DCHECKs as

 113   CHECK(isLoading());
 114   DCHECK_EQ(m_loadLimitState, LoadNotStarted);

But these assumptions were wrong.

If DCHECK is replaced with CHECK, it fires.
 
Blockedon: 670626
Blocking: 570205
Status: Started (was: Assigned)
Let me clarify the description.

 113   DCHECK(isLoading());
 114   DCHECK_EQ(m_loadLimitState, LoadNotStarted);

If we changed the first DCHECK to CHECK, the check at line 113 fired.

Also if DCHECK at line 147 or 158 was replaced with CHECK, it also fired, but if one of line 113 was also replaced, only the one at line 113 fired.

Thus, fixing the wrong assumption at line 113 would fix related issues that we are seeing.
Summary: WebFont: FontResource::startLoadLimitTimers() is called when isLoading() returns false (was: WebFont: FontResource::startLoadLimitTimers() is called twice)
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32c0cbefcb52226a0ec0765a2156d1c490377ec2

commit 32c0cbefcb52226a0ec0765a2156d1c490377ec2
Author: toyoshim <toyoshim@chromium.org>
Date: Mon Dec 05 10:56:50 2016

FontResource: use CHECKs to confirm if crrev.com/2551823002 fixes the issue

This patch is intended to confirm if the previous change can fix the
issue that fires these CHECKs. Once this patch can survive for some
days, I'd change them to DCHECKs. Otherwise, I'd revert this patch, and
find another potential issues.

Please feel free to revert this patch when you notice some crashes
that are caused by this patch.

BUG= 670638 

Review-Url: https://codereview.chromium.org/2550123002
Cr-Commit-Position: refs/heads/master@{#436257}

[modify] https://crrev.com/32c0cbefcb52226a0ec0765a2156d1c490377ec2/third_party/WebKit/Source/core/loader/resource/FontResource.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3314d6738177bd8b6ad6a8741dd0b26411810a27

commit 3314d6738177bd8b6ad6a8741dd0b26411810a27
Author: toyoshim <toyoshim@chromium.org>
Date: Tue Dec 06 05:17:35 2016

RemoteFontFaceSource should not call startLoadLimitTimers() if startLoad() fails

startLoad() can fail, and in such cases startLoadLimitTimers() should
not be called. Otherwise, DCHECK does not pass, and FontResource would
be confused.

BUG= 670638 
TEST=Tools/Scripts/run-webkit-tests http/tests/css css3/fonts fonts

Review-Url: https://codereview.chromium.org/2551823002
Cr-Commit-Position: refs/heads/master@{#436535}

[modify] https://crrev.com/3314d6738177bd8b6ad6a8741dd0b26411810a27/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/773c66c551f143e25ff6993e8af3377358592d52

commit 773c66c551f143e25ff6993e8af3377358592d52
Author: toyoshim <toyoshim@chromium.org>
Date: Tue Dec 06 05:18:40 2016

FontResource: introduce some CHECKs for setRevalidatingRequest

To confirm if timers were stopped correctly always when we are
revalidating the resource, add some CHECKs.

Once we confirm that this is implemented correctly, change CHECKs
to DCHECKs.

BUG= 670638 

Review-Url: https://codereview.chromium.org/2551803002
Cr-Commit-Position: refs/heads/master@{#436536}

[modify] https://crrev.com/773c66c551f143e25ff6993e8af3377358592d52/third_party/WebKit/Source/core/loader/resource/FontResource.cpp

Oops, crrev.com/2550123002 was submitted before crrev.com/2551823002 was...

Anyway all I have to do here is
- wait for a couple of days and see if CHECKs fire
- replace CHECK with DCHECK
- rebase and submit Shao's final patch (in crbug.com/570205)
#5 436257
57.0.2943.0 436483
#6 436535
#7 436536

(See issue 670626.)
Sounds like our fix works.
So, I'd try relanding the Shao's final CL.
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/87524014534512f530c216924f9d500fb89781de

commit 87524014534512f530c216924f9d500fb89781de
Author: toyoshim <toyoshim@chromium.org>
Date: Fri Dec 09 10:14:34 2016

FontResource: use DCHECKs for confirmed CHECKs

We used CHECKs to confirm if these checks are valid and can survive
on Canary. Now that we know these are correct, let's make it back
to DCHECKs.

BUG= 670638 

Review-Url: https://codereview.chromium.org/2558433002
Cr-Commit-Position: refs/heads/master@{#437510}

[modify] https://crrev.com/87524014534512f530c216924f9d500fb89781de/third_party/WebKit/Source/core/loader/resource/FontResource.cpp

Status: Fixed (was: Started)

Sign in to add a comment