Changeset 210822 in webkit


Ignore:
Timestamp:
Jan 17, 2017 11:24:23 AM (7 years ago)
Author:
Joseph Pecoraro
Message:

Crash when closing tab with debugger paused
​https://bugs.webkit.org/show_bug.cgi?id=161746
<rdar://problem/15607819>

Reviewed by Brian Burg and Brent Fulgham.

Source/WebCore:

  • page/Page.h:

(WebCore::Page::incrementNestedRunLoopCount):
(WebCore::Page::decrementNestedRunLoopCount):
(WebCore::Page::insideNestedRunLoop):
Keep track of whether or not this Page is inside of a nested run loop.
Currently the only nested run loop we know about is EventLoop used
by Web Inspector when debugging JavaScript.

(WebCore::Page::whenUnnested):
Callback that can be called when we are no longer inside of a nested
run loop.

(WebCore::Page::~Page):
Ensure we are not in a known nested run loop when destructing, since
that could be unsafe.

  • inspector/PageScriptDebugServer.cpp:

(WebCore::PageScriptDebugServer::runEventLoopWhilePausedInternal):
Increment and decrement as we go into or leave the nested runloop.

  • inspector/InspectorController.cpp:

(WebCore::InspectorController::inspectedPageDestroyed):
(WebCore::InspectorController::disconnectAllFrontends):
Rework destruction to allow disconnectAllFrontends to happen earlier
if necessary. WebKit clients may use this to disconnect remote
frontends when closing a Page.

Source/WebKit/mac:

  • WebView/WebView.mm:

(WebKit::DeferredPageDestructor::createDeferredPageDestructor):
(WebKit::DeferredPageDestructor::DeferredPageDestructor):
(WebKit::DeferredPageDestructor::tryDestruction):
(-[WebView _close]):
Defer destruction of the Page if we are in a nested runloop.

Source/WebKit2:

  • WebProcess/WebPage/WebPage.cpp:

(WebKit::DeferredPageDestructor::createDeferredPageDestructor):
(WebKit::DeferredPageDestructor::DeferredPageDestructor):
(WebKit::DeferredPageDestructor::tryDestruction):
(WebKit::WebPage::close):
Defer destruction of the Page and WebPage if we are in a nested runloop.
Also, proactively close all inspector frontends, including remote frontends.

  • WebProcess/WebPage/ios/WebPageIOS.mm:

(WebKit::WebPage::handleSyntheticClick):
(WebKit::WebPage::completeSyntheticClick):
Return early in some cases where a nested run loop may have closed
the WebPage on us while handling JavaScript events.

