Bug 148625 - Web Inspector: tighten up lifetimes for Agent-owned objects, and initialize agents using contexts
Summary: Web Inspector: tighten up lifetimes for Agent-owned objects, and initialize a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Blaze Burg
URL:
Keywords: InRadar
Depends on:
Blocks: InspectorHydra
  Show dependency treegraph
 
Reported: 2015-08-30 17:26 PDT by Blaze Burg
Modified: 2015-09-05 11:52 PDT (History)
8 users (show)

See Also:


Attachments
Proposed Fix (127.05 KB, patch)
2015-08-30 17:36 PDT, Blaze Burg
no flags Details | Formatted Diff | Diff
For EWS (127.03 KB, patch)
2015-09-05 09:13 PDT, Blaze Burg
no flags Details | Formatted Diff | Diff
For EWS (128.46 KB, patch)
2015-09-05 09:29 PDT, Blaze Burg
no flags Details | Formatted Diff | Diff
For EWS (136.79 KB, patch)
2015-09-05 10:30 PDT, Blaze Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Blaze Burg 2015-08-30 17:26:17 PDT
This makes the frontend connect/disconnect callbacks used only for enable/disable and handling per-frontend persistent data structures.
Comment 1 Radar WebKit Bug Importer 2015-08-30 17:26:30 PDT
<rdar://problem/22494085>
Comment 2 Blaze Burg 2015-08-30 17:36:59 PDT
Created attachment 260258 [details]
Proposed Fix

Depends on other bugs, so it won't apply yet.
Comment 3 Joseph Pecoraro 2015-09-02 18:58:59 PDT
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!
Comment 4 Blaze Burg 2015-09-05 09:13:46 PDT
Created attachment 260686 [details]
For EWS
Comment 5 WebKit Commit Bot 2015-09-05 09:15:28 PDT
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`)
Comment 6 Blaze Burg 2015-09-05 09:29:50 PDT
Created attachment 260688 [details]
For EWS
Comment 7 Blaze Burg 2015-09-05 10:27:00 PDT
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).
Comment 8 Blaze Burg 2015-09-05 10:30:21 PDT
Created attachment 260692 [details]
For EWS
Comment 9 WebKit Commit Bot 2015-09-05 11:52:25 PDT
Comment on attachment 260692 [details]
For EWS

Clearing flags on attachment: 260692

Committed r189438: <http://trac.webkit.org/changeset/189438>
Comment 10 WebKit Commit Bot 2015-09-05 11:52:29 PDT
All reviewed patches have been landed.  Closing bug.