807276 - Make nsIWebSocketChannel.idl scriptable

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Guillermo López :willyaranda (probably SLOW response)]

We need to instantiate the nsIWebSocketChannel from JavaScript.

Rest of the protocols like nsIFTPChannel, nsIWyciwygChannel, nsIHttpChannel are all scriptable.

Comment 1

[Guillermo López :willyaranda (probably SLOW response)]

Attachment: Patch

Comment 2

[Guillermo López :willyaranda (probably SLOW response)]

Comment on attachment 676975 [details] [diff] [review]
Patch

Asking for review from one of the netwerk peers.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 676975 [details] [diff] [review]
Patch

Uh...  Why do you need to instantiate this from JS, exactly?  This is not a general-purpose channel, unlike HTTP/FTP/etc.  It's not even an nsIChannel!

Comment 4

[Albert [:albert]]

We want to open a WebSocket in a Gecko component but we cant because idl is not defined as scriptable. We also tried to create a websocket like var ws = new WebSocket('ws://localhost', 'protocol') but we get a javascript error "WebSocket is not defined"

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 676975 [details] [diff] [review]
Patch

> We want to open a WebSocket in a Gecko component

In that case nsIWebSocketChannel is not good enough, right?  A bunch of the work is done by nsWebSocket, so you'd have to at least duplicate that.  And even then, you wouldn't be able to get data out of the channel, unless you also made nsIWebSocketListener scriptable at the very least.

> We also tried to create a websocket like var ws = new WebSocket('ws://localhost',
> 'protocol') but we get a javascript error "WebSocket is not defined"

Sure.  So it seems to me like the right solution here is to:

1)  Fix nsWebSocket to work without a window (which is slightly nontrivial, since the spec assumes a window is available afaict).

2)  Expose nsWebSocket in JS components.

Comment 6

[Albert [:albert]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Comment on attachment 676975 [details] [diff] [review]
> Patch
> 
> > We want to open a WebSocket in a Gecko component
> 
> In that case nsIWebSocketChannel is not good enough, right?  A bunch of the
> work is done by nsWebSocket, so you'd have to at least duplicate that.  And
> even then, you wouldn't be able to get data out of the channel, unless you
> also made nsIWebSocketListener scriptable at the very least.

I'm not agree, for testing we made nsIWebSocketChannel scriptable and we were able to send / receive data just overwriting a nsIWebSocketListener.

> > We also tried to create a websocket like var ws = new WebSocket('ws://localhost',
> > 'protocol') but we get a javascript error "WebSocket is not defined"
> 
> Sure.  So it seems to me like the right solution here is to:
> 
> 1)  Fix nsWebSocket to work without a window (which is slightly nontrivial,
> since the spec assumes a window is available afaict).
> 
> 2)  Expose nsWebSocket in JS components.

Can be another solutions, involving more work.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> and we were able to send / receive data just overwriting a nsIWebSocketListener.

And duplicating a bunch of the logic from WebSocket itself, or just putting a totally different non-message API on it?  And you had to make nsIWebSocketListener scriptable too, presumably...

Christian, thoughts on making both of these interfaces scriptable and letting people play with the result?

Comment 8

[Patrick McManus [:mcmanus]]

I originally thought these might end up scriptable to build some xpcshell websockets tests.. (separate from the mochitest coverage). but that ball has never been advanced.

Comment 9

[Albert [:albert]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> > and we were able to send / receive data just overwriting a nsIWebSocketListener.
> 
> And duplicating a bunch of the logic from WebSocket itself, or just putting
> a totally different non-message API on it?  And you had to make
> nsIWebSocketListener scriptable too, presumably...

Sorry, I did not mean overwriting, I mean implementing the methods of nsIWebSocketListener (onStart, onStop, onMessageAvailable, etc), which is the way to work with websockets as detailed in https://developer.mozilla.org/en-US/docs/WebSockets/Writing_WebSocket_client_applications

To summarize, after made nsIWebSocketChannel scriptable we create the websocket as follows:

  let ws = Cc["@mozilla.org/network/protocol;1?name=ws"].createInstance(Ci.nsIWebSocketChannel);
  ws.protocol = "push-notification";
  ws.asyncOpen(uri, nsURL, listener, null);

And the listener just implement the Ci.nsIWebSocketListener methods

Comment 10

[Jason Duell]

> We want to open a WebSocket in a Gecko component 

Please say more about the use case here.  Why do you need to use Websocket when you don't have a window?

I just looked over the WebsocketChannel code and it does seem likely that we could support this model, at least at the moment.  There's really nothing that we need a window for in the necko code itself (other than things like popping up auth windows, etc--but that's the case for http channels too).  But I'd like to have a good reason before we commit ourselves to keeping this working in the long term.   Also, test coverage would be good (i.e. bug 771884).

