WebCore:
authoralice.liu@apple.com <alice.liu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Dec 2007 22:51:30 +0000 (22:51 +0000)
committeralice.liu@apple.com <alice.liu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Dec 2007 22:51:30 +0000 (22:51 +0000)
        Reviewed by Darin.

        Fixed <rdar://problem/5592485> Safari crashed trying to get a motorcycle insurance quote
        on Geico.com WebCore::Document::inPageCache()

        Calling Node::willRemove on the focusedNode would immediately tell the document to remove
        the focused node, and trigger JS events.  This means that the document is mutated while
        the engine is trying to tell all child nodes that it's about to removed.  To avoid
        crashing, we need to hold off on mutating the document until node traversal is finished.

        * dom/ContainerNode.cpp:
        (WebCore::ContainerNode::removeChild):
        (WebCore::ContainerNode::removeChildren):
        * dom/Node.cpp:
        * dom/Node.h:
        (WebCore::Node::willRemove):
        * loader/FrameLoader.cpp:
        (WebCore::FrameLoader::clear):

LayoutTests:

        Reviewed by Darin.

        Fixed <rdar://problem/5592485> Safari crashed trying to get a motorcycle insurance quote
        on Geico.com WebCore::Document::inPageCache()

        * fast/events/nested-event-remove-node-crash-expected.txt: Added.
        * fast/events/nested-event-remove-node-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/nested-event-remove-node-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/nested-event-remove-node-crash.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/dom/ContainerNode.cpp
WebCore/dom/Document.cpp
WebCore/dom/Document.h
WebCore/dom/Node.cpp
WebCore/loader/FrameLoader.cpp

index ee724e4b64f75877c72b3cf717feca25c9c8cb64..85d3c4718e2fddc41e208ea861fd2b8af3a80ecd 100644 (file)
@@ -1,3 +1,13 @@
+2007-12-19  Alice Liu  <alice.liu@apple.com>
+
+        Reviewed by Darin.
+
+        Fixed <rdar://problem/5592485> Safari crashed trying to get a motorcycle insurance quote
+        on Geico.com WebCore::Document::inPageCache()
+
+        * fast/events/nested-event-remove-node-crash-expected.txt: Added.
+        * fast/events/nested-event-remove-node-crash.html: Added.
+
 2007-12-19  Andre Boule  <aboule@apple.com>
 
         Reviewed by Dan Bernstein.
 2007-12-19  Andre Boule  <aboule@apple.com>
 
         Reviewed by Dan Bernstein.
diff --git a/LayoutTests/fast/events/nested-event-remove-node-crash-expected.txt b/LayoutTests/fast/events/nested-event-remove-node-crash-expected.txt
new file mode 100644 (file)
index 0000000..53cdf1e
--- /dev/null
@@ -0,0 +1 @@
+PASSED
diff --git a/LayoutTests/fast/events/nested-event-remove-node-crash.html b/LayoutTests/fast/events/nested-event-remove-node-crash.html
new file mode 100644 (file)
index 0000000..4205c03
--- /dev/null
@@ -0,0 +1,76 @@
+<html>
+<head>
+<script>
+function sendXHR()
+{
+    XHR[numXHRs] = new XMLHttpRequest();
+    XHR[numXHRs].onreadystatechange = handleStateChange;
+    XHR[numXHRs].open("GET", "nested-event-remove-node-crash-expected.txt", true);
+    XHR[numXHRs].send(null);
+    numXHRs = numXHRs + 1;
+}
+
+function callback(response)
+{
+    document.getElementById("replaceMe").innerHTML = "";
+    document.getElementById("replaceMe").innerHTML = response;
+    if (window.layoutTestController && (run == 2))
+        layoutTestController.notifyDone();
+}
+
+function handleStateChange()
+{
+    if ((XHR[0].readyState == 4) && (run < 2)) { // yes this looks wrong but it's how to reproduce the bug
+        run = run + 1;
+        callback(XHR[0].responseText);
+    }
+}
+
+function test()
+{
+/*
+    1. focus a node
+    2. send an XHR who's handler will remove the node
+    3. the focused node's onblur will fire
+    4. the onblur event handler will send off an XHR who's handler will remove the node
+*/
+    document.getElementById("theSelect").focus();
+    sendXHR();
+    
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        layoutTestController.dumpAsText();
+    }
+}
+
+function GC()
+{
+    // Force GC.
+    if (window.GCController)
+        GCController.collect();
+    else {
+        for (var i = 0; i < 10000; ++i) {
+            ({ });
+        }
+    }
+}
+
+/* GLOBALS */
+var XHR = new Array();
+var numXHRs = 0;
+var run = 0;
+
+</script>
+</head>
+<body onload="test()">
+
+<div id="replaceMe">
+
+<div>
+<select id="theSelect" onblur="sendXHR();GC();">
+</select>
+</div>
+
+</div>
+</body>
+</html>
index 8f5988911731799865b3e62c2552cb7a37d104b7..9c5f654acbab5528a8111ad9808e884b9c2f6e11 100644 (file)
@@ -1,3 +1,24 @@
+2007-12-19  Alice Liu  <alice.liu@apple.com>
+
+        Reviewed by Darin.
+
+        Fixed <rdar://problem/5592485> Safari crashed trying to get a motorcycle insurance quote
+        on Geico.com WebCore::Document::inPageCache()
+
+        Calling Node::willRemove on the focusedNode would immediately tell the document to remove
+        the focused node, and trigger JS events.  This means that the document is mutated while
+        the engine is trying to tell all child nodes that it's about to removed.  To avoid
+        crashing, we need to hold off on mutating the document until node traversal is finished.
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeChild):
+        (WebCore::ContainerNode::removeChildren):
+        * dom/Node.cpp:
+        * dom/Node.h:
+        (WebCore::Node::willRemove):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::clear):
+
 2007-12-19  Andre Boule  <aboule@apple.com>
 
         Reviewed by Dan Bernstein.
 2007-12-19  Andre Boule  <aboule@apple.com>
 
         Reviewed by Dan Bernstein.
