977858 - Should be able to attach an nsiWebSocketListener to a web socket that's already open

Status: RESOLVED WONTFIX

(Core :: Networking: WebSockets, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

My understanding is that the only opportunity for adding an nsIWebSocketListener to a websocket is when you call |asyncOpen|. For devtools, it would be very handy if we could attach one after a web socket has already been opened (eg when we open the network monitor and there are already opened sockets we'd like to display activity from).

Comment 1

[Jason Duell]

Yes, right now you can only add the listener in asyncOpen.  What exactly does devtools need here?  To see the traffic, or to be able to alter it on the fly.

We're planning to make websockets work off-main thread (see bug 925623), so adding any listener arbitrarily that wants to receive notifications on the main thread is not going to fly.  But if you just need to *see* the data (versus be able to modify it, i.e. stick yourself in between the original listener and the websocket) then we could probably add some API for notifying another listener.

If you don't actually need to see all the data, just some summary of it (time, payload length, etc) then that would also be good to know, so we don't have to make lots of copies of the data for no reason.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ideally devtools would have access to everything; all the nitty gritty. It's pretty easy to console.log messages sent back and forth yourself as a developer so just making an interface that shows those messages doesn't provide that much value. Our network monitor should be able to show what went in which frame so you can optimize the network activity and all that great stuff.

I don't think we need to modify the data. We will need a way of getting all the existing websockets for a given page though, so we can iterate over them and add observers.

Comment 3

[Jason Duell]

> We will need a way of getting all the existing websockets for a given page 

I don't know how to get that, but smaug probably does.

Once you've got a pointer to the websockets on the page, we could add an API to the content/base/src/Websocket class: 

   SetObserver(nsIWebsocketListener, nsIEventTarget)

And that in turn could call a nsIWebsocketChannel with the same signature, and then we'd have all the info the channel needs to send an additional copy of the data to the devtools listener.  Note that I have an nsIEventTarget in the API--if the devtools listener is always main thread we could omit that.

My simple & sweet proposal is that we only support a single Observer for a given websocket channel, until/unless we need more than 1.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Would this |SetObserver| api be scriptable? All the existing devtools code for network monitoring is in JS land, and it would be nice if we didn't have to introduce any C++.

Everything else sounds solid.

Comment 5

[Jason Duell]

Yes the nsIWebsocketListener could be a JS implementation.

Comment 6

[Jason Duell]

> We will need a way of getting all the existing websockets for a given page though, so we can iterate over them and add observers.

Ollie, do you know how to do that?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

In C++, go through nsGlobalWindow::mEventTargetObjects and if it is possible to QI it to
nsIWebSocketListener, it is actually WebSocket. (WebSocket implements nsIWebSocketListener).

We could expose mEventTargetObjects in some way to scripts via [ChromeOnly] property in
Window.webidl (once window is converted to webidl), or add some helper to
DOMWindowUtils to access mEventTargetObjects.
Note, mEventTargetObjects contains also XHR, EventSource, DOMRequest, etc EventTarget objects
(but not Window itself nor any Nodes)

Comment 8

[Andrea Marchesini [:baku]]

Do we care about this bug? If not, maybe we can close it.

Comment 9

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

Yes, this can be closed.
The necessary API are now available (bug 1203802 and bug 1215092)
Honza