Bug 148538 – Web Inspector: InspectorController should support multiple frontend channels

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

First step.

[Radar WebKit Bug Importer]

<rdar://problem/22462778>

[Blaze Burg]

Created attachment 260191 [details]
Proposed Fix

Here we go.. expecting a few rounds with EWS.

[Joseph Pecoraro]

Comment on attachment 260191 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=260191&action=review

Leaving the review flag for my questions about first frontend connection, last frontend disconnection.

> Source/JavaScriptCore/CMakeLists.txt:326
>      inspector/InspectorAgentRegistry.cpp
> +    inspector/InspectorFrontendRouter.cpp
>      inspector/InspectorBackendDispatcher.cpp

Nit: `sort` order.

> Source/JavaScriptCore/inspector/InspectorFrontendRouter.h:40
> +    bool hasFrontends() const { return m_connections.size() > 0; }

Would !Vector::isEmpty be clearer?

> Source/JavaScriptCore/inspector/InspectorFrontendRouter.h:55
> +    Vector<FrontendChannel*, 2> m_connections;

Nice use of size hint!

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:116
> +    m_agents.didCreateFrontendAndBackend(frontendChannel, &m_backendDispatcher.get());

Is the plan that this should happen when adding the first frontend?

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:130
> +    m_agents.willDestroyFrontendAndBackend(DisconnectReason::InspectorDestroyed);

Is the plan that this should happen when removing the last frontend?

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:-154
> -    if (!m_frontendChannel)
> -        return;

I think the equivalent of this would be:

    if (!m_frontendRouter.hasFrontends())
        return;

I'm not quite sure of the consequences here.

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:276
> -    if (m_frontendChannel)
> -        agent->didCreateFrontendAndBackend(m_frontendChannel, m_backendDispatcher.get());
> +    agent->didCreateFrontendAndBackend(m_frontendRouter->leakChannel(), &m_backendDispatcher.get());
>  
>      m_agents.appendExtraAgent(WTF::move(agent));
>  
> -    if (m_frontendChannel)
> -        m_inspectorAgent->activateExtraDomain(domainName);
> +    m_inspectorAgent->activateExtraDomain(domainName);

Same here. The consequences here seem to just be that you might be doing work in an agent that gets repeated when connectFrontend happens.

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:20398
>      <ClInclude Include="..\ForwardingHeaders\inspector\InspectorFrontendChannel.h" />
> +    <ClInclude Include="..\ForwardingHeaders\inspector\InspectorFrontendRouter.h" />
>      <ClInclude Include="..\ForwardingHeaders\inspector\InspectorBackendDispatchers.h" />
>      <ClInclude Include="..\ForwardingHeaders\inspector\InspectorFrontendDispatchers.h" />

You could correct the sort order while you are here!

> Source/WebCore/inspector/InspectorController.cpp:266
>  #if ENABLE(REMOTE_INSPECTOR)
> -    if (!hasRemoteFrontend())
> +    if (!m_frontendRouter->hasRemoteFrontend() && didHaveRemoteFrontend)
>          m_page.remoteInspectorInformationDidChange();
>  #endif

The purpose of this here is to inform any remote debuggers if this debuggable has a debugger or not. So I think this may have been flawed originally, and it probably should be the same as first/last disconnect conditions. Or for now:

    if (m_frontendRouter->hasFrontends() && didHaveFrontends())

> Source/WebCore/testing/Internals.cpp:1736
> -    bool isAutomaticInspection = false;
>      m_frontendChannel = std::make_unique<InspectorFrontendChannelDummy>(frontendPage);
> -    page->inspectorController().connectFrontend(m_frontendChannel.get(), isAutomaticInspection);
> +    page->inspectorController().connectFrontend(m_frontendChannel.get());

Arg, I hate default parameters. It is easy to make mistakes. But this seems reasonable =)

[Blaze Burg]

Created attachment 260204 [details]
Proposed Fix (EWS)

[Blaze Burg]

Comment on attachment 260191 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=260191&action=review

>> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:116
>> +    m_agents.didCreateFrontendAndBackend(frontendChannel, &m_backendDispatcher.get());
> 
> Is the plan that this should happen when adding the first frontend?

Correct. I tried doing that earlier, but was hitting other issues so I reverted it to help isolate the problem. I will try altering the condition to what you said again, now that everything else is working.

>> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:130
>> +    m_agents.willDestroyFrontendAndBackend(DisconnectReason::InspectorDestroyed);
> 
> Is the plan that this should happen when removing the last frontend?

