Bug 215435 – Font loads quickly followed by navigations may fail indefinitely

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Myles C. Maxfield]

Font loads quickly followed by navigations may fail indefinitely

[Myles C. Maxfield]

Created attachment 406487 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Myles C. Maxfield]

<rdar://problem/65560550>

[Myles C. Maxfield]

(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.)

[Myles C. Maxfield]

Created attachment 406497 [details]
Patch for committing

[Myles C. Maxfield]

Committed r265603: <https://trac.webkit.org/changeset/265603>

[Radar WebKit Bug Importer]

<rdar://problem/66970271>

[Karl Rackler]

Reverted r265603 for reason:

Reverting per commiter because commit caused consisent crash with test.

Committed r265782: <https://trac.webkit.org/changeset/265782>

[Brady Eidson]

Created attachment 406957 [details]
Patch

[Darin Adler]

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?

[Brady Eidson]

(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.

[Brady Eidson]

(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.

[Darin Adler]

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.

[Brady Eidson]

(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.

[Brady Eidson]

The api-ios failure is not this patch.

[Darin Adler]

I guess Myles should review?

[Brady Eidson]

I had asked him to, and will remind him that he said he would :)

[Myles C. Maxfield]

Looking at this now.

[Myles C. Maxfield]

Verified that this patch does fix the bug.

[Myles C. Maxfield]

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 😕

[Myles C. Maxfield]

Comment on attachment 406957 [details]
Patch

Verified the bug reproduces without this patch.

[Brady Eidson]

(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 ;)

[EWS]

Committed r266148: <https://trac.webkit.org/changeset/266148>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406957 [details].

[Chris Dumez]

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.