Bug 219987 – Web Inspector: console.takeHeapSnapshot() appears to have no effect

• Component: Web Inspector
• Status: RESOLVED FIXED

[Dario D'Amico]

When console.takeHeapSnapshot() is executed, nothing happens in the Web Inspector snapshot list.

This affects Safari 14.0.1 on macOS Catalina 10.15.7.

Video of the repro: https://drive.google.com/file/d/1bx8gtPJVDepsvTpt7Qd3jAS9sKRQ2qV7/view

[Radar WebKit Bug Importer]

<rdar://problem/72434183>

[Dario D'Amico]

This behavior cannot be reproduced in Safari Technology Preview.
I tried Release 117 (Safari 14.1, WebKit 15611.1.7.2) and console.takeHeapSnapshot() just works.

See also https://bugs.webkit.org/show_bug.cgi?id=216057

I presume this could mean that this issue could be closed?

[Patrick Angle]

I've been looking at this issue today and can also get `console.takeHeapSnapshot()` to work, but it stops working after 15-45 seconds on the webkit.org homepage. Once it stops working, I can get it working again by using the `Take snapshot` (icon is a Camera) button above the snapshot list, so it appears this is still an issue in the latest builds. I'm still investigating the root cause of this behavior.

Can you confirm that after allowing some time to pass between page load and taking a snapshot that taking the snapshot via the console works for you?

[Patrick Angle]

Created attachment 416670 [details]
Patch v1.0

[Dario D'Amico]

I can confirm; I would add that not only the camera button makes it work again, but also reveal the snapshots taken using the console API, that were not showing. After clicking the camera once, it does not happen anymore.

While I didn't notice this behavior specifically when I suggested that the issue had to be closed, I also had a feeling I was having range/filtering/UI issues, but I wasn't able to repro so I just dismissed that as myself doing the wrong thing. I think your fix makes sense. Thanks!

[Dario D'Amico]

I would also mention that all my snapshot-taking trials were outside of a recording session; for my use case I am quite uninterested in the timeline itself, so I don't really have any UX expectation as to what should happen if you snapshot while recording.

However, talking about timeline recording itself, I kinda find it weird that the ranges don't update as the recording progresses; this is not only bad for the snapshot list, it is also bad for the memory pie charts that at some point will stop updating if the recording time has passed the END marker. You have to remember to move the END marker in the future if you want to keep seeing the pie charts updating in real time.

Does your patch address that too?

[Patrick Angle]

(In reply to Dario D'Amico from comment #5, #6)
Thanks for following up and confirming!

> However, talking about timeline recording itself, I kinda find it weird that
> the ranges don't update as the recording progresses; this is not only bad
> for the snapshot list, it is also bad for the memory pie charts that at some
> point will stop updating if the recording time has passed the END marker.
> You have to remember to move the END marker in the future if you want to
> keep seeing the pie charts updating in real time.
> 
> Does your patch address that too?

This patch doesn't address that, no. However, from the `Timelines` tab's navigation bar where you see `Timeline Recording [n] > [record type] > 0ms — 15.00s` you can click on the time portion (`0ms — 15.00s`) and select `Entire Recording` to avoid needing to manually select the full range.

[Devin Rousso]

(In reply to Patrick Angle from comment #7)
> (In reply to Dario D'Amico from comment #5, #6)
> Thanks for following up and confirming!
> 
> > However, talking about timeline recording itself, I kinda find it weird that the ranges don't update as the recording progresses; this is not only bad for the snapshot list, it is also bad for the memory pie charts that at some point will stop updating if the recording time has passed the END marker. You have to remember to move the END marker in the future if you want to keep seeing the pie charts updating in real time.
> > 
> > Does your patch address that too?
> 
> This patch doesn't address that, no. However, from the `Timelines` tab's navigation bar where you see `Timeline Recording [n] > [record type] > 0ms — 15.00s` you can click on the time portion (`0ms — 15.00s`) and select `Entire Recording` to avoid needing to manually select the full range.

You can also just double-click on any empty space in the overview graph area to select the entire recording.  <https://webkit.org/web-inspector/timelines-tab/#selecting-ranges>

[Devin Rousso]

Comment on attachment 416670 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:289
> +        this.dispatchEventToListeners(WI.TimelineView.Event.NeedsEntireSelectedRange);

We probably don't want to do this.  We automatically take heap snapshots every 10s, so this would mean that any attempt to narrow down the range of a timeline recording while it's still recording would be frustrating, as the selection would reset every 10s.

I intentionally made it so that we only select the full range when importing or clicking the button in the UI so that a malicious page (e.g. one that calls `console.takeHeapSnapshot()` frequently) can't make the UX awful.

I think a better solution to this would be to show some sort of banner in the JavaScript Allocations timeline view when a new heap snapshot is added that is outside of the selected range that has a button to change the selected range to either just extend to include the new snapshot or just select all of time.  I'm imagining something along the lines of what happens in the Console if you've selected the "Evaluations" filter and then evaluate `console.error(42);`.

[Blaze Burg]

Comment on attachment 416670 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:289
>> +        this.dispatchEventToListeners(WI.TimelineView.Event.NeedsEntireSelectedRange);
> 
> We probably don't want to do this.  We automatically take heap snapshots every 10s, so this would mean that any attempt to narrow down the range of a timeline recording while it's still recording would be frustrating, as the selection would reset every 10s.
> 
> I intentionally made it so that we only select the full range when importing or clicking the button in the UI so that a malicious page (e.g. one that calls `console.takeHeapSnapshot()` frequently) can't make the UX awful.
> 
> I think a better solution to this would be to show some sort of banner in the JavaScript Allocations timeline view when a new heap snapshot is added that is outside of the selected range that has a button to change the selected range to either just extend to include the new snapshot or just select all of time.  I'm imagining something along the lines of what happens in the Console if you've selected the "Evaluations" filter and then evaluate `console.error(42);`.

I like the console approach for consistency.

There are multiple ways to use heap snapshots, and I could see either option (fixed selection range or scrolling selection range) being useful in some situations.

Created attachment 416824 [details]
certificao

[Patrick Angle]

Created attachment 417029 [details]
Patch v2.0 - Banner Approach

[Patrick Angle]

Created attachment 417030 [details]
Video of Patch v2.0

[Devin Rousso]

Comment on attachment 417029 [details]
Patch v2.0 - Banner Approach

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

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:91
> +        this._rangeFilteredSnapshotsBanner = new WI.View;

Aside: I wonder if we should make a `WI.BannerView` or something that basically does all this so that we can avoid this duplicated logic (and CSS) in all the places that we use banners 🤔

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:95
> +        expandRangeButton.textContent = "Select Entire Range";

`WI.UIString`

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:387
> +        if (this._dataGrid.children.some((child) => child.hidden && (child.record.timestamp < this.filterStartTime || child.record.timestamp > this.filterEndTime) && (!filterTextRegex || filterTextRegex.test(child.filterableData)) )) {

Can you give an example of where `child.hidden` isn't enough?

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:388
> +            if (!this.subviews.includes(this._rangeFilteredSnapshotsBanner))

Rather than do an array search, can we just `!this._rangeFilteredSnapshotsBanner.isAttached` or `this._rangeFilteredSnapshotsBanner.parentView !== this`?

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:391
> +            if (this.subviews.includes(this._rangeFilteredSnapshotsBanner))

ditto (:388)

[Patrick Angle]

Comment on attachment 417029 [details]
Patch v2.0 - Banner Approach

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

>> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:91
>> +        this._rangeFilteredSnapshotsBanner = new WI.View;
> 
> Aside: I wonder if we should make a `WI.BannerView` or something that basically does all this so that we can avoid this duplicated logic (and CSS) in all the places that we use banners 🤔

Probably not the worst idea - the visual of a banner now at least appears in three places: Here, `WI.LogContentView.prototype._showHiddenMessagesBannerIfNeeded`, and `WI.LocalResourceOverrideWarningView`. I'll open a bug for this and reference those locations.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:93
> +        this._rangeFilteredSnapshotsBanner.element.appendChild(document.createTextNode("There are additional snapshots outside of the selected range."));

Also `WI.UIString` so I don't forget.

>> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:387
>> +        if (this._dataGrid.children.some((child) => child.hidden && (child.record.timestamp < this.filterStartTime || child.record.timestamp > this.filterEndTime) && (!filterTextRegex || filterTextRegex.test(child.filterableData)) )) {
> 
> Can you give an example of where `child.hidden` isn't enough?

Given a range of 0-15 seconds and the following three snapshots...
```
Snapshot 1    10.0s
Snapshot 2    14.0s
Snapshot 3    21.0s
```
...and filter text of `Snapshot 2` we would expect a single snapshot to be listed: `Snapshot 2`. In that situation, without the additional checks the banner will be visible even though we are showing all the possible snapshots we could, and the range is not reducing the visible snapshots. The child is marked as hidden if it is either out of range or filtered by the filter text, so the additional checks determine why it was hidden.

In the attached video, you can see that when I filter items using the filter text field, the banner goes away when there wouldn't be anything else to show with a wider range, and returns when removing the search since the range is now the only reason those items are not visible.

[Devin Rousso]

Comment on attachment 417029 [details]
Patch v2.0 - Banner Approach

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

>>> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:387
>>> +        if (this._dataGrid.children.some((child) => child.hidden && (child.record.timestamp < this.filterStartTime || child.record.timestamp > this.filterEndTime) && (!filterTextRegex || filterTextRegex.test(child.filterableData)) )) {
>> 
>> Can you give an example of where `child.hidden` isn't enough?
> 
> Given a range of 0-15 seconds and the following three snapshots...
> ```
> Snapshot 1    10.0s
> Snapshot 2    14.0s
> Snapshot 3    21.0s
> ```
> ...and filter text of `Snapshot 2` we would expect a single snapshot to be listed: `Snapshot 2`. In that situation, without the additional checks the banner will be visible even though we are showing all the possible snapshots we could, and the range is not reducing the visible snapshots. The child is marked as hidden if it is either out of range or filtered by the filter text, so the additional checks determine why it was hidden.
> 
> In the attached video, you can see that when I filter items using the filter text field, the banner goes away when there wouldn't be anything else to show with a wider range, and returns when removing the search since the range is now the only reason those items are not visible.

I see.  I didn't realize that you're doing this every time the filter changes as well.  Rather than doing that (which could be somewhat expensive), could we instead add some logic inside `layout` that just checks to see if the newly added `WI.HeapAllocationsTimelineDataGridNode` is immediately `hidden`?  It seems like extra work (and unnecessary exposing of info) to recalculate this each time the filter changes and in general layouts (even when no new heap snapshots are added).  I think we should exactly match the behavior of the Console so that we're consistent throughout Web Inspector.  IIRC, the Console shows a warning banner whenever a new message is added that is initially hidden.  It does this by keeping a `Set` of messages that are initially hidden, removing messages once they are shown.  So long as the `Set` has items, the banner stays visible (including if the filter changes so long as it doesn't cause all messages to become visible).  I think we should do something similar here, where we keep track of the initially hidden heap snapshots and continue showing the banner so long as at least one of those initially hidden heap snapshots is still hidden (you'd may want to hook into either `filterDidChange` as you have or create a similar `nodeWasFiltered` so that you can know when any initially hidden heap snapshot has been shown).  I think the goal of this should be "hey there's something new you haven't seen yet" not "hey there's something hidden from you".

Also, does `WI.TimelineView.Event.NeedsEntireSelectedRange` also cause the filter bar to be reset too?  If not, then the text "Select Entire Range" is not accurate because clicking it won't cause the banner to disappear as there will still be a filtered node.

[Patrick Angle]

Comment on attachment 417029 [details]
Patch v2.0 - Banner Approach

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

>>>> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:387
>>>> +        if (this._dataGrid.children.some((child) => child.hidden && (child.record.timestamp < this.filterStartTime || child.record.timestamp > this.filterEndTime) && (!filterTextRegex || filterTextRegex.test(child.filterableData)) )) {
>>> 
>>> Can you give an example of where `child.hidden` isn't enough?
>> 
>> Given a range of 0-15 seconds and the following three snapshots...
>> ```
>> Snapshot 1    10.0s
>> Snapshot 2    14.0s
>> Snapshot 3    21.0s
>> ```
>> ...and filter text of `Snapshot 2` we would expect a single snapshot to be listed: `Snapshot 2`. In that situation, without the additional checks the banner will be visible even though we are showing all the possible snapshots we could, and the range is not reducing the visible snapshots. The child is marked as hidden if it is either out of range or filtered by the filter text, so the additional checks determine why it was hidden.
>> 
>> In the attached video, you can see that when I filter items using the filter text field, the banner goes away when there wouldn't be anything else to show with a wider range, and returns when removing the search since the range is now the only reason those items are not visible.
> 
> I see.  I didn't realize that you're doing this every time the filter changes as well.  Rather than doing that (which could be somewhat expensive), could we instead add some logic inside `layout` that just checks to see if the newly added `WI.HeapAllocationsTimelineDataGridNode` is immediately `hidden`?  It seems like extra work (and unnecessary exposing of info) to recalculate this each time the filter changes and in general layouts (even when no new heap snapshots are added).  I think we should exactly match the behavior of the Console so that we're consistent throughout Web Inspector.  IIRC, the Console shows a warning banner whenever a new message is added that is initially hidden.  It does this by keeping a `Set` of messages that are initially hidden, removing messages once they are shown.  So long as the `Set` has items, the banner stays visible (including if the filter changes so long as it doesn't cause all messages to become visible).  I think we should do something similar here, where we keep track of the initially hidden heap snapshots and continue showing the banner so long as at least one of those initially hidden heap snapshots is still hidden (you'd may want to hook into either `filterDidChange` as you have or create a similar `nodeWasFiltered` so that you can know when any initially hidden heap snapshot has been shown).  I think the goal of this should be "hey there's something new you haven't seen yet" not "hey there's something hidden from you".
> 
> Also, does `WI.TimelineView.Event.NeedsEntireSelectedRange` also cause the filter bar to be reset too?  If not, then the text "Select Entire Range" is not accurate because clicking it won't cause the banner to disappear as there will still be a filtered node.

> I think the goal of this should be "hey there's something new you haven't seen yet" not "hey there's something hidden from you".

That makes sense.


> If not, then the text "Select Entire Range" is not accurate because clicking it won't cause the banner to disappear as there will still be a filtered node.

Maybe I wasn't clear in my explanation; the banner only cares if the range is causing items to not be shown, not if the filter text causes items to not be shown. But that current implementation will be moot once this becomes about showing "hey, there's something new you haven't seen yet", which all things considered seems like a better idea.

[Patrick Angle]

Created attachment 417095 [details]
Patch v3.0 - Unseen Banner

[Patrick Angle]

Created attachment 417096 [details]
Screenshot of Patch v3.x

[Devin Rousso]

Comment on attachment 417095 [details]
Patch v3.0 - Unseen Banner

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

r=me, awesome work!  Especially love `WI.BannerView` 🤩

> Source/WebInspectorUI/ChangeLog:13
> +        * UserInterface/Views/BannerView.css: Copied from Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.css.

lol this should probably just be `Added.` :P

> Source/WebInspectorUI/UserInterface/Views/BannerView.js:28
> +    constructor(message, {actionButtonMessage, showDismissButton})

there should probably be a ` = {}` after the object restructure so that someone could do `new WI.BannerView("Hello World!")`

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:92
> +        this._unseenRecordsBannerView = new WI.BannerView(WI.UIString("There are unseen snapshots that have been filtered", "There are unseen snapshots that have been filtered @ Heap Allocations Timeline View", "Message displayed in a banner when one or more snapshots that the user has not yet seen are being filtered."), {

NIT: how about just "new" instead of "unseen"?

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:299
> +        super.filterDidChange();

Style: personally, I usually put a newline after `super` calls (unless im using the return value), but up to you whether you also want to do that

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:392
> +        this._unseenRecords = this._unseenRecords.filter((record) => record.hidden );

Style: extra trailing space

[Patrick Angle]

Created attachment 417117 [details]
Patch v3.1 - Review Notes

[EWS]

Committed r271236: <https://trac.webkit.org/changeset/271236>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417117 [details].