Bug 215435 - Font loads quickly followed by navigations may fail indefinitely
Summary: Font loads quickly followed by navigations may fail indefinitely
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-12 18:53 PDT by Myles C. Maxfield
Modified: 2020-09-10 13:32 PDT (History)
12 users (show)

See Also:


Attachments
Patch (9.38 KB, patch)
2020-08-12 18:58 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (9.70 KB, patch)
2020-08-12 22:05 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (14.19 KB, patch)
2020-08-20 12:55 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2020-08-12 18:53:29 PDT
Font loads quickly followed by navigations may fail indefinitely
Comment 1 Myles C. Maxfield 2020-08-12 18:58:53 PDT
Created attachment 406487 [details]
Patch
Comment 2 Darin Adler 2020-08-12 19:48:22 PDT
Comment on attachment 406487 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406487&action=review

> LayoutTests/fast/loader/font-load-timer.html:19
> +window.location = "resources/font-load-timer.html";

It’s confusing that both the main file and the sub resource have the same filename.
Comment 3 Darin Adler 2020-08-12 19:53:23 PDT
Comment on attachment 406487 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406487&action=review

Had 3 more thoughts.

1) m_beginLoadingTimer is already a "suspendable timer". Seems possibly slightly suspect that we are implementing another suspend mechanism on top of that.

> Source/WebCore/css/CSSFontSelector.cpp:367
>      m_beginLoadingTimer.startOneShot(0_s);

2) Don’t we want to check m_fontLoadingTimerIsSuspended here, and not start the timer if it’s true? Or I guess we could assert it if we know it’s not true.

> Source/WebCore/css/CSSFontSelector.cpp:384
> +    m_beginLoadingTimer.startOneShot(0_s);

3) Seems slightly wasteful to start the timer even if m_fontsToBeginLoading is empty.
Comment 4 Myles C. Maxfield 2020-08-12 21:24:49 PDT
<rdar://problem/65560550>
Comment 5 Myles C. Maxfield 2020-08-12 22:02:41 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 406487 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406487&action=review
> 
> 1) m_beginLoadingTimer is already a "suspendable timer". Seems possibly
> slightly suspect that we are implementing another suspend mechanism on top
> of that.

Yeah, SuspendableTimer is integrated with ActiveDOMObject, which is where it plugs in to being suspended. Unfortunately, SuspendableTimerBase::suspend() and SuspendableTimerBase::resume() are private, and so can't be called from CSSFontSelector. (And I assume they're private for a good reason, though I don't know what it would be.)
Comment 6 Myles C. Maxfield 2020-08-12 22:05:21 PDT
Created attachment 406497 [details]
Patch for committing
Comment 7 Myles C. Maxfield 2020-08-13 01:13:13 PDT
Committed r265603: <https://trac.webkit.org/changeset/265603>
Comment 8 Radar WebKit Bug Importer 2020-08-13 01:14:22 PDT
<rdar://problem/66970271>
Comment 9 Karl Rackler 2020-08-17 16:19:46 PDT
Reverted r265603 for reason:

Reverting per commiter because commit caused consisent crash with test.

Committed r265782: <https://trac.webkit.org/changeset/265782>
Comment 10 Brady Eidson 2020-08-20 12:55:23 PDT
Created attachment 406957 [details]
Patch
Comment 11 Darin Adler 2020-08-20 17:11:50 PDT
Comment on attachment 406957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406957&action=review

> Source/WebCore/loader/DocumentLoader.cpp:318
> +        document->fontSelector().suspendFontLoadingTimer();

