Bug 148625 – Web Inspector: tighten up lifetimes for Agent-owned objects, and initialize agents using contexts

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

[Blaze Burg]

This makes the frontend connect/disconnect callbacks used only for enable/disable and handling per-frontend persistent data structures.

[Radar WebKit Bug Importer]

<rdar://problem/22494085>

[Blaze Burg]

Created attachment 260258 [details]
Proposed Fix

Depends on other bugs, so it won't apply yet.

[Joseph Pecoraro]

Comment on attachment 260258 [details]
Proposed Fix

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

r=me

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:127
> +    // FIXME: change this to notify agents which frontend has connected (by id).
> +    m_agents.didCreateFrontendAndBackend(nullptr, nullptr);

Yeah, I now think of this as a "firstFrontendConnected" setup event. In fact, for all the non-persistent agents (in Web) they could be lazily created at this point.

We had previously talked about automatically calling "enable" on agents. This would be the time to do that.

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:142
> +    // FIXME: change this to notify agents which frontend has disconnected (by id).
>      m_agents.willDestroyFrontendAndBackend(DisconnectReason::InspectorDestroyed);

Yeah, I now think of this as a "lastFrontendDisconnected" teardown event. In fact, for all the non-persistent agents (in Web) they could be destroyed at this point. But more likely it would be fine to just lazily create and not think about again.

We had previously talked about automatically calling "disable" on agents. This would be the time to do that.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:361
> -void InspectorCSSAgent::didCreateFrontendAndBackend(Inspector::FrontendRouter* frontendRouter, Inspector::BackendDispatcher* backendDispatcher)
> +void InspectorCSSAgent::didCreateFrontendAndBackend(Inspector::FrontendRouter*, Inspector::BackendDispatcher*)
>  {
> -    m_frontendDispatcher = std::make_unique<CSSFrontendDispatcher>(frontendRouter);
> -    m_backendDispatcher = CSSBackendDispatcher::create(backendDispatcher, this);
>  }

There is code in some of these agents that checks for a frontendDispatcher:

    inspector/InspectorCSSAgent.cpp
    1182:    if (m_frontendDispatcher)

    inspector/InspectorDatabaseAgent.cpp
    204:    if (m_frontendDispatcher && m_enabled)

    inspector/InspectorTimelineAgent.cpp
    187:    if (m_frontendDispatcher)
    216:    if (m_frontendDispatcher)

    inspector/InspectorWorkerAgent.cpp
    194:    if (m_frontendDispatcher && m_enabled)

    inspector/WebConsoleAgent.cpp
    74:    if (m_frontendDispatcher && m_monitoringXHREnabled) {

Seems you can eliminate the relevant check!

> Source/WebCore/inspector/InspectorController.cpp:113
> +    AgentContext baseContext = {
> +        *this,
> +        *m_injectedScriptManager,
> +        m_frontendRouter.get(),
> +        m_backendDispatcher.get()
> +    };
> +
> +    WebAgentContext webContext = {
> +        baseContext,
> +        m_instrumentingAgents.get()
> +    };
> +
> +    PageAgentContext pageContext = {
> +        webContext,
> +        m_page
> +    };

Nice!

[Blaze Burg]

Created attachment 260686 [details]
For EWS

[WebKit Commit Bot]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Blaze Burg]

Created attachment 260688 [details]
For EWS

[Blaze Burg]

Comment on attachment 260258 [details]
Proposed Fix

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

>> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:127
>> +    m_agents.didCreateFrontendAndBackend(nullptr, nullptr);
> 
> Yeah, I now think of this as a "firstFrontendConnected" setup event. In fact, for all the non-persistent agents (in Web) they could be lazily created at this point.
> 
> We had previously talked about automatically calling "enable" on agents. This would be the time to do that.

the next patch in series will change it to

enum class WasFirstFrontend { No, Yes };
enum class WasLastFrontend { No, Yes };
virtual void frontendAttached(FrontendToken, WasFirstFrontend);
virtual void frontendDetached(FrontendToken, WasLastFrontend);

>> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:142
>>      m_agents.willDestroyFrontendAndBackend(DisconnectReason::InspectorDestroyed);
> 
> Yeah, I now think of this as a "lastFrontendDisconnected" teardown event. In fact, for all the non-persistent agents (in Web) they could be destroyed at this point. But more likely it would be fine to just lazily create and not think about again.
> 
> We had previously talked about automatically calling "disable" on agents. This would be the time to do that.

We should check per-agent to make sure it's doing the right thing.

>> Source/WebCore/inspector/InspectorCSSAgent.cpp:361
>>  }
> 
> There is code in some of these agents that checks for a frontendDispatcher:
> 
>     inspector/InspectorCSSAgent.cpp
>     1182:    if (m_frontendDispatcher)
> 
>     inspector/InspectorDatabaseAgent.cpp
>     204:    if (m_frontendDispatcher && m_enabled)
> 
>     inspector/InspectorTimelineAgent.cpp
>     187:    if (m_frontendDispatcher)
>     216:    if (m_frontendDispatcher)
> 
>     inspector/InspectorWorkerAgent.cpp
>     194:    if (m_frontendDispatcher && m_enabled)
> 
>     inspector/WebConsoleAgent.cpp
>     74:    if (m_frontendDispatcher && m_monitoringXHREnabled) {
> 
> Seems you can eliminate the relevant check!

There were plenty others too. I removed them. Most of them looked like null checks, though some perform different logic when nothing is attached. I think I'll do a per-agent audit of these when I also go through and change data structures to be per-fronted (or use log-replay).

[Blaze Burg]

Created attachment 260692 [details]
For EWS

[WebKit Commit Bot]

Comment on attachment 260692 [details]
For EWS

Clearing flags on attachment: 260692

Committed r189438: <http://trac.webkit.org/changeset/189438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.