Attachment 373737 Details for Bug 199626 – Patch

[patch] Patch

bug-199626-20190709115311.patch (text/plain), 8.93 KB, created by Chris Dumez on 2019-07-09 11:53:12 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-07-09 11:53:12 PDT

Size: 8.93 KB

patch

obsolete

>Subversion Revision: 247259
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 5192537beda30a7a91b48fe51f9afe813c439205..75838bb5d9dd1cfd712d79ec4c955e4cf579eb41 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,28 @@
>+2019-07-09  Chris Dumez  <cdumez@apple.com>
>+
>+        Fix non thread-safe use of WeakPtr in DisplayRefreshMonitorMac::displayLinkFired()
>+        https://bugs.webkit.org/show_bug.cgi?id=199626
>+
>+        Reviewed by Ryosuke Niwa.
>+
>+        Fix non thread-safe use of WeakPtr in DisplayRefreshMonitorMac::displayLinkFired().
>+        DisplayRefreshMonitorMac gets constructed / destroyed on the main thread, it is
>+        not thread-safe to call makeWeakPtr() on a DisplayRefreshMonitorMac object like it
>+        was done before.
>+
>+        To address the issue, mark the object as ThreadSafeRefCounted and ref the object
>+        in the lambda instead.
>+
>+        * platform/graphics/DisplayRefreshMonitor.h:
>+        (WebCore::DisplayRefreshMonitor::stop):
>+        * platform/graphics/DisplayRefreshMonitorManager.cpp:
>+        (WebCore::DisplayRefreshMonitorManager::unregisterClient):
>+        * platform/graphics/mac/DisplayRefreshMonitorMac.cpp:
>+        (WebCore::DisplayRefreshMonitorMac::~DisplayRefreshMonitorMac):
>+        (WebCore::DisplayRefreshMonitorMac::stop):
>+        (WebCore::DisplayRefreshMonitorMac::displayLinkFired):
>+        * platform/graphics/mac/DisplayRefreshMonitorMac.h:
>+
> 2019-07-09  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][IFC] Remove InlineItem references from inline runs.
>diff --git a/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h b/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h
>index 18eeaa02963690801993bfff8343fffe4d3efc3e..d3542b083139bb93b08dd76a20787cfd2b9883f8 100644
>--- a/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h
>+++ b/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h
>@@ -30,7 +30,7 @@
> #include "PlatformScreen.h"
> #include <wtf/HashSet.h>
> #include <wtf/Lock.h>
>-#include <wtf/RefCounted.h>
>+#include <wtf/ThreadSafeRefCounted.h>
> #include <wtf/RefPtr.h>
> 
> namespace WebCore {
>@@ -38,7 +38,7 @@ namespace WebCore {
> class DisplayAnimationClient;
> class DisplayRefreshMonitorClient;
> 
>-class DisplayRefreshMonitor : public RefCounted<DisplayRefreshMonitor> {
>+class DisplayRefreshMonitor : public ThreadSafeRefCounted<DisplayRefreshMonitor> {
> public:
>     static RefPtr<DisplayRefreshMonitor> create(DisplayRefreshMonitorClient&);
>     WEBCORE_EXPORT virtual ~DisplayRefreshMonitor();
>@@ -48,6 +48,9 @@ public:
>     // Return true if callback request was scheduled, false if it couldn't be
>     // (e.g., hardware refresh is not available)
>     virtual bool requestRefreshCallback() = 0;
>+
>+    virtual void stop() { }
>+
>     void windowScreenDidChange(PlatformDisplayID);
>     
>     bool hasClients() const { return m_clients.size(); }
>diff --git a/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp b/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp
>index 8b4b69db0334ffe0143d04fda5f210ac6ef6838f..4d3cd6955d3ab7959409d1cce1fb9db58ccc98d5 100644
>--- a/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp
>+++ b/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp
>@@ -45,7 +45,8 @@ DisplayRefreshMonitorManager& DisplayRefreshMonitorManager::sharedManager()
> DisplayRefreshMonitor* DisplayRefreshMonitorManager::createMonitorForClient(DisplayRefreshMonitorClient& client)
> {
>     PlatformDisplayID clientDisplayID = client.displayID();
>-    for (const RefPtr<DisplayRefreshMonitor>& monitor : m_monitors) {
>+    for (auto& monitorWrapper : m_monitors) {
>+        auto& monitor = monitorWrapper.monitor;
>         if (monitor->displayID() != clientDisplayID)
>             continue;
>         monitor->addClient(client);
>@@ -59,7 +60,7 @@ DisplayRefreshMonitor* DisplayRefreshMonitorManager::createMonitorForClient(Disp
>     LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::createMonitorForClient() - created monitor %p", monitor.get());
>     monitor->addClient(client);
>     DisplayRefreshMonitor* result = monitor.get();
>-    m_monitors.append(WTFMove(monitor));
>+    m_monitors.append({ WTFMove(monitor) });
>     return result;
> }
> 
>@@ -78,7 +79,7 @@ void DisplayRefreshMonitorManager::unregisterClient(DisplayRefreshMonitorClient&
> 
>     PlatformDisplayID clientDisplayID = client.displayID();
>     for (size_t i = 0; i < m_monitors.size(); ++i) {
>-        RefPtr<DisplayRefreshMonitor> monitor = m_monitors[i];
>+        RefPtr<DisplayRefreshMonitor> monitor = m_monitors[i].monitor;
>         if (monitor->displayID() != clientDisplayID)
>             continue;
>         if (monitor->removeClient(client)) {
>@@ -108,7 +109,7 @@ void DisplayRefreshMonitorManager::displayDidRefresh(DisplayRefreshMonitor& moni
>         return;
>     LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::displayDidRefresh() - destroying monitor %p", &monitor);
> 
>-    size_t monitorIndex = m_monitors.find(&monitor);
>+    size_t monitorIndex = m_monitors.findMatching([&](auto& monitorWrapper) { return monitorWrapper.monitor == &monitor; });
>     if (monitorIndex != notFound)
>         m_monitors.remove(monitorIndex);
> }
>@@ -127,7 +128,8 @@ void DisplayRefreshMonitorManager::windowScreenDidChange(PlatformDisplayID displ
> 
> void DisplayRefreshMonitorManager::displayWasUpdated(PlatformDisplayID displayID)
> {
>-    for (const auto& monitor : m_monitors) {
>+    for (const auto& monitorWrapper : m_monitors) {
>+        auto& monitor = monitorWrapper.monitor;
>         if (displayID == monitor->displayID() && monitor->hasRequestedRefreshCallback())
>             monitor->displayLinkFired();
>     }
>diff --git a/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h b/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h
>index ae1a0ba2292baf5bbbce8ad9fa8429266d34cb6d..e269060d2d5275dba08a5c9f75863cf390079a38 100644
>--- a/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h
>+++ b/Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h
>@@ -57,7 +57,18 @@ private:
> 
>     DisplayRefreshMonitor* createMonitorForClient(DisplayRefreshMonitorClient&);
> 
>-    Vector<RefPtr<DisplayRefreshMonitor>> m_monitors;
>+    struct DisplayRefreshMonitorWrapper {
>+        DisplayRefreshMonitorWrapper(DisplayRefreshMonitorWrapper&&) = default;
>+        ~DisplayRefreshMonitorWrapper()
>+        {
>+            if (monitor)
>+                monitor->stop();
>+        }
>+
>+        RefPtr<DisplayRefreshMonitor> monitor;
>+    };
>+
>+    Vector<DisplayRefreshMonitorWrapper> m_monitors;
> };
> 
> }
>diff --git a/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp b/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp
>index beb6c0472522d5d6a0ada5661d2fcd77f9308093..4dce8b4ba827d7478ee384cc7e25e50898b50aad 100644
>--- a/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp
>+++ b/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp
>@@ -40,11 +40,17 @@ DisplayRefreshMonitorMac::DisplayRefreshMonitorMac(PlatformDisplayID displayID)
> 
> DisplayRefreshMonitorMac::~DisplayRefreshMonitorMac()
> {
>-    if (m_displayLink) {
>-        CVDisplayLinkStop(m_displayLink);
>-        CVDisplayLinkRelease(m_displayLink);
>-        m_displayLink = nullptr;
>-    }
>+    ASSERT(!m_displayLink);
>+}
>+
>+void DisplayRefreshMonitorMac::stop()
>+{
>+    if (!m_displayLink)
>+        return;
>+
>+    CVDisplayLinkStop(m_displayLink);
>+    CVDisplayLinkRelease(m_displayLink);
>+    m_displayLink = nullptr;
> }
> 
> static CVReturn displayLinkCallback(CVDisplayLinkRef, const CVTimeStamp*, const CVTimeStamp*, CVOptionFlags, CVOptionFlags*, void* data)
>@@ -89,9 +95,9 @@ void DisplayRefreshMonitorMac::displayLinkFired()
> 
>     setIsPreviousFrameDone(false);
> 
>-    RunLoop::main().dispatch([weakPtr = makeWeakPtr(*this)] {
>-        if (auto* monitor = weakPtr.get())
>-            handleDisplayRefreshedNotificationOnMainThread(monitor);
>+    RunLoop::main().dispatch([this, protectedThis = makeRef(*this)] {
>+        if (m_displayLink)
>+            handleDisplayRefreshedNotificationOnMainThread(this);
>     });
> }
> 
>diff --git a/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.h b/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.h
>index a454577d56ed1597ec2c9c57a808d319e78e6242..d4587fd45556ebc32d0295a42eaaba0566ea38bb 100644
>--- a/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.h
>+++ b/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.h
>@@ -34,7 +34,7 @@ typedef struct __CVDisplayLink *CVDisplayLinkRef;
> 
> namespace WebCore {
> 
>-class DisplayRefreshMonitorMac : public DisplayRefreshMonitor, public CanMakeWeakPtr<DisplayRefreshMonitorMac> {
>+class DisplayRefreshMonitorMac : public DisplayRefreshMonitor {
> public:
>     static Ref<DisplayRefreshMonitorMac> create(PlatformDisplayID displayID)
>     {
>@@ -45,6 +45,7 @@ public:
> 
>     void displayLinkFired() override;
>     bool requestRefreshCallback() override;
>+    void stop() override;
> 
> private:
>     explicit DisplayRefreshMonitorMac(PlatformDisplayID);

Actions: View | Formatted Diff | Diff

Attachments on bug 199626: 373731 | 373733 | 373736 | 373737