Deal with DOM modifications when evaluating source elements.
authoreric.carlson@apple.com <eric.carlson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2012 20:26:17 +0000 (20:26 +0000)
committereric.carlson@apple.com <eric.carlson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2012 20:26:17 +0000 (20:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=81163

Reviewed by Alexey Proskuryakov.

Source/WebCore:

Test: media/video-beforeload-remove-source.html

* dom/ContainerNode.cpp: Make NodeVector and collectNodes public, renamed as getChildNodes.
(WebCore::ContainerNode::takeAllChildrenFrom): collectNodes -> getChildNodes.
(WebCore::ContainerNode::willRemove): collectNodes -> getChildNodes.
(WebCore::ContainerNode::willRemoveChildren): collectNodes -> getChildNodes.
(WebCore::ContainerNode::insertedIntoDocument): collectNodes -> getChildNodes.
(WebCore::ContainerNode::removedFromDocument): collectNodes -> getChildNodes.
* dom/ContainerNode.h:
(WebCore::getChildNodes):

* editing/ReplaceSelectionCommand.cpp: Remove unused NodeVector declaration.

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::HTMLMediaElement): m_nextChildNodeToConsider and m_currentSourceNode
    are now RefPtrs.
(WebCore::HTMLMediaElement::loadTimerFired): Protect HTMLMediaElement from being deleted during
    a DOM modification during an event callback.
(WebCore::HTMLMediaElement::load): Ditto.
(WebCore::HTMLMediaElement::selectMediaResource): Set m_nextChildNodeToConsider to the first
    child node, it will be the first node considered.
(WebCore::HTMLMediaElement::havePotentialSourceChild): m_nextChildNodeToConsider and m_currentSourceNode
    are now RefPtrs.
(WebCore::HTMLMediaElement::selectNextSourceChild): Collect all child nodes in a vector before
    looking for <source> nodes because 'beforeload' event handlers can mutate the DOM. Don't
    use a <source> that is no longer a child node after 'beforeload'. Use 0 to represent the end
    of the child node list because m_nextChildNodeToConsider is now a RefPtr so using the previous
    sentinel, "this", would cause a retain cycle.
(WebCore::HTMLMediaElement::sourceWasAdded):  m_nextChildNodeToConsider and m_currentSourceNode
    are now RefPtrs.
(WebCore::HTMLMediaElement::sourceWillBeRemoved): Ditto.
(WebCore::HTMLMediaElement::getPluginProxyParams): Protect HTMLMediaElement from being deleted during
    a DOM modification during an event callback.
* html/HTMLMediaElement.h:

LayoutTests:

* media/video-beforeload-remove-source-expected.txt: Added.
* media/video-beforeload-remove-source.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/media/video-beforeload-remove-source-expected.txt [new file with mode: 0644]
LayoutTests/media/video-beforeload-remove-source.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.cpp
Source/WebCore/dom/ContainerNode.h
Source/WebCore/editing/ReplaceSelectionCommand.cpp
Source/WebCore/html/HTMLMediaElement.cpp
Source/WebCore/html/HTMLMediaElement.h

index 1716e08f698eb537da9cf00b155f33e39929f32a..77155262fd01dbacdc5ac46e513917bf5eb08954 100644 (file)
@@ -1,3 +1,13 @@
+2012-03-23  Eric Carlson  <eric.carlson@apple.com>
+
+        Deal with DOM modifications when evaluating source elements.
+        https://bugs.webkit.org/show_bug.cgi?id=81163
+
+        Reviewed by Alexey Proskuryakov.
+
+        * media/video-beforeload-remove-source-expected.txt: Added.
+        * media/video-beforeload-remove-source.html: Added.
+
 2012-03-23  Dan Bernstein  <mitz@apple.com>
 
         Added all tests that crashed more than once on the Lion WebKit2 bots between r111865
