⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 267626 in webkit


Ignore:
Timestamp:
Sep 26, 2020, 2:08:15 PM (6 years ago)
Author:
Alan Coon
Message:

Cherry-pick r266148. rdar://problem/69583151

Font loads quickly followed by navigations may fail indefinitely
<rdar://problem/65560550> and ​https://bugs.webkit.org/show_bug.cgi?id=215435

Reviewed by Myles C. Maxfield.

Source/WebCore:

Second take at this.

Myles took the first swipe at this, but a conflict with SuspendableTimer caused issues
in the form of layout test asserts with
http/tests/security/navigate-when-restoring-cached-page.html

His original ChangeLog entry:

Font loads are coalesced using a zero-delay timer. However, that zero-delay timer
can fire while the page is in the middle of a navigation, which will cause the font
loads to fail. Then, the second page can request those same fonts, which are marked
as failed, and as such will never actually load/use the desired web font.

This patch just stops the zero-delay timer during navigations, and resumes it
when resuming the document. This means:

  1. The second page in the above story will not see that the font has failed, or even started, and will then re-request the font and load it successfully
  2. If the user goes "back" to the previous page, the zero-delay timer is restarted, the CachedFont realizes it's already succeeded, and the previous page is rendered as expected.

Test: fast/loader/font-load-timer.html

---

Now the explanation of the failure it caused:
The font loading timer was a SuspendableTimer, which is an ActiveDOMObject.

An ActiveDOMObject was used to make sure the delayed font loads play well with the
page cache, which is still necessary.

But we also still need to suspend the timer manually when "stopLoading()" is called,
which doesn't play well with ActiveDOMObject's automatic suspend/resume.

My solution:

  • Make the timer "just a normal timer"
  • Make CSSFontSelector itself the ActiveDOMObject
  • Let DocumentLoader explicitly pause the font load timer
  • Rely on ActiveDOMObject to resume the timer

These keep the bug fixed and resolve the layout test ASSERT seen with
http/tests/security/navigate-when-restoring-cached-page.html

  • css/CSSFontSelector.cpp: (WebCore::CSSFontSelector::CSSFontSelector): (WebCore::CSSFontSelector::clearDocument): (WebCore::CSSFontSelector::beginLoadingFontSoon): (WebCore::CSSFontSelector::suspendFontLoadingTimer): (WebCore::CSSFontSelector::fontLoadingTimerFired): (WebCore::CSSFontSelector::stop): (WebCore::CSSFontSelector::suspend): (WebCore::CSSFontSelector::resume): (WebCore::CSSFontSelector::beginLoadTimerFired): Deleted.
  • css/CSSFontSelector.h:
  • loader/DocumentLoader.cpp: (WebCore::DocumentLoader::stopLoading):

LayoutTests:

1) The page has some content that has “font-family: WebFont” but there are no @font-face blocks on the page
2) In script, after the page has loaded, add an @font-face rule to the page with “font-family: WebFont” and some valid font URL
3) Synchronously, within the same turn of the run loop, trigger a synchronous layout of the element (using offsetWidth or something). This will add the font to the 0-delay time work list.
4) Synchronously, within the same turn of the run loop, navigate to a second page that doesn’t use the web font.
5) The second page waits some small-but-positive amount of time. This will cause the 0-delay timer to fire, but because the page is in the middle of navigating, the font load should fail.
6) The second page adds the same @font-face rule to itself using script. This should pull the same (failed) CachedResource object out of the memory cache.
7) Use the CSS Font Loading API to wait for the font load to complete
8) Make sure that the font is used on the second page (as a reference test). Today, the second page’s font load will fail because it pulled the failed font out of the memory cache. The test makes sure the second page’s font load succeeds.

  • fast/loader/font-load-timer-expected.html: Added.
  • fast/loader/font-load-timer.html: Added.
  • fast/loader/resources/font-load-timer-navigation-destination.html: Added.

git-svn-id: ​https://svn.webkit.org/repository/webkit/trunk@266148 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Location:
branches/safari-610-branch
Files:
3 added
5 edited

Legend:

