Wheel event callback removing the window causes crash in WebCore.
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Apr 2016 21:15:34 +0000 (21:15 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Apr 2016 21:15:34 +0000 (21:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=150871
<rdar://problem/23418283>

Reviewed by Simon Fraser.

Source/WebCore:

Null check the FrameView before using it, since the iframe may have been removed
from its parent document inside the event handler.

The new test triggered a cross-load side-effect, where wheel event filtering wasn't
reset between page loads. Fix by calling clearLatchedState() in EventHandler::clear(),
which resets the filtering.

Since the Frame destructor invokes EventHandler::clear, which invokes MainFrame methods,
we run the risk of attempting to dereference destroyed MainFrame elements of the current
Frame object. Instead, clear the EventHandler in the MainFrame destructor.

Finally, confirm that the mainFrame member is not being destroyed in the handful of
places that might attempt to access the mainFrame during object destruction (essentially
cleanup methods).

Test: fast/events/wheel-event-destroys-frame.html

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::clear): Protect against accessing mainFrame content during destruction.
* page/EventHandler.cpp:
(WebCore::EventHandler::clear): Call 'clearLatchedState' instead of endFilteringDeltas.
(WebCore::EventHandler::clearLatchedState): Null-check the filter before calling it.
* page/Frame.cpp:
(WebCore::Frame::~Frame): Do not call 'setView' in the destructor for a MainFrame.
(WebCore::Frame::setView): Check for a null event handler before invoking it.
(WebCore::Frame::setMainFrameWasDestroyed): Added. Mark that the MainFrame
member of the Frame is being destroyed (if the current Frame is a MainFrame) and clear
the EventHandler member so that it doesn't attempt to access mainFrame content.
(WebCore::Frame::mainFrame): When accessing the mainFrame member, assert that the
mainFrame is not being destroyed.
* page/MainFrame.cpp:
(WebCore::MainFrame::~MainFrame): Set the m_recentWheelEventDeltaFilter to nullptr to
prevent attempts to access it during object destruction. Call the new 'setMainFrameWasDestroyed'
method to reset eventHandler and mark the MainFrame as being in the process of destruction.
* page/WheelEventDeltaFilter.cpp:
(WebCore::WheelEventDeltaFilter::filteredDelta): Add logging.
* page/mac/EventHandlerMac.mm:
(WebCore::EventHandler::platformCompleteWheelEvent): Add null check.
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::scrollTo): Add logging.

LayoutTests:

* fast/events/wheel-event-destroys-frame-expected.txt: Added.
* fast/events/wheel-event-destroys-frame.html: Added.
* platform/ios-simulator/TestExpectations: Skip wheel-event test on iOS.

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/events/wheel-event-destroys-frame-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/wheel-event-destroys-frame.html [new file with mode: 0644]
LayoutTests/platform/ios-simulator/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/page/EventHandler.cpp
Source/WebCore/page/Frame.cpp
Source/WebCore/page/Frame.h
Source/WebCore/page/MainFrame.cpp
Source/WebCore/page/WheelEventDeltaFilter.cpp
Source/WebCore/page/mac/EventHandlerMac.mm
Source/WebCore/rendering/RenderLayer.cpp

index 8f1feb8..7bc0701 100644 (file)
@@ -1,3 +1,15 @@
+2016-04-07  Brent Fulgham  <bfulgham@apple.com>
+
+        Wheel event callback removing the window causes crash in WebCore.
+        https://bugs.webkit.org/show_bug.cgi?id=150871
+        <rdar://problem/23418283>
+
+        Reviewed by Simon Fraser.
+
+        * fast/events/wheel-event-destroys-frame-expected.txt: Added.
+        * fast/events/wheel-event-destroys-frame.html: Added.
+        * platform/ios-simulator/TestExpectations: Skip wheel-event test on iOS.
+
 2016-04-07  Saam barati  <sbarati@apple.com>
 
         Initial implementation of annex b.3.3 behavior was incorrect