diff --git a/LayoutTests/media/video-beforeload-remove-source-expected.txt b/LayoutTests/media/video-beforeload-remove-source-expected.txt
new file mode 100644 (file)
index 0000000..0383162
--- /dev/null
@@ -0,0 +1 @@
+Test passes if it does not crash.
diff --git a/LayoutTests/media/video-beforeload-remove-source.html b/LayoutTests/media/video-beforeload-remove-source.html
new file mode 100644 (file)
index 0000000..fd60144
--- /dev/null
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <script src=../resources/gc.js></script>
+    </head>
+    <body>
+    <video controls autoplay>
+        <b id="start"></b>
+        <source src="content/test.mp4" type="video/mp4">
+        <source src="content/test.ogv" type="video/ogg">
+        <b id="end"></b>
+    </video> 
+    <div>Test passes if it does not crash.</div>
+    </body>
+    <script>
+        if (window.layoutTestController) {
+            layoutTestController.dumpAsText();
+            layoutTestController.waitUntilDone();
+        }
+        
+        function removeNodes(start, end) {
+            var range = document.createRange();
+            range.setStart(start, 0);
+            range.setEnd(end, 0);
+            range.deleteContents();
+        }
+        
+        function beforeLoadFunc()
+        {
+            document.removeEventListener("beforeload", beforeLoadFunc, true);
+            var start = document.getElementById("start");
+            var end = document.getElementById("end");
+            removeNodes(start, end);
+        
+            gc();
+            if (window.layoutTestController)
+                setTimeout("layoutTestController.notifyDone()", 0);
+        }
+        
+        document.addEventListener("beforeload", beforeLoadFunc, true);
+    </script>
+</html>
index df96c0236e80633c318e0f2cc13d344e27ec1df2..ee5ad7ec0138b46486e68ffd6ba3aa530cc7c6bd 100644 (file)
@@ -1,3 +1,45 @@
+2012-03-23  Eric Carlson  <eric.carlson@apple.com>
+
+        Deal with DOM modifications when evaluating source elements.
+        https://bugs.webkit.org/show_bug.cgi?id=81163
+
+        Reviewed by Alexey Proskuryakov.
+
+        Test: media/video-beforeload-remove-source.html
+
+        * dom/ContainerNode.cpp: Make NodeVector and collectNodes public, renamed as getChildNodes.
+        (WebCore::ContainerNode::takeAllChildrenFrom): collectNodes -> getChildNodes.
+        (WebCore::ContainerNode::willRemove): collectNodes -> getChildNodes.
+        (WebCore::ContainerNode::willRemoveChildren): collectNodes -> getChildNodes.
+        (WebCore::ContainerNode::insertedIntoDocument): collectNodes -> getChildNodes.
+        (WebCore::ContainerNode::removedFromDocument): collectNodes -> getChildNodes.
+        * dom/ContainerNode.h:
+        (WebCore::getChildNodes):
+
+        * editing/ReplaceSelectionCommand.cpp: Remove unused NodeVector declaration.
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::HTMLMediaElement): m_nextChildNodeToConsider and m_currentSourceNode
+            are now RefPtrs.
+        (WebCore::HTMLMediaElement::loadTimerFired): Protect HTMLMediaElement from being deleted during
+            a DOM modification during an event callback.
+        (WebCore::HTMLMediaElement::load): Ditto.
+        (WebCore::HTMLMediaElement::selectMediaResource): Set m_nextChildNodeToConsider to the first
+            child node, it will be the first node considered.
+        (WebCore::HTMLMediaElement::havePotentialSourceChild): m_nextChildNodeToConsider and m_currentSourceNode
+            are now RefPtrs.
+        (WebCore::HTMLMediaElement::selectNextSourceChild): Collect all child nodes in a vector before
+            looking for <source> nodes because 'beforeload' event handlers can mutate the DOM. Don't
+            use a <source> that is no longer a child node after 'beforeload'. Use 0 to represent the end
+            of the child node list because m_nextChildNodeToConsider is now a RefPtr so using the previous 
+            sentinel, "this", would cause a retain cycle.
+        (WebCore::HTMLMediaElement::sourceWasAdded):  m_nextChildNodeToConsider and m_currentSourceNode
+            are now RefPtrs.
+        (WebCore::HTMLMediaElement::sourceWillBeRemoved): Ditto.
+        (WebCore::HTMLMediaElement::getPluginProxyParams): Protect HTMLMediaElement from being deleted during
+            a DOM modification during an event callback.
+        * html/HTMLMediaElement.h:
+
 2012-03-23  Dean Jackson  <dino@apple.com>
 
         Disable CSS_SHADERS in Apple builds
index 5e8c738ee24bff468c5beb93788a296eec294458..d96d09810e3bbddd73951f5caa161c6a8365c178 100644 (file)
@@ -55,25 +55,18 @@ typedef pair<RefPtr<Node>, unsigned> CallbackParameters;
 typedef pair<NodeCallback, CallbackParameters> CallbackInfo;
 typedef Vector<CallbackInfo> NodeCallbackQueue;
 
-typedef Vector<RefPtr<Node>, 11> NodeVector;
 static NodeCallbackQueue* s_postAttachCallbackQueue;
 
 static size_t s_attachDepth;
 static bool s_shouldReEnableMemoryCacheCallsAfterAttach;
 
