I Crashed Safari - A Bug in Apple's WebKit | Alexander O'Mara

While running the unit tests I had developed for my recent wheellistener JavaScript library against Safari, I found something unexpected. To say that Safari was failing one of the unit tests would be an understatement, it was flat out crashing the WebKit libraries.

Crash

So what went wrong?

After some testing, I narrowed down the issue to being caused by removing an iframe while an event was being triggered inside of it. As this was an edge-case, and was not something my library was causing, a quick workaround later and Safari was passing the unit test.

That’s all well and good, but why was WebKit crashing in the first place?

To the debugger!

lldb revealed the crash is triggered in the following function, on what I believe is assembly corresponding to the delegatesScrolling() call.

WebCore/page/FrameView.cpp

// ...
bool FrameView::wheelEvent(const PlatformWheelEvent& wheelEvent)
{
    // Note that to allow for rubber-band over-scroll behavior, even non-scrollable views
    // should handle wheel events.
#if !ENABLE(RUBBER_BANDING)
    if (!isScrollable())
        return false;
#endif

    if (delegatesScrolling()) {
        IntSize offset = scrollOffset();
        IntPoint oldPosition = scrollPosition();
        IntSize newOffset = IntSize(offset.width() - wheelEvent.deltaX(), offset.height() - wheelEvent.deltaY());
        if (offset != newOffset) {
            ScrollView::scrollTo(newOffset);
            scrollPositionChanged(oldPosition, scrollPosition());
            didChangeScrollOffset();
        }
        return true;
    }

    // We don't allow mouse wheeling to happen in a ScrollView that has had its scrollbars explicitly disabled.
    if (!canHaveScrollbars())
        return false;

    if (platformWidget())
        return false;

#if ENABLE(ASYNC_SCROLLING)
    if (Page* page = frame().page()) {
        if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
            if (scrollingCoordinator->coordinatesScrollingForFrameView(*this))
                return scrollingCoordinator->handleWheelEvent(*this, wheelEvent);
        }
    }
#endif

    return ScrollableArea::handleWheelEvent(wheelEvent);
}
// ...

However, the fault appears to lie with the function calling it, seen below.

WebCore/page/mac/EventHandlerMac.mm

// ...
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();

    ScrollLatchingState* latchingState = m_frame.mainFrame().latchingState();
    if (wheelEvent.useLatchedEventElement() && !latchingIsLockedToAncestorOfThisFrame(m_frame) && latchingState && latchingState->scrollableContainer()) {

        m_isHandlingWheelEvent = false;

        // WebKit2 code path
        if (!frameHasPlatformWidget(m_frame) && !latchingState->startedGestureAtScrollLimit() && scrollableContainer == latchingState->scrollableContainer() && scrollableArea && view != scrollableArea) {
            // If we did not start at the scroll limit, do not pass the event on to be handled by enclosing scrollable regions.
            return true;
        }

        if (!latchingState->startedGestureAtScrollLimit())
            view = frameViewForLatchingState(m_frame, latchingState);

        ASSERT(view);

        bool didHandleWheelEvent = view->wheelEvent(wheelEvent);
        if (scrollableContainer == latchingState->scrollableContainer()) {
            // If we are just starting a scroll event, and have nowhere left to scroll, allow
            // the enclosing frame to handle the scroll.
            didHandleWheelEvent = !latchingState->startedGestureAtScrollLimit();
        }

        // If the platform widget is handling the event, we always want to return false.
        if (scrollableArea == view && view->platformWidget())
            didHandleWheelEvent = false;

        return didHandleWheelEvent;
    }

    bool didHandleEvent = view->wheelEvent(wheelEvent);
    m_isHandlingWheelEvent = false;
    return didHandleEvent;
}
// ...

There is actually a comment suggesting they know about the issue.

We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed.

However their solution is to check that the view is not a null pointer by using ASSERT, which does not handle it gracefully, and is removed from release builds.

Interestingly, a similar function in the WebKit source code handles this situation properly, only calling wheelEvent if the view is not a null pointer.

WebCore/page/EventHandler.cpp

// ...
bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& event, ContainerNode*, ScrollableArea*)
{
    // We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed.
    FrameView* view = m_frame.view();

    bool didHandleEvent = view ? view->wheelEvent(event) : false;
    m_isHandlingWheelEvent = false;
    return didHandleEvent;
}
// ...

Currently, this can be seen affecting both the latest Nightly WebKit browser and Apple’s Safari, as well as anything that links against the same libraries, such as those using WebView.

Apparently, these are not the only functions affected by the issue. For example, I ran a test with pywebview, which I believe is using a legacy API for these libraries, and it crashes in the following function.

WebCore/page/mac/EventHandlerMac.mm

// ...
bool EventHandler::platformCompletePlatformWidgetWheelEvent(const PlatformWheelEvent& wheelEvent, const Widget& widget, ContainerNode* scrollableContainer)
{
    // WebKit1: Prevent multiple copies of the scrollWheel event from being sent to the NSScrollView widget.
    if (frameHasPlatformWidget(m_frame) && widget.isFrameView())
        return true;

    ScrollLatchingState* latchingState = m_frame.mainFrame().latchingState();
    if (!latchingState)
        return false;

    if (wheelEvent.useLatchedEventElement() && latchingState->scrollableContainer() && scrollableContainer == latchingState->scrollableContainer())
        return !latchingState->startedGestureAtScrollLimit();

    return false;
}
// ...

Fortunately this bug does not appear to be a security rick, and the crash can be avoided in JavaScript by deferring removal of the window element with setTimeout. Of course, it should not be possible to cause a crash in the first place. I have reported the issue to the WebKit bug tracker, so it will probably be fixed at some point.

Want to see the crash for yourself? Check out the live demo, and the HTML/JavaScript example source below. You can also check out the bug report for updates on the issue.

Live Demo | Bug Report

<!DOCTYPE html>
<html>
    <head>
        <meta charset="utf-8" />
        <title>WebKit Wheel Event Remove Window Crash</title>
    </head>
    <body>
        <script type="text/javascript">
/*<!--*/
(function(window) {'use strict';
var iframe = window.document.createElement('iframe');
iframe.style.position = 'fixed';
iframe.style.left = iframe.style.top = '5%';
iframe.style.width = iframe.style.height = '90%';
window.document.body.appendChild(iframe);
function iframeReady() {
    iframe.contentWindow.document.body.innerHTML = '<h1>Wheel here to crash.</h1>';
    iframe.contentWindow.addEventListener('wheel', function() {
        // Removing the window during event firing causes crash.
        window.document.body.removeChild(iframe);
    });
}
var pollInterval;
var pollReady = function() {
    if (iframe.contentWindow.document.body) {
        window.clearInterval(pollInterval);
        iframeReady();
    }
};
pollInterval = window.setInterval(pollReady, 100);
})(window);
/*-->*/
        </script>
    </body>
</html>

UPDATE: Safari has been fixed so it’s no longer possible to reproduce this crash in an updated Safari.