⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 176496 in webkit


Ignore:
Timestamp:
Nov 21, 2014, 7:29:58 PM (12 years ago)
Author:
Chris Dumez
Message:

Throttled DOMTimers can prevent their document from being garbage collected
​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:
Location:
trunk
Files:
1 added
10 edited
1 moved

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r176490 r176496  
     12014-11-21  Chris Dumez  <cdumez@apple.com>
     2
     3        Throttled DOMTimers can prevent their document from being garbage collected
     4        https://bugs.webkit.org/show_bug.cgi?id=138915
     5
     6        Reviewed by Andreas Kling.
     7
     8        Improve fast/dom/throttled-timer-running-on-document-destruction.html
     9        layout test to cover the case where the throttled timer is changing the
     10        style of an element on the *same* document when the document is
     11        destroyed.
     12
     13        * fast/dom/resources/frame-with-throttled-timer-animating-element-other-document.html: Renamed from LayoutTests/fast/dom/resources/frame-with-throttled-timer.html.
     14        * fast/dom/resources/frame-with-throttled-timer-animating-element-same-document.html: Added.
     15        * fast/dom/throttled-timer-running-on-document-destruction.html:
     16
    1172014-11-21  Chris Dumez  <cdumez@apple.com>
    218

  • trunk/LayoutTests/fast/dom/resources/frame-with-throttled-timer-animating-element-other-document.html

    r176495 r176496  
    1010  var testElement = testFrame.contentDocument.getElementById("testElement");
    1111  testElement.style["left"] = "" + Math.floor((Math.random() * 10) + 1) + "px";
    12 }, 5);
     12}, 0);
    1313</script>
    1414</body>

  • trunk/LayoutTests/fast/dom/throttled-timer-running-on-document-destruction.html

    r176402 r176496  
    22<body>
    33<script src="../../resources/js-test-pre.js"></script>
    4 <iframe id="testFrame" src="resources/frame-with-throttled-timer.html"></iframe>
    5 
    64<script>
    75description("Test that we don't crash if a throttled timer is still running when the document is destroyed.");
    86jsTestIsAsync = true;
    97
    10 function removeFrame()
     8var frameLoadedCount = 0;
     9
     10function removeFrames()
    1111{
    12   document.body.removeChild(document.getElementById("testFrame"));
     12  document.body.removeChild(document.getElementById("testFrame1"));
     13  document.body.removeChild(document.getElementById("testFrame2"));
    1314  gc();
    1415  testPassed("Did not crash.");
    … …  
    1617}
    1718
    18 setTimeout(removeFrame, 300);
     19function frameLoaded()
     20{
     21  ++frameLoadedCount;
     22  if (frameLoadedCount == 2)
     23    setTimeout(removeFrames, 100);
     24}
    1925
    2026</script>
     27<iframe id="testFrame1" src="resources/frame-with-throttled-timer-animating-element-same-document.html" onload="frameLoaded()"></iframe>
     28<iframe id="testFrame2" src="resources/frame-with-throttled-timer-animating-element-other-document.html" onload="frameLoaded()"></iframe>
     29
    2130<script src="../../resources/js-test-post.js"></script>
    2231</body>

  • trunk/Source/WebCore/ChangeLog

    r176495 r176496  
     12014-11-21  Chris Dumez  <cdumez@apple.com>
     2
     3        Throttled DOMTimers can prevent their document from being garbage collected
     4        https://bugs.webkit.org/show_bug.cgi?id=138915
     5
     6        Reviewed by Andreas Kling.
     7
     8        Throttled DOMTimers whose interval depend on viewport changes would
     9        keep a Vector of elements outside viewport causing them to be throttled
     10        so that we could check later on (upon scroll or layout) if those
     11        elements are still outside viewport. The issue is that these elements
     12        could potentially be removed from the document (and destroyed) after
     13        the timer has fired. To handle this, DOMTimer was ref'ing the
     14        elements. Unfortunately, this was causing us to leak the document
     15        as the elements in the Vector would keep the document alive.
     16
     17        To handle this issue, this patch updates the DOMTimer Vector to use
     18        weak pointers. The WeakPtrFactory is stored in ElementRareData to
     19        avoid wasting memory for all kinds of Elements (it is a fair assumption
     20        that the number of elements whose style is animated via timers is low).
     21
     22        Test: fast/dom/throttled-timer-running-on-document-destruction.html
     23
     24        * dom/Element.cpp:
     25        (WebCore::Element::createWeakPtr):
     26        * dom/Element.h:
     27        * dom/ElementRareData.cpp:
     28        * dom/ElementRareData.h:
     29        (WebCore::ElementRareData::weakPtrFactory):
     30        * page/DOMTimer.cpp:
     31        (WebCore::DOMTimerFireState::elementsChangedOutsideViewport):
     32        (WebCore::DOMTimer::updateThrottlingStateAfterViewportChange):
     33        * page/DOMTimer.h:
     34
    1352014-11-21  Gyuyoung Kim  <gyuyoung.kim@samsung.com>
    236

  • trunk/Source/WebCore/dom/Element.cpp

    r176212 r176496  
    11961196}
    11971197
     1198WeakPtr<Element> Element::createWeakPtr()
     1199{
     1200    return ensureElementRareData().weakPtrFactory().createWeakPtr();
     1201}
     1202
    11981203// Returns true is the given attribute is an event handler.
    11991204// We consider an event handler any attribute that begins with "on".

  • trunk/Source/WebCore/dom/Element.h

    r176212 r176496  
    552552
    553553    WEBCORE_EXPORT URL absoluteLinkURL() const;
     554    WeakPtr<Element> createWeakPtr();
    554555
    555556protected:

  • trunk/Source/WebCore/dom/ElementRareData.cpp

    r173234 r176496  
    4040    LayoutSize sizeForResizing;
    4141    IntSize scrollOffset;
    42     void* pointers[7];
     42    void* pointers[8];
    4343};
    4444

  • trunk/Source/WebCore/dom/ElementRareData.h

    r176084 r176496  
    3737class ElementRareData : public NodeRareData {
    3838public:
    39     explicit ElementRareData(RenderElement*);
     39    ElementRareData(Element&, RenderElement*);
    4040    ~ElementRareData();
    4141
    … …  
    114114    bool hasPendingResources() const { return m_hasPendingResources; }
    115115    void setHasPendingResources(bool has) { m_hasPendingResources = has; }
     116
     117    WeakPtrFactory<Element>& weakPtrFactory() { return m_weakPtrFactory; }
    116118
    117119private:
    … …  
    148150    RefPtr<PseudoElement> m_beforePseudoElement;
    149151    RefPtr<PseudoElement> m_afterPseudoElement;
     152    WeakPtrFactory<Element> m_weakPtrFactory;
    150153
    151154    void releasePseudoElement(PseudoElement*);
    … …  
    157160}
    158161
    159 inline ElementRareData::ElementRareData(RenderElement* renderer)
     162inline ElementRareData::ElementRareData(Element& element, RenderElement* renderer)
    160163    : NodeRareData(renderer)
    161164    , m_tabIndex(0)
    … …  
    176179    , m_regionOversetState(RegionUndefined)
    177180    , m_minimumSizeForResizing(defaultMinimumSizeForResizing())
     181    , m_weakPtrFactory(&element)
    178182{
    179183}

  • trunk/Source/WebCore/dom/Node.cpp

    r176084 r176496  
    351351    NodeRareData* data;
    352352    if (is<Element>(*this))
    353         data = std::make_unique<ElementRareData>(downcast<RenderElement>(m_data.m_renderer)).release();
     353        data = std::make_unique<ElementRareData>(downcast<Element>(*this), downcast<RenderElement>(m_data.m_renderer)).release();
    354354    else
    355355        data = std::make_unique<NodeRareData>(m_data.m_renderer).release();

  • trunk/Source/WebCore/page/DOMTimer.cpp

    r176402 r176496  
    100100    }
    101101
    102     void elementsChangedOutsideViewport(Vector<RefPtr<StyledElement>>& elements) const
    103     {
    104         copyToVector(m_elementsChangedOutsideViewport, elements);
     102    void elementsChangedOutsideViewport(Vector<WeakPtr<Element>>& elements) const
     103    {
     104        ASSERT(elements.isEmpty());
     105        elements.reserveCapacity(m_elementsChangedOutsideViewport.size());
     106        for (auto& element : m_elementsChangedOutsideViewport)
     107            elements.uncheckedAppend(element->createWeakPtr());
    105108    }
    106109
    … …  
    472475    ASSERT(isIntervalDependentOnViewport());
    473476    // Check if the elements that caused this timer to be throttled are still outside the viewport.
    474     for (auto& element : m_elementsCausingThrottling) {
     477    for (auto& weakElementPtr : m_elementsCausingThrottling) {
     478        Element* element = weakElementPtr.get();
    475479        // Skip elements that were removed from the document.
    476         if (!element->inDocument())
     480        if (!element || !element->inDocument())
    477481            continue;
    478482

  • trunk/Source/WebCore/page/DOMTimer.h

    r176297 r176496  
    3232#include <wtf/HashSet.h>
    3333#include <wtf/RefCounted.h>
     34#include <wtf/WeakPtr.h>
    3435
    3536namespace WebCore {
    … …  
    3738    class DOMTimerFireState;
    3839    class Document;
     40    class Element;
    3941    class HTMLPlugInElement;
    4042    class IntRect;
    … …  
    9294        double m_currentTimerInterval;
    9395        bool m_shouldForwardUserGesture;
    94         // Hold a reference to the elements in case they get removed from the
    95         // Document after the timer is throttled.
    96         Vector<RefPtr<StyledElement>> m_elementsCausingThrottling;
     96        // Use WeakPtrs because we don't want to keep the elements alive but we
     97        // still need to handle cases where the elements get destroyed after
     98        // the timer has fired.
     99        Vector<WeakPtr<Element>> m_elementsCausingThrottling;
    97100    };
    98101
Note: See TracChangeset for help on using the changeset viewer.