ASSERT(!m_frontendRouter->hasLocalFrontend()) when running Web Inspector tests
authorbburg@apple.com <bburg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Sep 2015 17:16:24 +0000 (17:16 +0000)
committerbburg@apple.com <bburg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Sep 2015 17:16:24 +0000 (17:16 +0000)
commit814afc7bbfcf88e90de1dc8aec527f1a6eb1ea95
tree253a609de1e21001757a6f9273a58437bccd9736
parentc2b6d6cfb8e539b5298655b29ff603f04926e276
ASSERT(!m_frontendRouter->hasLocalFrontend()) when running Web Inspector tests
https://bugs.webkit.org/show_bug.cgi?id=149006
Source/JavaScriptCore:

Reviewed by Joseph Pecoraro.

Prior to disconnecting, we need to know how many frontends remain connected.

* inspector/InspectorFrontendRouter.h: Add frontendCount().

Source/WebCore:

Reviewed by Joseph Pecoraro.

The patch fixes two defects:

    (1) the stub inspector frontend is not closed reliably when a test times out
    (2) frontend clients and channels are sometimes connected to the wrong controllers

When an inspector test times out, the test runner requests (via the inspected page's controller)
that the inspector close. But, the stub frontend works independently of InspectorClient,
so the inspected page's InspectorController cannot close the stub frontend. The assertion
failed because the stub frontend's channel was still connected to the inspected page's controller.

The fix is to route requests for the inspector window to close through the FrontendClient's
closeWindow() method rather than InspectorClient, so that the stub frontend can react.
The other code paths (i.e., through close() and closeLocalFrontend()) have been removed.

Now that the stub frontend eagerly closes its channel before the Page gets GC'd, several
methods invoked during test teardown must be reordered to avoid using dangling pointers.

The stub frontend in Internals has been rewritten to properly disconnect itself
from both the frontend and inspected page's inspector controllers.

While fixing this bug, I noticed that we are inconsistent about which inspector controller
(the inspected page's or the frontend page's) receives the FrontendClient and which takes
FrontendChannels. It is now the case for all configurations that the FrontendClient is
connected to the frontend page's inspector controller, and FrontendChannels are connected
to the inspected page's inspector controller. In the WK2 case, the Inspector Process
has an attached frontend client, and its inspected Web Process has frontend channels.

No new tests, covered by existing tests.

* inspector/InspectorClient.h:
* inspector/InspectorController.cpp:
(WebCore::InspectorController::~InspectorController):
(WebCore::InspectorController::inspectedPageDestroyed):

    This method is called from Page::~Page, so we should disconnect all frontends now
    before subframes are detached from the page, making InspectorController inaccessible.

(WebCore::InspectorController::disconnectFrontend):

    The teardown branch was never being run before, because we never disconnected the
    frontend's channel correctly. Some agents use the overlay during teardown, so notify
    agents before releasing the overlay page.

(WebCore::InspectorController::disconnectAllFrontends):

    The actions from close() are inlined and rearranged here, similar to disconnectFrontend.
    We have to notify agents before removing InspectorClient as some agents make use of it.

(WebCore::InspectorController::close): Deleted.
(WebCore::InspectorController::show): This assertion is vacuously true now.
* inspector/InspectorFrontendClientLocal.cpp:
(WebCore::InspectorFrontendClientLocal::inspectedPage): Added. Used by stub frontend.
* inspector/InspectorFrontendClientLocal.h:
(WebCore::InspectorFrontendClientLocal::frontendPage): Added.
* loader/EmptyClients.h:
* page/Page.cpp:
(WebCore::Page::~Page):

    Notify inspector before detaching frames, otherwise it will not be possible to
    cleanly disconnect the stub frontend's channel.

* testing/Internals.cpp:

    Rewrite the stub frontend to better encapsulate its setup and teardown logic.

(WebCore::InspectorStubFrontend::frontendPage): Added.
(WebCore::InspectorStubFrontend::InspectorStubFrontend): Added.
(WebCore::InspectorStubFrontend::~InspectorStubFrontend): Added.
(WebCore::InspectorStubFrontend::closeWindow): Added.
(WebCore::InspectorStubFrontend::sendMessageToFrontend): Added.
(WebCore::Internals::openDummyInspectorFrontend):
(WebCore::Internals::closeDummyInspectorFrontend):
(WebCore::InspectorFrontendClientDummy::~InspectorFrontendClientDummy): Deleted.
(WebCore::InspectorFrontendClientDummy::InspectorFrontendClientDummy): Deleted.
(WebCore::InspectorFrontendChannelDummy::~InspectorFrontendChannelDummy): Deleted.
(WebCore::InspectorFrontendChannelDummy::InspectorFrontendChannelDummy): Deleted.
(WebCore::InspectorFrontendChannelDummy::sendMessageToFrontend): Deleted.
* testing/Internals.h:

Source/WebKit/ios:

Reviewed by Joseph Pecoraro.

* WebCoreSupport/WebInspectorClientIOS.mm:
(WebInspectorClient::closeLocalFrontend): Deleted.
(WebInspectorFrontendClient::disconnectFromBackend): Deleted.

Source/WebKit/mac:

Reviewed by Joseph Pecoraro.

WK1 WebInspectorClient was connecting to the wrong controllers. Fix this, and
remove extra code paths for closing the frontend.

* WebCoreSupport/WebInspectorClient.h:
* WebCoreSupport/WebInspectorClient.mm:
(-[WebInspectorWindowController destroyInspectorView]):

    Disconnect the FrontendClient from the frontend page's inspector controller.
    Do this teardown before releasing the frontend, otherwise we can't use it.

(WebInspectorClient::inspectedPageDestroyed): Deleted.
(WebInspectorClient::closeLocalFrontend): Deleted.
(WebInspectorFrontendClient::disconnectFromBackend): Deleted.
* WebInspector/WebInspector.mm:
(-[WebInspector inspectedWebViewClosed]):

    Make sure to close ourself if the inspected page closes.

(-[WebInspector close:]):

    Go through the frontend instead of InspectorController.

* WebInspector/WebInspectorFrontend.h:
* WebInspector/WebInspectorFrontend.mm:
(-[WebInspectorFrontend close]):

Source/WebKit/win:

Reviewed by Joseph Pecoraro.

* WebCoreSupport/WebInspectorClient.cpp:
(WebInspectorFrontendClient::destroyInspectorView):

    Disconnect the FrontendClient from the frontend page's inspector controller.
    Do this teardown before releasing the frontend, otherwise we can't use it.

(WebInspectorFrontendClient::onClose):
(WebInspectorClient::inspectedPageDestroyed): Deleted.
(WebInspectorClient::closeLocalFrontend): Deleted.
* WebCoreSupport/WebInspectorClient.h: Drive-by cleanup for class declarations.
* WebInspector.cpp:
(WebInspector::close):

    Go through the frontend instead of InspectorController.

Source/WebKit2:

<rdar://problem/22654257>
<rdar://problem/22631369>

Reviewed by Joseph Pecoraro.

Stop using InspectorController to close the frontend page. Go through
the FrontendClient instead. Reduce redundant code paths.

This change seems to fix some recent crashes that were seen when
closing Safari with Web Inspector open. These were caused by the frontend
channel not being disconnected at the right time.

* WebProcess/WebCoreSupport/WebInspectorClient.cpp:
(WebKit::WebInspectorClient::inspectedPageDestroyed):
(WebKit::WebInspectorClient::closeLocalFrontend): Deleted.
* WebProcess/WebCoreSupport/WebInspectorClient.h:
* WebProcess/WebPage/WebInspector.cpp:
(WebKit::WebInspector::close):
* WebProcess/WebPage/WebInspectorUI.cpp:
(WebKit::WebInspectorUI::establishConnection):

    Save a pointer to the frontend's InspectorController since we may
    need to use it while the page is being destructed and its getter
    is no longer accessible.

(WebKit::WebInspectorUI::closeWindow):

    Explicitly remove the frontend client when closing the frontend.

* WebProcess/WebPage/WebInspectorUI.h:
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::inspector):

    Allow clients to specify whether an inspector should be eagerly created.
    Without this, we may accidentally create an instance during teardown.

