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

Changeset 286723 in webkit


Ignore:
Timestamp:
Dec 8, 2021, 1:25:57 PM (5 years ago)
Author:
Alan Coon
Message:

Cherry-pick r286481. rdar://problem/85928816

A Safari tab can rarely get stuck in a state where rendering updates stop happening
​https://bugs.webkit.org/show_bug.cgi?id=233784
rdar://85445072

Reviewed by Chris Dumez.

Sometimes a Safari tab can get into a state where rendering updates cease to happen,
which manifests as partially broken scrolling, blank tiles revealed when scrolling,
and somewhat broken page updates. I was able to sometimes reproduce this by clicking
on links in eBay emails from Mail on a system with two displays.

From the one time I reproduce with logging, the output indicated that DisplayRefreshMonitor::displayLinkFired()
would early return because isPreviousFrameDone() was false. The only way for that to occur,
barring memory corruption, is if DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() returned early,
which it does if the callback comes twice in a single event loop; this may explain the rarity.

So fix DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() call setIsPreviousFrameDone(true)
so the next callback can make progress

Also add some locking annotations and fix one missing lock, and some release logging.

Source/WebCore:

  • platform/graphics/DisplayRefreshMonitor.cpp: (WebCore::DisplayRefreshMonitor::stop): (WebCore::DisplayRefreshMonitor::firedAndReachedMaxUnscheduledFireCount): (WebCore::DisplayRefreshMonitor::displayLinkFired):
  • platform/graphics/DisplayRefreshMonitor.h: (WebCore::DisplayRefreshMonitor::WTF_REQUIRES_LOCK): (WebCore::DisplayRefreshMonitor::WTF_GUARDED_BY_LOCK): (WebCore::DisplayRefreshMonitor::setMaxUnscheduledFireCount): Deleted. (WebCore::DisplayRefreshMonitor::isScheduled const): Deleted. (WebCore::DisplayRefreshMonitor::setIsScheduled): Deleted. (WebCore::DisplayRefreshMonitor::isPreviousFrameDone const): Deleted. (WebCore::DisplayRefreshMonitor::setIsPreviousFrameDone): Deleted.

Source/WebKit:

  • WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp: (WebKit::DisplayRefreshMonitorMac::dispatchDisplayDidRefresh):

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

Location:
branches/safari-612.4.2.1-branch/Source
Files:
6 edited

Legend:

Unmodified
Added
Removed

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

    r286720 r286723  
     12021-12-03  Russell Epstein  <repstein@apple.com>
     2
     3        Cherry-pick r286481. rdar://problem/85928816
     4
     5    A Safari tab can rarely get stuck in a state where rendering updates stop happening
     6    https://bugs.webkit.org/show_bug.cgi?id=233784
     7    rdar://85445072
     8   
     9    Reviewed by Chris Dumez.
     10   
     11    Sometimes a Safari tab can get into a state where rendering updates cease to happen,
     12    which manifests as partially broken scrolling, blank tiles revealed when scrolling,
     13    and somewhat broken page updates. I was able to sometimes reproduce this by clicking
     14    on links in eBay emails from Mail on a system with two displays.
     15   
     16    From the one time I reproduce with logging, the output indicated that DisplayRefreshMonitor::displayLinkFired()
     17    would early return because isPreviousFrameDone() was false. The only way for that to occur,
     18    barring memory corruption, is if DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() returned early,
     19    which it does if the callback comes twice in a single event loop; this may explain the rarity.
     20   
     21    So fix DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() call setIsPreviousFrameDone(true)
     22    so the next callback can make progress
     23   
     24    Also add some locking annotations and fix one missing lock, and some release logging.
     25   
     26    Source/WebCore:
     27   
     28    * platform/graphics/DisplayRefreshMonitor.cpp:
     29    (WebCore::DisplayRefreshMonitor::stop):
     30    (WebCore::DisplayRefreshMonitor::firedAndReachedMaxUnscheduledFireCount):
     31    (WebCore::DisplayRefreshMonitor::displayLinkFired):
     32    * platform/graphics/DisplayRefreshMonitor.h:
     33    (WebCore::DisplayRefreshMonitor::WTF_REQUIRES_LOCK):
     34    (WebCore::DisplayRefreshMonitor::WTF_GUARDED_BY_LOCK):
     35    (WebCore::DisplayRefreshMonitor::setMaxUnscheduledFireCount): Deleted.
     36    (WebCore::DisplayRefreshMonitor::isScheduled const): Deleted.
     37    (WebCore::DisplayRefreshMonitor::setIsScheduled): Deleted.
     38    (WebCore::DisplayRefreshMonitor::isPreviousFrameDone const): Deleted.
     39    (WebCore::DisplayRefreshMonitor::setIsPreviousFrameDone): Deleted.
     40   
     41    Source/WebKit:
     42   
     43    * WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp:
     44    (WebKit::DisplayRefreshMonitorMac::dispatchDisplayDidRefresh):
     45   
     46   
     47    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@286481 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     48
     49    2021-12-02  Simon Fraser  <simon.fraser@apple.com>
     50
     51            A Safari tab can rarely get stuck in a state where rendering updates stop happening
     52            https://bugs.webkit.org/show_bug.cgi?id=233784
     53            rdar://85445072
     54
     55            Reviewed by Chris Dumez.
     56
     57            Sometimes a Safari tab can get into a state where rendering updates cease to happen,
     58            which manifests as partially broken scrolling, blank tiles revealed when scrolling,
     59            and somewhat broken page updates. I was able to sometimes reproduce this by clicking
     60            on links in eBay emails from Mail on a system with two displays.
     61
     62            From the one time I reproduce with logging, the output indicated that DisplayRefreshMonitor::displayLinkFired()
     63            would early return because isPreviousFrameDone() was false. The only way for that to occur,
     64            barring memory corruption, is if DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() returned early,
     65            which it does if the callback comes twice in a single event loop; this may explain the rarity.
     66
     67            So fix DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() call setIsPreviousFrameDone(true)
     68            so the next callback can make progress
     69
     70            Also add some locking annotations and fix one missing lock, and some release logging.
     71
     72            * platform/graphics/DisplayRefreshMonitor.cpp:
     73            (WebCore::DisplayRefreshMonitor::stop):
     74            (WebCore::DisplayRefreshMonitor::firedAndReachedMaxUnscheduledFireCount):
     75            (WebCore::DisplayRefreshMonitor::displayLinkFired):
     76            * platform/graphics/DisplayRefreshMonitor.h:
     77            (WebCore::DisplayRefreshMonitor::WTF_REQUIRES_LOCK):
     78            (WebCore::DisplayRefreshMonitor::WTF_GUARDED_BY_LOCK):
     79            (WebCore::DisplayRefreshMonitor::setMaxUnscheduledFireCount): Deleted.
     80            (WebCore::DisplayRefreshMonitor::isScheduled const): Deleted.
     81            (WebCore::DisplayRefreshMonitor::setIsScheduled): Deleted.
     82            (WebCore::DisplayRefreshMonitor::isPreviousFrameDone const): Deleted.
     83            (WebCore::DisplayRefreshMonitor::setIsPreviousFrameDone): Deleted.
     84
    1852021-12-02  Russell Epstein  <repstein@apple.com>
    286

  • branches/safari-612.4.2.1-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp

    r278253 r286723  
    8686{
    8787    stopNotificationMechanism();
     88
     89    Locker locker { m_lock };
    8890    setIsScheduled(false);
    8991}
    … …  
    153155bool DisplayRefreshMonitor::firedAndReachedMaxUnscheduledFireCount()
    154156{
    155     ASSERT(m_lock.isLocked());
    156 
    157157    if (isScheduled()) {
    158158        m_unscheduledFireCount = 0;
    … …  
    170170
    171171        // This may be off the main thread.
    172         if (!isPreviousFrameDone())
     172        if (!isPreviousFrameDone()) {
     173            RELEASE_LOG(DisplayLink, "[Web] DisplayRefreshMonitor::displayLinkFired for display %u - previous frame is not complete", displayID());
    173174            return;
     175        }
    174176
    175177        LOG_WITH_STREAM(DisplayLink, stream << "[Web] DisplayRefreshMonitor::displayLinkFired for display " << displayID() << " - scheduled " << isScheduled() << " unscheduledFireCount " << m_unscheduledFireCount << " of " << m_maxUnscheduledFireCount);

  • branches/safari-612.4.2.1-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h

    r278253 r286723  
    7373    WEBCORE_EXPORT virtual void dispatchDisplayDidRefresh(const DisplayUpdate&);
    7474
    75     Lock& lock() { return m_lock; }
    76     void setMaxUnscheduledFireCount(unsigned count) { m_maxUnscheduledFireCount = count; }
     75    Lock& lock() WTF_RETURNS_LOCK(m_lock) { return m_lock; }
     76    void setMaxUnscheduledFireCount(unsigned count) WTF_REQUIRES_LOCK(m_lock) { m_maxUnscheduledFireCount = count; }
    7777
    7878    // Returns true if the start was successful.
    … …  
    8080    WEBCORE_EXPORT virtual void stopNotificationMechanism() = 0;
    8181
    82     bool isScheduled() const { return m_scheduled; }
    83     void setIsScheduled(bool scheduled) { m_scheduled = scheduled; }
     82    bool isScheduled() const WTF_REQUIRES_LOCK(m_lock) { return m_scheduled; }
     83    void setIsScheduled(bool scheduled) WTF_REQUIRES_LOCK(m_lock) { m_scheduled = scheduled; }
    8484
    85     bool isPreviousFrameDone() const { return m_previousFrameDone; }
    86     void setIsPreviousFrameDone(bool done) { m_previousFrameDone = done; }
     85    bool isPreviousFrameDone() const WTF_REQUIRES_LOCK(m_lock) { return m_previousFrameDone; }
     86    void setIsPreviousFrameDone(bool done) WTF_REQUIRES_LOCK(m_lock) { m_previousFrameDone = done; }
    8787
    8888    WEBCORE_EXPORT void displayDidRefresh(const DisplayUpdate&);
    8989
    9090private:
    91     bool firedAndReachedMaxUnscheduledFireCount();
     91    bool firedAndReachedMaxUnscheduledFireCount() WTF_REQUIRES_LOCK(m_lock);
    9292
    9393    virtual void adjustPreferredFramesPerSecond(FramesPerSecond) { }
    … …  
    103103
    104104    Lock m_lock;
    105     bool m_scheduled { false };
    106     bool m_previousFrameDone { true };
     105    bool m_scheduled WTF_GUARDED_BY_LOCK(m_lock) { false };
     106    bool m_previousFrameDone WTF_GUARDED_BY_LOCK(m_lock) { true };
    107107   
    108     unsigned m_unscheduledFireCount { 0 };
    109     unsigned m_maxUnscheduledFireCount { 0 };
     108    unsigned m_unscheduledFireCount WTF_GUARDED_BY_LOCK(m_lock) { 0 };
     109    unsigned m_maxUnscheduledFireCount WTF_GUARDED_BY_LOCK(m_lock) { 0 };
    110110};
    111111

  • branches/safari-612.4.2.1-branch/Source/WebKit/ChangeLog

    r286722 r286723  
     12021-12-03  Russell Epstein  <repstein@apple.com>
     2
     3        Cherry-pick r286481. rdar://problem/85928816
     4
     5    A Safari tab can rarely get stuck in a state where rendering updates stop happening
     6    https://bugs.webkit.org/show_bug.cgi?id=233784
     7    rdar://85445072
     8   
     9    Reviewed by Chris Dumez.
     10   
     11    Sometimes a Safari tab can get into a state where rendering updates cease to happen,
     12    which manifests as partially broken scrolling, blank tiles revealed when scrolling,
     13    and somewhat broken page updates. I was able to sometimes reproduce this by clicking
     14    on links in eBay emails from Mail on a system with two displays.
     15   
     16    From the one time I reproduce with logging, the output indicated that DisplayRefreshMonitor::displayLinkFired()
     17    would early return because isPreviousFrameDone() was false. The only way for that to occur,
     18    barring memory corruption, is if DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() returned early,
     19    which it does if the callback comes twice in a single event loop; this may explain the rarity.
     20   
     21    So fix DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() call setIsPreviousFrameDone(true)
     22    so the next callback can make progress
     23   
     24    Also add some locking annotations and fix one missing lock, and some release logging.
     25   
     26    Source/WebCore:
     27   
     28    * platform/graphics/DisplayRefreshMonitor.cpp:
     29    (WebCore::DisplayRefreshMonitor::stop):
     30    (WebCore::DisplayRefreshMonitor::firedAndReachedMaxUnscheduledFireCount):
     31    (WebCore::DisplayRefreshMonitor::displayLinkFired):
     32    * platform/graphics/DisplayRefreshMonitor.h:
     33    (WebCore::DisplayRefreshMonitor::WTF_REQUIRES_LOCK):
     34    (WebCore::DisplayRefreshMonitor::WTF_GUARDED_BY_LOCK):
     35    (WebCore::DisplayRefreshMonitor::setMaxUnscheduledFireCount): Deleted.
     36    (WebCore::DisplayRefreshMonitor::isScheduled const): Deleted.
     37    (WebCore::DisplayRefreshMonitor::setIsScheduled): Deleted.
     38    (WebCore::DisplayRefreshMonitor::isPreviousFrameDone const): Deleted.
     39    (WebCore::DisplayRefreshMonitor::setIsPreviousFrameDone): Deleted.
     40   
     41    Source/WebKit:
     42   
     43    * WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp:
     44    (WebKit::DisplayRefreshMonitorMac::dispatchDisplayDidRefresh):
     45   
     46   
     47    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@286481 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     48
     49    2021-12-02  Simon Fraser  <simon.fraser@apple.com>
     50
     51            A Safari tab can rarely get stuck in a state where rendering updates stop happening
     52            https://bugs.webkit.org/show_bug.cgi?id=233784
     53            rdar://85445072
     54
     55            Reviewed by Chris Dumez.
     56
     57            Sometimes a Safari tab can get into a state where rendering updates cease to happen,
     58            which manifests as partially broken scrolling, blank tiles revealed when scrolling,
     59            and somewhat broken page updates. I was able to sometimes reproduce this by clicking
     60            on links in eBay emails from Mail on a system with two displays.
     61
     62            From the one time I reproduce with logging, the output indicated that DisplayRefreshMonitor::displayLinkFired()
     63            would early return because isPreviousFrameDone() was false. The only way for that to occur,
     64            barring memory corruption, is if DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() returned early,
     65            which it does if the callback comes twice in a single event loop; this may explain the rarity.
     66
     67            So fix DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() call setIsPreviousFrameDone(true)
     68            so the next callback can make progress
     69
     70            Also add some locking annotations and fix one missing lock, and some release logging.
     71
     72            * WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp:
     73            (WebKit::DisplayRefreshMonitorMac::dispatchDisplayDidRefresh):
     74
    1752021-12-03  Russell Epstein  <repstein@apple.com>
    276

  • branches/safari-612.4.2.1-branch/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDisplayRefreshMonitor.mm

    r278253 r286723  
    6868        return false;
    6969
    70     if (!isScheduled()) {
    71         LOG_WITH_STREAM(DisplayLink, stream << "RemoteLayerTreeDisplayRefreshMonitor::requestRefreshCallback - triggering update");
    72         static_cast<DrawingArea&>(*m_drawingArea.get()).triggerRenderingUpdate();
    73     }
     70    Locker locker { lock() };
     71
     72    if (isScheduled())
     73        return true;
     74
     75    LOG_WITH_STREAM(DisplayLink, stream << "RemoteLayerTreeDisplayRefreshMonitor::requestRefreshCallback - triggering update");
     76    static_cast<DrawingArea&>(*m_drawingArea.get()).triggerRenderingUpdate();
    7477
    7578    setIsScheduled(true);
    … …  
    7982void RemoteLayerTreeDisplayRefreshMonitor::didUpdateLayers()
    8083{
    81     setIsScheduled(false);
     84    {
     85        Locker locker { lock() };
     86        setIsScheduled(false);
    8287
    83     if (!isPreviousFrameDone())
    84         return;
     88        if (!isPreviousFrameDone())
     89            return;
    8590
    86     setIsPreviousFrameDone(false);
     91        setIsPreviousFrameDone(false);
     92    }
    8793    displayDidRefresh(m_currentUpdate);
    8894    m_currentUpdate = m_currentUpdate.nextUpdate();

  • branches/safari-612.4.2.1-branch/Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp

    r278185 r286723  
    6161{
    6262    // FIXME: This will perturb displayUpdate.
    63     if (!m_firstCallbackInCurrentRunloop)
     63    if (!m_firstCallbackInCurrentRunloop) {
     64        RELEASE_LOG(DisplayLink, "[Web] DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() for display %u - m_firstCallbackInCurrentRunloop is false", displayID());
     65        Locker locker { lock() };
     66        setIsPreviousFrameDone(true);
    6467        return;
     68    }
    6569
    6670    DisplayRefreshMonitor::dispatchDisplayDidRefresh(displayUpdate);
Note: See TracChangeset for help on using the changeset viewer.