Unmodified
Added
Removed

  • branches/safari-610-branch/LayoutTests/ChangeLog

    r267583 r267626  
     12020-09-25  Alan Coon  <alancoon@apple.com>
     2
     3        Cherry-pick r266148. rdar://problem/69583151
     4
     5    Font loads quickly followed by navigations may fail indefinitely
     6    <rdar://problem/65560550> and https://bugs.webkit.org/show_bug.cgi?id=215435
     7   
     8    Reviewed by Myles C. Maxfield.
     9   
     10    Source/WebCore:
     11   
     12    Second take at this.
     13   
     14    Myles took the first swipe at this, but a conflict with SuspendableTimer caused issues
     15    in the form of layout test asserts with
     16    http/tests/security/navigate-when-restoring-cached-page.html
     17   
     18    His original ChangeLog entry:
     19   
     20    Font loads are coalesced using a zero-delay timer. However, that zero-delay timer
     21    can fire while the page is in the middle of a navigation, which will cause the font
     22    loads to fail. Then, the second page can request those same fonts, which are marked
     23    as failed, and as such will never actually load/use the desired web font.
     24   
     25    This patch just stops the zero-delay timer during navigations, and resumes it
     26    when resuming the document. This means:
     27   
     28    1. The second page in the above story will not see that the font has failed, or
     29    even started, and will then re-request the font and load it successfully
     30    2. If the user goes "back" to the previous page, the zero-delay timer is restarted,
     31    the CachedFont realizes it's already succeeded, and the previous page is rendered
     32    as expected.
     33   
     34    Test: fast/loader/font-load-timer.html
     35   
     36    ---
     37   
     38    Now the explanation of the failure it caused:
     39    The font loading timer was a SuspendableTimer, which is an ActiveDOMObject.
     40   
     41    An ActiveDOMObject was used to make sure the delayed font loads play well with the
     42    page cache, which is still necessary.
     43   
     44    But we also still need to suspend the timer manually when "stopLoading()" is called,
     45    which doesn't play well with ActiveDOMObject's automatic suspend/resume.
     46   
     47    My solution:
     48    - Make the timer "just a normal timer"
     49    - Make CSSFontSelector itself the ActiveDOMObject
     50    - Let DocumentLoader explicitly pause the font load timer
     51    - Rely on ActiveDOMObject to resume the timer
     52   
     53    These keep the bug fixed and resolve the layout test ASSERT seen with
     54    http/tests/security/navigate-when-restoring-cached-page.html
     55   
     56    * css/CSSFontSelector.cpp:
     57    (WebCore::CSSFontSelector::CSSFontSelector):
     58    (WebCore::CSSFontSelector::clearDocument):
     59    (WebCore::CSSFontSelector::beginLoadingFontSoon):
     60    (WebCore::CSSFontSelector::suspendFontLoadingTimer):
     61    (WebCore::CSSFontSelector::fontLoadingTimerFired):
     62    (WebCore::CSSFontSelector::stop):
     63    (WebCore::CSSFontSelector::suspend):
     64    (WebCore::CSSFontSelector::resume):
     65    (WebCore::CSSFontSelector::beginLoadTimerFired): Deleted.
     66    * css/CSSFontSelector.h:
     67    * loader/DocumentLoader.cpp:
     68    (WebCore::DocumentLoader::stopLoading):
     69   
     70    LayoutTests:
     71   
     72    1) The page has some content that has “font-family: WebFont” but there are no @font-face blocks on the page
     73    2) In script, after the page has loaded, add an @font-face rule to the page with “font-family: WebFont” and some valid font URL
     74    3) Synchronously, within the same turn of the run loop, trigger a synchronous layout of the element (using offsetWidth or something). This will add the font to the 0-delay time work list.
     75    4) Synchronously, within the same turn of the run loop, navigate to a second page that doesn’t use the web font.
     76    5) The second page waits some small-but-positive amount of time. This will cause the 0-delay timer to fire, but because the page is in the middle of navigating, the font load should fail.
     77    6) The second page adds the same @font-face rule to itself using script. This should pull the same (failed) CachedResource object out of the memory cache.
     78    7) Use the CSS Font Loading API to wait for the font load to complete
     79    8) Make sure that the font is used on the second page (as a reference test). Today, the second page’s font load will fail because it pulled the failed font out of the memory cache. The test makes sure the second page’s font load succeeds.
     80   
     81    * fast/loader/font-load-timer-expected.html: Added.
     82    * fast/loader/font-load-timer.html: Added.
     83    * fast/loader/resources/font-load-timer-navigation-destination.html: Added.
     84   
     85   
     86    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266148 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     87
     88    2020-08-25  Brady Eidson  <beidson@apple.com>
     89
     90            Font loads quickly followed by navigations may fail indefinitely
     91            <rdar://problem/65560550> and https://bugs.webkit.org/show_bug.cgi?id=215435
     92
     93            Reviewed by Myles C. Maxfield.
     94
     95            1) The page has some content that has “font-family: WebFont” but there are no @font-face blocks on the page
     96            2) In script, after the page has loaded, add an @font-face rule to the page with “font-family: WebFont” and some valid font URL
     97            3) Synchronously, within the same turn of the run loop, trigger a synchronous layout of the element (using offsetWidth or something). This will add the font to the 0-delay time work list.
     98            4) Synchronously, within the same turn of the run loop, navigate to a second page that doesn’t use the web font.
     99            5) The second page waits some small-but-positive amount of time. This will cause the 0-delay timer to fire, but because the page is in the middle of navigating, the font load should fail.
     100            6) The second page adds the same @font-face rule to itself using script. This should pull the same (failed) CachedResource object out of the memory cache.
     101            7) Use the CSS Font Loading API to wait for the font load to complete
     102            8) Make sure that the font is used on the second page (as a reference test). Today, the second page’s font load will fail because it pulled the failed font out of the memory cache. The test makes sure the second page’s font load succeeds.
     103
     104            * fast/loader/font-load-timer-expected.html: Added.
     105            * fast/loader/font-load-timer.html: Added.
     106            * fast/loader/resources/font-load-timer-navigation-destination.html: Added.
     107
    11082020-09-22  Alan Coon  <alancoon@apple.com>
    2109

  • branches/safari-610-branch/Source/WebCore/ChangeLog

    r267584 r267626  
     12020-09-25  Alan Coon  <alancoon@apple.com>
     2
     3        Cherry-pick r266148. rdar://problem/69583151
     4
     5    Font loads quickly followed by navigations may fail indefinitely
     6    <rdar://problem/65560550> and https://bugs.webkit.org/show_bug.cgi?id=215435
     7   
     8    Reviewed by Myles C. Maxfield.
     9   
     10    Source/WebCore:
     11   
     12    Second take at this.
     13   
     14    Myles took the first swipe at this, but a conflict with SuspendableTimer caused issues
     15    in the form of layout test asserts with
     16    http/tests/security/navigate-when-restoring-cached-page.html
     17   
     18    His original ChangeLog entry:
     19   
     20    Font loads are coalesced using a zero-delay timer. However, that zero-delay timer
     21    can fire while the page is in the middle of a navigation, which will cause the font
     22    loads to fail. Then, the second page can request those same fonts, which are marked
     23    as failed, and as such will never actually load/use the desired web font.
     24   
     25    This patch just stops the zero-delay timer during navigations, and resumes it
     26    when resuming the document. This means:
     27   
     28    1. The second page in the above story will not see that the font has failed, or
     29    even started, and will then re-request the font and load it successfully
     30    2. If the user goes "back" to the previous page, the zero-delay timer is restarted,
     31    the CachedFont realizes it's already succeeded, and the previous page is rendered
     32    as expected.
     33   
     34    Test: fast/loader/font-load-timer.html
     35   
     36    ---
     37   
     38    Now the explanation of the failure it caused:
     39    The font loading timer was a SuspendableTimer, which is an ActiveDOMObject.
     40   
     41    An ActiveDOMObject was used to make sure the delayed font loads play well with the
     42    page cache, which is still necessary.
     43   
     44    But we also still need to suspend the timer manually when "stopLoading()" is called,
     45    which doesn't play well with ActiveDOMObject's automatic suspend/resume.
     46   
     47    My solution:
     48    - Make the timer "just a normal timer"
     49    - Make CSSFontSelector itself the ActiveDOMObject
     50    - Let DocumentLoader explicitly pause the font load timer
     51    - Rely on ActiveDOMObject to resume the timer
     52   
     53    These keep the bug fixed and resolve the layout test ASSERT seen with
     54    http/tests/security/navigate-when-restoring-cached-page.html
     55   
     56    * css/CSSFontSelector.cpp:
     57    (WebCore::CSSFontSelector::CSSFontSelector):
     58    (WebCore::CSSFontSelector::clearDocument):
     59    (WebCore::CSSFontSelector::beginLoadingFontSoon):
     60    (WebCore::CSSFontSelector::suspendFontLoadingTimer):
     61    (WebCore::CSSFontSelector::fontLoadingTimerFired):
     62    (WebCore::CSSFontSelector::stop):
     63    (WebCore::CSSFontSelector::suspend):
     64    (WebCore::CSSFontSelector::resume):
     65    (WebCore::CSSFontSelector::beginLoadTimerFired): Deleted.
     66    * css/CSSFontSelector.h:
     67    * loader/DocumentLoader.cpp:
     68    (WebCore::DocumentLoader::stopLoading):
     69   
     70    LayoutTests:
     71   
     72    1) The page has some content that has “font-family: WebFont” but there are no @font-face blocks on the page
     73    2) In script, after the page has loaded, add an @font-face rule to the page with “font-family: WebFont” and some valid font URL
     74    3) Synchronously, within the same turn of the run loop, trigger a synchronous layout of the element (using offsetWidth or something). This will add the font to the 0-delay time work list.
     75    4) Synchronously, within the same turn of the run loop, navigate to a second page that doesn’t use the web font.
     76    5) The second page waits some small-but-positive amount of time. This will cause the 0-delay timer to fire, but because the page is in the middle of navigating, the font load should fail.
     77    6) The second page adds the same @font-face rule to itself using script. This should pull the same (failed) CachedResource object out of the memory cache.
     78    7) Use the CSS Font Loading API to wait for the font load to complete
     79    8) Make sure that the font is used on the second page (as a reference test). Today, the second page’s font load will fail because it pulled the failed font out of the memory cache. The test makes sure the second page’s font load succeeds.
     80   
     81    * fast/loader/font-load-timer-expected.html: Added.
     82    * fast/loader/font-load-timer.html: Added.
     83    * fast/loader/resources/font-load-timer-navigation-destination.html: Added.
     84   
     85   
     86    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266148 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     87
     88    2020-08-25  Brady Eidson  <beidson@apple.com>
     89
     90            Font loads quickly followed by navigations may fail indefinitely
     91            <rdar://problem/65560550> and https://bugs.webkit.org/show_bug.cgi?id=215435
     92
     93            Reviewed by Myles C. Maxfield.
     94
     95            Second take at this.
     96
     97            Myles took the first swipe at this, but a conflict with SuspendableTimer caused issues
     98            in the form of layout test asserts with
     99            http/tests/security/navigate-when-restoring-cached-page.html
     100
     101            His original ChangeLog entry:
     102
     103            Font loads are coalesced using a zero-delay timer. However, that zero-delay timer
     104            can fire while the page is in the middle of a navigation, which will cause the font
     105            loads to fail. Then, the second page can request those same fonts, which are marked
     106            as failed, and as such will never actually load/use the desired web font.
     107
     108            This patch just stops the zero-delay timer during navigations, and resumes it
     109            when resuming the document. This means:
     110
     111            1. The second page in the above story will not see that the font has failed, or
     112            even started, and will then re-request the font and load it successfully
     113            2. If the user goes "back" to the previous page, the zero-delay timer is restarted,
     114            the CachedFont realizes it's already succeeded, and the previous page is rendered
     115            as expected.
     116
     117            Test: fast/loader/font-load-timer.html
     118
     119            ---
     120
     121            Now the explanation of the failure it caused:
     122            The font loading timer was a SuspendableTimer, which is an ActiveDOMObject.
     123
     124            An ActiveDOMObject was used to make sure the delayed font loads play well with the
     125            page cache, which is still necessary.
     126
     127            But we also still need to suspend the timer manually when "stopLoading()" is called,
     128            which doesn't play well with ActiveDOMObject's automatic suspend/resume.
     129
     130            My solution:
     131            - Make the timer "just a normal timer"
     132            - Make CSSFontSelector itself the ActiveDOMObject
     133            - Let DocumentLoader explicitly pause the font load timer
     134            - Rely on ActiveDOMObject to resume the timer
     135
     136            These keep the bug fixed and resolve the layout test ASSERT seen with
     137            http/tests/security/navigate-when-restoring-cached-page.html
     138
     139            * css/CSSFontSelector.cpp:
     140            (WebCore::CSSFontSelector::CSSFontSelector):
     141            (WebCore::CSSFontSelector::clearDocument):
     142            (WebCore::CSSFontSelector::beginLoadingFontSoon):
     143            (WebCore::CSSFontSelector::suspendFontLoadingTimer):
     144            (WebCore::CSSFontSelector::fontLoadingTimerFired):
     145            (WebCore::CSSFontSelector::stop):
     146            (WebCore::CSSFontSelector::suspend):
     147            (WebCore::CSSFontSelector::resume):
     148            (WebCore::CSSFontSelector::beginLoadTimerFired): Deleted.
     149            * css/CSSFontSelector.h:
     150            * loader/DocumentLoader.cpp:
     151            (WebCore::DocumentLoader::stopLoading):
     152
    11532020-09-22  Alan Coon  <alancoon@apple.com>
    2154

  • branches/safari-610-branch/Source/WebCore/css/CSSFontSelector.cpp

    r265782 r267626  
    6363
    6464CSSFontSelector::CSSFontSelector(Document& document)
    65     : m_document(makeWeakPtr(document))
     65    : ActiveDOMObject(document)
     66    , m_document(makeWeakPtr(document))
    6667    , m_cssFontFaceSet(CSSFontFaceSet::create(this))
    67     , m_beginLoadingTimer(&document, *this, &CSSFontSelector::beginLoadTimerFired)
     68    , m_fontLoadingTimer(*this, &CSSFontSelector::fontLoadingTimerFired)
    6869    , m_uniqueId(++fontSelectorId)
    6970    , m_version(0)
    … …  
    7475    LOG(Fonts, "CSSFontSelector %p ctor", this);
    7576
    76     m_beginLoadingTimer.suspendIfNeeded();
     77    suspendIfNeeded();
    7778}
    7879
    … …  
    334335{
    335336    if (!m_document) {
    336         ASSERT(!m_beginLoadingTimer.isActive());
     337        ASSERT(!m_fontLoadingTimer.isActive());
    337338        ASSERT(m_fontsToBeginLoading.isEmpty());
    338339        return;
    339340    }
    340341
    341     m_beginLoadingTimer.cancel();
     342    m_fontLoadingTimer.stop();
    342343
    343344    CachedResourceLoader& cachedResourceLoader = m_document->cachedResourceLoader();
    … …  
    362363    // Increment the request count now, in order to prevent didFinishLoad from being dispatched
    363364    // after this font has been requested but before it began loading. Balanced by
    364     // decrementRequestCount() in beginLoadTimerFired() and in clearDocument().
     365    // decrementRequestCount() in fontLoadingTimerFired() and in clearDocument().
    365366    m_document->cachedResourceLoader().incrementRequestCount(font);
    366367
    367     m_beginLoadingTimer.startOneShot(0_s);
    368 }
    369 
    370 void CSSFontSelector::beginLoadTimerFired()
     368    if (!m_fontLoadingTimerIsSuspended)
     369        m_fontLoadingTimer.startOneShot(0_s);
     370}
     371
     372void CSSFontSelector::suspendFontLoadingTimer()
     373{
     374    suspend(ReasonForSuspension::PageWillBeSuspended);
     375}
     376
     377void CSSFontSelector::fontLoadingTimerFired()
    371378{
    372379    Vector<CachedResourceHandle<CachedFont>> fontsToBeginLoading;
    … …  
    418425}
    419426
    420 }
     427void CSSFontSelector::stop()
     428{
     429    m_fontLoadingTimer.stop();
     430}
     431
     432void CSSFontSelector::suspend(ReasonForSuspension)
     433{
     434    if (m_fontLoadingTimerIsSuspended) {
     435        ASSERT(!m_fontLoadingTimer.isActive());
     436        return;
     437    }
     438
     439    m_fontLoadingTimerIsSuspended = m_fontLoadingTimer.isActive();
     440    m_fontLoadingTimer.stop();
     441}
     442
     443void CSSFontSelector::resume()
     444{
     445    if (!m_fontLoadingTimerIsSuspended)
     446        return;
     447
     448    if (!m_fontsToBeginLoading.isEmpty())
     449        m_fontLoadingTimer.startOneShot(0_s);
     450
     451    m_fontLoadingTimerIsSuspended = false;
     452}
     453
     454}

  • branches/safari-610-branch/Source/WebCore/css/CSSFontSelector.h

    r265782 r267626  
    2626#pragma once
    2727
     28#include "ActiveDOMObject.h"
    2829#include "CSSFontFace.h"
    2930#include "CSSFontFaceSet.h"
    … …  
    3132#include "Font.h"
    3233#include "FontSelector.h"
    33 #include "SuspendableTimer.h"
     34#include "Timer.h"
    3435#include <memory>
    3536#include <wtf/Forward.h>
    … …  
    4849class StyleRuleFontFace;
    4950
    50 class CSSFontSelector final : public FontSelector, public CSSFontFaceSetClient, public CanMakeWeakPtr<CSSFontSelector> {
     51class CSSFontSelector final : public FontSelector, public CSSFontFaceSetClient, public CanMakeWeakPtr<CSSFontSelector>, public ActiveDOMObject {
    5152public:
    5253    static Ref<CSSFontSelector> create(Document& document)
    … …  
    8182
    8283    void beginLoadingFontSoon(CachedFont&);
     84    void suspendFontLoadingTimer();
    8385
    8486    FontFaceSet* fontFaceSetIfExists();
    … …  
    9799    void fontModified() final;
    98100
    99     void beginLoadTimerFired();
     101    void fontLoadingTimerFired();
     102
     103    // ActiveDOMObject
     104    void stop() final;
     105    void suspend(ReasonForSuspension) final;
     106    void resume() final;
     107    const char* activeDOMObjectName() const final { return "CSSFontSelector"_s; }
    100108
    101109    struct PendingFontFaceRule {
    … …  
    113121    HashSet<RefPtr<CSSFontFace>> m_cssConnectionsPossiblyToRemove;
    114122    HashSet<RefPtr<StyleRuleFontFace>> m_cssConnectionsEncounteredDuringBuild;
    115     SuspendableTimer m_beginLoadingTimer;
     123
     124    Timer m_fontLoadingTimer;
     125    bool m_fontLoadingTimerIsSuspended { false };
    116126
    117127    unsigned m_uniqueId;

  • branches/safari-610-branch/Source/WebCore/loader/DocumentLoader.cpp

    r265782 r267626  
    3434#include "Archive.h"
    3535#include "ArchiveResourceCollection.h"
     36#include "CSSFontSelector.h"
    3637#include "CachedPage.h"
    3738#include "CachedRawResource.h"
    … …  
    314315    cancelAll(m_multipartSubresourceLoaders);
    315316
     317    if (auto* document = m_frame->document())
     318        document->fontSelector().suspendFontLoadingTimer();
     319
    316320    // Appcache uses ResourceHandle directly, DocumentLoader doesn't count these loads.
    317321    m_applicationCacheHost->stopLoadingInFrame(*m_frame);
Note: See TracChangeset for help on using the changeset viewer.