Closed Bug 1560421 Opened 5 years ago Closed 5 years ago

No frames are displayed for some WS connections

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox69 wontfix, firefox70 wontfix, firefox71 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 71
Tracking Flags:
Tracking Status
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

image.png
55.64 KB, image/png
Details
Bug 1560421 - No frames are displayed for some WS connections
47 bytes, text/x-phabricator-request
lizzard
: approval-mozilla-beta-
Details | Review

This is a follow up for bug 1555625 comment #7

WS frames are not displayed for some WS connections.

Honza

Blocks: network-websocket-inspector
Depends on: 1555625
Priority: -- → P2

Christoph, I don't see any WS connection when loading https://discordapp.com/ (using Chrome browser)
What exactly should I do with the page?

Honza

Flags: needinfo?(christoph-wa)

Hi Honza

After loading https://discordapp.com/, you can click on the button Open Discord in your browser > type in a random username > complete the CAPTCHA.

It will be led to https://discordapp.com/guild-discovery and a WS connection can be observed.

Attached patch ws-listener.patch (obsolete) — — Splinter Review

I've done more analysis and here are some comments:

  1. I am attaching a patch that uses will-navigate event to register WS listener. I think this is more correct since will-navigate is fired on the same window as navigate, but sooner.

You can see the server side implementation here (showing when these events are fired):

will-navigate https://searchfox.org/mozilla-central/rev/da14c413ef663eb1ba246799e94a240f81c42488/devtools/server/actors/targets/browsing-context.js#1658

navigate https://searchfox.org/mozilla-central/rev/da14c413ef663eb1ba246799e94a240f81c42488/devtools/server/actors/targets/browsing-context.js#1677

  1. I think that the patch above improves the implementation, but it doesn't fix the problem reported here. Even if, sometimes I am actually seeing frames for WS connection created by https://web.whatsapp.com

So, this might indicate some racing issues. I don't know how to check that but, could this be related to a worker?

The main problem here seems to be that webSocketOpened is not executed - I am clearly seeing that there are some frames sent/received (for unknown WS connection ID). Perhaps the connection is created in a worker and we are missing that?

STR :

  1. Make sure you have devtools.netmonitor.features.webSockets set to true
  2. Load the https://web.whatsapp.com (no login needed)
  3. Open the Network panel
  4. Reload the page
  5. Select WS upgrade request in the net panel
  6. Check the WebSockets side panel, there are no WS frames (it works in Chrome)

Here is my own test page that works
http://janodvarko.cz/test/websockets/

@Andrea, what do you think is the problem here?

Honza

Assignee: nobody → odvarko
Flags: needinfo?(amarchesini)
Attached image image.png —

Here is a screenshot with errors Heng Yeow sent me (I don't see them myself), but it also points to a worker problem.

@hengyeow: do you have STR describing how to reproduce the error?

Honza

Flags: needinfo?(E0032242)

Hi Honza

Here's my STR for the error shown:

  1. Go to https://web.whatsapp.com/
  2. Open network panel and the browser toolbox console
  3. Reload the network panel and the errors can be seen in the browser toolbox console
Flags: needinfo?(E0032242)

Also, one has to be logged into WhatsApp Web to observe the errors shown in Comment 4.

Assignee: odvarko → nobody

A webSocket object is created on the content process:
https://searchfox.org/mozilla-central/rev/543e1fbd04a85bae7b4d38357b185d99bcfc8e4b/dom/websocket/WebSocket.cpp#1413

But the listener is registered on the parent process. Because of this, when the notification is sent, we do a quick return, because there are not listeners on the current process:
https://searchfox.org/mozilla-central/rev/543e1fbd04a85bae7b4d38357b185d99bcfc8e4b/netwerk/protocol/websocket/WebSocketEventService.cpp#222-230

We should move the addListener() in the content process or change how WebSocketEventListener works.

Flags: needinfo?(amarchesini)
Flags: needinfo?(christoph-wa)

Julian, what is the right way to ensure that our WebSocketActor is instantiated in the content process?

Here is our actor:
https://searchfox.org/mozilla-central/rev/040aa667f419932adf425d92c7438f03230ad96b/devtools/server/actors/network-monitor/websocket-actor.js#20

Honza

Flags: needinfo?(jdescottes)

On DevTools side, the websocket-actor runs in the content process. First it is registered as a target scoped actor, so it will live in whichever process the target is. Here we are typically targeting a webpage so the page lives in the content process.

You can verify this easily with

const { DebuggerServer } = require("devtools/server/main");
console.log("@@@@@@ DebuggerServer.isInChildProcess", DebuggerServer.isInChildProcess);

Adding this right before webSocketEventService.addListener(innerWindowID, this); logs true whenever you open a regular toolbox.

Maybe :baku tried opening the browser toolbox, and since the browser toolbox is targeting the main process the websocket-actor was running in the main process. But I guarantee that for regular toolboxes, the websocket-actor runs in the content process.

Flags: needinfo?(jdescottes)

(In reply to Julian Descottes [:jdescottes] from comment #9)

On DevTools side, the websocket-actor runs in the content process. First it is registered as a target scoped actor, so it will live in whichever process the target is. Here we are typically targeting a webpage so the page lives in the content process.

You can verify this easily with

const { DebuggerServer } = require("devtools/server/main");
console.log("@@@@@@ DebuggerServer.isInChildProcess", DebuggerServer.isInChildProcess);

Adding this right before webSocketEventService.addListener(innerWindowID, this); logs true whenever you open a regular toolbox.

Thanks Julian!

Maybe :baku tried opening the browser toolbox, and since the browser toolbox is targeting the main process the websocket-actor was running in the main process. But I guarantee that for regular toolboxes, the websocket-actor runs in the content process.

Baku, did you test with the Browser Toolbox?

The testing in my case shows that the WebSocketActor indeed is running in the content process (when using the Toolbox for web page developers).

Honza

Flags: needinfo?(amarchesini)

:hsinyi, can you assign this to someone from the DOM team? It's important for devtools, but I'm not able to work on this right now. Thanks.
In case, I can give an overview about how webSocket events for devtools are dispatched.

Flags: needinfo?(amarchesini) → needinfo?(htsai)

(In reply to Andrea Marchesini [:baku] from comment #11)

:hsinyi, can you assign this to someone from the DOM team? It's important for devtools, but I'm not able to work on this right now. Thanks.
In case, I can give an overview about how webSocket events for devtools are dispatched.

Taking via email discussion!

Michal, could you have a look to see where the problem is?

Flags: needinfo?(htsai) → needinfo?(michal.novotny)

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

Created attachment 9073207 [details] [diff] [review]
ws-listener.patch

I've done more analysis and here are some comments:

  1. I am attaching a patch that uses will-navigate event to register WS listener. I think this is more correct since will-navigate is fired on the same window as navigate, but sooner.

This isn't true. this.targetActor.window.windowUtils.currentInnerWindowID is different in onWillNavigate and in onNavigate.

To sum the problem a bit:

  1. This patch cannot fix the problem because you're registering the listener for a wrong windowID.

  2. Without this patch the listener is registered too late. WS connection is created before the listener is registered, webSocketOpened isn't called and the connection isn't stored in connections map. So even if the frameSent/frameReceived are called after the listener is registered, they return early because the connection isn't found in the map.

I.e. you need to find a better place to register the listener, so it's registered before the WS connection is created and when you already have the correct windowID.

Flags: needinfo?(michal.novotny)

This also affects https://glitch.com/, afaik. In most cases the sockets never show messages – only in one case I could make it work. Chrome always worked

Attached patch temp.patch (obsolete) — — Splinter Review

I am attaching WIP patch that fixes the problem for me.

Alex, what do you think about the new "navigating" event?
It's sent when the new inner window is available for the first time (and so the WS inspector doesn't miss the moment when new WS connection is created)

Honza

Flags: needinfo?(poirot.alex)
Comment on attachment 9090036 [details] [diff] [review]
temp.patch

Review of attachment 9090036 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/network-monitor/websocket-actor.js
@@ +29,5 @@
>      this.connections = new Map();
>  
>      // Register for backend events.
> +    this.onNavigating = this.onNavigating.bind(this);
> +    this.targetActor.on("navigating", this.onNavigating);

You should not need to introduce a new event. We already have all what we need.
What you are looking for here is `window-ready`:
https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#1357-1377

This is fired every time there is a new `window` object/global created. You would need to filter out when `event.isTopLevel` if you do not want to register listeners against each individual inner `<iframe>`.
Flags: needinfo?(poirot.alex)
Attachment #9090036 - Attachment is obsolete: true

(In reply to Alexandre Poirot [:ochameau] from comment #17)

You should not need to introduce a new event. We already have all what we
need.
What you are looking for here is window-ready:
https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/
browsing-context.js#1357-1377

This is fired every time there is a new window object/global created. You
would need to filter out when event.isTopLevel if you do not want to
register listeners against each individual inner <iframe>.

Great, thanks Alex!
Patch attached in Phab
Honza

Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attachment #9073207 - Attachment is obsolete: true
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/feb77ead47f4
No frames are displayed for some WS connections r=ochameau

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

Honza, after giving this more time in Nightly, what are your thoughts about uplifting this to 70?

Flags: needinfo?(odvarko)
Regressions: 1581244

Comment on attachment 9091371 [details]
Bug 1560421 - No frames are displayed for some WS connections

Beta/Release Uplift Approval Request

  • User impact if declined: Only impacts web developers. Intercepting WS connection doesn't work in some cases.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): * It's developer tools feature that is not exposed to standard Firefox users.
  • It's been tested for sometime in Nightly.
  • Rather small patch.
  • String changes made/needed:
Flags: needinfo?(odvarko)
Attachment #9091371 - Flags: approval-mozilla-beta?

I would rather this ride with 71 since we only have 2 beta builds left this cycle. I'm still taking uplifts, but mostly skyline-related. Is there a strong need to get this into 70?

status-firefox69: --- → wontfix
status-firefox70: --- → fix-optional
Flags: needinfo?(odvarko)

Comment on attachment 9091371 [details]
Bug 1560421 - No frames are displayed for some WS connections

We have one beta build left before the RC build so let's let this ride with 71.

Attachment #9091371 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

I agree, this should ride with 71.

It also means that:

  • Keep WebSocket Inspector enabled in DevEdition
  • We have time to reflect on QA feedback
  • We have time to do something with the on-boarding UI

Honza

Flags: needinfo?(odvarko)
QA Whiteboard: [qa-71b-p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: