WebFont: FontResource::startLoadLimitTimers() is called when isLoading() returns false |
|||||
Issue descriptionFontResource::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.
,
Dec 2 2016
,
Dec 5 2016
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.
,
Dec 5 2016
,
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
,
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
,
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
,
Dec 6 2016
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)
,
Dec 7 2016
#5 436257 57.0.2943.0 436483 #6 436535 #7 436536 (See issue 670626.)
,
Dec 9 2016
Sounds like our fix works. So, I'd try relanding the Shao's final CL.
,
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
,
Dec 13 2016
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by toyoshim@chromium.org
, Dec 2 2016