Bug 149006 - ASSERT(!m_frontendRouter->hasLocalFrontend()) when running Web Inspector tests
Summary: ASSERT(!m_frontendRouter->hasLocalFrontend()) when running Web Inspector tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on: 149071
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-09 12:17 PDT by Alexey Proskuryakov
Modified: 2015-09-18 10:16 PDT (History)
11 users (show)

See Also:


Attachments
Proposed Fix (88.48 KB, patch)
2015-09-11 18:10 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (53.91 KB, patch)
2015-09-12 11:06 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (42.71 KB, patch)
2015-09-12 11:10 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (updated WebKit/win) (45.64 KB, patch)
2015-09-14 09:16 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (win EWS) (45.58 KB, patch)
2015-09-16 14:07 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (48.00 KB, patch)
2015-09-17 11:19 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (1.13 MB, application/zip)
2015-09-17 12:08 PDT, Build Bot
no flags Details
Proposed Fix (51.03 KB, patch)
2015-09-17 14:31 PDT, BJ Burg
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2015-09-09 12:17:02 PDT
Saw this assertion failure on inspector/dom/getAccessibilityPropertiesForNode.html:

https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r189542%20(15497)/results.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000104702b6a WTFCrash + 42 (Assertions.cpp:321)
1   com.apple.WebCore             	0x00000001099d45e6 WebCore::InspectorController::close() + 150 (InspectorController.cpp:331)
2   com.apple.WebKitLegacy        	0x000000011095a1cb -[WebInspector close:] + 75 (WebInspector.mm:162)
3   DumpRenderTree                	0x00000001038aa685 TestRunner::closeWebInspector() + 85 (TestRunnerMac.mm:777)
4   DumpRenderTree                	0x0000000103861fa9 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 6777 (DumpRenderTree.mm:2047)
5   DumpRenderTree                	0x00000001038604ca runTestingServerLoop() + 282 (DumpRenderTree.mm:1180)
6   DumpRenderTree                	0x000000010385fada dumpRenderTree(int, char const**) + 506 (DumpRenderTree.mm:1289)
7   DumpRenderTree                	0x000000010386269d DumpRenderTreeMain(int, char const**) + 125 (DumpRenderTree.mm:1424)
8   DumpRenderTree                	0x00000001038b8e92 main + 34 (DumpRenderTreeMain.mm:30)
9   libdyld.dylib                 	0x00007fff8c3d55fd start + 1
Comment 1 Radar WebKit Bug Importer 2015-09-09 12:17:22 PDT
<rdar://problem/22631369>
Comment 2 BJ Burg 2015-09-10 15:56:30 PDT
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.
Comment 3 BJ Burg 2015-09-11 18:10:18 PDT
Created attachment 261039 [details]
Proposed Fix

Hooo boy, it's finally working. Find me on IRC when you have questions.
Comment 4 WebKit Commit Bot 2015-09-11 18:12:15 PDT
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.
Comment 5 BJ Burg 2015-09-11 18:13:11 PDT
BTW, probably depends on 149071 to apply. I will land that and rebase tonight or tomorrow.
Comment 6 Joseph Pecoraro 2015-09-11 20:02:26 PDT
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.
Comment 7 BJ Burg 2015-09-12 11:06:12 PDT
Created attachment 261058 [details]
Proposed Fix

Rebased, should apply and be easier to review now.
Comment 8 BJ Burg 2015-09-12 11:10:50 PDT
Created attachment 261059 [details]
Proposed Fix
Comment 9 Alexey Proskuryakov 2015-09-12 11:48:22 PDT
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?
Comment 10 BJ Burg 2015-09-14 09:16:33 PDT
Created attachment 261111 [details]
Proposed Fix (updated WebKit/win)
Comment 11 Alexey Proskuryakov 2015-09-15 23:57:17 PDT
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]
Comment 12 BJ Burg 2015-09-16 14:07:28 PDT
Created attachment 261326 [details]
Proposed Fix (win EWS)
Comment 13 Joseph Pecoraro 2015-09-16 15:24:56 PDT
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 14 BJ Burg 2015-09-16 15:52:14 PDT
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 15 Joseph Pecoraro 2015-09-16 17:17:18 PDT
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.
Comment 16 BJ Burg 2015-09-17 11:19:11 PDT
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 17 Build Bot 2015-09-17 12:08:33 PDT
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
Comment 18 Build Bot 2015-09-17 12:08:38 PDT
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 19 Joseph Pecoraro 2015-09-17 12:20:26 PDT
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 20 BJ Burg 2015-09-17 13:32:53 PDT
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.
Comment 21 BJ Burg 2015-09-17 14:31:43 PDT
Created attachment 261424 [details]
Proposed Fix

Address Joe's latest comments.
Comment 22 Joseph Pecoraro 2015-09-17 16:07:58 PDT
> Page::~Page() // frontend Page
>     InspectorController:inspectedPageDestroyed()
>         FrontendClient::closeWindow()
>             FrontendClient::destroyInspectorView()
>                 InspectorClient::releaseFrontend()
>         InspectorController::disconnectAllFrontends()
>             InspectorClient::inspectedPageDestroyed()

This is gold!
Comment 23 Joseph Pecoraro 2015-09-17 16:19:41 PDT
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.
Comment 24 BJ Burg 2015-09-18 10:16:41 PDT
Committed r189970: <http://trac.webkit.org/changeset/189970>