Bug 200118 - Web Inspector: Timelines: disable related agents when the tab is closed
Summary: Web Inspector: Timelines: disable related agents when the tab is closed
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: Devin Rousso
URL:
Keywords: InRadar
Depends on: 200117
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-24 21:39 PDT by Devin Rousso
Modified: 2019-08-05 19:23 PDT (History)
11 users (show)

See Also:


Attachments
Patch (53.25 KB, patch)
2019-07-25 09:28 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (75.31 KB, patch)
2019-08-01 12:43 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (78.58 KB, patch)
2019-08-02 14:18 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-07-24 21:39:58 PDT
.
Comment 1 Devin Rousso 2019-07-25 09:28:24 PDT
Created attachment 374892 [details]
Patch
Comment 2 Build Bot 2019-07-25 09:30:04 PDT Comment hidden (obsolete)
Comment 3 Joseph Pecoraro 2019-07-31 19:34:50 PDT
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.
Comment 4 Devin Rousso 2019-08-01 11:32:31 PDT
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`.
Comment 5 Devin Rousso 2019-08-01 12:43:01 PDT
Created attachment 375336 [details]
Patch
Comment 6 Devin Rousso 2019-08-02 14:18:14 PDT
Created attachment 375455 [details]
Patch
Comment 7 Joseph Pecoraro 2019-08-05 18:23:47 PDT
Comment on attachment 375455 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2019-08-05 19:22:06 PDT
Comment on attachment 375455 [details]
Patch

Clearing flags on attachment: 375455

Committed r248286: <https://trac.webkit.org/changeset/248286>
Comment 9 WebKit Commit Bot 2019-08-05 19:22:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-08-05 19:23:21 PDT
<rdar://problem/53968929>