Location:
trunk/Source
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r210821 r210822  
     12017-01-17  Joseph Pecoraro  <pecoraro@apple.com>
     2
     3        Crash when closing tab with debugger paused
     4        https://bugs.webkit.org/show_bug.cgi?id=161746
     5        <rdar://problem/15607819>
     6
     7        Reviewed by Brian Burg and Brent Fulgham.
     8
     9        * page/Page.h:
     10        (WebCore::Page::incrementNestedRunLoopCount):
     11        (WebCore::Page::decrementNestedRunLoopCount):
     12        (WebCore::Page::insideNestedRunLoop):
     13        Keep track of whether or not this Page is inside of a nested run loop.
     14        Currently the only nested run loop we know about is EventLoop used
     15        by Web Inspector when debugging JavaScript.
     16
     17        (WebCore::Page::whenUnnested):
     18        Callback that can be called when we are no longer inside of a nested
     19        run loop.
     20
     21        (WebCore::Page::~Page):
     22        Ensure we are not in a known nested run loop when destructing, since
     23        that could be unsafe.
     24
     25        * inspector/PageScriptDebugServer.cpp:
     26        (WebCore::PageScriptDebugServer::runEventLoopWhilePausedInternal):
     27        Increment and decrement as we go into or leave the nested runloop.
     28
     29        * inspector/InspectorController.cpp:
     30        (WebCore::InspectorController::inspectedPageDestroyed):
     31        (WebCore::InspectorController::disconnectAllFrontends):
     32        Rework destruction to allow disconnectAllFrontends to happen earlier
     33        if necessary. WebKit clients may use this to disconnect remote
     34        frontends when closing a Page.
     35
    1362017-01-16  Filip Pizlo  <fpizlo@apple.com>
    237

  • trunk/Source/WebCore/inspector/InspectorController.cpp

    r209683 r210822  
    204204    m_injectedScriptManager->disconnect();
    205205
    206     // If the local frontend page was destroyed, close the window.
    207     if (m_inspectorFrontendClient)
    208         m_inspectorFrontendClient->closeWindow();
    209 
    210     // The frontend should call setInspectorFrontendClient(nullptr) under closeWindow().
    211     ASSERT(!m_inspectorFrontendClient);
    212 
    213206    // Clean up resources and disconnect local and remote frontends.
    214207    disconnectAllFrontends();
     208
     209    // Disconnect the client.
     210    m_inspectorClient->inspectedPageDestroyed();
     211    m_inspectorClient = nullptr;
    215212
    216213    m_agents.discardValues();
    … …  
    301298void InspectorController::disconnectAllFrontends()
    302299{
    303     // The local frontend client should be disconnected already.
     300    // If the local frontend page was destroyed, close the window.
     301    if (m_inspectorFrontendClient)
     302        m_inspectorFrontendClient->closeWindow();
     303
     304    // The frontend should call setInspectorFrontendClient(nullptr) under closeWindow().
    304305    ASSERT(!m_inspectorFrontendClient);
     306
     307    if (!m_frontendRouter->hasFrontends())
     308        return;
    305309
    306310    for (unsigned i = 0; i < m_frontendRouter->frontendCount(); ++i)
    … …  
    315319    // Destroy the inspector overlay's page.
    316320    m_overlay->freePage();
    317 
    318     // Disconnect local WK2 frontend and destroy the client.
    319     m_inspectorClient->inspectedPageDestroyed();
    320     m_inspectorClient = nullptr;
    321321
    322322    // Disconnect any remaining remote frontends.

  • trunk/Source/WebCore/inspector/PageScriptDebugServer.cpp

    r210758 r210822  
    115115    TimerBase::fireTimersInNestedEventLoop();
    116116
     117    m_page.incrementNestedRunLoopCount();
     118
    117119    EventLoop loop;
    118120    while (!m_doneProcessingDebuggerEvents && !loop.ended())
    119121        loop.cycle();
     122
     123    m_page.decrementNestedRunLoopCount();
    120124}
    121125

  • trunk/Source/WebCore/page/Page.cpp

    r210774 r210822  
    201201    , m_diagnosticLoggingClient(WTFMove(pageConfiguration.diagnosticLoggingClient))
    202202    , m_webGLStateTracker(WTFMove(pageConfiguration.webGLStateTracker))
    203     , m_subframeCount(0)
    204203    , m_openedByDOM(false)
    205204    , m_tabKeyCyclesThroughElements(true)
    … …  
    289288Page::~Page()
    290289{
     290    ASSERT(!m_nestedRunLoopCount);
     291    ASSERT(!m_unnestCallback);
     292
    291293    m_validationMessageClient = nullptr;
    292294    m_diagnosticLoggingClient = nullptr;
    … …  
    16851687#endif
    16861688
     1689void Page::incrementNestedRunLoopCount()
     1690{
     1691    m_nestedRunLoopCount++;
     1692}
     1693
     1694void Page::decrementNestedRunLoopCount()
     1695{
     1696    ASSERT(m_nestedRunLoopCount);
     1697    if (m_nestedRunLoopCount <= 0)
     1698        return;
     1699
     1700    m_nestedRunLoopCount--;
     1701
     1702    if (!m_nestedRunLoopCount && m_unnestCallback) {
     1703        callOnMainThread([this] {
     1704            if (insideNestedRunLoop())
     1705                return;
     1706
     1707            // This callback may destruct the Page.
     1708            if (m_unnestCallback) {
     1709                auto callback = m_unnestCallback;
     1710                m_unnestCallback = nullptr;
     1711                callback();
     1712            }
     1713        });
     1714    }
     1715}
     1716
     1717void Page::whenUnnested(std::function<void()> callback)
     1718{
     1719    ASSERT(!m_unnestCallback);
     1720
     1721    m_unnestCallback = callback;
     1722}
     1723
    16871724#if ENABLE(REMOTE_INSPECTOR)
    16881725bool Page::remoteInspectionAllowed() const

  • trunk/Source/WebCore/page/Page.h

    r210774 r210822  
    193193    int subframeCount() const { checkSubframeCountConsistency(); return m_subframeCount; }
    194194
     195    void incrementNestedRunLoopCount();
     196    void decrementNestedRunLoopCount();
     197    bool insideNestedRunLoop() const { return m_nestedRunLoopCount > 0; }
     198    WEBCORE_EXPORT void whenUnnested(std::function<void()>);
     199
    195200#if ENABLE(REMOTE_INSPECTOR)
    196201    WEBCORE_EXPORT bool remoteInspectionAllowed() const;
    … …  
    623628    std::unique_ptr<WebGLStateTracker> m_webGLStateTracker;
    624629
    625     int m_subframeCount;
     630    int m_nestedRunLoopCount { 0 };
     631    std::function<void()> m_unnestCallback;
     632
     633    int m_subframeCount { 0 };
    626634    String m_groupName;
    627635    bool m_openedByDOM;

  • trunk/Source/WebKit/mac/ChangeLog

    r210797 r210822  
     12017-01-17  Joseph Pecoraro  <pecoraro@apple.com>
     2
     3        Crash when closing tab with debugger paused
     4        https://bugs.webkit.org/show_bug.cgi?id=161746
     5        <rdar://problem/15607819>
     6
     7        Reviewed by Brian Burg and Brent Fulgham.
     8
     9        * WebView/WebView.mm:
     10        (WebKit::DeferredPageDestructor::createDeferredPageDestructor):
     11        (WebKit::DeferredPageDestructor::DeferredPageDestructor):
     12        (WebKit::DeferredPageDestructor::tryDestruction):
     13        (-[WebView _close]):
     14        Defer destruction of the Page if we are in a nested runloop.
     15
    1162017-01-16  Joseph Pecoraro  <pecoraro@apple.com>
    217

  • trunk/Source/WebKit/mac/WebView/WebView.mm

    r210779 r210822  
    583583}
    584584
     585namespace WebKit {
     586
     587class DeferredPageDestructor {
     588public:
     589    static void createDeferredPageDestructor(std::unique_ptr<Page> page)
     590    {
     591        new DeferredPageDestructor(WTFMove(page));
     592    }
     593
     594private:
     595    DeferredPageDestructor(std::unique_ptr<Page> page)
     596        : m_page(WTFMove(page))
     597    {
     598        tryDestruction();
     599    }
     600
     601    void tryDestruction()
     602    {
     603        if (m_page->insideNestedRunLoop()) {
     604            m_page->whenUnnested([this] { tryDestruction(); });
     605            return;
     606        }
     607
     608        m_page = nullptr;
     609        delete this;
     610    }
     611
     612    std::unique_ptr<Page> m_page;
     613};
     614
     615} // namespace WebKit
     616
    585617@interface WebView (WebFileInternal)
    586618#if !PLATFORM(IOS)
    … …  
    21492181    // See comment in HistoryItem::releaseAllPendingPageCaches() for more information.
    21502182    Page* page = _private->page;
    2151     _private->page = 0;
    2152     delete page;
     2183    _private->page = nullptr;
     2184    WebKit::DeferredPageDestructor::createDeferredPageDestructor(std::unique_ptr<Page>(page));
    21532185
    21542186#if !PLATFORM(IOS)

  • trunk/Source/WebKit2/ChangeLog

    r210797 r210822  
     12017-01-17  Joseph Pecoraro  <pecoraro@apple.com>
     2
     3        Crash when closing tab with debugger paused
     4        https://bugs.webkit.org/show_bug.cgi?id=161746
     5        <rdar://problem/15607819>
     6
     7        Reviewed by Brian Burg and Brent Fulgham.
     8
     9        * WebProcess/WebPage/WebPage.cpp:
     10        (WebKit::DeferredPageDestructor::createDeferredPageDestructor):
     11        (WebKit::DeferredPageDestructor::DeferredPageDestructor):
     12        (WebKit::DeferredPageDestructor::tryDestruction):
     13        (WebKit::WebPage::close):
     14        Defer destruction of the Page and WebPage if we are in a nested runloop.
     15        Also, proactively close all inspector frontends, including remote frontends.
     16
     17        * WebProcess/WebPage/ios/WebPageIOS.mm:
     18        (WebKit::WebPage::handleSyntheticClick):
     19        (WebKit::WebPage::completeSyntheticClick):
     20        Return early in some cases where a nested run loop may have closed
     21        the WebPage on us while handling JavaScript events.
     22
    1232017-01-16  Joseph Pecoraro  <pecoraro@apple.com>
    224

  • trunk/Source/WebKit2/WebProcess/WebPage/WebPage.cpp

    r210753 r210822  
    145145#include <WebCore/HistoryItem.h>
    146146#include <WebCore/HitTestResult.h>
     147#include <WebCore/InspectorController.h>
    147148#include <WebCore/JSDOMWindow.h>
    148149#include <WebCore/KeyboardEvent.h>
    … …  
    280281};
    281282
     283class DeferredPageDestructor {
     284public:
     285    static void createDeferredPageDestructor(std::unique_ptr<Page> page, WebPage* webPage)
     286    {
     287        new DeferredPageDestructor(WTFMove(page), webPage);
     288    }
     289
     290private:
     291    DeferredPageDestructor(std::unique_ptr<Page> page, WebPage* webPage)
     292        : m_page(WTFMove(page))
     293        , m_webPage(webPage)
     294    {
     295        tryDestruction();
     296    }
     297
     298    void tryDestruction()
     299    {
     300        if (m_page->insideNestedRunLoop()) {
     301            m_page->whenUnnested([this] { tryDestruction(); });
     302            return;
     303        }
     304
     305        m_page = nullptr;
     306        m_webPage = nullptr;
     307        delete this;
     308    }
     309
     310    std::unique_ptr<Page> m_page;
     311    RefPtr<WebPage> m_webPage;
     312};
     313
    282314DEFINE_DEBUG_ONLY_GLOBAL(WTF::RefCountedLeakCounter, webPageCounter, ("WebPage"));
    283315
    … …  
    10281060    }
    10291061
     1062    m_page->inspectorController().disconnectAllFrontends();
     1063
    10301064#if ENABLE(FULLSCREEN_API)
    10311065    m_fullScreenManager = nullptr;
    … …  
    10851119    m_printContext = nullptr;
    10861120    m_mainFrame->coreFrame()->loader().detachFromParent();
    1087     m_page = nullptr;
    10881121    m_drawingArea = nullptr;
     1122
     1123    DeferredPageDestructor::createDeferredPageDestructor(WTFMove(m_page), this);
    10891124
    10901125    bool isRunningModal = m_isRunningModal;
    … …  
    23992434    send(Messages::WebPageProxy::DidReceiveEvent(static_cast<uint32_t>(gestureEvent.type()), handled));
    24002435}
    2401 
    24022436#endif
    24032437

  • trunk/Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm

    r210687 r210822  
    536536    m_pendingSyntheticClickLocation = FloatPoint();
    537537
     538    if (m_isClosed)
     539        return;
     540
    538541    switch (WKObservedContentChange()) {
    539542    case WKContentVisibilityChange:
    … …  
    571574    RefPtr<Frame> oldFocusedFrame = m_page->focusController().focusedFrame();
    572575    RefPtr<Element> oldFocusedElement = oldFocusedFrame ? oldFocusedFrame->document()->focusedElement() : nullptr;
    573     m_userIsInteracting = true;
     576
     577    SetForScope<bool> userIsInteractingChange { m_userIsInteracting, true };
    574578
    575579    bool tapWasHandled = false;
    576580    m_lastInteractionLocation = roundedAdjustedPoint;
     581
    577582    tapWasHandled |= mainframe.eventHandler().handleMousePressEvent(PlatformMouseEvent(roundedAdjustedPoint, roundedAdjustedPoint, LeftButton, PlatformEvent::MousePressed, 1, false, false, false, false, currentTime(), WebCore::ForceAtClick, syntheticClickType));
     583    if (m_isClosed)
     584        return;
     585
    578586    tapWasHandled |= mainframe.eventHandler().handleMouseReleaseEvent(PlatformMouseEvent(roundedAdjustedPoint, roundedAdjustedPoint, LeftButton, PlatformEvent::MouseReleased, 1, false, false, false, false, currentTime(), WebCore::ForceAtClick, syntheticClickType));
     587    if (m_isClosed)
     588        return;
    579589
    580590    RefPtr<Frame> newFocusedFrame = m_page->focusController().focusedFrame();
    … …  
    587597    if (newFocusedElement && newFocusedElement == oldFocusedElement)
    588598        elementDidFocus(newFocusedElement.get());
    589 
    590     m_userIsInteracting = false;
    591599
    592600    if (!tapWasHandled || !nodeRespondingToClick || !nodeRespondingToClick->isElementNode())
Note: See TracChangeset for help on using the changeset viewer.