Closed Bug 1552458 Opened 5 years ago Closed 5 years ago

Implement backend actor for WebSocket inspection

Categories

(DevTools :: Netmonitor, enhancement, P3)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox69 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: Honza, Assigned: tanhengyeow)

References

(Blocks 1 open bug)

Details

(Whiteboard: [devtools-backward-compat])

Attachments

(3 files, 6 obsolete files)

Bug 1552458 - Implement backend actor for WebSocket inspection. r=Honza
47 bytes, text/x-phabricator-request
Details | Review
WebSocketActor-destroy-error.png
130.60 KB, image/png
Details
LongStringActor-payload.png
71.56 KB, image/png
Details

WebSocket inspection feature needs an backend actor responsible for intercepting WS communication and sending data back to the client.

Honza

Blocks: network-websocket-inspector
Type: defect → enhancement
Priority: -- → P3
Attached patch ws-listener-test.patch (obsolete) — — Splinter Review

I am attaching a test patch that registers nsIWebSocketEventListener to nsIWebSocketEventService service.

Some notes:

  1. The registration happens on the main process

  2. The callbacks (frameSent/frameReceived) are handled on the main process and provides correct data (e.g. I can see the payload)

  3. But, I am only seeing frameSent/frameReceived being executed. The other callbacks like: webSocketOpened/webSocketCreated/webSocketClosed are not executed for me.

@Alex, does the patch make sense?