Yup, see above.

>> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:-154
>> -        return;
> 
> I think the equivalent of this would be:
> 
>     if (!m_frontendRouter.hasFrontends())
>         return;
> 
> I'm not quite sure of the consequences here.

Is this code related to automatic inspection and auto-resuming? Will investigate.

>> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:276
>> +    m_inspectorAgent->activateExtraDomain(domainName);
> 
> Same here. The consequences here seem to just be that you might be doing work in an agent that gets repeated when connectFrontend happens.

How will we know to activate the domain once a frontend connects? Or are these always done together?

>> Source/WebCore/inspector/InspectorController.cpp:266
>>  #endif
> 
> The purpose of this here is to inform any remote debuggers if this debuggable has a debugger or not. So I think this may have been flawed originally, and it probably should be the same as first/last disconnect conditions. Or for now:
> 
>     if (m_frontendRouter->hasFrontends() && didHaveFrontends())

yeah, this condition will change once we want to allow multiple remotes to actually connect. But for now, when should it notify: when a frontend has become detached or when it has become attached?

>> Source/WebCore/testing/Internals.cpp:1736
>> +    page->inspectorController().connectFrontend(m_frontendChannel.get());
> 
> Arg, I hate default parameters. It is easy to make mistakes. But this seems reasonable =)

It is used so infrequently I thought it was appropriate here.

[Blaze Burg]

Created attachment 260214 [details]
Proposed Fix (EWS)

[Joseph Pecoraro]

(In reply to comment #5)
> Comment on attachment 260191 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260191&action=review
> 
> >> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:116
> >> +    m_agents.didCreateFrontendAndBackend(frontendChannel, &m_backendDispatcher.get());
> > 
> > Is the plan that this should happen when adding the first frontend?
> 
> Correct. I tried doing that earlier, but was hitting other issues so I
> reverted it to help isolate the problem. I will try altering the condition
> to what you said again, now that everything else is working.
> 
> >> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:130
> >> +    m_agents.willDestroyFrontendAndBackend(DisconnectReason::InspectorDestroyed);
> > 
> > Is the plan that this should happen when removing the last frontend?
> 
> Yup, see above.
> 
> >> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:-154
> >> -        return;
> > 
> > I think the equivalent of this would be:
> > 
> >     if (!m_frontendRouter.hasFrontends())
> >         return;
> > 
> > I'm not quite sure of the consequences here.
> 
> Is this code related to automatic inspection and auto-resuming? Will
> investigate.

Yes, it is. Ping me on IRC if needed and I can show you how to test this / explain how it works.

> >> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:276
> >> +    m_inspectorAgent->activateExtraDomain(domainName);
> > 
> > Same here. The consequences here seem to just be that you might be doing work in an agent that gets repeated when connectFrontend happens.
> 
> How will we know to activate the domain once a frontend connects? Or are
> these always done together?

The normal path is inside JSGlobalObjectInspectorController::connectFrontend (line 123). When a frontend connects we tell it about all the extra domains:

    m_inspectorAgent->activateExtraDomains(m_agents.extraDomains());

This path here (line 276) is handling when an extra domain is activated and a frontend is already connected, it does the normal agent setup and informs the frontend about this new extra domain.


> >> Source/WebCore/inspector/InspectorController.cpp:266
> >>  #endif
> > 
> > The purpose of this here is to inform any remote debuggers if this debuggable has a debugger or not. So I think this may have been flawed originally, and it probably should be the same as first/last disconnect conditions. Or for now:
> > 
> >     if (m_frontendRouter->hasFrontends() && didHaveFrontends())
> 
> yeah, this condition will change once we want to allow multiple remotes to
> actually connect. But for now, when should it notify: when a frontend has
> become detached or when it has become attached?

Given how uncommon a frontend connecting/disconnecting is, and how likely it is in practice to actually change some information about the remote debuggable it wouldn't be unreasonable to just always do it when a frontend connects/disconnects.

[Blaze Burg]

The new patch addresses these comments, except the vcxproj reorderings. I hope we nuke those files soon anyway.

[Joseph Pecoraro]

Comment on attachment 260214 [details]
Proposed Fix (EWS)

View in context: https://bugs.webkit.org/attachment.cgi?id=260214&action=review

r=me.

I think it might be nicer for clarity to have distinct methods for the code that gets run in the distinct cases.

    Controller::connectFrontend
    Controller::disconnectFrontend

    Controller::firstFrontendConnected (setup?)
    Controller::lastFrontendDisconnected (teardown?)

But it may just be diff ugliness that makes it harder to follow this.

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:-154
> -    if (!m_frontendChannel)
> -        return;

I still think this can be if (!hasFrontends).

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:-266
> -    if (m_frontendChannel)
> -        agent->didCreateFrontendAndBackend(m_frontendChannel, m_backendDispatcher.get());

Likewise. if (hasFrontend)

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:-271
> -    if (m_frontendChannel)
> -        m_inspectorAgent->activateExtraDomain(domainName);

Likewise. if (hasFrontend)

> Source/WebCore/inspector/InspectorController.cpp:272
>  #if ENABLE(REMOTE_INSPECTOR)
> -    if (!hasRemoteFrontend())
> +    if (!m_frontendRouter->hasFrontends() && didHaveRemoteFrontend)
>          m_page.remoteInspectorInformationDidChange();
> +#else
> +    UNUSED_PARAM(didHaveRemoteFrontend);
>  #endif
> +}

Lets simplify and just always call m_page.remoteInspectorInformationDidChange if we are removing the last frontend.

[Blaze Burg]

Created attachment 260530 [details]
Proposed Fix (for EWS)

[Blaze Burg]

Created attachment 260534 [details]
Proposed Fix (for Win EWS)

[Blaze Burg]

Created attachment 260541 [details]
Proposed Fix (for Win EWS)

[WebKit Commit Bot]

Comment on attachment 260541 [details]
Proposed Fix (for Win EWS)

Clearing flags on attachment: 260541

Committed r189338: <http://trac.webkit.org/changeset/189338>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 148785

[Joseph Pecoraro]

Bots are seeing ASSERTs:

ASSERTION FAILED: m_frontendRouter->hasFrontends()
/Volumes/Data/slave/yosemite-debug/build/Source/WebCore/inspector/WorkerInspectorController.cpp(130) : void WebCore::WorkerInspectorController::disconnectFrontend(Inspector::DisconnectReason)
1   0x114fb7190 WTFCrash
2   0x118b6c8eb WebCore::WorkerInspectorController::disconnectFrontend(Inspector::DisconnectReason)
3   0x118b6c627 WebCore::WorkerInspectorController::~WorkerInspectorController()
4   0x118b6caa5 WebCore::WorkerInspectorController::~WorkerInspectorController()
5   0x118b6cac9 WebCore::WorkerInspectorController::~WorkerInspectorController()
6   0x118b61844 WebCore::WorkerGlobalScope::~WorkerGlobalScope()
7   0x116e6e825 WebCore::DedicatedWorkerGlobalScope::~DedicatedWorkerGlobalScope()
8   0x116e6e845 WebCore::DedicatedWorkerGlobalScope::~DedicatedWorkerGlobalScope()
9   0x116e6e899 WebCore::DedicatedWorkerGlobalScope::~DedicatedWorkerGlobalScope()
10  0x116e70103 WTF::RefCounted<WebCore::WorkerGlobalScope>::deref()
11  0x117dc0f11 void WTF::derefIfNotNull<WebCore::WorkerGlobalScope>(WebCore::WorkerGlobalScope*)
12  0x118b91757 WTF::RefPtr<WebCore::WorkerGlobalScope>::operator=(std::nullptr_t)
13  0x118b8dc10 WebCore::WorkerThread::workerThread()
14  0x118b8d6e5 WebCore::WorkerThread::workerThreadStart(void*)
15  0x11501fd89 WTF::createThread(void (*)(void*), void*, char const*)::$_0::operator()() const
16  0x11501fd5c std::__1::__function::__func<WTF::createThread(void (*)(void*), void*, char const*)::$_0, std::__1::allocator<WTF::createThread(void (*)(void*), void*, char const*)::$_0>, void ()>::operator()()
17  0x114aa4e2a std::__1::function<void ()>::operator()() const
18  0x11501ebae WTF::threadEntryPoint(void*)
19  0x115020321 WTF::wtfThreadEntryPoint(void*)
20  0x7fff82a6a05a _pthread_body
21  0x7fff82a69fd7 _pthread_body
22  0x7fff82a673ed thread_start

[Blaze Burg]

Created attachment 260605 [details]
Proposed Fix (Fix Crash)

Turns out we were unconditionally disconnecting worker controller's frontend channel, when this work was already being done when the workers were spun down.

[WebKit Commit Bot]

Comment on attachment 260605 [details]
Proposed Fix (Fix Crash)

Clearing flags on attachment: 260605

Committed r189375: <http://trac.webkit.org/changeset/189375>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.