Comment 11

[Guillermo López :willyaranda (probably SLOW response)]

Hi,

we are trying to make a deep integration of TuMe app (http://www.tumeapp.com/) from TEF as a experiment on Firefox OS devices. We are trying to create an extension with a xpcom component that will talk, via websockets, with the TuMe servers, and then the component with some gaia apps (system, contacts, a TuMe app) to deeply integrate the services provided by TuMe on the OWD, such as contact sync, notifications and more.

Comment 12

[Jason Duell]

Comment on attachment 676975 [details] [diff] [review]
Patch

OK, I think I'm alright with making nsIWebSocketChannel scriptable so we can experiment with this kind of server integration.   Any objections?

> you wouldn't be able to get data out of the channel, unless you 
> also made nsIWebSocketListener scriptable at the very least.

JS can get the data just by implementing something that looks like a nsIWebSocketListener, right? I assume that's what they've been doing.  IIUC that will fail if the listener ever gets QI'd (unless they kludged that too).   But in any case, bz is right: you should also make nsIWebSocketListener scriptable, if nothing else so you can write QueryInterface using

   if (iid.equals(Components.interfaces.nsIWebSocketListener))
       return this 

Resubmit a new patch with a scriptable nsIWebSocketListener and I'll +r it unless I hear good reasons not to.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> JS can get the data just by implementing something that looks like a nsIWebSocketListener

Oh, nsIWebSocketListener is already scriptable? Then yes.

Comment 14

[Jason Duell]

Comment on attachment 676975 [details] [diff] [review]
Patch

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

> nsIWebSocketListener is already scriptable?

Oh, heh, so it is.  So then this patch should be fine?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 676975 [details] [diff] [review]
Patch

Yep.  You don't have to change the UUID, though.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Guillermo López (:willyaranda) from comment #11)
> we are trying to make a deep integration of TuMe app
> (http://www.tumeapp.com/) from TEF as a experiment on Firefox OS devices. We
> are trying to create an extension with a xpcom component that will talk, via
> websockets, with the TuMe servers, and then the component with some gaia
> apps (system, contacts, a TuMe app) to deeply integrate the services
> provided by TuMe on the OWD, such as contact sync, notifications and more.

If we supported WebSockets in workers, then wouldn't it be simple to also allow WebSockets in Chrome Workers or otherwise outside of a context that doesn't have a window? And, if so, couldn't we then expose the same W3C standard WebSockets API to extensions, instead of having extensions use a Gecko-only (XPCOM) API?

Now, maybe that is too complicated or too much work. Just, generally, I would like us to get the the point where we're exposing network API functionality through W3C APIs by default, or even only.

Comment 17

[Boris Zbarsky [:bzbarsky]]

If we had WebSocket in workers (which is in progress, but not done yet), then yes, it would automatically work in ChromeWorkers and could be made to work in arbitrary JS components too.

Comment 18

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

I believe in order to make WebSockets really useful in chrome workers would require chrome only
API to set origin.

Comment 19

[Jason Duell]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/f7753f215359

After talking w/smaug on IRC we decided making this scriptable for now is OK. I added a comment in the IDL noting that scriptable support may go away once we have Websockets for Workers (and a way to set origin for them).

Comment 20

[Jason Duell]

This time w/o syntax error starting comment :)

  https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6726f79a3b

Comment 21

[Guillermo López :willyaranda (probably SLOW response)]

Marking this as resolved?

Comment 22

[Guillermo López :willyaranda (probably SLOW response)]

Oh, we are seeing that this was backed out from m-c: http://hg.mozilla.org/mozilla-central/rev/7e4fa834e25d, any info?

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2e6726f79a3b

Comment 24

[Doug Turner (:dougt)]

Comment on attachment 676975 [details] [diff] [review]
Patch

required for push.  this is a very safe change.

Comment 25

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e5c96d2e2f73