1595119 - Websocket Inspector messages disappear when the 'keep all messages checkbox 'is checked and then filtered

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Peter Magyari (Desktop QA)]

Attachment: ws_messages.jpg

[Affected Versions]
Firefox 72.0a1(20191108095936)
Firefox 71.0b7(20191104135555)

[Preconditions]
Set 'devtools.netmonitor.features.webSockets' to "true" in about:config and restart firefox
Set 'devtools.netmonitor.ws.displayed-frames.limit' to "2"

[Steps to reproduce]

1. Launch Firefox
2. Navigate to: http://janodvarko.cz/test/websockets/
3. Press F12 to open Devtools
4. Go to the Network tab
5. Press "Connect" button from the page opened in step 2
6. Click on the "http://echo.websocket.org" websocket request
7. Click on the "Messages" panel in the right
8. Press the "Send" button on the website four times
9. Check the 'Keep all future messages' checkbox
10. Change filter from 'All' to 'Sent' or 'Received'

[Actual results]
None of the messages appear.
The 'x messages have been truncated to conserve memory' is not updated after filtering.

[Expected results]
First two 'sent/received' messages should appear and the number of truncated messages should update to '2' in this case.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report, good catch!

I can reproduce the issue on my machine (Win10, Nightly)

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Some instructions for anyone interested in fixing this bug:

1. The list of frames is rendered here:
   https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/devtools/client/netmonitor/src/components/websockets/FrameListContent.js#231

2. The frames property is calculated using getDisplayedFrames method here:
   https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/devtools/client/netmonitor/src/components/websockets/FrameListContent.js#345

3. getDisplayedFrames is implemented here
   https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/devtools/client/netmonitor/src/selectors/web-sockets.js#13

