Throttled DOMTimers can prevent their document from being garbage collected
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Nov 2014 03:29:58 +0000 (03:29 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Nov 2014 03:29:58 +0000 (03:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=138915

Reviewed by Andreas Kling.

Source/WebCore:

Throttled DOMTimers whose interval depend on viewport changes would
keep a Vector of elements outside viewport causing them to be throttled
so that we could check later on (upon scroll or layout) if those
elements are still outside viewport. The issue is that these elements
could potentially be removed from the document (and destroyed) after
the timer has fired. To handle this, DOMTimer was ref'ing the
elements. Unfortunately, this was causing us to leak the document
as the elements in the Vector would keep the document alive.

To handle this issue, this patch updates the DOMTimer Vector to use
weak pointers. The WeakPtrFactory is stored in ElementRareData to
avoid wasting memory for all kinds of Elements (it is a fair assumption
that the number of elements whose style is animated via timers is low).

Test: fast/dom/throttled-timer-running-on-document-destruction.html

* dom/Element.cpp:
(WebCore::Element::createWeakPtr):
* dom/Element.h:
* dom/ElementRareData.cpp:
* dom/ElementRareData.h:
(WebCore::ElementRareData::weakPtrFactory):
* page/DOMTimer.cpp:
(WebCore::DOMTimerFireState::elementsChangedOutsideViewport):
(WebCore::DOMTimer::updateThrottlingStateAfterViewportChange):
* page/DOMTimer.h:

LayoutTests:

Improve fast/dom/throttled-timer-running-on-document-destruction.html
layout test to cover the case where the throttled timer is changing the
style of an element on the *same* document when the document is
destroyed.

* fast/dom/resources/frame-with-throttled-timer-animating-element-other-document.html: Renamed from LayoutTests/fast/dom/resources/frame-with-throttled-timer.html.
* fast/dom/resources/frame-with-throttled-timer-animating-element-same-document.html: Added.
* fast/dom/throttled-timer-running-on-document-destruction.html:

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/resources/frame-with-throttled-timer-animating-element-other-document.html [moved from LayoutTests/fast/dom/resources/frame-with-throttled-timer.html with 98% similarity]
LayoutTests/fast/dom/resources/frame-with-throttled-timer-animating-element-same-document.html [new file with mode: 0644]
LayoutTests/fast/dom/throttled-timer-running-on-document-destruction.html
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/dom/ElementRareData.cpp
Source/WebCore/dom/ElementRareData.h
Source/WebCore/dom/Node.cpp
Source/WebCore/page/DOMTimer.cpp
Source/WebCore/page/DOMTimer.h

index 63e0f88..452a05f 100644 (file)
@@ -1,5 +1,21 @@
 2014-11-21  Chris Dumez  <cdumez@apple.com>
 
+        Throttled DOMTimers can prevent their document from being garbage collected
+        https://bugs.webkit.org/show_bug.cgi?id=138915
+
+        Reviewed by Andreas Kling.
+
+        Improve fast/dom/throttled-timer-running-on-document-destruction.html
+        layout test to cover the case where the throttled timer is changing the
+        style of an element on the *same* document when the document is
+        destroyed.
+
+        * fast/dom/resources/frame-with-throttled-timer-animating-element-other-document.html: Renamed from LayoutTests/fast/dom/resources/frame-with-throttled-timer.html.
+        * fast/dom/resources/frame-with-throttled-timer-animating-element-same-document.html: Added.
+        * fast/dom/throttled-timer-running-on-document-destruction.html:
+
+2014-11-21  Chris Dumez  <cdumez@apple.com>
+
         [iOS] Regression(r176202): line-height is wrong on marco.org
         https://bugs.webkit.org/show_bug.cgi?id=138970
 
@@ -9,6 +9,6 @@ setInterval(function() {
   var testFrame = document.getElementById("testFrame");
   var testElement = testFrame.contentDocument.getElementById("testElement");
   testElement.style["left"] = "" + Math.floor((Math.random() * 10) + 1) + "px";
-}, 5);
+}, 0);
 </script>
 </body>
diff --git a/LayoutTests/fast/dom/resources/frame-with-throttled-timer-animating-element-same-document.html b/LayoutTests/fast/dom/resources/frame-with-throttled-timer-animating-element-same-document.html
new file mode 100644 (file)
index 0000000..a6437fd
--- /dev/null
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<body>
+<div id='testElement' style='display: none'>TEST</div>
+<script>
+setInterval(function() {
+  // Change the style of a display:none element.
+  var testElement = document.getElementById("testElement");
+  testElement.style["left"] = "" + Math.floor((Math.random() * 10) + 1) + "px";
+}, 0);
+</script>
+</body>
index b28fde4..cdf4b9c 100644 (file)
@@ -1,22 +1,31 @@
 <!DOCTYPE html>
 <body>
 <script src="../../resources/js-test-pre.js"></script>
-<iframe id="testFrame" src="resources/frame-with-throttled-timer.html"></iframe>
-
 <script>
 description("Test that we don't crash if a throttled timer is still running when the document is destroyed.");
 jsTestIsAsync = true;
 
-function removeFrame()
+var frameLoadedCount = 0;
+
+function removeFrames()
 {
-  document.body.removeChild(document.getElementById("testFrame"));
+  document.body.removeChild(document.getElementById("testFrame1"));
+  document.body.removeChild(document.getElementById("testFrame2"));
   gc();
   testPassed("Did not crash.");
   finishJSTest();
 }
 
-setTimeout(removeFrame, 300);
+function frameLoaded()
+{
+  ++frameLoadedCount;
+  if (frameLoadedCount == 2)
+    setTimeout(removeFrames, 100);
+}
 
 </script>
+<iframe id="testFrame1" src="resources/frame-with-throttled-timer-animating-element-same-document.html" onload="frameLoaded()"></iframe>
+<iframe id="testFrame2" src="resources/frame-with-throttled-timer-animating-element-other-document.html" onload="frameLoaded()"></iframe>
+
 <script src="../../resources/js-test-post.js"></script>
 </body>
index 9596dd9..5f6182c 100644 (file)
@@ -1,3 +1,37 @@
+2014-11-21  Chris Dumez  <cdumez@apple.com>
+
+        Throttled DOMTimers can prevent their document from being garbage collected
+        https://bugs.webkit.org/show_bug.cgi?id=138915
+
+        Reviewed by Andreas Kling.
+
+        Throttled DOMTimers whose interval depend on viewport changes would
+        keep a Vector of elements outside viewport causing them to be throttled
+        so that we could check later on (upon scroll or layout) if those
+        elements are still outside viewport. The issue is that these elements
+        could potentially be removed from the document (and destroyed) after
+        the timer has fired. To handle this, DOMTimer was ref'ing the
+        elements. Unfortunately, this was causing us to leak the document
+        as the elements in the Vector would keep the document alive.
+
+        To handle this issue, this patch updates the DOMTimer Vector to use
+        weak pointers. The WeakPtrFactory is stored in ElementRareData to
+        avoid wasting memory for all kinds of Elements (it is a fair assumption
+        that the number of elements whose style is animated via timers is low).
+
+        Test: fast/dom/throttled-timer-running-on-document-destruction.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::createWeakPtr):
+        * dom/Element.h:
+        * dom/ElementRareData.cpp:
+        * dom/ElementRareData.h:
+        (WebCore::ElementRareData::weakPtrFactory):
+        * page/DOMTimer.cpp:
+        (WebCore::DOMTimerFireState::elementsChangedOutsideViewport):
+        (WebCore::DOMTimer::updateThrottlingStateAfterViewportChange):
+        * page/DOMTimer.h:
+
 2014-11-21  Gyuyoung Kim  <gyuyoung.kim@samsung.com>
 
         Unreviewed, EFL build fix since r176459.
index 1675a1d..370e6a1 100644 (file)
@@ -1195,6 +1195,11 @@ URL Element::absoluteLinkURL() const
     return document().completeURL(stripLeadingAndTrailingHTMLSpaces(linkAttribute));
 }
 
+WeakPtr<Element> Element::createWeakPtr()
+{
+    return ensureElementRareData().weakPtrFactory().createWeakPtr();
+}
+
 // Returns true is the given attribute is an event handler.
 // We consider an event handler any attribute that begins with "on".
 // It is a simple solution that has the advantage of not requiring any
index 951f466..12e13eb 100644 (file)
@@ -551,6 +551,7 @@ public:
     void clearHoverAndActiveStatusBeforeDetachingRenderer();
 
     WEBCORE_EXPORT URL absoluteLinkURL() const;
+    WeakPtr<Element> createWeakPtr();
 
 protected:
     Element(const QualifiedName&, Document&, ConstructionType);
index 3b1cd3c..b2af703 100644 (file)
@@ -39,7 +39,7 @@ struct SameSizeAsElementRareData : NodeRareData {
     RegionOversetState regionOversetState;
     LayoutSize sizeForResizing;
     IntSize scrollOffset;
-    void* pointers[7];
+    void* pointers[8];
 };
 
 static_assert(sizeof(ElementRareData) == sizeof(SameSizeAsElementRareData), "ElementRareData should stay small");
index da29109..9d9873a 100644 (file)
@@ -36,7 +36,7 @@ namespace WebCore {
 
 class ElementRareData : public NodeRareData {
 public:
-    explicit ElementRareData(RenderElement*);
+    ElementRareData(Element&, RenderElement*);
     ~ElementRareData();
 
     void setBeforePseudoElement(PassRefPtr<PseudoElement>);
@@ -114,6 +114,8 @@ public:
     bool hasPendingResources() const { return m_hasPendingResources; }
     void setHasPendingResources(bool has) { m_hasPendingResources = has; }
 
+    WeakPtrFactory<Element>& weakPtrFactory() { return m_weakPtrFactory; }
+
 private:
     short m_tabIndex;
     unsigned short m_childIndex;
@@ -147,6 +149,7 @@ private:
 
     RefPtr<PseudoElement> m_beforePseudoElement;
     RefPtr<PseudoElement> m_afterPseudoElement;
+    WeakPtrFactory<Element> m_weakPtrFactory;
 
     void releasePseudoElement(PseudoElement*);
 };
@@ -156,7 +159,7 @@ inline IntSize defaultMinimumSizeForResizing()
     return IntSize(LayoutUnit::max(), LayoutUnit::max());
 }
 
-inline ElementRareData::ElementRareData(RenderElement* renderer)
+inline ElementRareData::ElementRareData(Element& element, RenderElement* renderer)
     : NodeRareData(renderer)
     , m_tabIndex(0)
     , m_childIndex(0)
@@ -175,6 +178,7 @@ inline ElementRareData::ElementRareData(RenderElement* renderer)
     , m_childrenAffectedByPropertyBasedBackwardPositionalRules(false)
     , m_regionOversetState(RegionUndefined)
     , m_minimumSizeForResizing(defaultMinimumSizeForResizing())
+    , m_weakPtrFactory(&element)
 {
 }
 
index cac7ea1..0cc8ed3 100644 (file)
@@ -350,7 +350,7 @@ void Node::materializeRareData()
 {
     NodeRareData* data;
     if (is<Element>(*this))
-        data = std::make_unique<ElementRareData>(downcast<RenderElement>(m_data.m_renderer)).release();
+        data = std::make_unique<ElementRareData>(downcast<Element>(*this), downcast<RenderElement>(m_data.m_renderer)).release();
     else
         data = std::make_unique<NodeRareData>(m_data.m_renderer).release();
     ASSERT(data);
index b4362ce..7fac5b1 100644 (file)
@@ -99,9 +99,12 @@ public:
         return document && document->domTreeVersion() != m_initialDOMTreeVersion;
     }
 
-    void elementsChangedOutsideViewport(Vector<RefPtr<StyledElement>>& elements) const
+    void elementsChangedOutsideViewport(Vector<WeakPtr<Element>>& elements) const
     {
-        copyToVector(m_elementsChangedOutsideViewport, elements);
+        ASSERT(elements.isEmpty());
+        elements.reserveCapacity(m_elementsChangedOutsideViewport.size());
+        for (auto& element : m_elementsChangedOutsideViewport)
+            elements.uncheckedAppend(element->createWeakPtr());
     }
 
     static DOMTimerFireState* current;
@@ -471,9 +474,10 @@ void DOMTimer::updateThrottlingStateAfterViewportChange(const IntRect& visibleRe
 {
     ASSERT(isIntervalDependentOnViewport());
     // Check if the elements that caused this timer to be throttled are still outside the viewport.
-    for (auto& element : m_elementsCausingThrottling) {
+    for (auto& weakElementPtr : m_elementsCausingThrottling) {
+        Element* element = weakElementPtr.get();
         // Skip elements that were removed from the document.
-        if (!element->inDocument())
+        if (!element || !element->inDocument())
             continue;
 
         if (element->isInsideViewport(&visibleRect)) {
index 85b3762..e49fdb2 100644 (file)
 #include <memory>
 #include <wtf/HashSet.h>
 #include <wtf/RefCounted.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
 
     class DOMTimerFireState;
     class Document;
+    class Element;
     class HTMLPlugInElement;
     class IntRect;
     class ScheduledAction;
@@ -91,9 +93,10 @@ namespace WebCore {
         TimerThrottleState m_throttleState;
         double m_currentTimerInterval;
         bool m_shouldForwardUserGesture;
-        // Hold a reference to the elements in case they get removed from the
-        // Document after the timer is throttled.
-        Vector<RefPtr<StyledElement>> m_elementsCausingThrottling;
+        // Use WeakPtrs because we don't want to keep the elements alive but we
+        // still need to handle cases where the elements get destroyed after
+        // the timer has fired.
+        Vector<WeakPtr<Element>> m_elementsCausingThrottling;
     };
 
 } // namespace WebCore