diff --git a/LayoutTests/fast/events/wheel-event-destroys-frame-expected.txt b/LayoutTests/fast/events/wheel-event-destroys-frame-expected.txt
new file mode 100644 (file)
index 0000000..6fb47db
--- /dev/null
@@ -0,0 +1,3 @@
+This test should not crash
+
+
diff --git a/LayoutTests/fast/events/wheel-event-destroys-frame.html b/LayoutTests/fast/events/wheel-event-destroys-frame.html
new file mode 100644 (file)
index 0000000..4e67f07
--- /dev/null
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script>
+        if (window.testRunner) {
+            testRunner.waitUntilDone();
+            testRunner.dumpAsText();
+        }
+
+        function frameLoaded(iframe)
+        {
+            iframe.contentWindow.addEventListener('wheel', function() {
+                // Removing the window during event firing causes crash.
+                window.document.body.removeChild(iframe);
+                window.setTimeout(function() {
+                    if (window.testRunner)
+                        testRunner.notifyDone();
+                }, 0);
+            });
+
+            if (!window.eventSender)
+                return;
+
+            var iframeTarget = document.getElementById('iframe');
+            var iframeBounds = iframeTarget.getBoundingClientRect();
+
+            eventSender.mouseMoveTo(iframeBounds.left + 10, iframeBounds.top + 10);
+            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'began', 'none');
+            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'changed', 'none');
+            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, -1, 'changed', 'none');
+            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'ended', 'none');
+        }
+    </script>
+</head>
+<body>
+    <p>This test should not crash</p>
+    <iframe id="iframe" onload="frameLoaded(this)" src="data:text/html,<body>Wheel here</body>"></iframe>
+</body>
+</html>
index f54d5f1..4cbb370 100644 (file)
@@ -57,6 +57,7 @@ fast/history/page-cache-suspended-audiocontext.html
 # No mousewheel events on iOS
 fast/scrolling/iframe-scrollable-after-back.html [ Skip ]
 fast/scrolling/overflow-scrollable-after-back.html [ Skip ]
+fast/events/wheel-event-destroys-frame.html [ Skip ]
 
 # Not supported on iOS
 batterystatus
index 0d8036b..5727b30 100644 (file)
@@ -1,3 +1,52 @@
+2016-04-07  Brent Fulgham  <bfulgham@apple.com>
+
+        Wheel event callback removing the window causes crash in WebCore.
+        https://bugs.webkit.org/show_bug.cgi?id=150871
+        <rdar://problem/23418283>
+
+        Reviewed by Simon Fraser.
+
+        Null check the FrameView before using it, since the iframe may have been removed
+        from its parent document inside the event handler.
+        
+        The new test triggered a cross-load side-effect, where wheel event filtering wasn't
+        reset between page loads. Fix by calling clearLatchedState() in EventHandler::clear(),
+        which resets the filtering.
+
+        Since the Frame destructor invokes EventHandler::clear, which invokes MainFrame methods,
+        we run the risk of attempting to dereference destroyed MainFrame elements of the current
+        Frame object. Instead, clear the EventHandler in the MainFrame destructor.
+
+        Finally, confirm that the mainFrame member is not being destroyed in the handful of
+        places that might attempt to access the mainFrame during object destruction (essentially
+        cleanup methods).
+
+        Test: fast/events/wheel-event-destroys-frame.html
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::clear): Protect against accessing mainFrame content during destruction.
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::clear): Call 'clearLatchedState' instead of endFilteringDeltas.
+        (WebCore::EventHandler::clearLatchedState): Null-check the filter before calling it.
+        * page/Frame.cpp:
+        (WebCore::Frame::~Frame): Do not call 'setView' in the destructor for a MainFrame.
+        (WebCore::Frame::setView): Check for a null event handler before invoking it.
+        (WebCore::Frame::setMainFrameWasDestroyed): Added. Mark that the MainFrame
+        member of the Frame is being destroyed (if the current Frame is a MainFrame) and clear
+        the EventHandler member so that it doesn't attempt to access mainFrame content.
+        (WebCore::Frame::mainFrame): When accessing the mainFrame member, assert that the
+        mainFrame is not being destroyed.
+        * page/MainFrame.cpp:
+        (WebCore::MainFrame::~MainFrame): Set the m_recentWheelEventDeltaFilter to nullptr to
+        prevent attempts to access it during object destruction. Call the new 'setMainFrameWasDestroyed'
+        method to reset eventHandler and mark the MainFrame as being in the process of destruction.
+        * page/WheelEventDeltaFilter.cpp:
+        (WebCore::WheelEventDeltaFilter::filteredDelta): Add logging.
+        * page/mac/EventHandlerMac.mm:
+        (WebCore::EventHandler::platformCompleteWheelEvent): Add null check.
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::scrollTo): Add logging.
+
 2016-04-05  Ada Chan  <adachan@apple.com>
 
         Rename TextTrackRepresentationiOS to TextTrackRepresentationCocoa and enable on Mac
