New issue
Advanced search Search tips

Issue 603392 link

Starred by 2 users
Status: Fixed
Owner:
toyoshim@chromium.org
Closed: Apr 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Assertion failure at FontResource::fontLoadShortLimitCallback()

Project Member Reported by hirosh...@chromium.org, Apr 14 2016 
Version: 52.0.2706.0 (r386339)
OS: Linux

What steps will reproduce the problem?
(1) Build Chromium with GYP_DEFINES='asan=1 component=shared_library dcheck_always_on=1'
(2) Open https://polymerelements.github.io/polymer-starter-kit/, reload (F5) several times.

What is the expected output?
No crash.

What do you see instead?
Chromium crashes (once per 2~3 reloads) by an assertion failure:
ASSERTION FAILED: m_loadLimitState == UnderLimit
../../third_party/WebKit/Source/core/fetch/FontResource.cpp(168) : void blink::FontResource::fontLoadShortLimitCallback(Timer<blink::FontResource> *)
1   0x7f0429e430c9
2   0x7f044897db0a blink::TimerBase::runInternal()
3   0x7f044897dfe3
4   0x7f04266acdd8
5   0x7f0449e91a52 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&)
6   0x7f0426680d8b scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(scheduler::internal::WorkQueue*, scheduler::internal::TaskQueueImpl::Task*)
7   0x7f042667d039 scheduler::TaskQueueManager::DoWork(base::TimeTicks, bool)
8   0x7f0426687a25
9   0x7f0449e91a52 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&)
10  0x7f0449f13794 base::MessageLoop::RunTask(base::PendingTask const&)
11  0x7f0449f143d6 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&)
12  0x7f0449f152df base::MessageLoop::DoDelayedWork(base::TimeTicks*)
13  0x7f0449f1acf2 base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
14  0x7f0449f12977 base::MessageLoop::RunHandler()
15  0x7f0449f851cb base::RunLoop::Run()
16  0x7f0449f0fff9 base::MessageLoop::Run()
17  0x7f0443c7f62e
18  0x7f044251ab3f
19  0x7f044251bcda
20  0x7f044251d81c
21  0x7f0442519e1b content::ContentMain(content::ContentMainParams const&)
22  0x7f044b7e4c16
23  0x7f04318eaec5 __libc_start_main
24  0x7f044b725069

Please use labels and text to provide additional information.

Comment 1 by ksakamoto@chromium.org, Apr 14 2016

Components: Blink>WebFonts
Owner: toyoshim@chromium.org
Status: Assigned (was: Untriaged)
Looks like this is what Toyoshima-san mentioned here:
https://codereview.chromium.org/1829403002/#msg20

Comment 2 by hirosh...@chromium.org, Apr 14 2016 

The assertion was introduced by https://codereview.chromium.org/1802123002, and the failure occurs since that revision.

Comment 3 by toyoshim@chromium.org, Apr 15 2016

Status: Started (was: Assigned)
When FontResource::startLoadLimitTimersIfNeeded() is called in the reload, isLoaded() returns true (m_status is Pending) sometime (*).

If previous load hits any timeout, this assert fires for this reason.

There is one clear bug here, if the reload happens between short timeout and loading completion, this assertion fires.

But, it seems this assertion fires even if the reload is executed after the page load is finished.

The error (*) happens once per 2-3 reloads consistently in a lucky chrome session, but could not happen at all in another chrome session.

Let me take a close look.

By the way, can we remove the restrict view label because this is debug only assertion failure in a renderer?

Comment 4 by ksakamoto@chromium.org, Apr 18 2016

Labels: -Restrict-View-Google
This is non-security issue. Let's make it public,

Comment 5 by toyoshim@chromium.org, Apr 18 2016 

As we talked offline, we shouldn't need to care reload cases in FontResource because another instance should be created for reload.

When I reproduced the issue, same object seemed to be reused for reload, but I'll double check this point. If new instance was used, it looked no way to fail on this ASSERT.

Comment 6 by toyoshim@chromium.org, Apr 18 2016 

Tried with trunk and r386339, but I could not reproduce this issue today :(

Note: When it works without any assertion failure, new FontResource is created and used.

Comment 7 by toyoshim@chromium.org, Apr 18 2016 

I think I can find a hint to solve this problem.
New FontResource is created, but startLoadLimitTimersIfNeeded and fontLoadShortLimitCallback are called against the old object on reload.

Comment 8 by toyoshim@chromium.org, Apr 18 2016 

Oops, what I saw at #7 was new FontResource was created for some fonts, but the font hitting the assertion used the same FontResource with the previous load.

Here is a log what happens for a simple page that uses one WebFont.
[1:1:0418/185027:ERROR:FontResource.cpp(90)] @@@ FontResource
[1:1:0418/185027:ERROR:FontResource.cpp(91)] @@@   this: 0x7ed5a986a2e0
[1:1:0418/185027:ERROR:FontResource.cpp(92)] @@@   URL: "https://fonts.gstatic.com/s/poiretone/v4/HrI4ZJpJ3Fh0wa5ofYMK8fk_vArhqVIZ0nv9q090hN8.woff2"
[1:1:0418/185027:ERROR:FontResource.cpp(93)] @@@   limitState: 0
[1:1:0418/185027:ERROR:FontResource.cpp(103)] @@@ didAddClient
[1:1:0418/185027:ERROR:FontResource.cpp(104)] @@@   this: 0x7ed5a986a2e0
[1:1:0418/185027:ERROR:FontResource.cpp(105)] @@@   client: 0x7ec06f9eea98
[1:1:0418/185027:ERROR:FontResource.cpp(103)] @@@ didAddClient
[1:1:0418/185027:ERROR:FontResource.cpp(104)] @@@   this: 0x7ed5a986a2e0
[1:1:0418/185027:ERROR:FontResource.cpp(105)] @@@   client: 0x7ebbf9324d88
[1:1:0418/185027:ERROR:FontResource.cpp(118)] @@@ startLoadLimitTimersIfNeeded
[1:1:0418/185027:ERROR:FontResource.cpp(119)] @@@   this: 0x7ed5a986a2e0
[1:1:0418/185028:ERROR:FontResource.cpp(157)] @@@ fontLoadShortLimitCallback
[1:1:0418/185028:ERROR:FontResource.cpp(158)] @@@   this: 0x7ed5a986a2e0
[1:1:0418/185031:ERROR:FontResource.cpp(118)] @@@ startLoadLimitTimersIfNeeded
[1:1:0418/185031:ERROR:FontResource.cpp(119)] @@@   this: 0x7ed5a986a2e0
[1:1:0418/185032:ERROR:FontResource.cpp(157)] @@@ fontLoadShortLimitCallback
[1:1:0418/185032:ERROR:FontResource.cpp(158)] @@@   this: 0x7ed5a986a2e0
ASSERTION FAILED: m_loadLimitState == UnderLimit

It looks Shift-Reload assigns new FontResource, but Reload assigns the same FontResource.

Comment 9 by toyoshim@chromium.org, Apr 18 2016 

RevalidationPolicy was Revalidate for the second load, and what hiroshige taught me is that an used Resource object would be reused for Revalidate case, but another load operation ran to revalidate the content. That's the case FontResource implementation does not expect, and the root of this issue.

Note: This means if another frame owns the same Resource instance, its content will be updated suddenly, and probably another notifyFinished() is called? That would be another issue.
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 23 2016 

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

commit 27d8b39e91bfc8ffd53c27cfb8dee3029b122d24
Author: toyoshim <toyoshim@chromium.org>
Date: Sat Apr 23 03:59:21 2016

WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch changes Resource::setRevalidatingRequest to virtual
and reset m_loadLimitState at FontResource::setRevalidatingRequest.

BUG= 603392 

Review URL: https://codereview.chromium.org/1893233002

Cr-Commit-Position: refs/heads/master@{#389359}

[add] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/cachable-slow-ahem-loading.cgi
[add] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation-expected.txt
[add] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation.html
[add] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/resources/font-load-revalidation-frame.html
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/webfont-performance-duration-expected.txt
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/webfont-performance-duration.html
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/Source/core/fetch/FontResource.cpp
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/Source/core/fetch/FontResource.h
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/Source/core/fetch/Resource.h

Comment 11 by toyoshim@chromium.org, Apr 25 2016

Status: Fixed (was: Started)
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 25 2016

Labels: merge-merged-2716
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24

commit 27d8b39e91bfc8ffd53c27cfb8dee3029b122d24
Author: toyoshim <toyoshim@chromium.org>
Date: Sat Apr 23 03:59:21 2016

WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch changes Resource::setRevalidatingRequest to virtual
and reset m_loadLimitState at FontResource::setRevalidatingRequest.

BUG= 603392 

Review URL: https://codereview.chromium.org/1893233002

Cr-Commit-Position: refs/heads/master@{#389359}

[add] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/cachable-slow-ahem-loading.cgi
[add] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation-expected.txt
[add] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation.html
[add] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/resources/font-load-revalidation-frame.html
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/webfont-performance-duration-expected.txt
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/LayoutTests/http/tests/webfont/webfont-performance-duration.html
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/Source/core/fetch/FontResource.cpp
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/Source/core/fetch/FontResource.h
[modify] https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24/third_party/WebKit/Source/core/fetch/Resource.h
►

Sign in to add a comment