- Revision
- 157299
- Author
- [email protected]
- Date
- 2013-10-11 02:44:39 -0700 (Fri, 11 Oct 2013)
Log Message
Source/WebCore: Use after free in WebCore::DisplayRefreshMonitorClient::fireDisplayRefreshIfNeeded
https://bugs.webkit.org/show_bug.cgi?id=121033
Patch by Darin Adler <[email protected]> on 2013-10-10
Reviewed by Dean Jackson.
For safe iteration, use a set rather than a vector, and remove the clients from
the set if they are removed during iteration.
Test: fast/animation/request-animation-frame-remove-client.html
* platform/graphics/DisplayRefreshMonitor.cpp:
(WebCore::DisplayRefreshMonitor::DisplayRefreshMonitor): Initialize the
m_clientsToBeNotified pointer to null.
(WebCore::DisplayRefreshMonitor::removeClient): If there is a m_clientsToBeNotified
set, remove from it as well as the real m_clients set.
(WebCore::DisplayRefreshMonitor::displayDidRefresh): Use a HashSet instead of a
vector for the copy of the clients set we iterate.
* platform/graphics/DisplayRefreshMonitor.h: Moved some of the BlackBerry-specific
part of this out of the header. Added a new HashSet pointer, m_clientsToBeNotified,
to be used to remove clients during the notification process. Also added a FIXME.
* platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp: Moved the
DisplayAnimationClient class in here.
LayoutTests: Use after free in WebCore::DisplayRefreshMonitorClient::fireDisplayRefreshIfNeeded
http://webkit.org/b/121033
Update test to indicate it no longer crashes.
* TestExpectations: Mark test as passing.
* fast/animation/request-animation-frame-remove-client-expected.txt:
* fast/animation/request-animation-frame-remove-client.html:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (157298 => 157299)
--- trunk/LayoutTests/ChangeLog 2013-10-11 09:43:56 UTC (rev 157298)
+++ trunk/LayoutTests/ChangeLog 2013-10-11 09:44:39 UTC (rev 157299)
@@ -3,6 +3,17 @@
Use after free in WebCore::DisplayRefreshMonitorClient::fireDisplayRefreshIfNeeded
http://webkit.org/b/121033
+ Update test to indicate it no longer crashes.
+
+ * TestExpectations: Mark test as passing.
+ * fast/animation/request-animation-frame-remove-client-expected.txt:
+ * fast/animation/request-animation-frame-remove-client.html:
+
+2013-10-10 Dean Jackson <[email protected]>
+
+ Use after free in WebCore::DisplayRefreshMonitorClient::fireDisplayRefreshIfNeeded
+ http://webkit.org/b/121033
+
Reviewed by Darin Adler.
Test that assertion fires if you try to remove potential client while in a
Modified: trunk/LayoutTests/TestExpectations (157298 => 157299)
--- trunk/LayoutTests/TestExpectations 2013-10-11 09:43:56 UTC (rev 157298)
+++ trunk/LayoutTests/TestExpectations 2013-10-11 09:44:39 UTC (rev 157299)
@@ -65,4 +65,3 @@
# Skipping it for now, then put it again when the spec decides it
fast/mediastream/MediaStream-onended.html [ Skip ]
-webkit.org/b/121033 fast/animation/request-animation-frame-remove-client.html [ Pass Crash ]
Modified: trunk/LayoutTests/fast/animation/request-animation-frame-remove-client-expected.txt (157298 => 157299)
--- trunk/LayoutTests/fast/animation/request-animation-frame-remove-client-expected.txt 2013-10-11 09:43:56 UTC (rev 157298)
+++ trunk/LayoutTests/fast/animation/request-animation-frame-remove-client-expected.txt 2013-10-11 09:44:39 UTC (rev 157299)
@@ -1,3 +1,3 @@
-This test crashes.
+This test does not crash.
Modified: trunk/LayoutTests/fast/animation/request-animation-frame-remove-client.html (157298 => 157299)
--- trunk/LayoutTests/fast/animation/request-animation-frame-remove-client.html 2013-10-11 09:43:56 UTC (rev 157298)
+++ trunk/LayoutTests/fast/animation/request-animation-frame-remove-client.html 2013-10-11 09:44:39 UTC (rev 157299)
@@ -1,4 +1,4 @@
-<p>This test crashes.</p>
+<p>This test does not crash.</p>
<iframe></iframe>
<iframe></iframe>
<iframe></iframe>
Modified: trunk/Source/WebCore/ChangeLog (157298 => 157299)
--- trunk/Source/WebCore/ChangeLog 2013-10-11 09:43:56 UTC (rev 157298)
+++ trunk/Source/WebCore/ChangeLog 2013-10-11 09:44:39 UTC (rev 157299)
@@ -1,3 +1,30 @@
+2013-10-10 Darin Adler <[email protected]>
+
+ Use after free in WebCore::DisplayRefreshMonitorClient::fireDisplayRefreshIfNeeded
+ https://bugs.webkit.org/show_bug.cgi?id=121033
+
+ Reviewed by Dean Jackson.
+
+ For safe iteration, use a set rather than a vector, and remove the clients from
+ the set if they are removed during iteration.
+
+ Test: fast/animation/request-animation-frame-remove-client.html
+
+ * platform/graphics/DisplayRefreshMonitor.cpp:
+ (WebCore::DisplayRefreshMonitor::DisplayRefreshMonitor): Initialize the
+ m_clientsToBeNotified pointer to null.
+ (WebCore::DisplayRefreshMonitor::removeClient): If there is a m_clientsToBeNotified
+ set, remove from it as well as the real m_clients set.
+ (WebCore::DisplayRefreshMonitor::displayDidRefresh): Use a HashSet instead of a
+ vector for the copy of the clients set we iterate.
+
+ * platform/graphics/DisplayRefreshMonitor.h: Moved some of the BlackBerry-specific
+ part of this out of the header. Added a new HashSet pointer, m_clientsToBeNotified,
+ to be used to remove clients during the notification process. Also added a FIXME.
+
+ * platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp: Moved the
+ DisplayAnimationClient class in here.
+
2013-10-10 Dean Jackson <[email protected]>
Use after free in WebCore::DisplayRefreshMonitorClient::fireDisplayRefreshIfNeeded
Modified: trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp (157298 => 157299)
--- trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp 2013-10-11 09:43:56 UTC (rev 157298)
+++ trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp 2013-10-11 09:44:39 UTC (rev 157299)
@@ -28,7 +28,6 @@
#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
#include "DisplayRefreshMonitor.h"
-
#include <wtf/CurrentTime.h>
#include <wtf/Ref.h>
@@ -60,6 +59,7 @@
, m_previousFrameDone(true)
, m_unscheduledFireCount(0)
, m_displayID(displayID)
+ , m_clientsToBeNotified(nullptr)
#if PLATFORM(MAC)
, m_displayLink(0)
#endif
@@ -82,6 +82,8 @@
bool DisplayRefreshMonitor::removeClient(DisplayRefreshMonitorClient* client)
{
+ if (m_clientsToBeNotified)
+ m_clientsToBeNotified->remove(client);
return m_clients.remove(client);
}
@@ -90,7 +92,7 @@
double monotonicAnimationStartTime;
{
MutexLocker lock(m_mutex);
- if (!m_scheduled)
+ if (!m_scheduled)
++m_unscheduledFireCount;
else
m_unscheduledFireCount = 0;
@@ -102,14 +104,27 @@
// The call back can cause all our clients to be unregistered, so we need to protect
// against deletion until the end of the method.
Ref<DisplayRefreshMonitor> protect(*this);
-
- Vector<DisplayRefreshMonitorClient*> clients;
- copyToVector(m_clients, clients);
- for (size_t i = 0; i < clients.size(); ++i) {
- DisplayRefreshMonitorClient* client = clients[i];
- ASSERT(m_clients.contains(client));
+
+ // Copy the hash table and remove clients from it one by one so we don't notify
+ // any client twice, but can respond to removal of clients during the delivery process.
+ HashSet<DisplayRefreshMonitorClient*> clientsToBeNotified = m_clients;
+ m_clientsToBeNotified = &clientsToBeNotified;
+ while (!clientsToBeNotified.isEmpty()) {
+ // Take a random client out of the set. Ordering doesn't matter.
+ // FIXME: Would read more cleanly if HashSet had a take function.
+ auto it = clientsToBeNotified.begin();
+ DisplayRefreshMonitorClient* client = *it;
+ clientsToBeNotified.remove(it);
+
client->fireDisplayRefreshIfNeeded(monotonicAnimationStartTime);
+
+ // This checks if this function was reentered. In that case, stop iterating
+ // since it's not safe to use the set any more.
+ if (m_clientsToBeNotified != &clientsToBeNotified)
+ break;
}
+ if (m_clientsToBeNotified == &clientsToBeNotified)
+ m_clientsToBeNotified = nullptr;
{
MutexLocker lock(m_mutex);
Modified: trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h (157298 => 157299)
--- trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2013-10-11 09:43:56 UTC (rev 157298)
+++ trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2013-10-11 09:44:39 UTC (rev 157299)
@@ -44,6 +44,7 @@
namespace WebCore {
+class DisplayAnimationClient;
class DisplayRefreshMonitor;
class DisplayRefreshMonitorManager;
@@ -74,17 +75,6 @@
PlatformDisplayID m_displayID;
};
-#if PLATFORM(BLACKBERRY)
-class DisplayAnimationClient : public BlackBerry::Platform::AnimationFrameRateClient {
-public:
- DisplayAnimationClient(DisplayRefreshMonitor *);
- ~DisplayAnimationClient() { }
-private:
- virtual void animationFrameChanged();
- DisplayRefreshMonitor *m_monitor;
-};
-#endif
-
//
// Monitor for display refresh messages for a given screen
//
@@ -116,7 +106,7 @@
}
private:
- DisplayRefreshMonitor(PlatformDisplayID);
+ explicit DisplayRefreshMonitor(PlatformDisplayID);
void displayDidRefresh();
static void handleDisplayRefreshedNotificationOnMainThread(void* data);
@@ -128,9 +118,10 @@
int m_unscheduledFireCount; // Number of times the display link has fired with no clients.
PlatformDisplayID m_displayID;
Mutex m_mutex;
-
- typedef HashSet<DisplayRefreshMonitorClient*> DisplayRefreshMonitorClientSet;
- DisplayRefreshMonitorClientSet m_clients;
+
+ HashSet<DisplayRefreshMonitorClient*> m_clients;
+ HashSet<DisplayRefreshMonitorClient*>* m_clientsToBeNotified;
+
#if PLATFORM(BLACKBERRY)
public:
void displayLinkFired();
@@ -139,6 +130,7 @@
void startAnimationClient();
void stopAnimationClient();
#endif
+
#if PLATFORM(MAC)
public:
void displayLinkFired(double nowSeconds, double outputTimeSeconds);
@@ -171,7 +163,9 @@
DisplayRefreshMonitor* ensureMonitorForClient(DisplayRefreshMonitorClient*);
// We know nothing about the values of PlatformDisplayIDs, so use UnsignedWithZeroKeyHashTraits.
- typedef HashMap<uint64_t, RefPtr<DisplayRefreshMonitor>, WTF::IntHash<uint64_t>, WTF::UnsignedWithZeroKeyHashTraits<uint64_t> > DisplayRefreshMonitorMap;
+ // FIXME: Since we know nothing about these values, this is not sufficient.
+ // Even with UnsignedWithZeroKeyHashTraits, there are still two special values used for empty and deleted hash table slots.
+ typedef HashMap<uint64_t, RefPtr<DisplayRefreshMonitor>, WTF::IntHash<uint64_t>, WTF::UnsignedWithZeroKeyHashTraits<uint64_t>> DisplayRefreshMonitorMap;
DisplayRefreshMonitorMap m_monitors;
};
Modified: trunk/Source/WebCore/platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp (157298 => 157299)
--- trunk/Source/WebCore/platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp 2013-10-11 09:43:56 UTC (rev 157298)
+++ trunk/Source/WebCore/platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp 2013-10-11 09:44:39 UTC (rev 157299)
@@ -27,6 +27,15 @@
namespace WebCore {
+class DisplayAnimationClient : public BlackBerry::Platform::AnimationFrameRateClient {
+public:
+ DisplayAnimationClient(DisplayRefreshMonitor *);
+ ~DisplayAnimationClient() { }
+private:
+ virtual void animationFrameChanged();
+ DisplayRefreshMonitor* m_monitor;
+};
+
DisplayAnimationClient::DisplayAnimationClient(DisplayRefreshMonitor *monitor)
: m_monitor(monitor)
{
