Source/WebCore: Improve handling of frame removal during requestAnimationFrame callba...
authorjamesr@google.com <jamesr@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Dec 2011 01:33:41 +0000 (01:33 +0000)
committerjamesr@google.com <jamesr@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Dec 2011 01:33:41 +0000 (01:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=74036

Reviewed by Adam Barth.

See bug for details.

Test: fast/animation/request-animation-frame-detach-element.html

* dom/Document.cpp:
(WebCore::Document::removedLastRef):
(WebCore::Document::detach):
* dom/Document.h:
* dom/ScriptedAnimationController.cpp:
(WebCore::ScriptedAnimationController::~ScriptedAnimationController):
(WebCore::ScriptedAnimationController::serviceScriptedAnimations):
(WebCore::ScriptedAnimationController::scheduleAnimation):
* dom/ScriptedAnimationController.h:
(WebCore::ScriptedAnimationController::create):
(WebCore::ScriptedAnimationController::clearDocumentPointer):
* page/FrameView.cpp:
(WebCore::FrameView::serviceScriptedAnimations):

LayoutTests: Add some tests for removing frames from the document while servicing requestAnimationFrame callbacks
https://bugs.webkit.org/show_bug.cgi?id=74036

Reviewed by Adam Barth.

* fast/animation/request-animation-frame-detach-element-expected.txt: Added.
* fast/animation/request-animation-frame-detach-element.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/animation/request-animation-frame-detach-element-expected.txt [new file with mode: 0644]
LayoutTests/fast/animation/request-animation-frame-detach-element.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/dom/ScriptedAnimationController.cpp
Source/WebCore/dom/ScriptedAnimationController.h
Source/WebCore/page/FrameView.cpp

index 1bae626..e8f7766 100644 (file)
@@ -1,3 +1,13 @@
+2011-12-08  James Robinson  <jamesr@chromium.org>
+
+        Add some tests for removing frames from the document while servicing requestAnimationFrame callbacks
+        https://bugs.webkit.org/show_bug.cgi?id=74036
+
+        Reviewed by Adam Barth.
+
+        * fast/animation/request-animation-frame-detach-element-expected.txt: Added.
+        * fast/animation/request-animation-frame-detach-element.html: Added.
+
 2011-12-08  Jacob Goldstein  <jacobg@adobe.com>
 
         Convert some fast/regions pixel tests to reftests
diff --git a/LayoutTests/fast/animation/request-animation-frame-detach-element-expected.txt b/LayoutTests/fast/animation/request-animation-frame-detach-element-expected.txt
new file mode 100644 (file)
index 0000000..797a749
--- /dev/null
@@ -0,0 +1 @@
+Test passes is there is no crash.  
diff --git a/LayoutTests/fast/animation/request-animation-frame-detach-element.html b/LayoutTests/fast/animation/request-animation-frame-detach-element.html
new file mode 100644 (file)
index 0000000..2acd468
--- /dev/null
@@ -0,0 +1,25 @@
+<!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);
+    });
+    window.frames[1].webkitRequestAnimationFrame(function() {
+    });
+
+    if (window.layoutTestController) {
+        window.setTimeout(function() {
+            layoutTestController.display();
+            layoutTestController.notifyDone();
+        }, 50);
+    }
+}
+</script>
+Test passes is there is no crash.
+<iframe></iframe>
+<iframe></iframe>
index 0cea301..82a293f 100644 (file)
@@ -1,3 +1,28 @@
+2011-12-08  James Robinson  <jamesr@chromium.org>
+
+        Improve handling of frame removal during requestAnimationFrame callback invocation
+        https://bugs.webkit.org/show_bug.cgi?id=74036
+
+        Reviewed by Adam Barth.
+
+        See bug for details.
+
+        Test: fast/animation/request-animation-frame-detach-element.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::removedLastRef):
+        (WebCore::Document::detach):
+        * dom/Document.h:
+        * dom/ScriptedAnimationController.cpp:
+        (WebCore::ScriptedAnimationController::~ScriptedAnimationController):
+        (WebCore::ScriptedAnimationController::serviceScriptedAnimations):
+        (WebCore::ScriptedAnimationController::scheduleAnimation):
+        * dom/ScriptedAnimationController.h:
+        (WebCore::ScriptedAnimationController::create):
+        (WebCore::ScriptedAnimationController::clearDocumentPointer):
+        * page/FrameView.cpp:
+        (WebCore::FrameView::serviceScriptedAnimations):
+
 2011-12-08  Yongjun Zhang  <yongjun_zhang@apple.com>
 
         Use bitfield for bool data members in BitmapImage.
index a59b7a6..7085e1a 100644 (file)
@@ -613,7 +613,9 @@ void Document::removedLastRef()
 
 #if ENABLE(REQUEST_ANIMATION_FRAME)
         // FIXME: consider using ActiveDOMObject.
