Diff
Modified: branches/safari-537.73-branch/LayoutTests/ChangeLog (158604 => 158605)
--- branches/safari-537.73-branch/LayoutTests/ChangeLog 2013-11-04 23:44:48 UTC (rev 158604)
+++ branches/safari-537.73-branch/LayoutTests/ChangeLog 2013-11-04 23:47:46 UTC (rev 158605)
@@ -1,3 +1,19 @@
+2013-11-04 Dean Jackson <[email protected]>
+
+ <rdar://problem/15292359>
+ Merge r157299
+
+ 2013-10-10 Dean Jackson <[email protected]>
+
+ 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-11-04 Lucas Forschler <[email protected]>
Merge r157436
Modified: branches/safari-537.73-branch/LayoutTests/TestExpectations (158604 => 158605)
--- branches/safari-537.73-branch/LayoutTests/TestExpectations 2013-11-04 23:44:48 UTC (rev 158604)
+++ branches/safari-537.73-branch/LayoutTests/TestExpectations 2013-11-04 23:47:46 UTC (rev 158605)
@@ -18,7 +18,5 @@
webkit.org/b/118301 fast/dom/timer-throttling-hidden-page.html [ Skip ]
-webkit.org/b/121033 fast/animation/request-animation-frame-remove-client.html [ Pass Crash ]
-
webkit.org/b/57700 mathml/presentation/row.xhtml [ Failure ]
webkit.org/b/57700 mathml/presentation/mo.xhtml [ Failure ]
\ No newline at end of file
Modified: branches/safari-537.73-branch/LayoutTests/fast/animation/request-animation-frame-remove-client-expected.txt (158604 => 158605)
--- branches/safari-537.73-branch/LayoutTests/fast/animation/request-animation-frame-remove-client-expected.txt 2013-11-04 23:44:48 UTC (rev 158604)
+++ branches/safari-537.73-branch/LayoutTests/fast/animation/request-animation-frame-remove-client-expected.txt 2013-11-04 23:47:46 UTC (rev 158605)
@@ -1,3 +1,3 @@
-This test crashes.
+This test does not crash.
Modified: branches/safari-537.73-branch/LayoutTests/fast/animation/request-animation-frame-remove-client.html (158604 => 158605)
--- branches/safari-537.73-branch/LayoutTests/fast/animation/request-animation-frame-remove-client.html 2013-11-04 23:44:48 UTC (rev 158604)
+++ branches/safari-537.73-branch/LayoutTests/fast/animation/request-animation-frame-remove-client.html 2013-11-04 23:47:46 UTC (rev 158605)
@@ -1,4 +1,4 @@
-<p>This test crashes.</p>
+<p>This test does not crash.</p>
<iframe></iframe>
<iframe></iframe>
<iframe></iframe>
Modified: branches/safari-537.73-branch/Source/WebCore/ChangeLog (158604 => 158605)
--- branches/safari-537.73-branch/Source/WebCore/ChangeLog 2013-11-04 23:44:48 UTC (rev 158604)
+++ branches/safari-537.73-branch/Source/WebCore/ChangeLog 2013-11-04 23:47:46 UTC (rev 158605)
@@ -1,3 +1,35 @@
+2013-11-04 Dean Jackson <[email protected]>
+
+ <rdar://problem/15292359>
+ Merge r157299
+
+ 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-11-04 Lucas Forschler <[email protected]>
Merge r157436
Modified: branches/safari-537.73-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp (158604 => 158605)
--- branches/safari-537.73-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp 2013-11-04 23:44:48 UTC (rev 158604)
+++ branches/safari-537.73-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp 2013-11-04 23:47:46 UTC (rev 158605)
@@ -28,7 +28,6 @@
#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
#include "DisplayRefreshMonitor.h"
-
#include <wtf/CurrentTime.h>
namespace WebCore {
@@ -59,6 +58,7 @@
, m_previousFrameDone(true)
, m_unscheduledFireCount(0)
, m_displayID(displayID)
+ , m_clientsToBeNotified(nullptr)
#if PLATFORM(MAC)
, m_displayLink(0)
#endif
@@ -94,7 +94,7 @@
double monotonicAnimationStartTime;
{
MutexLocker lock(m_mutex);
- if (!m_scheduled)
+ if (!m_scheduled)
++m_unscheduledFireCount;
else
m_unscheduledFireCount = 0;
@@ -105,21 +105,34 @@
// The call back can cause all our clients to be unregistered, so we need to protect
// against deletion until the end of the method.
- RefPtr<DisplayRefreshMonitor> protector(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));
+ RefPtr<DisplayRefreshMonitor> protect(this);
+
+ // 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);
m_previousFrameDone = true;
}
-
+
DisplayRefreshMonitorManager::sharedManager()->displayDidRefresh(this);
}
Modified: branches/safari-537.73-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h (158604 => 158605)
--- branches/safari-537.73-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2013-11-04 23:44:48 UTC (rev 158604)
+++ branches/safari-537.73-branch/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2013-11-04 23:47:46 UTC (rev 158605)
@@ -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,11 @@
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;
+ DisplayRefreshMonitorClientSet* m_clientsToBeNotified;
+
#if PLATFORM(BLACKBERRY)
public:
void displayLinkFired();
@@ -139,6 +131,7 @@
void startAnimationClient();
void stopAnimationClient();
#endif
+
#if PLATFORM(MAC)
public:
void displayLinkFired(double nowSeconds, double outputTimeSeconds);
@@ -171,7 +164,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: branches/safari-537.73-branch/Source/WebCore/platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp (158604 => 158605)
--- branches/safari-537.73-branch/Source/WebCore/platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp 2013-11-04 23:44:48 UTC (rev 158604)
+++ branches/safari-537.73-branch/Source/WebCore/platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp 2013-11-04 23:47:46 UTC (rev 158605)
@@ -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)
{
