Bug 148625

<rdar://problem/22494085>

Created attachment 260258 [details] Proposed Fix Depends on other bugs, so it won't apply yet.

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!

Created attachment 260686 [details] For EWS

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`)

Created attachment 260688 [details] For EWS

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).

Created attachment 260692 [details] For EWS

Comment on attachment 260692 [details] For EWS Clearing flags on attachment: 260692 Committed r189438: <http://trac.webkit.org/changeset/189438>

All reviewed patches have been landed. Closing bug.