- Revision
- 286481
- Author
- simon.fra...@apple.com
- Date
- 2021-12-02 22:05:49 -0800 (Thu, 02 Dec 2021)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (286480 => 286481)
--- trunk/Source/WebCore/ChangeLog 2021-12-03 04:49:45 UTC (rev 286480)
+++ trunk/Source/WebCore/ChangeLog 2021-12-03 06:05:49 UTC (rev 286481)
@@ -1,3 +1,39 @@
+2021-12-02 Simon Fraser <simon.fra...@apple.com>
+
+ 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.
+
+ * 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.
+
2021-12-02 Tyler Wilcock <tyle...@apple.com>
AX: In AccessibilityRenderObject::documentLinks, use existing image-map document links instead of always creating new ones
Modified: trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp (286480 => 286481)
--- trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp 2021-12-03 04:49:45 UTC (rev 286480)
+++ trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp 2021-12-03 06:05:49 UTC (rev 286481)
@@ -85,6 +85,8 @@
void DisplayRefreshMonitor::stop()
{
stopNotificationMechanism();
+
+ Locker locker { m_lock };
setIsScheduled(false);
}
@@ -152,8 +154,6 @@
bool DisplayRefreshMonitor::firedAndReachedMaxUnscheduledFireCount()
{
- ASSERT(m_lock.isLocked());
-
if (isScheduled()) {
m_unscheduledFireCount = 0;
return false;
@@ -169,8 +169,10 @@
Locker locker { m_lock };
// This may be off the main thread.
- if (!isPreviousFrameDone())
+ if (!isPreviousFrameDone()) {
+ RELEASE_LOG(DisplayLink, "[Web] DisplayRefreshMonitor::displayLinkFired for display %u - previous frame is not complete", displayID());
return;
+ }
LOG_WITH_STREAM(DisplayLink, stream << "[Web] DisplayRefreshMonitor::displayLinkFired for display " << displayID() << " - scheduled " << isScheduled() << " unscheduledFireCount " << m_unscheduledFireCount << " of " << m_maxUnscheduledFireCount);
if (firedAndReachedMaxUnscheduledFireCount()) {
Modified: trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h (286480 => 286481)
--- trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2021-12-03 04:49:45 UTC (rev 286480)
+++ trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2021-12-03 06:05:49 UTC (rev 286481)
@@ -72,23 +72,23 @@
WEBCORE_EXPORT virtual void dispatchDisplayDidRefresh(const DisplayUpdate&);
- Lock& lock() { return m_lock; }
- void setMaxUnscheduledFireCount(unsigned count) { m_maxUnscheduledFireCount = count; }
+ Lock& lock() WTF_RETURNS_LOCK(m_lock) { return m_lock; }
+ void setMaxUnscheduledFireCount(unsigned count) WTF_REQUIRES_LOCK(m_lock) { m_maxUnscheduledFireCount = count; }
// Returns true if the start was successful.
WEBCORE_EXPORT virtual bool startNotificationMechanism() = 0;
WEBCORE_EXPORT virtual void stopNotificationMechanism() = 0;
- bool isScheduled() const { return m_scheduled; }
- void setIsScheduled(bool scheduled) { m_scheduled = scheduled; }
+ bool isScheduled() const WTF_REQUIRES_LOCK(m_lock) { return m_scheduled; }
+ void setIsScheduled(bool scheduled) WTF_REQUIRES_LOCK(m_lock) { m_scheduled = scheduled; }
- bool isPreviousFrameDone() const { return m_previousFrameDone; }
- void setIsPreviousFrameDone(bool done) { m_previousFrameDone = done; }
+ bool isPreviousFrameDone() const WTF_REQUIRES_LOCK(m_lock) { return m_previousFrameDone; }
+ void setIsPreviousFrameDone(bool done) WTF_REQUIRES_LOCK(m_lock) { m_previousFrameDone = done; }
WEBCORE_EXPORT void displayDidRefresh(const DisplayUpdate&);
private:
- bool firedAndReachedMaxUnscheduledFireCount();
+ bool firedAndReachedMaxUnscheduledFireCount() WTF_REQUIRES_LOCK(m_lock);
virtual void adjustPreferredFramesPerSecond(FramesPerSecond) { }
@@ -102,11 +102,11 @@
std::optional<FramesPerSecond> m_maxClientPreferredFramesPerSecond;
Lock m_lock;
- bool m_scheduled { false };
- bool m_previousFrameDone { true };
+ bool m_scheduled WTF_GUARDED_BY_LOCK(m_lock) { false };
+ bool m_previousFrameDone WTF_GUARDED_BY_LOCK(m_lock) { true };
- unsigned m_unscheduledFireCount { 0 };
- unsigned m_maxUnscheduledFireCount { 0 };
+ unsigned m_unscheduledFireCount WTF_GUARDED_BY_LOCK(m_lock) { 0 };
+ unsigned m_maxUnscheduledFireCount WTF_GUARDED_BY_LOCK(m_lock) { 0 };
};
}
Modified: trunk/Source/WebKit/ChangeLog (286480 => 286481)
--- trunk/Source/WebKit/ChangeLog 2021-12-03 04:49:45 UTC (rev 286480)
+++ trunk/Source/WebKit/ChangeLog 2021-12-03 06:05:49 UTC (rev 286481)
@@ -1,3 +1,29 @@
+2021-12-02 Simon Fraser <simon.fra...@apple.com>
+
+ 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.
+
+ * WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp:
+ (WebKit::DisplayRefreshMonitorMac::dispatchDisplayDidRefresh):
+
2021-12-02 Chris Dumez <cdu...@apple.com>
Regression(r283179) Google Drive freezes after downloading a folder
Modified: trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDisplayRefreshMonitor.mm (286480 => 286481)
--- trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDisplayRefreshMonitor.mm 2021-12-03 04:49:45 UTC (rev 286480)
+++ trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDisplayRefreshMonitor.mm 2021-12-03 06:05:49 UTC (rev 286481)
@@ -67,11 +67,14 @@
if (!m_drawingArea)
return false;
- if (!isScheduled()) {
- LOG_WITH_STREAM(DisplayLink, stream << "RemoteLayerTreeDisplayRefreshMonitor::requestRefreshCallback - triggering update");
- static_cast<DrawingArea&>(*m_drawingArea.get()).triggerRenderingUpdate();
- }
+ Locker locker { lock() };
+ if (isScheduled())
+ return true;
+
+ LOG_WITH_STREAM(DisplayLink, stream << "RemoteLayerTreeDisplayRefreshMonitor::requestRefreshCallback - triggering update");
+ static_cast<DrawingArea&>(*m_drawingArea.get()).triggerRenderingUpdate();
+
setIsScheduled(true);
return true;
}
@@ -78,12 +81,15 @@
void RemoteLayerTreeDisplayRefreshMonitor::didUpdateLayers()
{
- setIsScheduled(false);
+ {
+ Locker locker { lock() };
+ setIsScheduled(false);
- if (!isPreviousFrameDone())
- return;
+ if (!isPreviousFrameDone())
+ return;
- setIsPreviousFrameDone(false);
+ setIsPreviousFrameDone(false);
+ }
displayDidRefresh(m_currentUpdate);
m_currentUpdate = m_currentUpdate.nextUpdate();
}
Modified: trunk/Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp (286480 => 286481)
--- trunk/Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp 2021-12-03 04:49:45 UTC (rev 286480)
+++ trunk/Source/WebKit/WebProcess/WebPage/mac/DisplayRefreshMonitorMac.cpp 2021-12-03 06:05:49 UTC (rev 286481)
@@ -60,8 +60,12 @@
void DisplayRefreshMonitorMac::dispatchDisplayDidRefresh(const DisplayUpdate& displayUpdate)
{
// FIXME: This will perturb displayUpdate.
- if (!m_firstCallbackInCurrentRunloop)
+ if (!m_firstCallbackInCurrentRunloop) {
+ RELEASE_LOG(DisplayLink, "[Web] DisplayRefreshMonitorMac::dispatchDisplayDidRefresh() for display %u - m_firstCallbackInCurrentRunloop is false", displayID());
+ Locker locker { lock() };
+ setIsPreviousFrameDone(true);
return;
+ }
DisplayRefreshMonitor::dispatchDisplayDidRefresh(displayUpdate);
}
