Fix non thread-safe use of WeakPtr in DisplayRefreshMonitorMac::displayLinkFired()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jul 2019 20:03:08 +0000 (20:03 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jul 2019 20:03:08 +0000 (20:03 +0000)
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:

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/DisplayRefreshMonitor.h
Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp
Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h
Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp
Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.h

index 94ac2ac..f1606c9 100644 (file)
@@ -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  Sihui Liu  <sihui_liu@apple.com>
 
         Only allow fetching and removing session credentials from WebsiteDataStore
index 18eeaa0..d3542b0 100644 (file)
@@ -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(); }
index 8b4b69d..4d3cd69 100644 (file)
@@ -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();
     }
index ae1a0ba..e269060 100644 (file)
@@ -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;
 };
 
 }
index beb6c04..4dce8b4 100644 (file)
@@ -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);
     });
 }
 
index a454577..d4587fd 100644 (file)
@@ -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);