-static inline void collectNodes(Node* node, NodeVector& nodes)
-{
-    for (Node* child = node->firstChild(); child; child = child->nextSibling())
-        nodes.append(child);
-}
-
 static void collectTargetNodes(Node* node, NodeVector& nodes)
 {
     if (node->nodeType() != Node::DOCUMENT_FRAGMENT_NODE) {
         nodes.append(node);
         return;
     }
-    collectNodes(node, nodes);
+    getChildNodes(node, nodes);
 }
 
 void ContainerNode::removeAllChildren()
@@ -84,7 +77,7 @@ void ContainerNode::removeAllChildren()
 void ContainerNode::takeAllChildrenFrom(ContainerNode* oldParent)
 {
     NodeVector children;
-    collectNodes(oldParent, children);
+    getChildNodes(oldParent, children);
     oldParent->removeAllChildren();
 
     for (unsigned i = 0; i < children.size(); ++i) {
@@ -353,7 +346,7 @@ void ContainerNode::willRemove()
     RefPtr<Node> protect(this);
 
     NodeVector children;
-    collectNodes(this, children);
+    getChildNodes(this, children);
     for (size_t i = 0; i < children.size(); ++i) {
         if (children[i]->parentNode() != this) // Check for child being removed from subtree while removing.
             continue;
@@ -378,7 +371,7 @@ static void willRemoveChildren(ContainerNode* container)
     container->document()->nodeChildrenWillBeRemoved(container);
 
     NodeVector children;
-    collectNodes(container, children);
+    getChildNodes(container, children);
 
 #if ENABLE(MUTATION_OBSERVERS)
     ChildListMutationScope mutation(container);
@@ -757,7 +750,7 @@ void ContainerNode::insertedIntoDocument()
     insertedIntoTree(false);
 
     NodeVector children;
-    collectNodes(this, children);
+    getChildNodes(this, children);
     for (size_t i = 0; i < children.size(); ++i) {
         // If we have been removed from the document during this loop, then
         // we don't want to tell the rest of our children that they've been
@@ -778,7 +771,7 @@ void ContainerNode::removedFromDocument()
     removedFromTree(false);
 
     NodeVector children;
-    collectNodes(this, children);
+    getChildNodes(this, children);
     for (size_t i = 0; i < children.size(); ++i) {
         // If we have been added to the document during this loop, then we
         // don't want to tell the rest of our children that they've been
index e629e71eb789539fd9ce41bd3f606ac0795958e7..970836bd63871e018ad285d0d2037acb3094bcdb 100644 (file)
@@ -227,6 +227,15 @@ inline Node* Node::lastChild() const
     return toContainerNode(this)->lastChild();
 }
 
+typedef Vector<RefPtr<Node>, 11> NodeVector;
+
+inline void getChildNodes(Node* node, NodeVector& nodes)
+{
+    ASSERT(!nodes.size());
+    for (Node* child = node->firstChild(); child; child = child->nextSibling())
+        nodes.append(child);
+}
+
 } // namespace WebCore
 
 #endif // ContainerNode_h
index af5a8be13e1014c5b330ddb6db5c9d3eb07b2fd7..f36142c85b54ec67a3f2ef8d1d9d8cd8cc6bcddb 100644 (file)
@@ -60,8 +60,6 @@
 
 namespace WebCore {
 
-typedef Vector<RefPtr<Node> > NodeVector;
-
 using namespace HTMLNames;
 
 enum EFragmentType { EmptyFragment, SingleTextNodeFragment, TreeFragment };
index eed680850997f81d0bd5cfaf70ca17ec09b2ae69..b186d5f23b1d0871b1fa39597cb9146bbd83335e 100644 (file)
@@ -195,8 +195,6 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document* docum
     , m_lastTimeUpdateEventWallTime(0)
     , m_lastTimeUpdateEventMovieTime(numeric_limits<float>::max())
     , m_loadState(WaitingForSource)
-    , m_currentSourceNode(0)
-    , m_nextChildNodeToConsider(0)
 #if ENABLE(PLUGIN_PROXY_FOR_VIDEO)
     , m_proxyWidget(0)
 #endif
@@ -557,6 +555,8 @@ void HTMLMediaElement::scheduleEvent(const AtomicString& eventName)
 
 void HTMLMediaElement::loadTimerFired(Timer<HTMLMediaElement>*)
 {
+    RefPtr<HTMLMediaElement> protect(this); // loadNextSourceChild may fire 'beforeload', which can make arbitrary DOM mutations.
+
 #if ENABLE(VIDEO_TRACK)
     if (m_pendingLoadFlags & TextTrackResource)
         configureNewTextTracks();
@@ -613,6 +613,8 @@ String HTMLMediaElement::canPlayType(const String& mimeType) const
 
 void HTMLMediaElement::load(ExceptionCode& ec)
 {
+    RefPtr<HTMLMediaElement> protect(this); // loadInternal may result in a 'beforeload' event, which can make arbitrary DOM mutations.
+    
     LOG(Media, "HTMLMediaElement::load()");
 
     if (userGestureRequiredForLoad() && !ScriptController::processingUserGesture())
@@ -771,7 +773,7 @@ void HTMLMediaElement::selectMediaResource()
         // source element child in tree order.
         if (node) {
             mode = children;
-            m_nextChildNodeToConsider = 0;
+            m_nextChildNodeToConsider = node;
             m_currentSourceNode = 0;
         } else {
             // Otherwise the media element has neither a src attribute nor a source element 
@@ -2730,8 +2732,8 @@ bool HTMLMediaElement::havePotentialSourceChild()
 {
     // Stash the current <source> node and next nodes so we can restore them after checking
     // to see there is another potential.
-    HTMLSourceElement* currentSourceNode = m_currentSourceNode;
-    Node* nextNode = m_nextChildNodeToConsider;
+    RefPtr<HTMLSourceElement> currentSourceNode = m_currentSourceNode;
+    RefPtr<Node> nextNode = m_nextChildNodeToConsider;
 
     KURL nextURL = selectNextSourceChild(0, DoNothing);
 
@@ -2750,7 +2752,7 @@ KURL HTMLMediaElement::selectNextSourceChild(ContentType *contentType, InvalidUR
         LOG(Media, "HTMLMediaElement::selectNextSourceChild");
 #endif
 
-    if (m_nextChildNodeToConsider == sourceChildEndOfListValue()) {
+    if (!m_nextChildNodeToConsider) {
 #if !LOG_DISABLED
         if (shouldLog)
             LOG(Media, "HTMLMediaElement::selectNextSourceChild -> 0x0000, \"\"");
@@ -2761,17 +2763,24 @@ KURL HTMLMediaElement::selectNextSourceChild(ContentType *contentType, InvalidUR
     KURL mediaURL;
     Node* node;
     HTMLSourceElement* source = 0;
-    bool lookingForStartNode = m_nextChildNodeToConsider;
-    bool canUse = false;
     String type;
+    bool lookingForStartNode = m_nextChildNodeToConsider;
+    bool canUseSourceElement = false;
+    bool okToLoadSourceURL;
+
+    NodeVector potentialSourceNodes;
+    getChildNodes(this, potentialSourceNodes);
 
-    for (node = firstChild(); !canUse && node; node = node->nextSibling()) {
+    for (unsigned i = 0; !canUseSourceElement && i < potentialSourceNodes.size(); ++i) {
+        node = potentialSourceNodes[i].get();
         if (lookingForStartNode && m_nextChildNodeToConsider != node)
             continue;
         lookingForStartNode = false;
-        
+
         if (!node->hasTagName(sourceTag))
             continue;
+        if (node->parentNode() != this)
+            continue;
 
         source = static_cast<HTMLSourceElement*>(node);
 
@@ -2808,34 +2817,41 @@ KURL HTMLMediaElement::selectNextSourceChild(ContentType *contentType, InvalidUR
         }
 
         // Is it safe to load this url?
-        if (!isSafeToLoadURL(mediaURL, actionIfInvalid) || !dispatchBeforeLoadEvent(mediaURL.string()))
+        okToLoadSourceURL = isSafeToLoadURL(mediaURL, actionIfInvalid) && dispatchBeforeLoadEvent(mediaURL.string());
+
+        // A 'beforeload' event handler can mutate the DOM, so check to see if the source element is still a child node.
+        if (node->parentNode() != this) {
+            LOG(Media, "HTMLMediaElement::selectNextSourceChild : 'beforeload' removed current element");
+            source = 0;
+            goto check_again;
+        }
+
+        if (!okToLoadSourceURL)
             goto check_again;
 
         // Making it this far means the <source> looks reasonable.
-        canUse = true;
+        canUseSourceElement = true;
 
 check_again:
-        if (!canUse && actionIfInvalid == Complain)
+        if (!canUseSourceElement && actionIfInvalid == Complain && source)
             source->scheduleErrorEvent();
     }
 
-    if (canUse) {
+    if (canUseSourceElement) {
         if (contentType)
             *contentType = ContentType(type);
         m_currentSourceNode = source;
         m_nextChildNodeToConsider = source->nextSibling();
-        if (!m_nextChildNodeToConsider)
-            m_nextChildNodeToConsider = sourceChildEndOfListValue();
     } else {
         m_currentSourceNode = 0;
-        m_nextChildNodeToConsider = sourceChildEndOfListValue();
+        m_nextChildNodeToConsider = 0;
     }
 
 #if !LOG_DISABLED
     if (shouldLog)
-        LOG(Media, "HTMLMediaElement::selectNextSourceChild -> %p, %s", m_currentSourceNode, canUse ? urlForLogging(mediaURL).utf8().data() : "");
+        LOG(Media, "HTMLMediaElement::selectNextSourceChild -> %p, %s", m_currentSourceNode.get(), canUseSourceElement ? urlForLogging(mediaURL).utf8().data() : "");
 #endif
-    return canUse ? mediaURL : KURL();
+    return canUseSourceElement ? mediaURL : KURL();
 }
 
 void HTMLMediaElement::sourceWasAdded(HTMLSourceElement* source)
@@ -2867,20 +2883,20 @@ void HTMLMediaElement::sourceWasAdded(HTMLSourceElement* source)
         return;
     }
 
-    if (m_nextChildNodeToConsider != sourceChildEndOfListValue())
+    if (m_nextChildNodeToConsider)
         return;
     
     // 4.8.9.5, resource selection algorithm, source elements section:
-    // 20 - Wait until the node after pointer is a node other than the end of the list. (This step might wait forever.)
-    // 21 - Asynchronously await a stable state...
-    // 22 - Set the element's delaying-the-load-event flag back to true (this delays the load event again, in case 
+    // 21. Wait until the node after pointer is a node other than the end of the list. (This step might wait forever.)
+    // 22. Asynchronously await a stable state...
+    // 23. Set the element's delaying-the-load-event flag back to true (this delays the load event again, in case 
     // it hasn't been fired yet).
     setShouldDelayLoadEvent(true);
 
-    // 23 - Set the networkState back to NETWORK_LOADING.
+    // 24. Set the networkState back to NETWORK_LOADING.
     m_networkState = NETWORK_LOADING;
     
-    // 24 - Jump back to the find next candidate step above.
+    // 25. Jump back to the find next candidate step above.
     m_nextChildNodeToConsider = source;
     scheduleNextSourceChild();
 }
@@ -2902,8 +2918,8 @@ void HTMLMediaElement::sourceWillBeRemoved(HTMLSourceElement* source)
     if (source == m_nextChildNodeToConsider) {
         m_nextChildNodeToConsider = m_nextChildNodeToConsider->nextSibling();
         if (!m_nextChildNodeToConsider)
-            m_nextChildNodeToConsider = sourceChildEndOfListValue();
-        LOG(Media, "HTMLMediaElement::sourceRemoved - m_nextChildNodeToConsider set to %p", m_nextChildNodeToConsider);
+            m_nextChildNodeToConsider = 0;
+        LOG(Media, "HTMLMediaElement::sourceRemoved - m_nextChildNodeToConsider set to %p", m_nextChildNodeToConsider.get());
     } else if (source == m_currentSourceNode) {
         // Clear the current source node pointer, but don't change the movie as the spec says:
         // 4.8.8 - Dynamically modifying a source element and its attribute when the element is already 
@@ -3500,6 +3516,8 @@ void HTMLMediaElement::setMediaPlayerProxy(WebMediaPlayerProxy* proxy)
 
 void HTMLMediaElement::getPluginProxyParams(KURL& url, Vector<String>& names, Vector<String>& values)
 {
+    RefPtr<HTMLMediaElement> protect(this); // selectNextSourceChild may fire 'beforeload', which can make arbitrary DOM mutations.
+
     Frame* frame = document()->frame();
 
     if (isVideo()) {
index d14e439a1d62042c4bf268ca3f0acb72d094ab1d..9ba96ff38a3f806e724c14ea3c21edbe37f5c0b0 100644 (file)
@@ -528,9 +528,8 @@ private:
     // Loading state.
     enum LoadState { WaitingForSource, LoadingFromSrcAttr, LoadingFromSourceElement };
     LoadState m_loadState;
-    HTMLSourceElement* m_currentSourceNode;
-    Node* m_nextChildNodeToConsider;
-    Node* sourceChildEndOfListValue() { return static_cast<Node*>(this); }
+    RefPtr<HTMLSourceElement> m_currentSourceNode;
+    RefPtr<Node> m_nextChildNodeToConsider;
 
     OwnPtr<MediaPlayer> m_player;
 #if ENABLE(PLUGIN_PROXY_FOR_VIDEO)