Bug 149006

<rdar://problem/22631369>

Joe and I believe that this is caused when the inspected page times out and doesn't close the dummy inspector frontend. Later, the InspectorController expects its frontend to close, but requests the InspectorClient to close the frontend. In the case of a dummy frontend, the InspectorClient doesn't know about it, thus doesn't close anything. Possible solutions: * In -[WebInspector close], break early if (!_frontend) * Change the API so InspectorController can request (via FrontendClient) that the frontend should close.

Created attachment 261039 [details] Proposed Fix Hooo boy, it's finally working. Find me on IRC when you have questions.

Attachment 261039 [details] did not pass style-queue: ERROR: Source/WebKit/win/WebInspector.cpp:108: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

BTW, probably depends on 149071 to apply. I will land that and rebase tonight or tomorrow.

Comment on attachment 261039 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261039&action=review > Source/WebCore/ChangeLog:87 > +2015-09-11 Brian Burg <bburg@apple.com> Looks like this patch has both changes (good for bots, bad for reviewability). > Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:130 > + m_page.corePage()->inspectorController().setInspectorFrontendClient(nullptr); This is not covered by the ChangeLog (I happened to be wondering about this one and looked for it). This is when the Inspector does InspectorFrontendHost.closeWindow(). I think this makes sense to do.

Created attachment 261058 [details] Proposed Fix Rebased, should apply and be easier to review now.

Created attachment 261059 [details] Proposed Fix

Looks like Windows build is still failing: ..\..\win\WebCoreSupport\WebInspectorClient.cpp(423): error C2064: term does not evaluate to a function taking 0 arguments ..\..\win\WebInspector.cpp(132): error C2039: 'close': is not a member of 'WebCore::InspectorController' ..\..\win\WebInspector.cpp(131): warning C4390: ';': empty controlled statement found; is this the intent?

Created attachment 261111 [details] Proposed Fix (updated WebKit/win)

Windows build is still broken: ..\..\win\WebCoreSupport\WebInspectorClient.cpp(423): error C2064: term does not evaluate to a function taking 0 arguments [C:\cygwin\home\buildbot\WebKit\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]

Created attachment 261326 [details] Proposed Fix (win EWS)

Comment on attachment 261326 [details] Proposed Fix (win EWS) View in context: https://bugs.webkit.org/attachment.cgi?id=261326&action=review This looks good. I have a couple questions though. > Source/WebCore/page/Page.cpp:258 > + // If the inspector is notified after frames are disconnected, then frontends > + // may not be able to cleanly disconnect their connections to the backend. > + m_inspectorController->inspectedPageDestroyed(); Why not? I think it makes sense to do it earlier just because why notify about detaching frames if the page is going away entirely. But I don't understand the "may not be able to cleanly disconnect" if a frame is being detached. The ChangeLog mentions the stub frontend channel was the issue here. What does that do that is different from normal behavior? > Source/WebCore/testing/Internals.cpp:1755 > + Page* inspectedPage = contextDocument()->frame()->page(); > + ASSERT(inspectedPage); I don't find these ASSERTs particularly useful. For most of them, if they weren't there we would crash immediately on the next line anyways. > Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:675 > + if (Page* inspectedPage = [_inspectedWebView.get() page]) > + inspectedPage->inspectorController().disconnectFrontend(_inspectorClient); When I expand to see surrounding code, there is code later on in this method that looks wrong: 689 if (Page* inspectedPage = [_inspectedWebView.get() page]) { 690 inspectedPage->inspectorController().setInspectorFrontendClient(nullptr); 691 inspectedPage->inspectorController().disconnectFrontend(_inspectorClient); 692 } 693 Looks like this is wrong and should be removed? > Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:-435 > - m_inspectedWebView->page()->inspectorController().setInspectorFrontendClient(nullptr); > - m_inspectedWebView->page()->inspectorController().disconnectFrontend(m_inspectorClient); Here is the same code in windows and you are removing these lines!

Comment on attachment 261326 [details] Proposed Fix (win EWS) View in context: https://bugs.webkit.org/attachment.cgi?id=261326&action=review >> Source/WebCore/page/Page.cpp:258 >> + m_inspectorController->inspectedPageDestroyed(); > > Why not? I think it makes sense to do it earlier just because why notify about detaching frames if the page is going away entirely. But I don't understand the "may not be able to cleanly disconnect" if a frame is being detached. The ChangeLog mentions the stub frontend channel was the issue here. What does that do that is different from normal behavior? If we don't do this, then the frontend's DOMWindow will get GC'd at some arbitrary time after being detached (thus calling ~Page, ~InspectorController, and finally ~FrontendClient, which knows to disconnect the FrontendChannel), and at that time the inspected page's InspectorController will have been deallocated. This is basically what happens right now, and it causes crashes because FrontendRouter still has a dangling channel. Other code paths don't have this problem because the frontend page will be deterministically destructed before or with the inspected page (WK1) or are separated by process boundary and have no relation (WK2). >> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:675 >> + inspectedPage->inspectorController().disconnectFrontend(_inspectorClient); > > When I expand to see surrounding code, there is code later on in this method that looks wrong: > > 689 if (Page* inspectedPage = [_inspectedWebView.get() page]) { > 690 inspectedPage->inspectorController().setInspectorFrontendClient(nullptr); > 691 inspectedPage->inspectorController().disconnectFrontend(_inspectorClient); > 692 } > 693 > > Looks like this is wrong and should be removed? Good catch, this is the code that is replaced and should be deleted. >> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:-435 >> - m_inspectedWebView->page()->inspectorController().disconnectFrontend(m_inspectorClient); > > Here is the same code in windows and you are removing these lines! This is the correct change.