-        m_scriptedAnimationController = nullptr;
+        if (m_scriptedAnimationController)
+            m_scriptedAnimationController->clearDocumentPointer();
+        m_scriptedAnimationController.clear();
 #endif
 
 #ifndef NDEBUG
@@ -1834,7 +1836,9 @@ void Document::detach()
 
 #if ENABLE(REQUEST_ANIMATION_FRAME)
     // FIXME: consider using ActiveDOMObject.
-    m_scriptedAnimationController = nullptr;
+    if (m_scriptedAnimationController)
+        m_scriptedAnimationController->clearDocumentPointer();
+    m_scriptedAnimationController.clear();
 #endif
 
     RenderObject* render = renderer();
index 43c7ebd..f4d0bba 100644 (file)
@@ -1441,7 +1441,7 @@ private:
     unsigned m_wheelEventHandlerCount;
 
 #if ENABLE(REQUEST_ANIMATION_FRAME)
-    OwnPtr<ScriptedAnimationController> m_scriptedAnimationController;
+    RefPtr<ScriptedAnimationController> m_scriptedAnimationController;
 #endif
 
     Timer<Document> m_pendingTasksTimer;
index 3e50a88..8875f55 100644 (file)
@@ -61,6 +61,10 @@ ScriptedAnimationController::ScriptedAnimationController(Document* document, Pla
     windowScreenDidChange(displayID);
 }
 
+ScriptedAnimationController::~ScriptedAnimationController()
+{
+}
+
 void ScriptedAnimationController::suspend()
 {
     ++m_suspendCount;
@@ -119,8 +123,17 @@ void ScriptedAnimationController::serviceScriptedAnimations(DOMTimeStamp time)
     // missing any callbacks, we keep iterating through the list of candiate callbacks and firing
     // them until nothing new becomes visible.
     bool firedCallback;
+
+    // Invoking callbacks may detach elements from our document, which clear's the document's
+    // reference to us, so take a defensive reference.
+    RefPtr<ScriptedAnimationController> protector(this);
     do {
         firedCallback = false;
+        // A previous iteration may have detached this Document from the DOM tree.
+        // If so, then we do not need to process any more callbacks.
+        if (!m_document)
+            continue;
+
         // A previous iteration may have invalidated style (or layout).  Update styles for each iteration
         // for now since all we check is the existence of a renderer.
         m_document->updateStyleIfNeeded();
@@ -161,6 +174,9 @@ void ScriptedAnimationController::windowScreenDidChange(PlatformDisplayID displa
 
 void ScriptedAnimationController::scheduleAnimation()
 {
+    if (!m_document)
+        return;
+
 #if USE(REQUEST_ANIMATION_FRAME_TIMER)
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
     if (!m_useTimer) {
index dfdd503..d2c9c0c 100644 (file)
@@ -35,8 +35,7 @@
 #include "Timer.h"
 #endif
 #include "PlatformScreen.h"
-#include <wtf/Noncopyable.h>
-#include <wtf/PassOwnPtr.h>
+#include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
 #include <wtf/Vector.h>
 
@@ -46,17 +45,18 @@ class Document;
 class Element;
 class RequestAnimationFrameCallback;
 
-class ScriptedAnimationController
+class ScriptedAnimationController : public RefCounted<ScriptedAnimationController>
 #if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
-    : public DisplayRefreshMonitorClient
+    , public DisplayRefreshMonitorClient
 #endif
 {
-WTF_MAKE_NONCOPYABLE(ScriptedAnimationController);
 public:
-    static PassOwnPtr<ScriptedAnimationController> create(Document* document, PlatformDisplayID displayID)
+    static PassRefPtr<ScriptedAnimationController> create(Document* document, PlatformDisplayID displayID)
     {
-        return adoptPtr(new ScriptedAnimationController(document, displayID));
+        return adoptRef(new ScriptedAnimationController(document, displayID));
     }
+    ~ScriptedAnimationController();
+    void clearDocumentPointer() { m_document = 0; }
 
     typedef int CallbackId;
 
@@ -70,7 +70,7 @@ public:
     void windowScreenDidChange(PlatformDisplayID);
 
 private:
-    explicit ScriptedAnimationController(Document*, PlatformDisplayID);
+    ScriptedAnimationController(Document*, PlatformDisplayID);
     
     typedef Vector<RefPtr<RequestAnimationFrameCallback> > CallbackList;
     CallbackList m_callbacks;
index 876ea10..547a1c1 100644 (file)
@@ -2085,8 +2085,13 @@ void FrameView::unscheduleRelayout()
 #if ENABLE(REQUEST_ANIMATION_FRAME)
 void FrameView::serviceScriptedAnimations(DOMTimeStamp time)
 {
+    Vector<RefPtr<Document> > documents;
+
     for (Frame* frame = m_frame.get(); frame; frame = frame->tree()->traverseNext())
-        frame->document()->serviceScriptedAnimations(time);
+        documents.append(frame->document());
+
+    for (size_t i = 0; i < documents.size(); ++i)
+        documents[i]->serviceScriptedAnimations(time);
 }
 #endif