* WebProcess/WebPage/WebPage.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@189970 268f45cc-cd09-0410-ab3c-d52691b4dbfc
32 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/inspector/InspectorFrontendRouter.h
Source/WebCore/ChangeLog
Source/WebCore/inspector/InspectorClient.h
Source/WebCore/inspector/InspectorController.cpp
Source/WebCore/inspector/InspectorController.h
Source/WebCore/inspector/InspectorFrontendClientLocal.cpp
Source/WebCore/inspector/InspectorFrontendClientLocal.h
Source/WebCore/loader/EmptyClients.h
Source/WebCore/page/Page.cpp
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebKit/ios/ChangeLog
Source/WebKit/ios/WebCoreSupport/WebInspectorClientIOS.mm
Source/WebKit/mac/ChangeLog
Source/WebKit/mac/WebCoreSupport/WebInspectorClient.h
Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm
Source/WebKit/mac/WebInspector/WebInspector.mm
Source/WebKit/mac/WebInspector/WebInspectorFrontend.h
Source/WebKit/mac/WebInspector/WebInspectorFrontend.mm
Source/WebKit/win/ChangeLog
Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp
Source/WebKit/win/WebCoreSupport/WebInspectorClient.h
Source/WebKit/win/WebInspector.cpp
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp
Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.h
Source/WebKit2/WebProcess/WebPage/WebInspector.cpp
Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp
Source/WebKit2/WebProcess/WebPage/WebInspectorUI.h
Source/WebKit2/WebProcess/WebPage/WebPage.cpp
Source/WebKit2/WebProcess/WebPage/WebPage.h