Comment on attachment 261326 [details] Proposed Fix (win EWS) View in context: https://bugs.webkit.org/attachment.cgi?id=261326&action=review >>> Source/WebCore/page/Page.cpp:258 >>> + m_inspectorController->inspectedPageDestroyed(); >> >> Why not? I think it makes sense to do it earlier just because why notify about detaching frames if the page is going away entirely. But I don't understand the "may not be able to cleanly disconnect" if a frame is being detached. The ChangeLog mentions the stub frontend channel was the issue here. What does that do that is different from normal behavior? > > If we don't do this, then the frontend's DOMWindow will get GC'd at some arbitrary time after being detached (thus calling ~Page, ~InspectorController, and finally ~FrontendClient, which knows to disconnect the FrontendChannel), and at that time the inspected page's InspectorController will have been deallocated. This is basically what happens right now, and it causes crashes because FrontendRouter still has a dangling channel. > > Other code paths don't have this problem because the frontend page will be deterministically destructed before or with the inspected page (WK1) or are separated by process boundary and have no relation (WK2). I still don't follow this completely. If there was a dangling channel it sounds like a legitimate issue with the teardown process. The order of these few operations doesn't seem like they would change that. If the frontend stub uses window.open to create its frontend Page then frame detaching shouldn't matter. My understanding is that the new window is a new WebCore::Page that is mostly unrelated to the inspected WebCore::Page. Each may have their own frame tree, but they are separate. Still, this does seem like a worthwhile change. Why inform the inspector about each frame detaching if we are closing the entire page! Moving this early just seems like a good idea. I just don't think the comment is accurate. >>> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:675 >>> + inspectedPage->inspectorController().disconnectFrontend(_inspectorClient); >> >> When I expand to see surrounding code, there is code later on in this method that looks wrong: >> >> 689 if (Page* inspectedPage = [_inspectedWebView.get() page]) { >> 690 inspectedPage->inspectorController().setInspectorFrontendClient(nullptr); >> 691 inspectedPage->inspectorController().disconnectFrontend(_inspectorClient); >> 692 } >> 693 >> >> Looks like this is wrong and should be removed? > > Good catch, this is the code that is replaced and should be deleted. Okay r- for this then.

Created attachment 261404 [details] Proposed Fix The latest patch adds a crash fix to WebInspectorUI's disconnection code, and fixes the comment that we agreed was incorrect.

Comment on attachment 261404 [details] Proposed Fix Attachment 261404 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/180375 New failing tests: inspector/dom/highlightQuad.html inspector/console/console-api.html inspector/console/clearMessages.html inspector/runtime/saveResult.html inspector/codemirror/prettyprinting-css.html inspector/console/messageRepeatCountUpdated.html inspector/dom/pseudo-element-static.html inspector/unit-tests/event-listener.html

