overflowchanged event could cause a crash
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Dec 2013 19:10:31 +0000 (19:10 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Dec 2013 19:10:31 +0000 (19:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=125978

Reviewed by Tim Horton.

Source/WebCore:

Made the event asynchrnous by re-using Document's event queuing ability. Also removed
the infrastructure to queue up events in FrameView.

Test: fast/events/overflowchanged-inside-selection-collapse-crash.html

* dom/Document.cpp:
(WebCore::Document::recalcStyle):
(WebCore::Document::enqueueOverflowEvent):
* dom/Document.h:
* page/FrameView.cpp:
(WebCore::FrameView::FrameView):
(WebCore::FrameView::~FrameView):
(WebCore::FrameView::layout):
(WebCore::FrameView::performPostLayoutTasks):
(WebCore::FrameView::updateOverflowStatus):
* page/FrameView.h:
* rendering/RenderBlock.cpp:
(WebCore::OverflowEventDispatcher::~OverflowEventDispatcher):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::scrollRectToVisible):
* rendering/RenderMarquee.cpp:
(WebCore::RenderMarquee::start):

LayoutTests:

Add a regression test.

* fast/events/overflowchanged-inside-selection-collapse-crash-expected.txt: Added.
* fast/events/overflowchanged-inside-selection-collapse-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/overflowchanged-inside-selection-collapse-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/overflowchanged-inside-selection-collapse-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderMarquee.cpp

index e1e3fe8..868ca3c 100644 (file)
@@ -1,3 +1,15 @@
+2013-12-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        overflowchanged event could cause a crash
+        https://bugs.webkit.org/show_bug.cgi?id=125978
+
+        Reviewed by Tim Horton.
+
+        Add a regression test.
+
+        * fast/events/overflowchanged-inside-selection-collapse-crash-expected.txt: Added.
+        * fast/events/overflowchanged-inside-selection-collapse-crash.html: Added.
+
 2013-12-19  Mario Sanchez Prada  <mario.prada@samsung.com>
 
         [ATK] [WK2] platform/gtk/accessibility/roles-exposed.html is failing
diff --git a/LayoutTests/fast/events/overflowchanged-inside-selection-collapse-crash-expected.txt b/LayoutTests/fast/events/overflowchanged-inside-selection-collapse-crash-expected.txt
new file mode 100644 (file)
index 0000000..3a6013d
--- /dev/null
@@ -0,0 +1,3 @@
+This tests removing the iframe for which overflowchanged is dispatched. WebKit should not crash.
+
+PASS.
diff --git a/LayoutTests/fast/events/overflowchanged-inside-selection-collapse-crash.html b/LayoutTests/fast/events/overflowchanged-inside-selection-collapse-crash.html
new file mode 100644 (file)
index 0000000..b172015
--- /dev/null
@@ -0,0 +1,40 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function prepare() {
+    var iframe = document.createElement('iframe');
+    document.body.appendChild(iframe);
+    var doc = iframe.contentDocument;
+    doc.body.innerHTML = '<div>hello</div>';
+    doc.execCommand('SelectAll', false, null);
+    iframe.style.width = '50px';
+    iframe.style.height = '50px';
+    iframe.offsetLeft; // Force layout
+    iframe.style.width = '200%';
+    iframe.style.height = '200%';
+    return iframe.contentWindow.getSelection();
+}
+
+document.body.addEventListener('overflowchanged', function () {
+    document.body.innerHTML = 'This tests removing the iframe for which overflowchanged is dispatched. WebKit should not crash.';
+    GCController.collect();
+    setTimeout(function () {
+        document.body.innerHTML += '<br><br>PASS.';
+        testRunner.notifyDone();
+    }, 0);
+});
+
+if (window.GCController) {
+    testRunner.waitUntilDone();
+    prepare().collapseToStart();
+} else
+    document.write('This test requires GCController.');
+
+</script>
+</body>
+</html>
index de5cdb5..8b98930 100644 (file)
@@ -1,3 +1,33 @@
+2013-12-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        overflowchanged event could cause a crash
+        https://bugs.webkit.org/show_bug.cgi?id=125978
+
+        Reviewed by Tim Horton.
+
+        Made the event asynchrnous by re-using Document's event queuing ability. Also removed
+        the infrastructure to queue up events in FrameView.
+
+        Test: fast/events/overflowchanged-inside-selection-collapse-crash.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::recalcStyle):
+        (WebCore::Document::enqueueOverflowEvent):
+        * dom/Document.h:
+        * page/FrameView.cpp:
+        (WebCore::FrameView::FrameView):
+        (WebCore::FrameView::~FrameView):
+        (WebCore::FrameView::layout):
+        (WebCore::FrameView::performPostLayoutTasks):
+        (WebCore::FrameView::updateOverflowStatus):
+        * page/FrameView.h:
+        * rendering/RenderBlock.cpp:
+        (WebCore::OverflowEventDispatcher::~OverflowEventDispatcher):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::scrollRectToVisible):
+        * rendering/RenderMarquee.cpp:
+        (WebCore::RenderMarquee::start):
+
 2013-12-19  Daniel Bates  <dabates@apple.com>
 
         Fix the Windows build after <http://trac.webkit.org/changeset/160841>