Note that this method returns list that is already filtered (based on the UI mentioned in the STRs, comment #0)

4. Finally, there is a code that calculates the final number of rendered frames here:
   https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/devtools/client/netmonitor/src/components/websockets/FrameListContent.js#245-262

This logic involves:

• Frames returned by getDisplayedFrames
• The frame limit given by devtools.netmonitor.ws.displayed-frames.limit pref
• State of the Keep all future messages checkbox

Something is wrong around how this calculation is done.

Honza

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Harald, the current devtools.netmonitor.ws.displayed-frames.limit is only limiting number of displayed frames. Perhaps we should have a pref for limiting the back-end too (i.e. limit for number of stored frames)?

Honza

Comment 4

[:Harald Kirschner :digitarald]

limit for number of stored frames

Can you confirm that we are not limiting the number of stored frames at all, atm?

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Some more comments:

1. The backend is creating LongStringActor for every sent/received frame payload here
   https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/devtools/server/actors/network-monitor/websocket-actor.js#106,131

These actors are not explicitly removed (e.g. when they would reach a limit)

2. The client side is handling WS sent/received notifications here:
   https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/devtools/client/netmonitor/src/connector/firefox-data-provider.js#468,478

This fires Redux action WS_ADD_FRAME

3. The action is handled by WS reducer here:
   https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/devtools/client/netmonitor/src/reducers/web-sockets.js#82

It adds WS frame into a frames map.

There is no logic that would explicitly remove frames from the map when reaching a limit.

---

I am raising the priority since this could be perf/memory issue.

Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Hengyeow: can you please look at this, it's important to avoid huge memory consumption in case of high WS traffic.
See my instructions, it should be simple to fix.

Honza

Comment 7

[Finn Williams]

@Honza I can try to look at this tonight or tomorrow

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Finn Williams from comment #7)

@Honza I can try to look at this tonight or tomorrow

Excellent, thanks!
Honza

Comment 9

[Seifeddine Besbes]

Hello @Honza,

Can I try to look at this ?

Best regards,
Seif

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Assigned to you, thanks for the help!
Honza

Comment 11

[Seifeddine Besbes]

Hello @Honza,

I have fixed the reported bug but i have a question about the perf/memory consumption, what should be the limit ?

Thank you,
Seif

Comment 12

[Seifeddine Besbes]

Attachment: Bug 1595119 - Websocket Inspector messages disappear when the keep all messages checkbox is checked and then filtered. r=honza

Depends on D60822

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Some comments related to proper fix

1. The number of truncated messages should always reflect the amount of removed messages
2. The number of truncated messages should not change when the filter changes (we don't know what message types have been removed)
3. The number of truncated messages resets when the user clicks the Clear button

@Harald, does that make sense?

Honza

Comment 14

[:Harald Kirschner :digitarald]

Yes, those all go back to a basic requirement that filtering must not affect truncation. Filtering should just hide messages but not truncate them.

If we want to remove that requirement, it's also an option. This would allow users to view more Sent messages but would make it impossible to go back to prior received messages that have been filtered/truncated.

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Seifeddinebesbes, are you still interested in fixing this bug? I left some review comments in the patch/Phab.

Honza

Comment 16

[Seifeddine Besbes]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #15)

Seifeddinebesbes, are you still interested in fixing this bug? I left some review comments in the patch/Phab.

Honza

Honza,

Thank you for your feedback,
I apologize for the delay, I have been a bit busy for the past few weeks.

I will deal with this bug these few days.

Seif

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 18

[kaeyosthaeron]

What kind of changes happened on the code base to give more attention on the future fix?

Comment 20

[Eugene Simakin]

Attachment: Bug 1595119 - Fix wrong slicing of displayed WebSocket messages in devtools when 'Keep all future messages' is enabled r=Honza

devtools when 'Keep all future messages' is enabled r=Honza

Comment 21

[Eugene Simakin]

Hi folks,

I made a fix for this issue and added a unit test for verification. Please, review.

STR in the first message are quite outdated, so here is my update on that.

[Affected Versions]
105.0a1

[Preconditions]
Set 'devtools.netmonitor.msg.displayed-messages.limit' to "10"

[Steps to reproduce]

1. Open https://www.piesocket.com/websocket-tester and DevTools
2. Go to Network tab
3. Filter requests by WS type (will be easier to find the one)
4. Click on "Connect" button at the website
5. Select the WebSocket request and go to Response tab
6. Wait while the displayed messages limit is reached (should not take a long time)
7. Wait while there will be some number (~50, 100) of truncated messages. More truncated messages - more clear the bug.
8. Check 'Keep all future messages' checkbox
9. Clear all messages
10. Wait until the 'Keep all future messages' checkbox appear again (displayed messages limit)
11. Actual results

[Actual results]
None of the messages appear.
The 'x messages have been truncated to conserve memory' is not updated after filtering.
The messages will appear again when the counter at the bottom will exceed the truncated messages number.

[Expected results]
Messages should appear.
The 'x messages have been truncated to conserve memory' should be updated to '0 messages have been truncated to conserve memory' since 'Keep all future messages' is enabled (not sure here, please, verify).

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thank you for the patch!

Please take a look at my comments in Phab
https://phabricator.services.mozilla.com/D155763

Comment 23

[Eugene Simakin]

Done. Please, review.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D155763	Bug 1595119 - Fix wrong slicing of displayed WebSocket messages in devtools when 'Keep all future messages' is enabled r=Honza	simakin.eugene	Honza: Inactive

:simakin.eugene, could you please find another reviewer?

For more information, please visit auto_nag documentation.

Comment 25

[Eugene Simakin]

Done. Please, check.
Should I change the commit message?

Comment 26

[Julian Descottes [:jdescottes]]

Hi Eugene, the bot notification was a bug (investigated at Bug 1789646), we can keep Honza as your reviewer here.

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comments posted to Phab (I think I know why the test is failing intermittently).

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a72369dae9cc
Fix wrong slicing of displayed WebSocket messages in devtools when 'Keep all future messages' is enabled r=Honza

Comment 29

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/a72369dae9cc

Comment 30

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

I managed to reproduce this issue using the STR from Comment 21 on Firefox 105.0(build ID: 20220915150737) on Ubuntu 22. Verified as fixed on Firefox 106.0(build ID: 20221010110315) and Nightly 107.0a1(build ID: 20221012094509) on Ubuntu 22, Windows 10, macOS 12.