index efa813c..f4f9e27 100644 (file)
@@ -604,7 +604,11 @@ void FrameLoader::clear(Document* newDocument, bool clearWindowProperties, bool
     }
 
     m_frame.selection().prepareForDestruction();
-    m_frame.eventHandler().clear();
+
+    // We may call this code during object destruction, so need to make sure eventHandler is present.
+    if (auto eventHandler = m_frame.eventHandlerPtr())
+        eventHandler->clear();
+
     if (clearFrameView && m_frame.view())
         m_frame.view()->clear();
 
index 35a02d3..35b7972 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2006-2016 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2012 Digia Plc. and/or its subsidiary(-ies)
  *
@@ -455,9 +455,7 @@ void EventHandler::clear()
     m_mousePressed = false;
     m_capturesDragging = false;
     m_capturingMouseEventsElement = nullptr;
-#if PLATFORM(MAC)
-    m_frame.mainFrame().resetLatchingState();
-#endif
+    clearLatchedState();
 #if ENABLE(TOUCH_EVENTS) && !ENABLE(IOS_TOUCH_EVENTS)
     m_originatingTouchPointTargets.clear();
     m_originatingTouchPointDocument = nullptr;
@@ -2693,7 +2691,8 @@ void EventHandler::clearLatchedState()
 #if PLATFORM(MAC)
     m_frame.mainFrame().resetLatchingState();
 #endif
-    m_frame.mainFrame().wheelEventDeltaFilter()->endFilteringDeltas();
+    if (auto filter = m_frame.mainFrame().wheelEventDeltaFilter())
+        filter->endFilteringDeltas();
 }
 
 void EventHandler::defaultWheelEventHandler(Node* startNode, WheelEvent* wheelEvent)
index 05ec93c..68fd835 100644 (file)
@@ -5,7 +5,7 @@
  *                     2000 Simon Hausmann <hausmann@kde.org>
  *                     2000 Stefan Schimanski <1Stein@gmx.de>
  *                     2001 George Staikos <staikos@kde.org>
- * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2016 Apple Inc. All rights reserved.
  * Copyright (C) 2005 Alexey Proskuryakov <ap@nypop.com>
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008 Eric Seidel <eric@webkit.org>
@@ -160,7 +160,6 @@ Frame::Frame(Page& page, HTMLFrameOwnerElement* ownerElement, FrameLoaderClient&
     , m_script(std::make_unique<ScriptController>(*this))
     , m_editor(std::make_unique<Editor>(*this))
     , m_selection(std::make_unique<FrameSelection>(this))
-    , m_eventHandler(std::make_unique<EventHandler>(*this))
     , m_animationController(std::make_unique<AnimationController>(*this))
 #if PLATFORM(IOS)
     , m_overflowAutoScrollTimer(*this, &Frame::overflowAutoScrollTimerFired)
@@ -169,6 +168,7 @@ Frame::Frame(Page& page, HTMLFrameOwnerElement* ownerElement, FrameLoaderClient&
     , m_pageZoomFactor(parentPageZoomFactor(this))
     , m_textZoomFactor(parentTextZoomFactor(this))
     , m_activeDOMObjectsAndAnimationsSuspendedCount(0)
+    , m_eventHandler(std::make_unique<EventHandler>(*this))
 {
     AtomicString::init();
     HTMLNames::init();
@@ -251,7 +251,9 @@ void Frame::setView(RefPtr<FrameView>&& view)
     if (m_view)
         m_view->unscheduleRelayout();
     
-    eventHandler().clear();
+    // This may be called during destruction, so need to do a null check.
+    if (m_eventHandler)
+        m_eventHandler->clear();
 
     m_view = WTFMove(view);
 
index 30095c8..bb2745e 100644 (file)
@@ -5,7 +5,7 @@
  *                     2000-2001 Simon Hausmann <hausmann@kde.org>
  *                     2000-2001 Dirk Mueller <mueller@kde.org>
  *                     2000 Stefan Schimanski <1Stein@gmx.de>
- * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2016 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008 Eric Seidel <eric@webkit.org>
  *
@@ -151,6 +151,7 @@ namespace WebCore {
 
         Editor& editor() const;
         EventHandler& eventHandler() const;
+        EventHandler* eventHandlerPtr() const;
         FrameLoader& loader() const;
         NavigationScheduler& navigationScheduler() const;
         FrameSelection& selection() const;
@@ -278,6 +279,7 @@ namespace WebCore {
 
     protected:
         Frame(Page&, HTMLFrameOwnerElement*, FrameLoaderClient&);
+        void setMainFrameWasDestroyed();
 
     private:
         HashSet<FrameDestructionObserver*> m_destructionObservers;
@@ -296,7 +298,6 @@ namespace WebCore {
         const std::unique_ptr<ScriptController> m_script;
         const std::unique_ptr<Editor> m_editor;
         const std::unique_ptr<FrameSelection> m_selection;
-        const std::unique_ptr<EventHandler> m_eventHandler;
         const std::unique_ptr<AnimationController> m_animationController;
 
 #if ENABLE(DATA_DETECTION)
@@ -326,6 +327,10 @@ namespace WebCore {
         float m_textZoomFactor;
 
         int m_activeDOMObjectsAndAnimationsSuspendedCount;
+        bool m_mainFrameWasDestroyed { false };
+
+    protected:
+        std::unique_ptr<EventHandler> m_eventHandler;
     };
 
     inline void Frame::init()
@@ -398,11 +403,22 @@ namespace WebCore {
         return *m_eventHandler;
     }
 
+    inline EventHandler* Frame::eventHandlerPtr() const
+    {
+        return m_eventHandler.get();
+    }
+
     inline MainFrame& Frame::mainFrame() const
     {
+        ASSERT_WITH_SECURITY_IMPLICATION(!m_mainFrameWasDestroyed);
         return m_mainFrame;
     }
 
+    inline void Frame::setMainFrameWasDestroyed()
+    {
+        m_mainFrameWasDestroyed = false;
+    }
+
 } // namespace WebCore
 
 #endif // Frame_h
index afd8586..584644f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -66,6 +66,11 @@ MainFrame::~MainFrame()
 {
     if (m_diagnosticLoggingClient)
         m_diagnosticLoggingClient->mainFrameDestroyed();
+
+    m_recentWheelEventDeltaFilter = nullptr;
+    m_eventHandler = nullptr;
+
+    setMainFrameWasDestroyed();
 }
 
 Ref<MainFrame> MainFrame::create(Page& page, PageConfiguration& configuration)
index 597b666..4d70bbf 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -31,6 +31,8 @@
 #endif
 
 #include "FloatSize.h"
+#include "Logging.h"
+#include "TextStream.h"
 
 namespace WebCore {
     
@@ -58,6 +60,7 @@ bool WheelEventDeltaFilter::isFilteringDeltas() const
 
 FloatSize WheelEventDeltaFilter::filteredDelta() const
 {
+    LOG_WITH_STREAM(Scrolling, stream << "BasicWheelEventDeltaFilter::filteredDelta returning " << m_currentFilteredDelta);
     return m_currentFilteredDelta;
 }
 
index 8f1df0d..b20c8d7 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006, 2007, 2008, 2009, 2014-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2006-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -1049,9 +1049,10 @@ static FrameView* frameViewForLatchingState(Frame& frame, ScrollLatchingState* l
 
 bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& wheelEvent, ContainerNode* scrollableContainer, ScrollableArea* scrollableArea)
 {
-    // We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed.
-    ASSERT(m_frame.view());
     FrameView* view = m_frame.view();
+    // We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed.
+    if (!view)
+        return false;
 
     ScrollLatchingState* latchingState = m_frame.mainFrame().latchingState();
     if (wheelEvent.useLatchedEventElement() && !latchingIsLockedToAncestorOfThisFrame(m_frame) && latchingState && latchingState->scrollableContainer()) {
index 03dfe77..1b5666d 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2006-2016 Apple Inc. All rights reserved.
  *
  * Portions are Copyright (C) 1998 Netscape Communications Corporation.
  *
@@ -2346,6 +2346,8 @@ void RenderLayer::scrollTo(const ScrollPosition& position)
     if (!box)
         return;
 
+    LOG_WITH_STREAM(Scrolling, stream << "RenderLayer::scrollTo " << position);
+
     ScrollPosition newPosition = position;
     if (!box->isHTMLMarquee()) {
         // Ensure that the dimensions will be computed if they need to be (for overflow:hidden blocks).