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

Changeset 286481 in webkit


Ignore:
Timestamp:
Dec 2, 2021, 10:05:49 PM (5 years ago)
Author:
Simon Fraser
Message:

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):

Location:
trunk/Source
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r286477 r286481  
     12021-12-02  Simon Fraser  <simon.fraser@apple.com>
     2
     3        A Safari tab can rarely get stuck in a state where rendering updates stop happening
     4        https://bugs.webkit.org/show_bug.cgi?id=233784
     5        rdar://85445072
     6
     7        Reviewed by Chris Dumez.
     8
     9        Sometimes a Safari tab can get into a state where rendering updates cease to happen,
     10        which manifests as partially broken scrolling, blank tiles revealed when scrolling,
     11        and somewhat broken page updates. I was able to sometimes reproduce this by clicking
     12        on links in eBay emails from Mail on a system with two displays.
     13
     14        From the one time I reproduce with logging, the output indicated that DisplayRefreshMonitor::displayLinkFired()
     15        would early return because isPreviousFrameDone() was false. The only way for that to occur,
     16        barring memory corruption, is if DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() returned early,
     17        which it does if the callback comes twice in a single event loop; this may explain the rarity.
     18
     19        So fix DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() call setIsPreviousFrameDone(true)
     20        so the next callback can make progress
     21
     22        Also add some locking annotations and fix one missing lock, and some release logging.
     23
     24        * platform/graphics/DisplayRefreshMonitor.cpp:
     25        (WebCore::DisplayRefreshMonitor::stop):
     26        (WebCore::DisplayRefreshMonitor::firedAndReachedMaxUnscheduledFireCount):
     27        (WebCore::DisplayRefreshMonitor::displayLinkFired):
     28        * platform/graphics/DisplayRefreshMonitor.h:
     29        (WebCore::DisplayRefreshMonitor::WTF_REQUIRES_LOCK):
     30        (WebCore::DisplayRefreshMonitor::WTF_GUARDED_BY_LOCK):
     31        (WebCore::DisplayRefreshMonitor::setMaxUnscheduledFireCount): Deleted.
     32        (WebCore::DisplayRefreshMonitor::isScheduled const): Deleted.
     33        (WebCore::DisplayRefreshMonitor::setIsScheduled): Deleted.
     34        (WebCore::DisplayRefreshMonitor::isPreviousFrameDone const): Deleted.
     35        (WebCore::DisplayRefreshMonitor::setIsPreviousFrameDone): Deleted.
     36
    1372021-12-02  Tyler Wilcock  <tyler_w@apple.com>
    238

  • trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp

    r278253 r286481  
    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);

  • trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h

    r278253 r286481  
    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

  • trunk/Source/WebKit/ChangeLog

    r286479 r286481  
     12021-12-02  Simon Fraser  <simon.fraser@apple.com>
     2
     3        A Safari tab can rarely get stuck in a state where rendering updates stop happening
     4        https://bugs.webkit.org/show_bug.cgi?id=233784
     5        rdar://85445072
     6
     7        Reviewed by Chris Dumez.
     8
     9        Sometimes a Safari tab can get into a state where rendering updates cease to happen,
     10        which manifests as partially broken scrolling, blank tiles revealed when scrolling,
     11        and somewhat broken page updates. I was able to sometimes reproduce this by clicking
     12        on links in eBay emails from Mail on a system with two displays.
     13
     14        From the one time I reproduce with logging, the output indicated that DisplayRefreshMonitor::displayLinkFired()
     15        would early return because isPreviousFrameDone() was false. The only way for that to occur,
     16        barring memory corruption, is if DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() returned early,
     17        which it does if the callback comes twice in a single event loop; this may explain the rarity.
     18
     19        So fix DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() call setIsPreviousFrameDone(true)
     20        so the next callback can make progress
     21
     22        Also add some locking annotations and fix one missing lock, and some release logging.
     23
     24        * WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp:
     25        (WebKit::DisplayRefreshMonitorMac::dispatchDisplayDidRefresh):
     26
    1272021-12-02  Chris Dumez  <cdumez@apple.com>
    228

  • trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDisplayRefreshMonitor.mm

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

  • trunk/Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp

    r278185 r286481  
    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.