index 82a6d82..3fb9ebc 100644 (file)
@@ -1745,7 +1745,6 @@ void Document::recalcStyle(Style::Change change)
         PostAttachCallbackDisabler disabler(*this);
         WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
 
-        frameView.pauseScheduledEvents();
         frameView.beginDeferredRepaints();
 
         if (m_pendingStyleRecalcShouldForce)
@@ -1772,7 +1771,6 @@ void Document::recalcStyle(Style::Change change)
         if (m_styleResolver)
             m_styleSheetCollection.resetCSSFeatureFlags();
 
-        frameView.resumeScheduledEvents();
         frameView.endDeferredRepaints();
     }
 
@@ -3715,6 +3713,11 @@ void Document::enqueueDocumentEvent(PassRefPtr<Event> event)
     m_eventQueue.enqueueEvent(event);
 }
 
+void Document::enqueueOverflowEvent(PassRefPtr<Event> event)
+{
+    m_eventQueue.enqueueEvent(event);
+}
+
 PassRefPtr<Event> Document::createEvent(const String& eventType, ExceptionCode& ec)
 {
     RefPtr<Event> event = EventFactory::create(eventType);
index b6a4376..709e61f 100644 (file)
@@ -1052,6 +1052,7 @@ public:
 
     void enqueueWindowEvent(PassRefPtr<Event>);
     void enqueueDocumentEvent(PassRefPtr<Event>);
+    void enqueueOverflowEvent(PassRefPtr<Event>);
     void enqueuePageshowEvent(PageshowEventPersistence);
     void enqueueHashchangeEvent(const String& oldURL, const String& newURL);
     void enqueuePopstateEvent(PassRefPtr<SerializedScriptValue> stateObject);
index 435794e..00f5d1a 100644 (file)
@@ -199,7 +199,6 @@ FrameView::FrameView(Frame& frame)
     , m_milestonesPendingPaint(0)
     , m_visualUpdatesAllowedByClient(true)
     , m_scrollPinningBehavior(DoNotPin)
-    , m_scheduledEventSuppressionCount(0)
 {
     init();
 
@@ -226,11 +225,8 @@ PassRefPtr<FrameView> FrameView::create(Frame& frame, const IntSize& initialSize
 
 FrameView::~FrameView()
 {
-    if (m_postLayoutTasksTimer.isActive()) {
+    if (m_postLayoutTasksTimer.isActive())
         m_postLayoutTasksTimer.stop();
-        m_scheduledEventSuppressionCount = 0;
-        m_scheduledEvents.clear();
-    }
     
     removeFromAXObjectCache();
     resetScrollbars();
@@ -243,7 +239,6 @@ FrameView::~FrameView()
     setHasVerticalScrollbar(false);
     
     ASSERT(!m_scrollCorner);
-    ASSERT(m_scheduledEvents.isEmpty());
 
     ASSERT(frame().view() != this || !frame().contentRenderer());
 }
@@ -1241,8 +1236,6 @@ void FrameView::layout(bool allowSubtree)
 
         layer = root->enclosingLayer();
 
-        pauseScheduledEvents();
-
         bool disableLayoutState = false;
         if (subtree) {
             disableLayoutState = root->view().shouldDisableLayoutStateForSubtree(root);
@@ -1327,9 +1320,7 @@ void FrameView::layout(bool allowSubtree)
     if (document.hasListenerType(Document::OVERFLOWCHANGED_LISTENER))
         updateOverflowStatus(layoutWidth() < contentsWidth(), layoutHeight() < contentsHeight());
 
-    if (m_postLayoutTasksTimer.isActive())
-        resumeScheduledEvents();
-    else {
+    if (!m_postLayoutTasksTimer.isActive()) {
         if (!m_inSynchronousPostLayout) {
             if (inChildFrameLayoutWithFrameFlattening)
                 updateWidgetPositions();
@@ -1345,10 +1336,8 @@ void FrameView::layout(bool allowSubtree)
             // can make us need to update again, and we can get stuck in a nasty cycle unless
             // we call it through the timer here.
             m_postLayoutTasksTimer.startOneShot(0);
-            if (needsLayout()) {
-                pauseScheduledEvents();
+            if (needsLayout())
                 layout();
-            }
         }
     }
 
@@ -2562,41 +2551,6 @@ bool FrameView::shouldUpdate(bool immediateRequested) const
     return true;
 }
 
-struct FrameView::ScheduledEvent {
-    ScheduledEvent(PassRefPtr<Event> e, PassRefPtr<Node> t) : event(e), target(t) { }
-    RefPtr<Event> event;
-    RefPtr<Node> target;
-};
-
-void FrameView::scheduleEvent(PassRefPtr<Event> event, PassRefPtr<Node> eventTarget)
-{
-    if (!m_scheduledEventSuppressionCount) {
-        eventTarget->dispatchEvent(event, IGNORE_EXCEPTION);
-        return;
-    }
-    m_scheduledEvents.append(ScheduledEvent(event, eventTarget));
-}
-
-void FrameView::pauseScheduledEvents()
-{
-    ++m_scheduledEventSuppressionCount;
-}
-
-void FrameView::resumeScheduledEvents()
-{
-    ASSERT(m_scheduledEventSuppressionCount);
-    --m_scheduledEventSuppressionCount;
-    if (m_scheduledEventSuppressionCount)
-        return;
-
-    Vector<ScheduledEvent> eventsToDispatch = std::move(m_scheduledEvents);
-    for (auto& scheduledEvent : eventsToDispatch) {
-        if (!scheduledEvent.target->inDocument())
-            continue;
-        scheduledEvent.target->dispatchEvent(scheduledEvent.event.release(), IGNORE_EXCEPTION);
-    }
-}
-
 void FrameView::scrollToAnchor()
 {
     RefPtr<Node> anchorNode = m_maintainScrollPositionAnchor;
@@ -2764,8 +2718,6 @@ void FrameView::performPostLayoutTasks()
 
     scrollToAnchor();
 
-    resumeScheduledEvents();
-
     sendResizeEventIfNeeded();
 }
 
@@ -2955,9 +2907,12 @@ void FrameView::updateOverflowStatus(bool horizontalOverflow, bool verticalOverf
     if (horizontalOverflowChanged || verticalOverflowChanged) {
         m_horizontalOverflow = horizontalOverflow;
         m_verticalOverflow = verticalOverflow;
-        
-        scheduleEvent(OverflowEvent::create(horizontalOverflowChanged, horizontalOverflow,
-            verticalOverflowChanged, verticalOverflow), m_viewportRenderer->element());
+
+        RefPtr<OverflowEvent> overflowEvent = OverflowEvent::create(horizontalOverflowChanged, horizontalOverflow,
+            verticalOverflowChanged, verticalOverflow);
+        overflowEvent->setTarget(m_viewportRenderer->element());
+
+        frame().document()->enqueueOverflowEvent(overflowEvent.release());
     }
     
 }
index 1c627df..d657ae4 100644 (file)
@@ -41,7 +41,6 @@ namespace WebCore {
 
 class AXObjectCache;
 class Element;
-class Event;
 class FloatSize;
 class Frame;
 class HTMLFrameOwnerElement;
@@ -254,9 +253,6 @@ public:
 
     void restoreScrollbar();
 
-    void scheduleEvent(PassRefPtr<Event>, PassRefPtr<Node>);
-    void pauseScheduledEvents();
-    void resumeScheduledEvents();
     void postLayoutTimerFired(Timer<FrameView>*);
 
     bool wasScrolledByUser() const;
@@ -674,11 +670,6 @@ private:
     bool m_visualUpdatesAllowedByClient;
     
     ScrollPinningBehavior m_scrollPinningBehavior;
-
-    unsigned m_scheduledEventSuppressionCount;
-
-    struct ScheduledEvent;
-    Vector<ScheduledEvent> m_scheduledEvents;
 };
 
 inline void FrameView::incrementVisuallyNonEmptyCharacterCount(unsigned count)
index 9f9153c..3e40256 100644 (file)
@@ -157,8 +157,12 @@ public:
 
         bool horizontalLayoutOverflowChanged = hasHorizontalLayoutOverflow != m_hadHorizontalLayoutOverflow;
         bool verticalLayoutOverflowChanged = hasVerticalLayoutOverflow != m_hadVerticalLayoutOverflow;
-        if (horizontalLayoutOverflowChanged || verticalLayoutOverflowChanged)
-            m_block->view().frameView().scheduleEvent(OverflowEvent::create(horizontalLayoutOverflowChanged, hasHorizontalLayoutOverflow, verticalLayoutOverflowChanged, hasVerticalLayoutOverflow), m_block->element());
+        if (!horizontalLayoutOverflowChanged && !verticalLayoutOverflowChanged)
+            return;
+
+        RefPtr<OverflowEvent> overflowEvent = OverflowEvent::create(horizontalLayoutOverflowChanged, hasHorizontalLayoutOverflow, verticalLayoutOverflowChanged, hasVerticalLayoutOverflow);
+        overflowEvent->setTarget(m_block->element());
+        m_block->document().enqueueOverflowEvent(overflowEvent.release());
     }
 
 private:
index 045aa75..71ce460 100644 (file)
@@ -2423,7 +2423,6 @@ void RenderLayer::scrollRectToVisible(const LayoutRect& rect, const ScrollAlignm
     // We may end up propagating a scroll event. It is important that we suspend events until 
     // the end of the function since they could delete the layer or the layer's renderer().
     FrameView& frameView = renderer().view().frameView();
-    frameView.pauseScheduledEvents();
 
     bool restrictedByLineClamp = false;
     if (renderer().parent()) {
@@ -2507,8 +2506,6 @@ void RenderLayer::scrollRectToVisible(const LayoutRect& rect, const ScrollAlignm
 
     if (parentLayer)
         parentLayer->scrollRectToVisible(newRect, alignX, alignY);
-
-    frameView.resumeScheduledEvents();
 }
 
 void RenderLayer::updateCompositingLayersAfterScroll()
index 28e4e74..6d3003b 100644 (file)
@@ -156,11 +156,6 @@ void RenderMarquee::start()
     if (m_timer.isActive() || m_layer->renderer().style().marqueeIncrement().isZero())
         return;
 
-    // We may end up propagating a scroll event. It is important that we suspend events until 
-    // the end of the function since they could delete the layer, including the marquee.
-    FrameView& frameView = m_layer->renderer().view().frameView();
-    frameView.pauseScheduledEvents();
-
     if (!m_suspended && !m_stopped) {
         if (isHorizontal())
             m_layer->scrollToOffset(IntSize(m_start, 0));
@@ -173,8 +168,6 @@ void RenderMarquee::start()
     }
 
     m_timer.startRepeating(speed() * 0.001);
-
-    frameView.resumeScheduledEvents();
 }
 
 void RenderMarquee::suspend()