Created attachment 261409 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Comment on attachment 261404 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261404&action=review r- for a few remaining issues / questions. Sorry it took me this long to very closely review this patch =( > Source/WebCore/ChangeLog:23 > + 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. Not sure this part is still accurate. > Source/WebCore/inspector/InspectorController.cpp:301 > + while (InspectorInstrumentation::hasFrontends()) > + InspectorInstrumentation::frontendDeleted(); Wait. What? I'm having a how did this ever work moment. =( Imagine this scenario: 1. Create WebCore::Page 1 2. Locally Inspect Page 1 => InspectorInstrumentation::addFrontend [frontends=1] 3. Create WebCore::Page 2 in the same process (e.g. window.open) 4. Locally Inspect Page 2 => InspectorInstrumentation::addFrontend [frontends=2] 5. Close WebCore::Page 2 => inspectedPageDestroyed => disconnectAllFrontends => while (fronted) deleteFrontend => [frontends=0] => But there is still a frontend! I think this should only be deleting the frontends for _this_ Page. InspectorInstrumentation::deleteFrontends(m_inspectorRouter.frontendConnections()); Ultimately this just affects performance of the FAST_RETURN in InspectorInstrumentation. But it sounds worth fixing. > Source/WebCore/page/Page.cpp:257 > + // Notify the inspector before tearing down frames so it doesn't do unnecessary work > + // to process frame-closing instrumentation events right before closing itself. Nit: I don't think the comment is necessary. > Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:78 > void WebInspectorClient::inspectedPageDestroyed() > { > - closeLocalFrontend(); > delete this; > } Should this have rolled closeLocalFrontend into here? r- for this question. > Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:81 > + if (m_page->inspector()) By calling WebPage::inspector you might be creating the WebKit::WebInspector for the first time! That seems like it could be a bit inefficient. Might be better to add something like WebPage::hasLocalInspector()? > Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:122 > + m_frontendController->setInspectorFrontendClient(nullptr); > + m_frontendController = nullptr; Should we be worried about closeWindow happening multiple times? closeWindow() checks if (m_backendConnection). We should probably do that here too: if (m_frontendController) m_frontendController->setInspectorFrontendClient(nullptr); m_frontendController = nullptr;

Comment on attachment 261404 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261404&action=review >> Source/WebCore/ChangeLog:23 >> + methods invoked during test teardown must be reordered to avoid using dangling pointers. > > Not sure this part is still accurate. This is referring to InspectorController::disconnectFrontend. I guess it could be removed as this is explained below. >> Source/WebCore/inspector/InspectorController.cpp:301 >> + InspectorInstrumentation::frontendDeleted(); > > Wait. What? I'm having a how did this ever work moment. =( > > Imagine this scenario: > 1. Create WebCore::Page 1 > 2. Locally Inspect Page 1 => InspectorInstrumentation::addFrontend [frontends=1] > 3. Create WebCore::Page 2 in the same process (e.g. window.open) > 4. Locally Inspect Page 2 => InspectorInstrumentation::addFrontend [frontends=2] > 5. Close WebCore::Page 2 => inspectedPageDestroyed => disconnectAllFrontends => while (fronted) deleteFrontend => [frontends=0] > => But there is still a frontend! > > I think this should only be deleting the frontends for _this_ Page. > > InspectorInstrumentation::deleteFrontends(m_inspectorRouter.frontendConnections()); > > Ultimately this just affects performance of the FAST_RETURN in InspectorInstrumentation. But it sounds worth fixing. Oops, forgot that we still have multiple Pages per WebProcess when using window.open. This should be easy to fix, by adding FrontendRouter::frontendCount. InspectorInstrumentation is decrementing a static counter, so no need to know the concrete FrontendChannels involved. >> Source/WebCore/page/Page.cpp:257 >> + // to process frame-closing instrumentation events right before closing itself. > > Nit: I don't think the comment is necessary. Ok. It's in the changelog. >> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:78 >> } > > Should this have rolled closeLocalFrontend into here? r- for this question. No. It matches other WK1 InspectorClients that simply kill themselves at this point, after everything else has been destroyed. Here's the sequence of calls now: Page::~Page() // frontend Page InspectorController:inspectedPageDestroyed() FrontendClient::closeWindow() FrontendClient::destroyInspectorView() // If the frontend window is closed by user, jump in from here instead. InspectorClient::releaseFrontend() InspectorController::disconnectAllFrontends() InspectorClient::inspectedPageDestroyed() Page::~Page() // inspected page InspectorController::inspectedPageDestroyed() InspectorController::disconnectAllFrontends() InspectorClient::inspectedPageDestroyed() >> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:81 >> + if (m_page->inspector()) > > By calling WebPage::inspector you might be creating the WebKit::WebInspector for the first time! That seems like it could be a bit inefficient. > > Might be better to add something like WebPage::hasLocalInspector()? I would prefer making it possible to specify the lazy creation policy and set a default for the parameter. enum class LazyCreationPolicy { UseExistingOnly, CreateIfNeeded }; I did this in my next patch version and it makes it obvious where we are also probably instantiating where we don't mean to. >> Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:122 >> + m_frontendController = nullptr; > > Should we be worried about closeWindow happening multiple times? closeWindow() checks if (m_backendConnection). We should probably do that here too: > > if (m_frontendController) > m_frontendController->setInspectorFrontendClient(nullptr); > m_frontendController = nullptr; Doesn't hurt to add a guard. It seems like closeWindow is not something that can fail, though.

Created attachment 261424 [details] Proposed Fix Address Joe's latest comments.

> Page::~Page() // frontend Page > InspectorController:inspectedPageDestroyed() > FrontendClient::closeWindow() > FrontendClient::destroyInspectorView() > InspectorClient::releaseFrontend() > InspectorController::disconnectAllFrontends() > InspectorClient::inspectedPageDestroyed() This is gold!

Comment on attachment 261424 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=261424&action=review This looks much better! r=me > Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:82 > + if (m_page->inspector(WebPage::LazyCreationPolicy::UseExistingOnly)) > + m_page->inspector()->close(); Style: Could simplify a bit: if (WebInspector* inspector = m_page->inspector(WebPage::LazyCreationPolicy::UseExistingOnly)) inspector->close(); > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2952 > + if (!m_inspector && behavior == LazyCreationPolicy::UseExistingOnly) > + return nullptr; > if (!m_inspector) > m_inspector = WebInspector::create(this); Might as well combine these. Weird to see !m_inspector twice.

Committed r189970: <http://trac.webkit.org/changeset/189970>