@Andrea, I am testing the new interface implemented in bug 1542170, but some callbacks are not executed for me? (see #3) What could be wrong here?

Honza

Assignee: nobody → odvarko
Flags: needinfo?(poirot.alex)
Flags: needinfo?(amarchesini)

Yes, this is normal because webSocketOpened/webSocketCreated/webSocketClosed are delivered by the WebSocket object, which runs on the content process. FrameSent/frameReceived are delivered on the parent process and sent, via IPC message, to the content process as well.

Can the actor live in the content process instead of the parent? If not we need to find a way to send those 2 messages to the parent process as well.

Flags: needinfo?(amarchesini) → needinfo?(odvarko)
Attached patch ws-listener-test2.patch (obsolete) — — Splinter Review

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

Yes, this is normal because webSocketOpened/webSocketCreated/webSocketClosed are delivered by the WebSocket object, which runs on the content process. FrameSent/frameReceived are delivered on the parent process and sent, via IPC message, to the content process as well.

Thanks Andrea, it makes sense to me now.

Can the actor live in the content process instead of the parent? If not we need to find a way to send those 2 messages to the parent process as well.

Yes, the actor can be instantiated in the content process and I updated my testing patch to do that. It works a expected now.

Honza

Flags: needinfo?(odvarko)
Attachment #9065660 - Attachment is obsolete: true
Attached patch websocket-actor1.js (obsolete) — — Splinter Review

The attached patch is introducing a new WebSocketActor (+ spec & front) that should be responsible for intercepting WebSocket traffic on specific page. It should run within content process.

@Julian, the actor isn't instantiated for me. I even tried to do it in ContentProcessTargetActor, but not sure if this is ok.

What could be wrong with my patch?

Honza

Flags: needinfo?(jdescottes)
Attachment #9066007 - Attachment is obsolete: true
Assignee: odvarko → nobody

Implement backed actor for WebSocket inspection.

Attachment #9067311 - Attachment filename: websocket-actor1.js → websocket-actor1.patch
Attachment #9067311 - Attachment is patch: true
Attachment #9067311 - Attachment mime type: application/x-javascript → text/plain
Summary: Implement backed actor for WebSocket inspection → Implement backend actor for WebSocket inspection
Comment on attachment 9067311 [details] [diff] [review]
websocket-actor1.js

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

(not a complete review, just focused on making sure we could instanciate the actor)

::: devtools/client/netmonitor/src/connector/firefox-connector.js
@@ +75,5 @@
>        this.tabTarget.on("navigate", this.navigate);
>  
>        // Initialize Emulation front for network throttling.
>        this.emulationFront = await this.tabTarget.getFront("emulation");
> +      this.webSocketFront = await this.tabTarget.getFront("webSocket");

The actor will be instanciated on demand as soon as you call a method on it. Maybe you want to add a startListening/stopListening API?

::: devtools/server/actors/network-monitor/websocket-actor.js
@@ +8,5 @@
> +const { Actor, ActorClassWithSpec } = require("devtools/shared/protocol");
> +const { webSocketSpec } = require("devtools/shared/specs/websocket");
> +
> +const webSocketEventService = Cc["@mozilla.org/websocketevent/service;1"]
> +  .getService(Ci.nsIWebSocketEventService);

We should add `const { Cc, Ci } = require("chrome");` (this only breaks in the Browser Content Toolbox, not really sure why at the moment)

::: devtools/server/actors/utils/actor-registry.js
@@ +233,5 @@
>        prefix: "changes",
>        constructor: "ChangesActor",
>        type: { target: true },
>      });
> +    this.registerModule("devtools/server/actor/network-monitor/websocket-actor", {

should be actors/ instead of actor/ here
Flags: needinfo?(jdescottes)
Attached patch websocket-actor2.patch (obsolete) — — Splinter Review

@Julian, thanks for the review, works like a charm now!

@HengYeow: please look at the patch now and try it out

STR (just for testing purposes):

  1. Load http://janodvarko.cz/test/websockets/
  2. Reload to see at least one request in the Network panel
  3. Click any request to open the Headers side panel. getFrames is called on the actor now, check out the Browser console (but, no frames at this point)
  4. Create WS connection by clicking on Connect and send a few frames
  5. Close & open the side panel to execute the getFrames again, check out the browser console, you should see the frames.

Honza

Assignee: nobody → odvarko
Flags: needinfo?(poirot.alex) → needinfo?(E0032242)
Assignee: odvarko → E0032242

Summary of actor's API that should be implemented in this report:

Methods

  • getFrames(httpChannelId) - returns list of frames for specific channel (with cropped payload, we probably want to use LongStringActor [1])
  • startListening() - adds nsIWebSocketEventService listeners
  • stopListening() - removes nsIWebSocketEventService listener
  • getFramePayload(frameId) - returns full payload for specific frame

Events

  • frameSent - sends frame data with a flag 'sent': { type: 'sent', data: frame }
  • frameReceived - sends frame data with a flag 'received': { type: 'received, data: frame }

[1] Some useful links:

Support for connectionOpened/Closed and isConnectionAlive can be done in different report

Anything else?
Honza

Attachment #9067311 - Attachment is obsolete: true

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

Anything else?

Could you also take a look at CDP to get some inspiration about what API are there?

Honza

Implement backend actor for WebSocket inspection

Attachment #9068966 - Attachment description: Bug 1552458 - Implement backend actor for WebSocket inspection. r=Honza *** Bug 1552458 - Implement backend actor for WebSocket inspection. r=Honza → Bug 1552458 - Implement backend actor for WebSocket inspection. r=Honza
Attachment #9067316 - Attachment is obsolete: true
Flags: needinfo?(E0032242) → needinfo?(odvarko)

Hi Honza!

Attached a preliminary Phabricator patch for this bug. Also, I looked through CDP's protocol and below is the summary of findings (method implementations are based on CDP).

Similar APIs:

  • webSocketClosed(requestId, timestamp)
  • webSocketCreated(requestId, url, initiator)
  • webSocketFrameReceived(requestId, timestamp, response)
  • webSocketFrameSent(requestId, timestamp, response)

Additional APIs from CDP:

  • webSocketFrameError(requestId, timestamp, errorMessage)
  • webSocketHandshakeResponseReceived(requestId, timestamp, response)
  • webSocketWillSendHandshakeRequest(requestId, timestamp, wallTime, request)

Great, thanks. This looks good so far!

Honza

Flags: needinfo?(odvarko)

Hi Honza

I updated the Phabricator patch and a working MVP can be observed now!

Some notes for STR:

  1. WebSocketActor starts listening on FirefoxConnector.addListeners now, not when HeadersPanel is opened.
  2. I'm toggling between these two test sites test 1, test 2 for testing, both of them are intermittent.
  3. When a frame is sent/received, console logs can be seen from both WebSocketActor and WebSocketFront (events emitted from WebSocketActor received and printed out)
  4. Opening the HeadersPanel of the respective WebSocket request from the Network Panel would call 2 functions (for testing purposes):
    1. getFrames(httpChannelId) - Returns an array of frames whose payload is wrapped by LongStringActor
    2. getFramePayload(frameId) - Returns the full payload of the specified frame. (Always returns the first frame for now)

I'm facing an exception when the WebSocketActor is destroyed (when I closed the whole DevTools). I've attached an image to show the error.

As discussed in one of the meetings, we would want to store the frames/relevant data in the store and information in the WebSocketFront should communicate with FirefoxConnector and FirefoxDataProvider and then eventually reach the store. Should we do this in the same bug?

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

Summary of actor's API that should be implemented in this report:

Methods

  • getFrames(httpChannelId) - returns list of frames for specific channel (with cropped payload, we probably want to use LongStringActor [1])
  • startListening() - adds nsIWebSocketEventService listeners
  • stopListening() - removes nsIWebSocketEventService listener
  • getFramePayload(frameId) - returns full payload for specific frame

Events

  • frameSent - sends frame data with a flag 'sent': { type: 'sent', data: frame }
  • frameReceived - sends frame data with a flag 'received': { type: 'received, data: frame }

[1] Some useful links:

Support for connectionOpened/Closed and isConnectionAlive can be done in different report

Anything else?
Honza

Flags: needinfo?(odvarko)

Thanks for the patch!

(In reply to Heng Yeow (:tanhengyeow) from comment #13)

I'm facing an exception when the WebSocketActor is destroyed (when I closed the whole DevTools). I've attached an image to show the error.

It's because stopListening is called twice and you are trying to remove the WS listener twice. The second try fails with the above exception

  1. It's called from WebSocketActor.destroy
  2. It's called through WebSocketFront.stopListening from FirefoxConnector.removeListeners

I think you can introduce a helper flag indicating that the listener has been removed. The same flag can be used to ensure that the actor doesn't add the listener twice. The name could be e.g. listening (set to true in startListening and to false in stopListening)

As discussed in one of the meetings, we would want to store the frames/relevant data in the store and information in the WebSocketFront should communicate with FirefoxConnector and FirefoxDataProvider and then eventually reach the store. Should we do this in the same bug?

Yep, feel free to do it in the same bug & in the same patch.

Honza

Flags: needinfo?(odvarko)
Attached patch temp.patch (obsolete) — — Splinter Review

Attaching a patch that fixes WS listener after page reload. We need to re-register the listeners since the inner window changes

Honza

Hi Honza!

Updated the Phab patch! To observe a payload that is wrapped in LongStringActor, the payload must be longer than the defined constant, which is 10000.

You should see something like the attached image if the payload exceeds 10000.

Flags: needinfo?(odvarko)
Attachment #9070529 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #9067948 - Attachment is obsolete: true

(In reply to Heng Yeow (:tanhengyeow) from comment #17)

Hi Honza!

Updated the Phab patch! To observe a payload that is wrapped in LongStringActor, the payload must be longer than the defined constant, which is 10000.

You should see something like the attached image if the payload exceeds 10000.

Great work here, thanks!

I commented in Phab, the patch needs rebase on top of the latest m-c

Honza

Latest patch ready for review :) Try results green.

Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de1dc8a5ce54
Implement backend actor for WebSocket inspection. r=Honza

(In reply to Pulsebot from comment #21)

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de1dc8a5ce54
Implement backend actor for WebSocket inspection. r=Honza

Thanks for quick fix.
Comments in Phab
Honza

Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee33e076cbcb
Implement backend actor for WebSocket inspection. r=Honza,jdescottes

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox69: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Whiteboard: [devtools-backward-compat]

Summary of the final patch:

  1. WebSocketActor lives in the content process and implements callback methods based on this interface file.

  2. onNavigate and onWillNavigate implemented in WebSocketActor to facilitate calling the startListening/stopListening functions used to intercept WebSocket traffic. Required because the innerWindowID changes based on the current view e.g. on first reload of the Network Panel. This is important as the function webSocketEventService.addListener(innerWindowID, this); depends on the innerWindowID. If the value changes, we need to register webSocketEventService with a new listener.

  3. Spec file and WebSocketFront setup to intercept events emitted by the backend WebSocketActor. WebSocketFront is able to call methods APIs implemented in WebSocketActor as long as the methods are defined in the spec file. When FirefoxConnector is connected to/disconnected from the backend, this.webSocketFront.startListening()/this.webSocketFront.stopListening() gets called respectively. Complements the previous point with a boolean flag to check if webSocketEventService has an existing listener. A new listener would only be registered if there are no existing listeners.

  4. Implementation is hidden under the pref setting devtools.netmonitor.features.webSockets.

  5. WebSocketFront transports events/data emitted by WebSocketActor to the FirefoxConnector (connects to the Firefox backend), and to the FirefoxDataProvider (acts as a single point to keep track of all RDP requests). Frame payload is wrapped in a LongStringActor object, to be resolved in the frontend using the getLongString method.

  6. With reference to comment 8, getFrames(httpChannelId) and getFramePayload(frameId) would not be implemented as discussed. This is because the events/data emitted by WebSocketActor would be added to the Redux Store from the FirefoxDataProvider. Work would be done in Bug 1555625.

Flags: needinfo?(E0032242)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: