Changeset 100307 in webkit


Ignore:
Timestamp:
Nov 15, 2011 12:13:53 PM (13 years ago)
Author:
commit-queue@webkit.org
Message:

Source/WebCore: Event listener for active DOM object that is also DOM node can be garbage collected prematurely.
​https://bugs.webkit.org/show_bug.cgi?id=70421

Patch by Eugene Nalimov <​enal@google.com> on 2011-11-15
Reviewed by Adam Barth.

Problem demonstrated itself when HTMLAudioElement was changed to become active DOM object.
Before that there were no DOM objects that simultaneously were nodes and active objects.
DOM object could be held in one of 3 maps -- node map, active objects map, and all other
objects map, and HTMLAudioElement should be in 2 maps simultaneously. When it was in the
active DOM objects map only, its event listener could be garbage collected, because special
code that groups listeners with wrappers could handle only wrappers for objects in the node
map. If we put HTMLAudioElement into nodes map, it would not be active DOM node, and can be
garbage collected prematurely itself (see ​https://bugs.webkit.org/show_bug.cgi?id=66878).
Fix is to introduce 4th map -- active nodes map, and change the code accordingly.

Test: media/audio-garbage-collect.html

  • bindings/scripts/CodeGeneratorV8.pm:

(GenerateNamedConstructorCallback):
(GetDomMapFunction):

  • bindings/v8/DOMDataStore.cpp:

(WebCore::DOMDataStore::DOMDataStore):
(WebCore::DOMDataStore::getDOMWrapperMap):
(WebCore::DOMDataStore::weakNodeCallback):

  • bindings/v8/DOMDataStore.h:

(WebCore::DOMDataStore::activeDomNodeMap):

  • bindings/v8/ScopedDOMDataStore.cpp:

(WebCore::ScopedDOMDataStore::ScopedDOMDataStore):
(WebCore::ScopedDOMDataStore::~ScopedDOMDataStore):

  • bindings/v8/StaticDOMDataStore.cpp:

(WebCore::StaticDOMDataStore::StaticDOMDataStore):

  • bindings/v8/StaticDOMDataStore.h:
  • bindings/v8/V8DOMMap.cpp:

(WebCore::getActiveDOMNodeMap):
(WebCore::removeAllDOMObjects):
(WebCore::visitActiveDOMNodes):

  • bindings/v8/V8DOMMap.h:
  • bindings/v8/V8DOMWrapper.cpp:

(WebCore::V8DOMWrapper::setJSWrapperForDOMNode):
(WebCore::V8DOMWrapper::getWrapperSlow):

  • bindings/v8/V8GCController.cpp:

(WebCore::GCPrologueSpecialCase):
(WebCore::void):
(WebCore::Node):
(WebCore::GCPrologueVisitor::visitDOMWrapper):
(WebCore::V8GCController::gcPrologue):
(WebCore::GCEpilogueHelper::GCEpilogueSpecialCase):
(WebCore::GCEpilogueVisitor::visitDOMWrapper):
(WebCore::V8GCController::gcEpilogue):

  • dom/Node.h:

(WebCore::Node::isActiveNode):

  • html/HTMLAudioElement.h:

(WebCore::HTMLAudioElement::isActiveNode):

LayoutTests: Event listener for active DOM object that is also DOM node can be garbage collected prematurely.
​https://bugs.webkit.org/show_bug.cgi?id=70421 and ​https://bugs.webkit.org/show_bug.cgi?id=66878

Patch by Eugene Nalimov <​enal@google.com> on 2011-11-15
Reviewed by Adam Barth.

  • media/audio-garbage-collect-expected.txt: Added.
  • media/audio-garbage-collect.html: Added.
Location:
trunk
Files:
2 added
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r100305 r100307  
     12011-11-15  Eugene Nalimov  <enal@google.com>
     2
     3        Event listener for active DOM object that is also DOM node can be garbage collected prematurely.
     4        https://bugs.webkit.org/show_bug.cgi?id=70421 and https://bugs.webkit.org/show_bug.cgi?id=66878
     5
     6        Reviewed by Adam Barth.
     7
     8        * media/audio-garbage-collect-expected.txt: Added.
     9        * media/audio-garbage-collect.html: Added.
     10
    1112011-11-15  Robert Hogan  <robert@webkit.org>
    212

  • trunk/Source/WebCore/ChangeLog

    r100298 r100307  
     12011-11-15  Eugene Nalimov  <enal@google.com>
     2
     3        Event listener for active DOM object that is also DOM node can be garbage collected prematurely.
     4        https://bugs.webkit.org/show_bug.cgi?id=70421
     5
     6        Reviewed by Adam Barth.
     7
     8        Problem demonstrated itself when HTMLAudioElement was changed to become active DOM object.
     9        Before that there were no DOM objects that simultaneously were nodes and active objects.
     10        DOM object could be held in one of 3 maps -- node map, active objects map, and all other
     11        objects map, and HTMLAudioElement should be in 2 maps simultaneously. When it was in the
     12        active DOM objects map only, its event listener could be garbage collected, because special
     13        code that groups listeners with wrappers could handle only wrappers for objects in the node
     14        map. If we put HTMLAudioElement into nodes map, it would not be active DOM node, and can be
     15        garbage collected prematurely itself (see https://bugs.webkit.org/show_bug.cgi?id=66878).
     16        Fix is to introduce 4th map -- active nodes map, and change the code accordingly.
     17
     18        Test: media/audio-garbage-collect.html
     19
     20        * bindings/scripts/CodeGeneratorV8.pm:
     21        (GenerateNamedConstructorCallback):
     22        (GetDomMapFunction):
     23        * bindings/v8/DOMDataStore.cpp:
     24        (WebCore::DOMDataStore::DOMDataStore):
     25        (WebCore::DOMDataStore::getDOMWrapperMap):
     26        (WebCore::DOMDataStore::weakNodeCallback):
     27        * bindings/v8/DOMDataStore.h:
     28        (WebCore::DOMDataStore::activeDomNodeMap):
     29        * bindings/v8/ScopedDOMDataStore.cpp:
     30        (WebCore::ScopedDOMDataStore::ScopedDOMDataStore):
     31        (WebCore::ScopedDOMDataStore::~ScopedDOMDataStore):
     32        * bindings/v8/StaticDOMDataStore.cpp:
     33        (WebCore::StaticDOMDataStore::StaticDOMDataStore):
     34        * bindings/v8/StaticDOMDataStore.h:
     35        * bindings/v8/V8DOMMap.cpp:
     36        (WebCore::getActiveDOMNodeMap):
     37        (WebCore::removeAllDOMObjects):
     38        (WebCore::visitActiveDOMNodes):
     39        * bindings/v8/V8DOMMap.h:
     40        * bindings/v8/V8DOMWrapper.cpp:
     41        (WebCore::V8DOMWrapper::setJSWrapperForDOMNode):
     42        (WebCore::V8DOMWrapper::getWrapperSlow):
     43        * bindings/v8/V8GCController.cpp:
     44        (WebCore::GCPrologueSpecialCase):
     45        (WebCore::void):
     46        (WebCore::Node):
     47        (WebCore::GCPrologueVisitor::visitDOMWrapper):
     48        (WebCore::V8GCController::gcPrologue):
     49        (WebCore::GCEpilogueHelper::GCEpilogueSpecialCase):
     50        (WebCore::GCEpilogueVisitor::visitDOMWrapper):
     51        (WebCore::V8GCController::gcEpilogue):
     52        * dom/Node.h:
     53        (WebCore::Node::isActiveNode):
     54        * html/HTMLAudioElement.h:
     55        (WebCore::HTMLAudioElement::isActiveNode):
     56
    1572011-11-15  David Kilzer  <ddkilzer@apple.com>
    258

  • trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm

    r100229 r100307  
    17781778
    17791779    my $DOMObject = "DOMObject";
    1780     # A DOMObject that is an ActiveDOMObject and also a DOMNode should be treated as an ActiveDOMObject.
    1781     if ($dataNode->extendedAttributes->{"ActiveDOMObject"}) {
     1780    # A DOMObject that is an ActiveDOMObject and also a DOMNode should be treated as an DOMNode here.
     1781    # setJSWrapperForDOMNode() will look if node is active and choose correct map to add node to.
     1782    if (IsNodeSubType($dataNode)) {
     1783        $DOMObject = "DOMNode";
     1784    } elsif ($dataNode->extendedAttributes->{"ActiveDOMObject"}) {
    17821785        $DOMObject = "ActiveDOMObject";
    1783     } elsif (IsNodeSubType($dataNode)) {
    1784         $DOMObject = "DOMNode";
    17851786    }
    17861787    push(@implContent, <<END);
    … …  
    31063107    my $type = shift;
    31073108    return "getDOMSVGElementInstanceMap()" if $type eq "SVGElementInstance";
     3109    return "getActiveDOMNodeMap()" if (IsNodeSubType($dataNode) && $dataNode->extendedAttributes->{"ActiveDOMObject"});
    31083110    return "getDOMNodeMap()" if (IsNodeSubType($dataNode));
    31093111    return "getActiveDOMObjectMap()" if $dataNode->extendedAttributes->{"ActiveDOMObject"};

  • trunk/Source/WebCore/bindings/v8/DOMDataStore.cpp

    r95901 r100307  
    8787DOMDataStore::DOMDataStore()
    8888    : m_domNodeMap(0)
     89    , m_activeDomNodeMap(0)
    8990    , m_domObjectMap(0)
    9091    , m_activeDomObjectMap(0)
    … …  
    109110    case DOMNodeMap:
    110111        return m_domNodeMap;
     112    case ActiveDOMNodeMap:
     113        return m_activeDomNodeMap;
    111114    case DOMObjectMap:
    112115        return m_domObjectMap;
    … …  
    150153    for (size_t i = 0; i < list.size(); ++i) {
    151154        DOMDataStore* store = list[i];
    152         if (store->domNodeMap().removeIfPresent(node, v8Object)) {
     155        DOMNodeMapping& nodeMap = node->isActiveNode() ? store->activeDomNodeMap() : store->domNodeMap();
     156        if (nodeMap.removeIfPresent(node, v8Object)) {
    153157            node->deref(); // Nobody overrides Node::deref so it's safe
    154158            return; // There might be at most one wrapper for the node in world's maps

  • trunk/Source/WebCore/bindings/v8/DOMDataStore.h

    r95901 r100307  
    6666        enum DOMWrapperMapType {
    6767            DOMNodeMap,
     68            ActiveDOMNodeMap,
    6869            DOMObjectMap,
    6970            ActiveDOMObjectMap,
    … …  
    8283
    8384        DOMNodeMapping& domNodeMap() { return *m_domNodeMap; }
     85        DOMNodeMapping& activeDomNodeMap() { return *m_activeDomNodeMap; }
    8486        DOMWrapperMap<void>& domObjectMap() { return *m_domObjectMap; }
    8587        DOMWrapperMap<void>& activeDomObjectMap() { return *m_activeDomObjectMap; }
    … …  
    9092        // Need by V8GCController.
    9193        static void weakActiveDOMObjectCallback(v8::Persistent<v8::Value> v8Object, void* domObject);
     94        static void weakNodeCallback(v8::Persistent<v8::Value> v8Object, void* domObject);
    9295
    9396    protected:
    94         static void weakNodeCallback(v8::Persistent<v8::Value> v8Object, void* domObject);
    9597        static void weakDOMObjectCallback(v8::Persistent<v8::Value> v8Object, void* domObject);
    9698#if ENABLE(SVG)
    … …  
    99101       
    100102        DOMNodeMapping* m_domNodeMap;
     103        DOMNodeMapping* m_activeDomNodeMap;
    101104        DOMWrapperMap<void>* m_domObjectMap;
    102105        DOMWrapperMap<void>* m_activeDomObjectMap;

  • trunk/Source/WebCore/bindings/v8/ScopedDOMDataStore.cpp

    r95901 r100307  
    3838{
    3939    m_domNodeMap = new DOMWrapperMap<Node>(&DOMDataStore::weakNodeCallback);
     40    m_activeDomNodeMap = new DOMWrapperMap<Node>(&DOMDataStore::weakNodeCallback);
    4041    m_domObjectMap = new DOMWrapperMap<void>(&DOMDataStore::weakDOMObjectCallback);
    4142    m_activeDomObjectMap = new DOMWrapperMap<void>(&DOMDataStore::weakActiveDOMObjectCallback);
    … …  
    4849{
    4950    delete m_domNodeMap;
     51    delete m_activeDomNodeMap;
    5052    delete m_domObjectMap;
    5153    delete m_activeDomObjectMap;

  • trunk/Source/WebCore/bindings/v8/StaticDOMDataStore.cpp

    r95901 r100307  
    3838    : DOMDataStore()
    3939    , m_staticDomNodeMap(&DOMDataStore::weakNodeCallback)
     40    , m_staticActiveDomNodeMap(&DOMDataStore::weakNodeCallback)
    4041    , m_staticDomObjectMap(&DOMDataStore::weakDOMObjectCallback)
    4142    , m_staticActiveDomObjectMap(&DOMDataStore::weakActiveDOMObjectCallback)
    … …  
    4546{
    4647    m_domNodeMap = &m_staticDomNodeMap;
     48    m_activeDomNodeMap = &m_staticActiveDomNodeMap;
    4749    m_domObjectMap = &m_staticDomObjectMap;
    4850    m_activeDomObjectMap = &m_staticActiveDomObjectMap;

  • trunk/Source/WebCore/bindings/v8/StaticDOMDataStore.h

    r95901 r100307  
    5252private:
    5353    IntrusiveDOMWrapperMap m_staticDomNodeMap;
     54    IntrusiveDOMWrapperMap m_staticActiveDomNodeMap;
    5455    DOMWrapperMap<void> m_staticDomObjectMap;
    5556    DOMWrapperMap<void> m_staticActiveDomObjectMap;

  • trunk/Source/WebCore/bindings/v8/V8DOMMap.cpp

    r95901 r100307  
    6565}
    6666
     67DOMNodeMapping& getActiveDOMNodeMap()
     68{
     69    return getDOMDataStore().activeDomNodeMap();
     70}
     71
    6772DOMWrapperMap<void>& getDOMObjectMap()
    6873{
    … …  
    95100        DOMData::removeObjectsFromWrapperMap<Node>(&store, store.domNodeMap());
    96101
     102        // Remove all active DOM nodes.
     103        DOMData::removeObjectsFromWrapperMap<Node>(&store, store.activeDomNodeMap());
     104
    97105#if ENABLE(SVG)
    98106        // Remove all SVG element instances in the wrapper map.
    … …  
    117125
    118126        store->domNodeMap().visit(store, visitor);
     127    }
     128}
     129
     130void visitActiveDOMNodes(DOMWrapperMap<Node>::Visitor* visitor)
     131{
     132    v8::HandleScope scope;
     133
     134    DOMDataList& list = DOMDataStore::allStores();
     135    for (size_t i = 0; i < list.size(); ++i) {
     136        DOMDataStore* store = list[i];
     137
     138        store->activeDomNodeMap().visit(store, visitor);
    119139    }
    120140}

  • trunk/Source/WebCore/bindings/v8/V8DOMMap.h

    r95901 r100307  
    159159    // A map from DOM node to its JS wrapper.
    160160    DOMNodeMapping& getDOMNodeMap();
     161    DOMNodeMapping& getActiveDOMNodeMap();
    161162    void visitDOMNodes(DOMWrapperMap<Node>::Visitor*);
     163    void visitActiveDOMNodes(DOMWrapperMap<Node>::Visitor*);
    162164
    163165    // A map from a DOM object (non-node) to its JS wrapper. This map does not contain the DOM objects which can have pending activity (active dom objects).

  • trunk/Source/WebCore/bindings/v8/V8DOMWrapper.cpp

    r98426 r100307  
    9393{
    9494    ASSERT(V8DOMWrapper::maybeDOMWrapper(wrapper));
    95     getDOMNodeMap().set(node, wrapper);
     95    if (node->isActiveNode())
     96        getActiveDOMNodeMap().set(node, wrapper);
     97    else
     98        getDOMNodeMap().set(node, wrapper);
    9699}
    97100
    … …  
    296299        return *wrapper;
    297300    }
    298     DOMNodeMapping& domNodeMap = context->world()->domDataStore()->domNodeMap();
     301    DOMDataStore* store = context->world()->domDataStore();
     302    DOMNodeMapping& domNodeMap = node->isActiveNode() ? store->activeDomNodeMap() : store->domNodeMap();
    299303    return domNodeMap.get(node);
    300304}

  • trunk/Source/WebCore/bindings/v8/V8GCController.cpp

    r97557 r100307  
    144144#endif // NDEBUG
    145145
    146 class GCPrologueVisitor : public DOMWrapperMap<void>::Visitor {
    147 public:
    148     void visitDOMWrapper(DOMDataStore* store, void* object, v8::Persistent<v8::Object> wrapper)
    149     {
    150         WrapperTypeInfo* typeInfo = V8DOMWrapper::domWrapperType(wrapper); 
    151 
     146class SpecialCasePrologueObjectHandler {
     147public:
     148    static bool process(void* object, v8::Persistent<v8::Object> wrapper, WrapperTypeInfo* typeInfo)
     149    {
    152150        // Additional handling of message port ensuring that entangled ports also
    153151        // have their wrappers entangled. This should ideally be handled when the
    … …  
    162160            if (port1->isEntangled() || port1->hasPendingActivity())
    163161                wrapper.ClearWeak();
    164         } else {
     162            return true;
     163        }
     164        return false;
     165    }
     166};
     167
     168class SpecialCasePrologueNodeHandler {
     169public:
     170    static bool process(Node* object, v8::Persistent<v8::Object> wrapper, WrapperTypeInfo* typeInfo)
     171    {
     172        UNUSED_PARAM(object);
     173        UNUSED_PARAM(wrapper);
     174        UNUSED_PARAM(typeInfo);
     175        return false;
     176    }
     177};
     178
     179template<typename T, typename S>
     180class GCPrologueVisitor : public DOMWrapperMap<T>::Visitor {
     181public:
     182    void visitDOMWrapper(DOMDataStore* store, T* object, v8::Persistent<v8::Object> wrapper)
     183    {
     184        WrapperTypeInfo* typeInfo = V8DOMWrapper::domWrapperType(wrapper); 
     185
     186        if (!S::process(object, wrapper, typeInfo)) {
    165187            ActiveDOMObject* activeDOMObject = typeInfo->toActiveDOMObject(wrapper);
    166188            if (activeDOMObject && activeDOMObject->hasPendingActivity())
    … …  
    374396    // Run through all objects with possible pending activity making their
    375397    // wrappers non weak if there is pending activity.
    376     GCPrologueVisitor prologueVisitor;
    377     visitActiveDOMObjects(&prologueVisitor);
     398    GCPrologueVisitor<void, SpecialCasePrologueObjectHandler> prologueObjectVisitor;
     399    visitActiveDOMObjects(&prologueObjectVisitor);
     400    GCPrologueVisitor<Node, SpecialCasePrologueNodeHandler> prologueNodeVisitor;
     401    visitActiveDOMNodes(&prologueNodeVisitor);
    378402
    379403    // Create object groups.
    380404    GrouperVisitor grouperVisitor;
    381405    visitDOMNodes(&grouperVisitor);
     406    visitActiveDOMNodes(&grouperVisitor);
    382407    visitDOMObjects(&grouperVisitor);
    383408    grouperVisitor.applyGrouping();
    … …  
    388413}
    389414
    390 class GCEpilogueVisitor : public DOMWrapperMap<void>::Visitor {
    391 public:
    392     void visitDOMWrapper(DOMDataStore* store, void* object, v8::Persistent<v8::Object> wrapper)
    393     {
    394         WrapperTypeInfo* typeInfo = V8DOMWrapper::domWrapperType(wrapper);
     415class SpecialCaseEpilogueObjectHandler {
     416public:
     417    static bool process(void* object, v8::Persistent<v8::Object> wrapper, WrapperTypeInfo* typeInfo)
     418    {
    395419        if (V8MessagePort::info.equals(typeInfo)) {
    396420            MessagePort* port1 = static_cast<MessagePort*>(object);
    … …  
    400424            if ((!wrapper.IsWeak() && !wrapper.IsNearDeath()) || port1->hasPendingActivity())
    401425                wrapper.MakeWeak(port1, &DOMDataStore::weakActiveDOMObjectCallback);
    402         } else {
     426            return true;
     427        }
     428        return false;
     429    }
     430};
     431
     432class SpecialCaseEpilogueNodeHandler {
     433public:
     434    static bool process(Node* object, v8::Persistent<v8::Object> wrapper, WrapperTypeInfo* typeInfo)
     435    {
     436        UNUSED_PARAM(object);
     437        UNUSED_PARAM(wrapper);
     438        UNUSED_PARAM(typeInfo);
     439        return false;
     440    }
     441};
     442
     443template<typename T, typename S, v8::WeakReferenceCallback callback>
     444class GCEpilogueVisitor : public DOMWrapperMap<T>::Visitor {
     445public:
     446    void visitDOMWrapper(DOMDataStore* store, T* object, v8::Persistent<v8::Object> wrapper)
     447    {
     448        WrapperTypeInfo* typeInfo = V8DOMWrapper::domWrapperType(wrapper);
     449        if (!S::process(object, wrapper, typeInfo)) {
    403450            ActiveDOMObject* activeDOMObject = typeInfo->toActiveDOMObject(wrapper);
    404451            if (activeDOMObject && activeDOMObject->hasPendingActivity()) {
    … …  
    409456                // the main base class of the object's class) and pointer
    410457                // identity is required by DOM map functions.
    411                 wrapper.MakeWeak(object, &DOMDataStore::weakActiveDOMObjectCallback);
     458                wrapper.MakeWeak(object, callback);
    412459            }
    413460        }
    … …  
    445492    // Run through all objects with pending activity making their wrappers weak
    446493    // again.
    447     GCEpilogueVisitor epilogueVisitor;
    448     visitActiveDOMObjects(&epilogueVisitor);
     494    GCEpilogueVisitor<void, SpecialCaseEpilogueObjectHandler, &DOMDataStore::weakActiveDOMObjectCallback> epilogueObjectVisitor;
     495    visitActiveDOMObjects(&epilogueObjectVisitor);
     496    GCEpilogueVisitor<Node, SpecialCaseEpilogueNodeHandler, &DOMDataStore::weakNodeCallback> epilogueNodeVisitor;
     497    visitActiveDOMNodes(&epilogueNodeVisitor);
    449498
    450499    workingSetEstimateMB = getActualMemoryUsageInMB();

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

    r99992 r100307  
    191191    Node* lastDescendant() const;
    192192    Node* firstDescendant() const;
     193
     194    virtual bool isActiveNode() const { return false; }
    193195   
    194196    // Other methods (not part of DOM)

  • trunk/Source/WebCore/html/HTMLAudioElement.h

    r98005 r100307  
    4343    virtual bool hasPendingActivity() const { return isPlaying() || HTMLMediaElement::hasPendingActivity(); }
    4444
     45    virtual bool isActiveNode() const { return true; }
     46
    4547private:
    4648    HTMLAudioElement(const QualifiedName&, Document*);
Note: See TracChangeset for help on using the changeset viewer.