index fb12053f509d19972129837e83dbd4fc29820065..bb1e2a6483a1b34753d9362be840bd41c3b09f00 100644 (file)
@@ -391,6 +391,8 @@ bool ContainerNode::removeChild(Node* oldChild, ExceptionCode& ec)
         return false;
     }
 
         return false;
     }
 
+    document()->removeFocusedNodeOfSubtree(child.get());
+    
     // FIXME: After sending the mutation events, "this" could be destroyed.
     // We can prevent that by doing a "ref", but first we have to make sure
     // that no callers call with ref count == 0 and parent = 0 (as of this
     // FIXME: After sending the mutation events, "this" could be destroyed.
     // We can prevent that by doing a "ref", but first we have to make sure
     // that no callers call with ref count == 0 and parent = 0 (as of this
@@ -449,6 +451,9 @@ bool ContainerNode::removeChildren()
     for (n = m_firstChild; n; n = n->nextSibling())
         willRemoveChild(n);
     
     for (n = m_firstChild; n; n = n->nextSibling())
         willRemoveChild(n);
     
+    // exclude this node when looking for removed focusedNode since only children will be removed
+    document()->removeFocusedNodeOfSubtree(this, true);
+
     forbidEventDispatch();
     while ((n = m_firstChild) != 0) {
         Node *next = n->nextSibling();
     forbidEventDispatch();
     while ((n = m_firstChild) != 0) {
         Node *next = n->nextSibling();
index 3a47772d71e8ccc2dbb596c155744919a125826d..1cc753cf87e683933c45dcac7dd4ed136cae285a 100644 (file)
@@ -2199,11 +2199,26 @@ void Document::setActiveNode(PassRefPtr<Node> newActiveNode)
     m_activeNode = newActiveNode;
 }
 
     m_activeNode = newActiveNode;
 }
 
-void Document::focusedNodeRemoved(Node* node)
+void Document::focusedNodeRemoved()
 {
     setFocusedNode(0);
 }
 
 {
     setFocusedNode(0);
 }
 
+void Document::removeFocusedNodeOfSubtree(Node* node, bool amongChildrenOnly)
+{
+    if (!m_focusedNode || this->inPageCache()) // If the document is in the page cache, then we don't need to clear out the focused node.
+        return;
+        
+    bool nodeInSubtree = false;
+    if (amongChildrenOnly)
+        nodeInSubtree = m_focusedNode->isDescendantOf(node);
+    else
+        nodeInSubtree = (m_focusedNode == node) || m_focusedNode->isDescendantOf(node);
+    
+    if (nodeInSubtree)
+        document()->focusedNodeRemoved();
+}
+
 void Document::hoveredNodeDetached(Node* node)
 {
     if (!m_hoverNode || (node != m_hoverNode && (!m_hoverNode->isTextNode() || node != m_hoverNode->parent())))
 void Document::hoveredNodeDetached(Node* node)
 {
     if (!m_hoverNode || (node != m_hoverNode && (!m_hoverNode->isTextNode() || node != m_hoverNode->parent())))
index 5e641500a488a3f463a74c0fa6fb49325592d99c..645c1474f63843eba4b5ecd25adac67efc18ebb1 100644 (file)
@@ -446,7 +446,8 @@ public:
     void setActiveNode(PassRefPtr<Node>);
     Node* activeNode() const { return m_activeNode.get(); }
 
     void setActiveNode(PassRefPtr<Node>);
     Node* activeNode() const { return m_activeNode.get(); }
 
-    void focusedNodeRemoved(Node*);
+    void focusedNodeRemoved();
+    void removeFocusedNodeOfSubtree(Node*, bool amongChildrenOnly = false);
     void hoveredNodeDetached(Node*);
     void activeChainNodeDetached(Node*);
 
     void hoveredNodeDetached(Node*);
     void activeChainNodeDetached(Node*);
 
index d1cf842ef0ba4c19a4bedc472e244c6d096180a6..ce265bf0d8764e912db2f257014d5f99e70670dc 100644 (file)
@@ -838,9 +838,6 @@ void Node::attach()
 
 void Node::willRemove()
 {
 
 void Node::willRemove()
 {
-    // If the document is in the page cache, then we don't need to clear out the focused node.
-    if (!document()->inPageCache() && m_focused)
-        document()->focusedNodeRemoved(this);
 }
 
 void Node::detach()
 }
 
 void Node::detach()
index 5e76a57fa4eb0952c94cb162571324a6fa39a647..6d02b32e71d8f8709e22c41e582c2469bd01a545 100644 (file)
@@ -801,6 +801,8 @@ void FrameLoader::clear(bool clearWindowProperties, bool clearScriptObjects)
         if (m_frame->document()->attached()) {
             m_frame->document()->willRemove();
             m_frame->document()->detach();
         if (m_frame->document()->attached()) {
             m_frame->document()->willRemove();
             m_frame->document()->detach();
+            
+            m_frame->document()->removeFocusedNodeOfSubtree(m_frame->document());
         }
     }
 
         }
     }