Reviewed by Darin Adler.
authormitz@apple.com <mitz@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Nov 2007 01:54:14 +0000 (01:54 +0000)
committermitz@apple.com <mitz@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Nov 2007 01:54:14 +0000 (01:54 +0000)
        - fix <rdar://problem/5523503> Safari crashes clicking scroll bar in FaceBook 'Trips'

        Layers and listboxes are two kinds of ScrollBarClient that can be
        removed while the scrollbar is tracking the mouse. The scrollbar is not
        destroyed until later, and meanwhile it can try to call the client,
        which results in a crash.

        * manual-tests/stale-scrollbar-client-crash.html: Added.
        * platform/ScrollBar.h:
        (WebCore::Scrollbar::setClient): Added.
        * rendering/RenderLayer.cpp:
        (WebCore::RenderLayer::destroyScrollbar): Call Scrollbar::setClient().
        * rendering/RenderListBox.cpp:
        (WebCore::RenderListBox::~RenderListBox): Ditto.

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

WebCore/ChangeLog
WebCore/manual-tests/stale-scrollbar-client-crash.html [new file with mode: 0644]
WebCore/platform/ScrollBar.h
WebCore/rendering/RenderLayer.cpp
WebCore/rendering/RenderListBox.cpp

index 39f7160bc3edb9a059c8f47e4e70533064ea5174..a3a3b08b01daf47a8b3ea9827e7b35881545ad29 100644 (file)
@@ -1,3 +1,22 @@
+2007-11-07  Dan Bernstein  <mitz@apple.com>
+
+        Reviewed by Darin Adler.
+
+        - fix <rdar://problem/5523503> Safari crashes clicking scroll bar in FaceBook 'Trips'
+
+        Layers and listboxes are two kinds of ScrollBarClient that can be
+        removed while the scrollbar is tracking the mouse. The scrollbar is not
+        destroyed until later, and meanwhile it can try to call the client,
+        which results in a crash.
+
+        * manual-tests/stale-scrollbar-client-crash.html: Added.
+        * platform/ScrollBar.h:
+        (WebCore::Scrollbar::setClient): Added.
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::destroyScrollbar): Call Scrollbar::setClient().
+        * rendering/RenderListBox.cpp:
+        (WebCore::RenderListBox::~RenderListBox): Ditto.
+
 2007-11-07  Adam Roben  <aroben@apple.com>
 
         Fix <rdar://5569268> Crash when opening any FTP site in second tab/window
diff --git a/WebCore/manual-tests/stale-scrollbar-client-crash.html b/WebCore/manual-tests/stale-scrollbar-client-crash.html
new file mode 100644 (file)
index 0000000..037b21c
--- /dev/null
@@ -0,0 +1,54 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
+        "http://www.w3.org/TR/html4/strict.dtd">
+<html lang="en">
+<head>
+</head>
+<body>
+<p><b>BUG ID:</b> <a href="rdar://problem/5523503">rdar://problem/5523503</a> Safari crashes clicking scroll bar in FaceBook 'Trips'</p>
+
+<p id="test" style="background-color:skyblue; padding:3px;"><b>STEPS TO TEST:</b> 
+Drag the scroll thumb in each of the vertical scrollbars below.
+</p>
+
+<p id="success" style="background-color:palegreen; padding:3px;"><b>TEST PASS:</b> 
+Each scrollbar will disappear when clicked, along with the box containing it, but the browser will not crash as you continue to drag.
+</p>
+
+<p id="failure" style="background-color:#FF3300; padding:3px;"><b>TEST FAIL:</b>  
+The scrollbar will disappear and Safari will crash as you continue dragging.
+</p>
+
+<div style="height: 120px;">
+    <div id="overflow" style="overflow: auto; height: 100px; width: 100px; background-color: lightblue;">
+        <div style="height: 200px;"></div>
+    </div>
+</div>
+<div>
+    <select multiple="true" id="listbox" style="height: 100px; width: 100px;">
+        <option>One</option>
+        <option>Two</option>
+        <option>Three</option>
+        <option>Four</option>
+        <option>Five</option>
+        <option>Six</option>
+        <option>Seven</option>
+        <option>Eight</option>
+        <option>Nine</option>
+        <option>Ten</option>
+    </select>
+</div>
+<script>
+    var overflow = document.getElementById("overflow");
+    var listbox = document.getElementById("listbox");
+
+    function mousedown(event)
+    {
+        if (event.target.id)
+            setTimeout(event.target.id + '.style.display = "none"', 0);
+    }
+
+    overflow.addEventListener("mousedown", mousedown, false);
+    listbox.addEventListener("mousedown", mousedown, false);
+</script>
+</body>
+</html>
index 09a129568310def0bd2e0f3ea8671556822aad0f..36e86163f095b1e8153359f06ad5196e2709a87f 100644 (file)
@@ -59,6 +59,8 @@ protected:
 public:
     virtual ~Scrollbar() {}
 
+    void setClient(ScrollbarClient* client) { m_client = client; }
+
     virtual bool isWidget() const = 0;
 
     ScrollbarOrientation orientation() const { return m_orientation; }
index f90d3b4995f266f1373169a277405c141b8a1031..ac9f034ec30e55c7529a753c037f269dcf92d6b2 100644 (file)
@@ -1025,6 +1025,7 @@ void RenderLayer::destroyScrollbar(ScrollbarOrientation orientation)
     if (scrollbar) {
         if (scrollbar->isWidget())
             static_cast<PlatformScrollbar*>(scrollbar.get())->removeFromParent();
+        scrollbar->setClient(0);
 
         // FIXME: Destroy the engine scrollbar.
         scrollbar = 0;
index 276ab494c94f55c60995b6d1592685f7efc013cc..9f2dca3a6b95ee26f2cbe3d91b23a69afa4fd405 100644 (file)
@@ -79,9 +79,13 @@ RenderListBox::RenderListBox(HTMLSelectElement* element)
 
 RenderListBox::~RenderListBox()
 {
-    if (m_vBar && m_vBar->isWidget())
-        if (FrameView* view = node()->document()->view())
-            view->removeChild(static_cast<PlatformScrollbar*>(m_vBar.get()));
+    if (m_vBar) {
+        if (m_vBar->isWidget()) {
+            if (FrameView* view = node()->document()->view())
+                view->removeChild(static_cast<PlatformScrollbar*>(m_vBar.get()));
+        }
+        m_vBar->setClient(0);
+    }
 }
 
 void RenderListBox::setStyle(RenderStyle* style)