Confused a bit by this call. Why does this "suspend" loading? Is it valuable to put it in a state where it can be resumed later?
Comment 12 Brady Eidson 2020-08-20 17:23:38 PDT
(In reply to Darin Adler from comment #11)
> Comment on attachment 406957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406957&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:318
> > +        document->fontSelector().suspendFontLoadingTimer();
> 
> Confused a bit by this call. Why does this "suspend" loading? Is it valuable
> to put it in a state where it can be resumed later?

This is the whole key of the fix, actually. It was in Myle's first pass, and is the same here.

It is *required* to put it in a state where it can be resumed later, otherwise the fonts will never load.
Comment 13 Brady Eidson 2020-08-20 17:27:12 PDT
(In reply to Darin Adler from comment #11)
> Comment on attachment 406957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406957&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:318
> > +        document->fontSelector().suspendFontLoadingTimer();
> 
> Confused a bit by this call. Why does this "suspend" loading? 

Also, to be clear, it suspends a timer, not actual loading.
Comment 14 Darin Adler 2020-08-20 17:46:34 PDT
Comment on attachment 406957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406957&action=review

>>>> Source/WebCore/loader/DocumentLoader.cpp:318
>>>> +        document->fontSelector().suspendFontLoadingTimer();
>>> 
>>> Confused a bit by this call. Why does this "suspend" loading? Is it valuable to put it in a state where it can be resumed later?
>> 
>> This is the whole key of the fix, actually. It was in Myle's first pass, and is the same here.
>> 
>> It is *required* to put it in a state where it can be resumed later, otherwise the fonts will never load.
> 
> Also, to be clear, it suspends a timer, not actual loading.

Oh, I didn’t realize Myles original fix was rolled out. I expect to see a change, not new code.
Comment 15 Brady Eidson 2020-08-20 18:36:53 PDT
(In reply to Darin Adler from comment #14)
> Comment on attachment 406957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406957&action=review
> 
> >>>> Source/WebCore/loader/DocumentLoader.cpp:318
> >>>> +        document->fontSelector().suspendFontLoadingTimer();
> >>> 
> >>> Confused a bit by this call. Why does this "suspend" loading? Is it valuable to put it in a state where it can be resumed later?
> >> 
> >> This is the whole key of the fix, actually. It was in Myle's first pass, and is the same here.
> >> 
> >> It is *required* to put it in a state where it can be resumed later, otherwise the fonts will never load.
> > 
> > Also, to be clear, it suspends a timer, not actual loading.
> 
> Oh, I didn’t realize Myles original fix was rolled out. I expect to see a
> change, not new code.

I'm not sure what you mean by this.
Comment 16 Brady Eidson 2020-08-21 08:50:24 PDT
The api-ios failure is not this patch.
Comment 17 Darin Adler 2020-08-21 09:32:19 PDT
I guess Myles should review?
Comment 18 Brady Eidson 2020-08-21 10:04:09 PDT
I had asked him to, and will remind him that he said he would :)
Comment 19 Myles C. Maxfield 2020-08-25 12:47:50 PDT
Looking at this now.
Comment 20 Myles C. Maxfield 2020-08-25 14:08:20 PDT
Verified that this patch does fix the bug.
Comment 21 Myles C. Maxfield 2020-08-25 14:16:40 PDT
Comment on attachment 406957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406957&action=review

I think this is good. Just a few more checks to run on my local machine.

> Source/WebCore/css/CSSFontSelector.cpp:374
> +    suspend(ReasonForSuspension::PageWillBeSuspended);

Do you actually know this to be true? (Where "this" = "the reason for the suspension is that the page will be suspended")

> Source/WebCore/css/CSSFontSelector.cpp:452
> +void CSSFontSelector::suspend(ReasonForSuspension)
> +{
> +    if (m_fontLoadingTimerIsSuspended) {
> +        ASSERT(!m_fontLoadingTimer.isActive());
> +        return;
> +    }
> +
> +    m_fontLoadingTimerIsSuspended = m_fontLoadingTimer.isActive();
> +    m_fontLoadingTimer.stop();
> +}
> +
> +void CSSFontSelector::resume()
> +{
> +    if (!m_fontLoadingTimerIsSuspended)
> +        return;
> +
> +    if (!m_fontsToBeginLoading.isEmpty())
> +        m_fontLoadingTimer.startOneShot(0_s);
> +
> +    m_fontLoadingTimerIsSuspended = false;
> +}

Right, this makes sense.

> Source/WebCore/css/CSSFontSelector.h:104
> +    void stop() final;

Nit: CSSFontSelector does lots of font selection related stuff, so CSSFontSelector::stop() is kind of a misleading name. It stops the loading timer; it doesn't stop selecting fonts.

I realize this is an overridden method so it has to be named that way, and making CSSFontSelector own an ActiveDOMObject (and forward all the calls) rather than be an ActiveDOMObject is probably not worth the benefit. But 😕
Comment 22 Myles C. Maxfield 2020-08-25 14:27:31 PDT
Comment on attachment 406957 [details]
Patch

Verified the bug reproduces without this patch.
Comment 23 Brady Eidson 2020-08-25 15:05:15 PDT
(In reply to Myles C. Maxfield from comment #21)
> Comment on attachment 406957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406957&action=review
> 
> I think this is good. Just a few more checks to run on my local machine.
> 
> > Source/WebCore/css/CSSFontSelector.cpp:374
> > +    suspend(ReasonForSuspension::PageWillBeSuspended);
> 
> Do you actually know this to be true? (Where "this" = "the reason for the
> suspension is that the page will be suspended")

Reason is almost always ignored (and is in this case)

"WillBeSuspended" is a catchall.

> 
> > Source/WebCore/css/CSSFontSelector.cpp:452
> > +void CSSFontSelector::suspend(ReasonForSuspension)
> > +{
> > +    if (m_fontLoadingTimerIsSuspended) {
> > +        ASSERT(!m_fontLoadingTimer.isActive());
> > +        return;
> > +    }
> > +
> > +    m_fontLoadingTimerIsSuspended = m_fontLoadingTimer.isActive();
> > +    m_fontLoadingTimer.stop();
> > +}
> > +
> > +void CSSFontSelector::resume()
> > +{
> > +    if (!m_fontLoadingTimerIsSuspended)
> > +        return;
> > +
> > +    if (!m_fontsToBeginLoading.isEmpty())
> > +        m_fontLoadingTimer.startOneShot(0_s);
> > +
> > +    m_fontLoadingTimerIsSuspended = false;
> > +}
> 
> Right, this makes sense.
> 
> > Source/WebCore/css/CSSFontSelector.h:104
> > +    void stop() final;
> 
> Nit: CSSFontSelector does lots of font selection related stuff, so
> CSSFontSelector::stop() is kind of a misleading name. It stops the loading
> timer; it doesn't stop selecting fonts.
> 
> I realize this is an overridden method so it has to be named that way, and
> making CSSFontSelector own an ActiveDOMObject (and forward all the calls)
> rather than be an ActiveDOMObject is probably not worth the benefit. But 😕

Well, between when "stop" and "Resume" is called, it had BETTER not be doing the other stuff you say it might ;)
Comment 24 EWS 2020-08-25 15:08:56 PDT
Committed r266148: <https://trac.webkit.org/changeset/266148>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406957 [details].
Comment 25 Chris Dumez 2020-09-10 13:32:53 PDT
Comment on attachment 406957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406957&action=review

> Source/WebCore/css/CSSFontSelector.cpp:77
> +    suspendIfNeeded();

It is unsafe to call suspendIfNeeded() inside the constructor. This call should be moved CSSFontSelector::create() to avoid potentially hitting assertions in debug.
The reason it is unsafe is that suspendIfNeeded() may call suspend() which may ref |this|. If you ref |this| here, we hit the adoption requirement assertion in RefCounted.