Bug 200118 – Web Inspector: Timelines: disable related agents when the tab is closed

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

[Devin Rousso]

.

[Devin Rousso]

Created attachment 374892 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Joseph Pecoraro]

Comment on attachment 374892 [details]
Patch

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

This looks correct on the frontend. I have a few questions on the backend changes.

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:86
> +    m_tracking = false;

We already returned above if we are tracking so this seems wrong.

> Source/WebCore/inspector/agents/InspectorMemoryAgent.cpp:84
> +    m_tracking = false;
> +
> +    ResourceUsageThread::removeObserver(this);

This doesn't seem balanced. start/stopTracking handle the observer. If disable doesn't do anything while tracking then this seems unnecessary.

> Source/WebCore/inspector/agents/InspectorMemoryAgent.cpp:-101
> -    if (!m_enabled)
> -        return;

Nice. Yeah this point shouldn't even be reached if we're enabled.

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:-97
> -    m_instrumentingAgents.setPersistentInspectorTimelineAgent(this);

This change is confusing. The original idea behind a `persistent` agent was that it would always be around in InstrumentingAgents even if the frontend had not enabled (and I believe even if a frontend wasn't connected...)

Maybe the `persistent` part is not needed anymore and this would be just like the other instrumenting agent pointers.

> Source/WebInspectorUI/UserInterface/Controllers/HeapManager.js:26
>  WI.HeapManager = class HeapManager extends WI.Object

This can now have a warning comment at the top about multi-target support. We'll eventually need to get a snapshot from each target, and operate on HeapSnapshots per-target.

> Source/WebInspectorUI/UserInterface/Controllers/HeapManager.js:76
> +        HeapAgent.snapshot((error, timestamp, snapshotStringData) => {

This for example either would need a target or would snapshot all targets in a multi-target world.

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:269
> +        const objectGroup = undefined;
> +        WI.heapManager.getRemoteObject(this._node, objectGroup, (error, remoteObjectPayload) => {

Interesting... this should probably have an object group, but it is the global object so maybe we don't care since it will likely be kept alive forever. Though global objects in isolated worlds are maybe getting leaked from this... only while Web Inspector is open.

[Devin Rousso]

Comment on attachment 374892 [details]
Patch

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

>> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:86
>> +    m_tracking = false;
> 
> We already returned above if we are tracking so this seems wrong.

I actually don't think we should have the earlier return (I probably forgot to remove this from one of my earlier iterations).  If we `disable` while actively tracking, that SHOULD stop tracking.  `disable` should take precedence over everything.

>> Source/WebCore/inspector/agents/InspectorMemoryAgent.cpp:84
>> +    ResourceUsageThread::removeObserver(this);
> 
> This doesn't seem balanced. start/stopTracking handle the observer. If disable doesn't do anything while tracking then this seems unnecessary.

See previous response.

>> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:-97
>> -    m_instrumentingAgents.setPersistentInspectorTimelineAgent(this);
> 
> This change is confusing. The original idea behind a `persistent` agent was that it would always be around in InstrumentingAgents even if the frontend had not enabled (and I believe even if a frontend wasn't connected...)
> 
> Maybe the `persistent` part is not needed anymore and this would be just like the other instrumenting agent pointers.

The only reason we had `persistent` was to support auto-capture whenever the main frame navigates and `console.profile`/`console.profileEnd`.  If the Timelines tab isn't enabled, I don't think those should be either.

I do think it could use a better name, like `inspectorTimelineAgent` and `activelyTrackingInspectorTimelineAgent`.

[Devin Rousso]

Created attachment 375336 [details]
Patch

[Devin Rousso]

Created attachment 375455 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 375455 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 375455 [details]
Patch

Clearing flags on attachment: 375455

Committed r248286: <https://trac.webkit.org/changeset/248286>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53968929>