- Revision
- 116792
- Author
- simon.fra...@apple.com
- Date
- 2012-05-11 11:56:32 -0700 (Fri, 11 May 2012)
Log Message
Source/WebCore: CVDisplayLink keeps running after a single requestAnimationFrame
https://bugs.webkit.org/show_bug.cgi?id=86174
Reviewed by Sam Weinig.
A DisplayRefreshMonitor would keep its CVDisplayLink alive for as long
as it had clients, and the client is the ScriptedAnimationController, which
lives on the document. So a single requestAnimationFrame call would kick
off a CVDisplayLink which lived until the document was destroyed.
Fix by having the DisplayRefreshMonitor kill itself if the CVDisplayLink
fires for 10 times with no scheduled callbacks (to avoid creation/deletion
thrash on pages that call requestAnimationFrames with short setTimeouts,
as some do).
Use a HashMap in DisplayRefreshMonitorManager for the set of DisplayRefreshMonitor,
with the displayID as the key (using UnsignedWithZeroKeyHashTraits<uint64_t> since
we want to allow for 0 to be a valid displayID).
Use a HashSet in DisplayRefreshMonitor for the client set, so that we don't have to
worry about adding clients twice.
Also fix a possible crash when the only client of a DisplayRefreshMonitor
was removed from inside the callback by making DisplayRefreshMonitor ref-counted,
with a protector.
Test: fast/animation/request-animation-frame-detach-element2.html
* platform/graphics/DisplayRefreshMonitor.cpp:
(WebCore::DisplayRefreshMonitor::DisplayRefreshMonitor): Initialize m_unscheduledFireCount
(WebCore::DisplayRefreshMonitor::handleDisplayRefreshedNotificationOnMainThread): Renamed
from refreshDisplayOnMainThread, since it doesn't just refresh the display.
(WebCore::DisplayRefreshMonitor::addClient): No longer inline.
(WebCore::DisplayRefreshMonitor::removeClient): No longer inline.
(WebCore::DisplayRefreshMonitor::displayDidRefresh): Keep track of m_unscheduledFireCount,
which we used to kill this monitor if it has been idle for a while.
Use a RefPtr<DisplayRefreshMonitor> to prevent deletion while running the callback.
Copy the clients to a vector to protect against mutating the set while enumerating it.
Notify the DisplayRefreshMonitorManager when we're done, so that it can decide
to delete inactive monitors.
(WebCore::DisplayRefreshMonitorManager::ensureMonitorForClient): find or allocate
a DisplayRefreshMonitor for a given client.
(WebCore::DisplayRefreshMonitorManager::registerClient): Simplified by use of HashMap.
(WebCore::DisplayRefreshMonitorManager::unregisterClient): Ditto.
(WebCore::DisplayRefreshMonitorManager::scheduleAnimation): Ditto. Uses
ensureMonitorForClient() since an earlier inactive monitor may have been removed.
(WebCore::DisplayRefreshMonitorManager::displayDidRefresh): Remove inactive
monitors.
* platform/graphics/DisplayRefreshMonitor.h: Make DisplayRefreshMonitor
ref-counted, to make it easier to avoid deletion while it's on the stack.
(WebCore::DisplayRefreshMonitor::create):
(DisplayRefreshMonitor):
(WebCore::DisplayRefreshMonitor::shouldBeTerminated):
(DisplayRefreshMonitorManager):
* platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp: Attempt to
keep things building.
(WebCore::DisplayRefreshMonitor::~DisplayRefreshMonitor):
(WebCore::DisplayRefreshMonitor::displayLinkFired):
to give the manager a chance to kill this monitor.
* platform/graphics/mac/DisplayRefreshMonitorMac.cpp:
(WebCore::DisplayRefreshMonitor::~DisplayRefreshMonitor):
(WebCore::DisplayRefreshMonitor::displayLinkFired): Don't bail early
if not scheduled; we want to call handleDisplayRefreshedNotificationOnMainThread(),
LayoutTests: CVDisplayLink keeps running after a single requestAnimationFrame
https://bugs.webkit.org/show_bug.cgi?id=86174
Reviewed by Sam Weinig.
Test removing a frame inside its requestAnimationFrame callback.
* fast/animation/request-animation-frame-detach-element2-expected.txt: Added.
* fast/animation/request-animation-frame-detach-element2.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (116791 => 116792)
--- trunk/LayoutTests/ChangeLog 2012-05-11 18:50:41 UTC (rev 116791)
+++ trunk/LayoutTests/ChangeLog 2012-05-11 18:56:32 UTC (rev 116792)
@@ -1,3 +1,15 @@
+2012-05-11 Simon Fraser <simon.fra...@apple.com>
+
+ CVDisplayLink keeps running after a single requestAnimationFrame
+ https://bugs.webkit.org/show_bug.cgi?id=86174
+
+ Reviewed by Sam Weinig.
+
+ Test removing a frame inside its requestAnimationFrame callback.
+
+ * fast/animation/request-animation-frame-detach-element2-expected.txt: Added.
+ * fast/animation/request-animation-frame-detach-element2.html: Added.
+
2012-05-11 Ian Vollick <voll...@chromium.org>
[chromium] Ensure that animations continue to run when transform-style is changed
Added: trunk/LayoutTests/fast/animation/request-animation-frame-detach-element2-expected.txt (0 => 116792)
--- trunk/LayoutTests/fast/animation/request-animation-frame-detach-element2-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/animation/request-animation-frame-detach-element2-expected.txt 2012-05-11 18:56:32 UTC (rev 116792)
@@ -0,0 +1 @@
+Test passes is there is no crash.
Property changes on: trunk/LayoutTests/fast/animation/request-animation-frame-detach-element2-expected.txt
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/fast/animation/request-animation-frame-detach-element2.html (0 => 116792)
--- trunk/LayoutTests/fast/animation/request-animation-frame-detach-element2.html (rev 0)
+++ trunk/LayoutTests/fast/animation/request-animation-frame-detach-element2.html 2012-05-11 18:56:32 UTC (rev 116792)
@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<script>
+if (window.layoutTestController) {
+ layoutTestController.dumpAsText();
+ layoutTestController.waitUntilDone();
+}
+window._onload_ = function() {
+ var el = document.getElementsByTagName("iframe")[0];
+ window.frames[0].webkitRequestAnimationFrame(function() {
+ el.parentNode.removeChild(el);
+ });
+
+ if (window.layoutTestController) {
+ window.setTimeout(function() {
+ layoutTestController.display();
+ layoutTestController.notifyDone();
+ }, 50);
+ }
+}
+</script>
+Test passes is there is no crash.
+<iframe></iframe>
+<iframe></iframe>
Property changes on: trunk/LayoutTests/fast/animation/request-animation-frame-detach-element2.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Modified: trunk/Source/WebCore/ChangeLog (116791 => 116792)
--- trunk/Source/WebCore/ChangeLog 2012-05-11 18:50:41 UTC (rev 116791)
+++ trunk/Source/WebCore/ChangeLog 2012-05-11 18:56:32 UTC (rev 116792)
@@ -1,3 +1,70 @@
+2012-05-11 Simon Fraser <simon.fra...@apple.com>
+
+ CVDisplayLink keeps running after a single requestAnimationFrame
+ https://bugs.webkit.org/show_bug.cgi?id=86174
+
+ Reviewed by Sam Weinig.
+
+ A DisplayRefreshMonitor would keep its CVDisplayLink alive for as long
+ as it had clients, and the client is the ScriptedAnimationController, which
+ lives on the document. So a single requestAnimationFrame call would kick
+ off a CVDisplayLink which lived until the document was destroyed.
+
+ Fix by having the DisplayRefreshMonitor kill itself if the CVDisplayLink
+ fires for 10 times with no scheduled callbacks (to avoid creation/deletion
+ thrash on pages that call requestAnimationFrames with short setTimeouts,
+ as some do).
+
+ Use a HashMap in DisplayRefreshMonitorManager for the set of DisplayRefreshMonitor,
+ with the displayID as the key (using UnsignedWithZeroKeyHashTraits<uint64_t> since
+ we want to allow for 0 to be a valid displayID).
+
+ Use a HashSet in DisplayRefreshMonitor for the client set, so that we don't have to
+ worry about adding clients twice.
+
+ Also fix a possible crash when the only client of a DisplayRefreshMonitor
+ was removed from inside the callback by making DisplayRefreshMonitor ref-counted,
+ with a protector.
+
+ Test: fast/animation/request-animation-frame-detach-element2.html
+
+ * platform/graphics/DisplayRefreshMonitor.cpp:
+ (WebCore::DisplayRefreshMonitor::DisplayRefreshMonitor): Initialize m_unscheduledFireCount
+ (WebCore::DisplayRefreshMonitor::handleDisplayRefreshedNotificationOnMainThread): Renamed
+ from refreshDisplayOnMainThread, since it doesn't just refresh the display.
+ (WebCore::DisplayRefreshMonitor::addClient): No longer inline.
+ (WebCore::DisplayRefreshMonitor::removeClient): No longer inline.
+ (WebCore::DisplayRefreshMonitor::displayDidRefresh): Keep track of m_unscheduledFireCount,
+ which we used to kill this monitor if it has been idle for a while.
+ Use a RefPtr<DisplayRefreshMonitor> to prevent deletion while running the callback.
+ Copy the clients to a vector to protect against mutating the set while enumerating it.
+ Notify the DisplayRefreshMonitorManager when we're done, so that it can decide
+ to delete inactive monitors.
+
+ (WebCore::DisplayRefreshMonitorManager::ensureMonitorForClient): find or allocate
+ a DisplayRefreshMonitor for a given client.
+ (WebCore::DisplayRefreshMonitorManager::registerClient): Simplified by use of HashMap.
+ (WebCore::DisplayRefreshMonitorManager::unregisterClient): Ditto.
+ (WebCore::DisplayRefreshMonitorManager::scheduleAnimation): Ditto. Uses
+ ensureMonitorForClient() since an earlier inactive monitor may have been removed.
+ (WebCore::DisplayRefreshMonitorManager::displayDidRefresh): Remove inactive
+ monitors.
+ * platform/graphics/DisplayRefreshMonitor.h: Make DisplayRefreshMonitor
+ ref-counted, to make it easier to avoid deletion while it's on the stack.
+ (WebCore::DisplayRefreshMonitor::create):
+ (DisplayRefreshMonitor):
+ (WebCore::DisplayRefreshMonitor::shouldBeTerminated):
+ (DisplayRefreshMonitorManager):
+ * platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp: Attempt to
+ keep things building.
+ (WebCore::DisplayRefreshMonitor::~DisplayRefreshMonitor):
+ (WebCore::DisplayRefreshMonitor::displayLinkFired):
+ to give the manager a chance to kill this monitor.
+ * platform/graphics/mac/DisplayRefreshMonitorMac.cpp:
+ (WebCore::DisplayRefreshMonitor::~DisplayRefreshMonitor):
+ (WebCore::DisplayRefreshMonitor::displayLinkFired): Don't bail early
+ if not scheduled; we want to call handleDisplayRefreshedNotificationOnMainThread(),
+
2012-05-11 Pavel Feldman <pfeld...@chromium.org>
Web Inspector: move canEditScriptSource and setScriptSource from DebuggerPresentationModel into ResourceBinding
Modified: trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp (116791 => 116792)
--- trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp 2012-05-11 18:50:41 UTC (rev 116791)
+++ trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp 2012-05-11 18:56:32 UTC (rev 116792)
@@ -57,6 +57,7 @@
, m_active(true)
, m_scheduled(false)
, m_previousFrameDone(true)
+ , m_unscheduledFireCount(0)
, m_displayID(displayID)
#if PLATFORM(MAC)
, m_displayLink(0)
@@ -67,28 +68,56 @@
{
}
-void DisplayRefreshMonitor::refreshDisplayOnMainThread(void* data)
+void DisplayRefreshMonitor::handleDisplayRefreshedNotificationOnMainThread(void* data)
{
DisplayRefreshMonitor* monitor = static_cast<DisplayRefreshMonitor*>(data);
- monitor->notifyClients();
+ monitor->displayDidRefresh();
}
-void DisplayRefreshMonitor::notifyClients()
+void DisplayRefreshMonitor::addClient(DisplayRefreshMonitorClient* client)
{
+ m_clients.add(client);
+}
+
+bool DisplayRefreshMonitor::removeClient(DisplayRefreshMonitorClient* client)
+{
+ DisplayRefreshMonitorClientSet::iterator it = m_clients.find(client);
+ if (it != m_clients.end()) {
+ m_clients.remove(it);
+ return true;
+ }
+ return false;
+}
+
+void DisplayRefreshMonitor::displayDidRefresh()
+{
double timestamp;
{
MutexLocker lock(m_mutex);
+ if (!m_scheduled)
+ ++m_unscheduledFireCount;
+ else
+ m_unscheduledFireCount = 0;
+
m_scheduled = false;
timestamp = m_timestamp;
}
- for (size_t i = 0; i < m_clients.size(); ++i)
- m_clients[i]->fireDisplayRefreshIfNeeded(timestamp);
+ // 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)
+ clients[i]->fireDisplayRefreshIfNeeded(timestamp);
{
MutexLocker lock(m_mutex);
m_previousFrameDone = true;
}
+
+ DisplayRefreshMonitorManager::sharedManager()->displayDidRefresh(this);
}
DisplayRefreshMonitorManager* DisplayRefreshMonitorManager::sharedManager()
@@ -97,13 +126,18 @@
return &manager;
}
-size_t DisplayRefreshMonitorManager::findMonitor(PlatformDisplayID displayID) const
+DisplayRefreshMonitor* DisplayRefreshMonitorManager::ensureMonitorForClient(DisplayRefreshMonitorClient* client)
{
- for (size_t i = 0; i < m_monitors.size(); ++i)
- if (m_monitors[i]->displayID() == displayID)
- return i;
-
- return notFound;
+ DisplayRefreshMonitorMap::iterator it = m_monitors.find(client->m_displayID);
+ if (it == m_monitors.end()) {
+ RefPtr<DisplayRefreshMonitor> monitor = DisplayRefreshMonitor::create(client->m_displayID);
+ monitor->addClient(client);
+ DisplayRefreshMonitor* result = monitor.get();
+ m_monitors.add(client->m_displayID, monitor.release());
+ return result;
+ }
+
+ return it->second.get();
}
void DisplayRefreshMonitorManager::registerClient(DisplayRefreshMonitorClient* client)
@@ -111,33 +145,22 @@
if (!client->m_displayIDIsSet)
return;
- size_t index = findMonitor(client->m_displayID);
- DisplayRefreshMonitor* monitor;
-
- if (index == notFound) {
- monitor = new DisplayRefreshMonitor(client->m_displayID);
- m_monitors.append(monitor);
- } else
- monitor = m_monitors[index];
-
- monitor->addClient(client);
+ ensureMonitorForClient(client);
}
void DisplayRefreshMonitorManager::unregisterClient(DisplayRefreshMonitorClient* client)
{
if (!client->m_displayIDIsSet)
return;
-
- size_t index = findMonitor(client->m_displayID);
- if (index == notFound)
+
+ DisplayRefreshMonitorMap::iterator it = m_monitors.find(client->m_displayID);
+ if (it == m_monitors.end())
return;
- DisplayRefreshMonitor* monitor = m_monitors[index];
+ DisplayRefreshMonitor* monitor = it->second.get();
if (monitor->removeClient(client)) {
- if (!monitor->hasClients()) {
- m_monitors.remove(index);
- delete monitor;
- }
+ if (!monitor->hasClients())
+ m_monitors.remove(it);
}
}
@@ -146,13 +169,21 @@
if (!client->m_displayIDIsSet)
return false;
- size_t i = findMonitor(client->m_displayID);
- ASSERT(i != notFound);
+ DisplayRefreshMonitor* monitor = ensureMonitorForClient(client);
client->m_scheduled = true;
- return m_monitors[i]->requestRefreshCallback();
+ return monitor->requestRefreshCallback();
}
+void DisplayRefreshMonitorManager::displayDidRefresh(DisplayRefreshMonitor* monitor)
+{
+ if (monitor->shouldBeTerminated()) {
+ DisplayRefreshMonitorMap::iterator it = m_monitors.find(monitor->displayID());
+ ASSERT(it != m_monitors.end());
+ m_monitors.remove(it);
+ }
+}
+
void DisplayRefreshMonitorManager::windowScreenDidChange(PlatformDisplayID displayID, DisplayRefreshMonitorClient* client)
{
if (client->m_displayIDIsSet && client->m_displayID == displayID)
Modified: trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h (116791 => 116792)
--- trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2012-05-11 18:50:41 UTC (rev 116791)
+++ trunk/Source/WebCore/platform/graphics/DisplayRefreshMonitor.h 2012-05-11 18:56:32 UTC (rev 116792)
@@ -29,8 +29,11 @@
#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
#include "PlatformScreen.h"
+#include <wtf/HashMap.h>
+#include <wtf/HashSet.h>
+#include <wtf/RefCounted.h>
+#include <wtf/RefPtr.h>
#include <wtf/Threading.h>
-#include <wtf/Vector.h>
#if PLATFORM(BLACKBERRY)
#include <BlackBerryPlatformAnimation.h>
#endif
@@ -86,9 +89,13 @@
// Monitor for display refresh messages for a given screen
//
-class DisplayRefreshMonitor {
+class DisplayRefreshMonitor : public RefCounted<DisplayRefreshMonitor> {
public:
- DisplayRefreshMonitor(PlatformDisplayID);
+ static PassRefPtr<DisplayRefreshMonitor> create(PlatformDisplayID displayID)
+ {
+ return adoptRef(new DisplayRefreshMonitor(displayID));
+ }
+
~DisplayRefreshMonitor();
// Return true if callback request was scheduled, false if it couldn't be
@@ -97,30 +104,33 @@
void windowScreenDidChange(PlatformDisplayID);
bool hasClients() const { return m_clients.size(); }
- void addClient(DisplayRefreshMonitorClient* client) { m_clients.append(client); }
- bool removeClient(DisplayRefreshMonitorClient* client)
+ void addClient(DisplayRefreshMonitorClient*);
+ bool removeClient(DisplayRefreshMonitorClient*);
+
+ PlatformDisplayID displayID() const { return m_displayID; }
+
+ bool shouldBeTerminated() const
{
- size_t i = m_clients.find(client);
- if (i == notFound)
- return false;
- m_clients.remove(i);
- return true;
+ const int maxInactiveFireCount = 10;
+ return !m_scheduled && m_unscheduledFireCount > maxInactiveFireCount;
}
- PlatformDisplayID displayID() const { return m_displayID; }
-
private:
- void notifyClients();
- static void refreshDisplayOnMainThread(void* data);
+ DisplayRefreshMonitor(PlatformDisplayID);
+ void displayDidRefresh();
+ static void handleDisplayRefreshedNotificationOnMainThread(void* data);
+
double m_timestamp;
bool m_active;
bool m_scheduled;
bool m_previousFrameDone;
+ int m_unscheduledFireCount; // Number of times the display link has fired with no clients.
PlatformDisplayID m_displayID;
- DisplayRefreshMonitorManager* m_manager;
Mutex m_mutex;
- Vector<DisplayRefreshMonitorClient*> m_clients;
+
+ typedef HashSet<DisplayRefreshMonitorClient*> DisplayRefreshMonitorClientSet;
+ DisplayRefreshMonitorClientSet m_clients;
#if PLATFORM(BLACKBERRY)
public:
void displayLinkFired();
@@ -154,11 +164,15 @@
void windowScreenDidChange(PlatformDisplayID, DisplayRefreshMonitorClient*);
private:
+ friend class DisplayRefreshMonitor;
+ void displayDidRefresh(DisplayRefreshMonitor*);
+
DisplayRefreshMonitorManager() { }
+ DisplayRefreshMonitor* ensureMonitorForClient(DisplayRefreshMonitorClient*);
- size_t findMonitor(PlatformDisplayID) const;
-
- Vector<DisplayRefreshMonitor*> m_monitors;
+ // 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;
+ DisplayRefreshMonitorMap m_monitors;
};
}
Modified: trunk/Source/WebCore/platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp (116791 => 116792)
--- trunk/Source/WebCore/platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp 2012-05-11 18:50:41 UTC (rev 116791)
+++ trunk/Source/WebCore/platform/graphics/blackberry/DisplayRefreshMonitorBlackBerry.cpp 2012-05-11 18:56:32 UTC (rev 116792)
@@ -40,7 +40,7 @@
DisplayRefreshMonitor::~DisplayRefreshMonitor()
{
stopAnimationClient();
- cancelCallOnMainThread(DisplayRefreshMonitor::refreshDisplayOnMainThread, this);
+ cancelCallOnMainThread(DisplayRefreshMonitor::handleDisplayRefreshedNotificationOnMainThread, this);
}
void DisplayRefreshMonitor::startAnimationClient()
@@ -83,7 +83,7 @@
m_timestamp = currentTime();
- callOnMainThread(refreshDisplayOnMainThread, this);
+ callOnMainThread(handleDisplayRefreshedNotificationOnMainThread, this);
}
}
Modified: trunk/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp (116791 => 116792)
--- trunk/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp 2012-05-11 18:50:41 UTC (rev 116791)
+++ trunk/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp 2012-05-11 18:56:32 UTC (rev 116792)
@@ -54,7 +54,7 @@
m_displayLink = 0;
}
- cancelCallOnMainThread(DisplayRefreshMonitor::refreshDisplayOnMainThread, this);
+ cancelCallOnMainThread(DisplayRefreshMonitor::handleDisplayRefreshedNotificationOnMainThread, this);
}
bool DisplayRefreshMonitor::requestRefreshCallback()
@@ -87,7 +87,7 @@
void DisplayRefreshMonitor::displayLinkFired(double nowSeconds, double outputTimeSeconds)
{
MutexLocker lock(m_mutex);
- if (!m_scheduled || !m_previousFrameDone)
+ if (!m_previousFrameDone)
return;
m_previousFrameDone = false;
@@ -95,7 +95,7 @@
double webKitNow = currentTime();
m_timestamp = webKitNow - nowSeconds + outputTimeSeconds;
- callOnMainThread(refreshDisplayOnMainThread, this);
+ callOnMainThread(handleDisplayRefreshedNotificationOnMainThread, this);
}
}
