733573 - Expose a client TCP socket API to web applications

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-0.1

The propose wip does expose a mozTCPSocket API to web applications. The API is not final since there is probably tons of discussions about it before having something final but this is a prelimimary step to start building socket applications over B2G/Gaia.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 603468 [details] [diff] [review]
wip-0.1

\o/

I'm not the right guy to look this over, but I suspect we want an API as close to WebSocket as possible.  And if possible, exactly the same :).

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

For information I'm not planning to write the API myself.
I just want to have something in b2g/ I can start playing with.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 603468 [details] [diff] [review]
wip-0.1

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

Pretty cool. I did a drive-by review, I hope you don't mind! I even refrained from the usual "meh meh gecko coding style meh meh" comments. ;)

::: b2g/components/TCPSocket.js
@@ +32,5 @@
> +  },
> +
> +  get type() {
> +    return this._type;
> +  },

There should be any need for those getters if you want them to be readonly. XPConnect will enforce that already.

@@ +85,5 @@
> +  },
> +
> +  get ondata() {
> +    return this._ondata;
> +  },

Why the getters/setters?

Also, you might be interested in my EventTarget.jsm module that I wrote in bug 729173, attachment 600293 [details] [diff] [review].

@@ +143,5 @@
> +  onDataAvailable: function ts_onDataAvailable(request, context, inputStream, offset, count) {
> +    LOG("onDataAvailable called\n");
> +    let sInputStream = Cc["@mozilla.org/scriptableinputstream;1"]
> +                         .createInstance(Ci.nsIScriptableInputStream);
> +    sInputStream.init(inputStream);

You can reuse the same nsIScriptableInputStream. Makes the code a little nimbler for long streaming connections in terms of GC/CC. Just save the instance on `this` and re-init it with the new `inputStream`.

@@ +145,5 @@
> +    let sInputStream = Cc["@mozilla.org/scriptableinputstream;1"]
> +                         .createInstance(Ci.nsIScriptableInputStream);
> +    sInputStream.init(inputStream);
> +
> +    let response = sInputStream.read(count);

This can throw.

@@ +200,5 @@
> +          rs.fireError(request, "DNS_ERROR");
> +        }
> +      }
> +    }
> +    dns.asyncResolve(aHost, 0, listener, Services.tm.currentThread);

Why do the DNS lookup separately? nsISocketTransportService::createTransport() will happily take a hostname.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yay!

Only comment is that I think that we should support send'ing ArrayBuffers, Blobs and strings. But we can add that once we have a more finalized API.

cc'ing Andrew Sutherland who has expressed interest in drafting an API.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 603468 [details] [diff] [review]
wip-0.1

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

Only comment is that I think we can return a socket-object synchronously. But have a .readyState ("connecting"/"open"/"closed") which indicates the current state. That way there'll be fewer objects to juggle with.

Comment 6

[Jason Duell]

Comment on attachment 603468 [details] [diff] [review]
wip-0.1

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

What's the use case for this API?  Just "I need a simple client-only TCP socket to contact some server that won't speak WebSockets to me for some reason"? (In line with our new "the Web is the platform" slogan, I guess we call these "legacy TCP applications" :). In the long run I assume we'll also need an API that allows TCP server sockets that listen (but that can probably be a separate nsB2GTCPServerSocket class, based off nsIServerSocket).  And of course a UDP socket type.  And quite possibly an even lower-level API that can enable traffic-sniffing so we can do things like write an app to make a cellphone a wifi hotspot (have we thought about that yet?).  Those should all be separate bugs, I assume, and this one should have a more modest name :)

Comments below--I'm going to ask Patrick or Honza to also look over this (assigning for now to Patrick, since Honza always gets all the reviews).

::: b2g/components/TCPSocket.js
@@ +103,5 @@
> +    this.transport.securityCallbacks = SecurityCallbacks;
> +    
> +    // Alright to open streams here as they are non-blocking by default
> +    this.outputStream = this.transport.openOutputStream(0, 0, 0);
> +    this.inputStream = this.transport.openInputStream(0, 0, 0);

Patrick, does this code need to checking some max connection limit to see if it's OK to be opening another socket?

@@ +111,5 @@
> +    pump.init(this.inputStream, -1, -1, 0, 0, false);
> +    pump.asyncRead(this, null);
> +  },
> +  
> +  stop: function ts_stop() {

shouldn't this be "close", not "stop"?  I see close in the IDL, not stop.

@@ +120,5 @@
> +  },
> +
> +  write: function ts_write(data) {
> +    this.outputStream.write(data, data.length);
> +    this.outputStream.flush();

note that nsSocketOutputStream::Flush is a placebo (just returns NS_OK), so not sure you need this.  Why do you want to flush, anyway?

::: b2g/components/b2g.idl
@@ +18,5 @@
> +
> +
> +[scriptable, uuid(0f2abcca-b483-4539-a3e8-345707f75c44)]
> +interface nsIB2GTCPSocketEvent : nsIDOMEvent {
> +	readonly attribute DOMString data;

It might be useful to have a pointer to the nsIB2GTCPSocket here, so that a single callback function could handle multiple sockets (and be able to figure out which socket a given event belongs to)?  Or perhaps that's just more complexity, and we should leave it up to JS client.  Thoughts? (I'm not much of a JS programmer).

@@ +34,5 @@
> +
> +  void open();
> +  void close();
> +
> +  void write(in DOMString data);

It seems likely you may also want to add

  void suspend()
  void resume()

else the client has no way to do stop/slow incoming data.

I suspect having a 'readyState' and/or isAlive method (see nsISocketTransport isAlive and the various STATUS_ codes) would be useful. Also we'll probably want to provide a way to access the socket's remote/local hosts/ports (I assume via the transport's getScriptablePeerAddr/getScriptableSelfAddr methods).

@@ +47,5 @@
> +    * the request.result will be a nsIB2GTCPSocket if successful,
> +    * and and error is fired in other cases.
> +    * options currently only supports { ssl: true } to open SSL connections
> +    */
> +  nsIDOMDOMRequest createSocket(in DOMString host, in long port, in jsval options);

why are you returning a nsIDOMDOMRequest instead of a nsIB2GTCPSocket here?

(I suppose it's too late to start enforcing camel-casing like nsIB2gTcpSocket?)

So you don't provide a way to set socket options:
  
   http://www.mozilla.org/projects/nspr/reference/html/priotyp.html#17191

Yet I don't see a way to set these via the nsISocketTransportService/nsISocketTransport IDLs.  Odd.  Perhaps Patrick or Honza can tell me if I missed that. I would think that being able to at least disable Nagle  (PR_SockOpt_NoDelay) would be useful for certain telnet/chat-like applications, for instance.

(Aha--I see we always disable Nagle, as of bug 542401: 

   http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsSocketTransport2.cpp#1140

That will not be friendly to naively written JS apps that don't coalesce writes into the largest writes possible.  We weren't really considering this use pattern (only tuned, in-browser writes) when we set NoDelay to be the default--Patrick, perhaps we should set Nagle-on to be the default and provide a way to disable it in the nsISocketTransport API, disable it for all in-browser protocols, and add a way to this B2G Socket API to disable it for power app users?).

If we want to add NoDelay or any other socket options (like buffer size), it's a bit tricky the way this API is structured, as the socket gets created and opened in one call here, so no logical place to set options before opening.

Comment 7

[Jason Duell]

Comment on attachment 603468 [details] [diff] [review]
wip-0.1

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

Whoops I stomped on Jonas' feedback+--sorry!

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 603468 [details] [diff] [review]
wip-0.1

nagle creates unexpected network patterns and delays. It should be off by default. An option to turn it on if your application can tolerate delays for the purpose of coalescing would be fine.

if we need to check any limits we can do that in necko socketransport directly.

I understand the non-websockets use case - "write an email client in html5". however, I'm interested in hearing the security story around this. websockets goes to a lot of effort to avoid letting JS applicatons send arbitrary data to ports behind firewalls (e.g. the sec-websocket-accept dance during the handshake). Is that not necessary here, or is the app model just presumed to be more secure than random js off the internet on a webpage and that's sufficient?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, the use-case is speaking to software and hardware that doesn't talk WebSocket. Like mail servers and TCP-enabled TVs for example.

We will need a security model for this though. It's not intended to be something exposed to all web pages.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Patrick McManus [:mcmanus] from comment #8)
> Comment on attachment 603468 [details] [diff] [review]
> wip-0.1
> 
> I understand the non-websockets use case - "write an email client in html5".
> however, I'm interested in hearing the security story around this.
> websockets goes to a lot of effort to avoid letting JS applicatons send
> arbitrary data to ports behind firewalls (e.g. the sec-websocket-accept
> dance during the handshake). Is that not necessary here, or is the app model
> just presumed to be more secure than random js off the internet on a webpage
> and that's sufficient?

Raw socket access needs permission.  Random JS off the internet doesn't get that permission.

The model for "installed apps" has been worked out in some detail but not communicated well.  We'll be doing so with the security team later this week.  Details to be posted to dev-b2g.

Comment 11

[Patrick McManus [:mcmanus]]

from an api pov you might want some way to transition from plaintext to tls.. this pattern is used in smtp (the starttls command), and in setting up TLS tunnels through HTTP proxies (CONNECT in plaintext HTTP starts a TLS tunnel) as two examples.

Comment 12

[Patrick McManus [:mcmanus]]

one other thought about the api: you have it defined essentially as a consumer of data pushed at it at whatever rate gecko wants to do it.. right now that would pretty much be as fast as it can.

1] some api to [un]pause the data events would be good. TCP has flow control, it can stop reading from a long stream and eventually the peer will stop sending data.

2] maybe some notion of priority for the socket.. even if it is just foreground vs background.. right now necko wouldn't do much of anything with that, but I would hope to have the infrastructure to do so someday. (i.e syncing your podcatcher or updating your maps database could yield to your IM client.)

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 603468 [details] [diff] [review]
wip-0.1

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

There should be clear documentation on how the events (onconnect, ondisconnect, onerror) get fired.

::: b2g/components/TCPSocket.js
@@ +55,5 @@
> +  this._options = aOptions;
> +  this._onconnect = null;
> +  this._ondisconnect = null;
> +  this._ondata = null;
> +  this._onwindow = aWindow;

this._window ?

@@ +103,5 @@
> +    this.transport.securityCallbacks = SecurityCallbacks;
> +    
> +    // Alright to open streams here as they are non-blocking by default
> +    this.outputStream = this.transport.openOutputStream(0, 0, 0);
> +    this.inputStream = this.transport.openInputStream(0, 0, 0);

According the args:
The first 0 means you want to open buffered+non-blocking socket, I'd personally open the output stream as blocking (see bellow).
The second and third 0s mean you want to let the socket layer decide on the buffer size.

Unfortunatelly the default buffer size is too small (786432 bytes).

You have to pass 65536, 65536 that gives full 4GB buffer allocated with 64kB granularity.  This will ensure that larger writes will not cause write() to block soon after the default small buffer gets filled up.

And yes, we could have some limit per/domain or per/page for how many sockets to create. 


BTW, is there going to be some permission model to e.g. disallow apps to create connections?

@@ +108,5 @@
> +
> +    let pump = Cc["@mozilla.org/network/input-stream-pump;1"]
> +                 .createInstance(Ci.nsIInputStreamPump);
> +    pump.init(this.inputStream, -1, -1, 0, 0, false);
> +    pump.asyncRead(this, null);

You actually shouldn't use the pump here.  You can just asyncWait() for the inputstream (QI to nsIAsyncInputStream).  ondisconnect can be detected by read of 0 bytes from the stream.  onerror can be detected by throw of inputstream.read().

@@ +115,5 @@
> +  stop: function ts_stop() {
> +    LOG("shutdown called\n"); 
> +    this.outputStream.close();
> +    this.inputStream.close();
> +    this.transport.close(Components.results.NS_OK);

So, close() (or stop() ?) shuts the socket both sides.  OK...

@@ +120,5 @@
> +  },
> +
> +  write: function ts_write(data) {
> +    this.outputStream.write(data, data.length);
> +    this.outputStream.flush();

flush() is nop here.

The write() method may not write all the data if only some of the string data fits the buffer space left.  I think this happens also in case of setting the output stream as BLOCKING.  You have to buffer the renaming data and send them when the output stream gets writable again...

What if the consumer wants to send a file?  We should allow send of data streams larger then 4GB.  Then, you need another method writeStream(stream) that will write the given nsIInputStream out.

@@ +128,5 @@
> +    LOG("onStartRequest called\n");
> +    if (!this._onconnect)
> +      return;
> +
> +    this._onconnect.handleEvent(new TCPSocketEvent("onconnect", ""));

Hmm.. I'm not 100% sure when this onStartRequest gets called.  Doesn't this get called only after the first data is available on the socket?  Or even worse, when the pipe copier gets just open?  It's actually quit later then actual connect.

Better may be to use socket transport state events for firing onconnect.

@@ +145,5 @@
> +    let sInputStream = Cc["@mozilla.org/scriptableinputstream;1"]
> +                         .createInstance(Ci.nsIScriptableInputStream);
> +    sInputStream.init(inputStream);
> +
> +    let response = sInputStream.read(count);

inputStream is buffer and when we are here, there is something to read.  So it may not be true this can throw, but I'm not 100% sure.

I believe instead of throw onStopRequest with an error will get fired.

@@ +201,5 @@
> +        }
> +      }
> +    }
> +    dns.asyncResolve(aHost, 0, listener, Services.tm.currentThread);
> +    return request;

Ah, this is the reason you return the request.  You can create the socket directly with a host name.  No "manual" DNS is needed.

@@ +245,5 @@
> +    LOG('Cert Error:\n');
> +    LOG('  targetSite: ' + targetSite + '\n');
> +    LOG('  domainMismatch: ' + status.isDomainMismatch + '\n');
> +    LOG('  isNotValidAtThisTime: ' + status.isBotValidAtThisTime + '\n');
> +  },

These should throw onerror with some info.

@@ +248,5 @@
> +    LOG('  isNotValidAtThisTime: ' + status.isBotValidAtThisTime + '\n');
> +  },
> +    
> +  getInterface: function sc_getInterface(iid) {
> +    return this;

Ugly...

::: b2g/components/b2g.idl
@@ +24,5 @@
> +
> +[scriptable, uuid(98e52480-647c-11e1-a004-b75546d8a7e7)]
> +interface nsIB2GTCPSocket : nsIDOMEventTarget {
> +  attribute nsIDOMEventListener onconnect;
> +  attribute nsIDOMEventListener ondisconnect;

ondisconnect gets called exactly when?

I think you have to have onerror as well.

Actually, according the event model, I would inspire with how XHR works.  Jason also mentions this in his feedback.  On the other hand, this is a bit different.  XHR is 1 request/1 response.

This is a bidirectional stream that may well be closed only from one side (ours or peers).  Then ondisconnect event doesn't mean we cannot write() to the socket more data.  And close() doesn't mean that we cannot get one or more ondata.

@@ +47,5 @@
> +    * the request.result will be a nsIB2GTCPSocket if successful,
> +    * and and error is fired in other cases.
> +    * options currently only supports { ssl: true } to open SSL connections
> +    */
> +  nsIDOMDOMRequest createSocket(in DOMString host, in long port, in jsval options);

TLS support?  In that case, how cert errors will be handled?

Comment 14

[Jason Duell]

> some api to [un]pause the data events would be good.

Patrick: Is that the same as providing suspend/resume, or did you have something different in mind?

re: Nagle, and whether we need to allow setting socket options in general.  Yes, I think bug 542401 comment 16 is right, so we can disable Nagle by default (we should document that in the nsB2GTCPSocket API).  Having this patch allow Nagle to be turned back on is not an immediate priority.  In general, looking at the Java socket API

  http://docs.oracle.com/javase/1.4.2/docs/api/java/net/Socket.html

I guess we can totally skip setting any sockoptions for now, and if we need them we can add another constructor without (host, port) that will return a non-connected socket that we can set options on before calling a new connect() method.  For now it's not clear we need any of the socket options, so this can all be followup work IMO.

> You have to pass 65536, 65536 that gives full 4GB buffer allocated with 
> 64kB granularity

Honza, so if the InputStream is set this large, does the TCP socket buffer size not matter much, i.e. we don't need to provide set_sockopt access to setting it?

ReadyState and event model:

> according the event model, I would inspire with how XHR works.

nsIWebSocket seems a closer fit to me.  But yes.

So as Honza points out we can have "half-closed" TCP sockets until both ends explicitly close the connection, so we probably want 'readyState' to have these states:

  CONNECTING,
  OPEN,
  CLOSED_LOCAL
  CLOSED_REMOTE
  CLOSED

I think we need a onremoteclose event (so client can know when no more traffic will arrive from other end). And make sure that we throw if we call write() while CLOSED_LOCAL.  Once we have onremoteclose, I don't think we need ondisconnect, unless I'm missing something.  The client will know the socket is closed when both close() has been called and onremoteclose has fired. 

Do we care about providing an event for DNS resolution, so our example JS email client can display "resolving smtp.foo.com", like Firefox does for websites?  nsITransportEventSink provides a STATUS_RESOLVED event if we do.  We could add an onresolution event (now or in followup).  Or we could provide the equivalent someday by providing a DNS resolution API to JS (an app that wants the 'resolving' message could use the DNS API to display when resolution is complete, then pass the IP to the socket API).

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Patrick McManus [:mcmanus] from comment #12)
> 2] maybe some notion of priority for the socket.. even if it is just
> foreground vs background.. right now necko wouldn't do much of anything with
> that, but I would hope to have the infrastructure to do so someday. (i.e
> syncing your podcatcher or updating your maps database could yield to your
> IM client.)

Gecko can do this automatically, no?  Or do you mean some bit devs can set on their connection to explicitly make it lower priority?

Comment 16

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #14)
> > some api to [un]pause the data events would be good.
> 
> Patrick: Is that the same as providing suspend/resume, or did you have
> something different in mind?
> 

same thing as I understand it.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> (In reply to Patrick McManus [:mcmanus] from comment #12)
> > 2] maybe some notion of priority for the socket.. even if it is just
> > foreground vs background.. right now necko wouldn't do much of anything with
> > that, but I would hope to have the infrastructure to do so someday. (i.e
> > syncing your podcatcher or updating your maps database could yield to your
> > IM client.)
> 
> Gecko can do this automatically, no?  Or do you mean some bit devs can set
> on their connection to explicitly make it lower priority?

sorry for being unclear - I meant the api should provide some bits devs can set to declare low priority for background stuff.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #14)
> > You have to pass 65536, 65536 that gives full 4GB buffer allocated with 
> > 64kB granularity
> 
> Honza, so if the InputStream is set this large, does the TCP socket buffer
> size not matter much, i.e. we don't need to provide set_sockopt access to
> setting it?

The buffer I refer to is the intermediate pipe buffer over the socket.  It has nothing to do with the TCP buffer.

I think there is no need to expose the TCP options to change the buffer sizes.  Usually you don't need to change those options since modern systems have heuristics to adjust the size them self when not specified.

> 
> ReadyState and event model:
> 
> > according the event model, I would inspire with how XHR works.
> 
> nsIWebSocket seems a closer fit to me.  But yes.

Yeah, that would be a better example, right.

> 
> So as Honza points out we can have "half-closed" TCP sockets until both ends
> explicitly close the connection, so we probably want 'readyState' to have
> these states:
> 
>   CONNECTING,
>   OPEN,
>   CLOSED_LOCAL
>   CLOSED_REMOTE
>   CLOSED

As I was thinking of this more, I realized we don't shutdown() sockets, we just close() them what means to close bidirectionally.  And we do so when we call close() on the transport or both streams get closed.  So probably no need to worry about this, however it could make protocol implementations in general harder.

> 
> I think we need a onremoteclose event (so client can know when no more
> traffic will arrive from other end). And make sure that we throw if we call
> write() while CLOSED_LOCAL.  Once we have onremoteclose, I don't think we
> need ondisconnect, unless I'm missing something.  The client will know the
> socket is closed when both close() has been called and onremoteclose has
> fired. 

I don't know... ondisconnect is good (eqs to EOF or onstoprequest), but it's hard to say whether to call it also when we close() first, the current code does it, I believe.

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #17)
> the current code does it, I believe.

the current *patch* (wip-0.1).

Comment 19

[Jason Duell]

> I realized we don't shutdown() sockets, we just close() them what 
> means to close bidirectionally

Huh--you're right.  Do we know if there are likely to be apps that rely on shutdown being available?  Seems like you could design a protocol (and if you can, someone probably has) so that one end expects an EOF and replies with something before closing its end too--I don't think we can support that w/o shutdown IIUC.

>  it's hard to say whether to call [ondisconnect|onremoteclose|whatever_name] also when we close() first, the current code does it, I believe.

The websocket protocol does dispatch an onclose even if it's the client JS that has called close().  I'd go with that just to be consistent.  (I'd also go with 'onclose' instead of 'ondisconnect/onserverclose').

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #19)
> > I realized we don't shutdown() sockets, we just close() them what 
> > means to close bidirectionally
> 
> Huh--you're right.  Do we know if there are likely to be apps that rely on
> shutdown being available?  Seems like you could design a protocol (and if
> you can, someone probably has) so that one end expects an EOF and replies
> with something before closing its end too--I don't think we can support that
> w/o shutdown IIUC.
> 

We don't support shutdown even in the opposite way.  Gecko has implemented a tun of protocols w/o using shutdown.

So, I think to keep things simple we are probably OK with what the patch does.

On strong complains we would have to change the core of nsSocketTransport, but it's not needed to be done now.


> >  it's hard to say whether to call [ondisconnect|onremoteclose|whatever_name] also when we close() first, the current code does it, I believe.
> 
> The websocket protocol does dispatch an onclose even if it's the client JS
> that has called close().  I'd go with that just to be consistent.  (I'd also
> go with 'onclose' instead of 'ondisconnect/onserverclose').

I think to have a single handler of onclose for both remote and local close is OK.  If onclose provides the error code (that, though, may be NS_BASE_STREAM_CLOSED on both clean local and clean remote close) then we may be in better position.

Comment 21

[Jason Duell]

> If onclose provides the error code

websockets has onerror for errors (and we could avoid calling it for NS_BASE_STREAM_CLOSED).  Seems cleaner to me than educating programmers about NS_BASE_STREAM_CLOSED, but I don't feel strongly about it.

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-0.2

This wip address many code comments and is the API exposed to web applications is much closer to the one proposed by WebSocket:

 interface nsIB2GTCPSocket : nsISupports
{
  readonly attribute unsigned short port;
  readonly attribute DOMString host;
  readonly attribute boolean ssl;

  // Bug 723206 - Constructors implemented in JS from IDL should be
  //              allowed to have arguments
  //
  //  Once bug 723206 will be fixed, this method could be replaced by
  //  arguments when instantiating a TCPSocket object. For example it will
  //  be possible to do (similarly to the WebSocket API):
  //    var s = new MozTCPSocket(host, port); 
  //
  void open(in DOMString host, in unsigned short port, [optional] in boolean ssl);
  void suspend();
  void resume();
  void close();

  void send(in jsval data);

  //ready state
  const unsigned short CONNECTING = 0;
  const unsigned short OPEN = 1;
  const unsigned short CLOSING = 2;
  const unsigned short CLOSED = 3;
  readonly attribute unsigned short readyState;

  // event handler attributes
  attribute nsIDOMEventListener onopen;
  attribute nsIDOMEventListener onmessage;
  attribute nsIDOMEventListener onerror;
  attribute nsIDOMEventListener onclose;
};

Here are the big differences:
 - close is exposed as a scriptable method so web application can decide to close the connection.

 - There is no url/extensions/protocol properties. But instead there is host/port/ssl.

 - send takes a jsval and not a nsIVariant so JS coercions should be better (from what I have understand).

 - bufferedAmount is not exposed. I don't know if we want that?

 - binaryType is not exposed. The current code does not yet support Blob/ArrayBuffer.



I also have a few questions about errors handling.

It seems like the WebSocket API just fire onerror without any data. The current WIP fires onerror with a string describing the reason of the failure. What do we want here? 
 1. firing onerror without informations
 2. firing onerror with an error string
 3. firing onerror with a numeric error number


Also when the stream is opened and there is an error, should we fire both onerror and onclose, or just on error, or even just onclose?

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jason Duell (:jduell) from comment #6)
> > +[scriptable, uuid(0f2abcca-b483-4539-a3e8-345707f75c44)]
> > +interface nsIB2GTCPSocketEvent : nsIDOMEvent {
> > +	readonly attribute DOMString data;
> 
> It might be useful to have a pointer to the nsIB2GTCPSocket here, so that a
> single callback function could handle multiple sockets (and be able to
> figure out which socket a given event belongs to)?  Or perhaps that's just
> more complexity, and we should leave it up to JS client.  Thoughts? (I'm not
> much of a JS programmer).

Maybe the event target can points to the socket instance?

> I suspect having a 'readyState' and/or isAlive method (see
> nsISocketTransport isAlive and the various STATUS_ codes) would be useful.
> Also we'll probably want to provide a way to access the socket's
> remote/local hosts/ports (I assume via the transport's
> getScriptablePeerAddr/getScriptableSelfAddr methods).

At the moment I have exposed |readyState| similarly to WebSocket and also the remote host/port.

I can add isAlive(), STATUS_CODE and the local host/port but it starts to differs from the WebSocket API a lot.

> 
> @@ +47,5 @@
> > +    * the request.result will be a nsIB2GTCPSocket if successful,
> > +    * and and error is fired in other cases.
> > +    * options currently only supports { ssl: true } to open SSL connections
> > +    */
> > +  nsIDOMDOMRequest createSocket(in DOMString host, in long port, in jsval options);
> 
> why are you returning a nsIDOMDOMRequest instead of a nsIB2GTCPSocket here?
> 
> (I suppose it's too late to start enforcing camel-casing like
> nsIB2gTcpSocket?)

Sounds like there is already a few places with B2G in the source code, sorry :/

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Patrick McManus [:mcmanus] from comment #11)
> from an api pov you might want some way to transition from plaintext to
> tls.. this pattern is used in smtp (the starttls command), and in setting up
> TLS tunnels through HTTP proxies (CONNECT in plaintext HTTP starts a TLS
> tunnel) as two examples.

This is not done in this WIP but as soon I get the use case with SMTP in my client email application I will start to see how it works.

Comment 25

[Andreas Gal :gal]

nsIB2GTCPSocket => nsIMozRawSocket or something like that or nsITCPSocket. You guys should take this to web-api and see what people think.

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jason Duell (:jduell) from comment #14)
> 
> Do we care about providing an event for DNS resolution, so our example JS
> email client can display "resolving smtp.foo.com", like Firefox does for
> websites?  nsITransportEventSink provides a STATUS_RESOLVED event if we do. 
> We could add an onresolution event (now or in followup).  Or we could
> provide the equivalent someday by providing a DNS resolution API to JS (an
> app that wants the 'resolving' message could use the DNS API to display when
> resolution is complete, then pass the IP to the socket API).

I think this should belong to a different API.

Comment 27

[David :Bienvenu]

(In reply to Vivien Nicolas (:vingtetun) from comment #24)

> This is not done in this WIP but as soon I get the use case with SMTP in my
> client email application I will start to see how it works.

Most imap servers support STARTTLS as well.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-0.3

(In reply to Andreas Gal :gal from comment #25)
> nsIB2GTCPSocket => nsIMozRawSocket or something like that or nsITCPSocket.
> You guys should take this to web-api and see what people think.

Renamed to nsITCPSocket.

I will bring the discussion to web-api. (you mean the mailing list right?)

The initial goal of this bug was to exposed a small API to b2g-only and starts to have something to play with (I should have choosen an other name!). Having a full featurred/final API will take some times.
What do you think of landing the API based on WebSocket first and iterates over that?

Comment 29

[Andreas Gal :gal]

Iterative is good, but iterate towards the real goal. A privileged raw socket API. But yeah we can iterate quickly and we can break the API along the way often and hard.

How difficult is it to bring up the thunderbird imap/pop/smtp code on top of this? Is that stuff JS or C++?

Comment 30

[David :Bienvenu]

(In reply to Andreas Gal :gal from comment #29)

> 
> How difficult is it to bring up the thunderbird imap/pop/smtp code on top of
> this? Is that stuff JS or C++?

the protocol code is all c++, so everything needs to be written more or less from scratch in js. The basic stuff is not hard; the devil is in the myriad of details.

Comment 31

[Andrew Sutherland [:asuth] (he/him)]

(In reply to David :Bienvenu from comment #30)
> (In reply to Andreas Gal :gal from comment #29)
> > How difficult is it to bring up the thunderbird imap/pop/smtp code on top of
> > this? Is that stuff JS or C++?
> 
> the protocol code is all c++, so everything needs to be written more or less
> from scratch in js. The basic stuff is not hard; the devil is in the myriad
> of details.

There is a fairly simple node.js pure JS IMAP binding that could be used as the basis for a simple proof-of-concept e-mail client:
https://github.com/mscdex/node-imap/

I have looked around a bit for other JS IMAP implementations, and there isn't much available.  There is another node binding that uses a C parser which is less desirable.

Comment 32

[David :Bienvenu]

(In reply to Andrew Sutherland (:asuth) from comment #31)

> There is a fairly simple node.js pure JS IMAP binding that could be used as
> the basis for a simple proof-of-concept e-mail client:
> https://github.com/mscdex/node-imap/
yes, I suspect that's the one I pointed Vivien at.

Comment 33

[Jason Duell]

> - bufferedAmount is not exposed. I don't know if we want that?

Ah--I see we've got some big differences here in the send() semantics.  You're passing OPEN_BLOCKING to the openOutputStream call--this will mean that send() will block if the output buffers are full.  Are we ok with JS blocking on the network?  I suspect not (Websockets/XHR both have nonblocking send), but it may depend on your use case (should be fine for workers for instance).

If we want nonblocking semantics, we've got a few choices: 

1) If we don't pass OPEN_BLOCKING, then the outputStream will throw NS_BASE_STREAM_WOULD_BLOCK to JS when the buffers are full.  This is simple, but different than WS/XHR semantics, and puts the burden on the JS app to set a timeout or otherwise arrange to try the send later.  nsISocketTransport clients can know when it's OK to try sending more data by setting an eventsink on the transport that gets notified of STATUS_SENDING_TO events--I suppose you could expose something similar. (We could also allow a flag to the JS API to set blocking/nonblocking at socket creation time, and support both models in JS).

2) Do as WS/XHR do, and buffer large amounts of data for the user, up to some (possibly unspecified) limit, at which point we throw an error and close the socket.   Most apps can ignore buffering, and those which do large amounts of sending can monitor 'bufferedAmount':  if it keeps increasing, the network is not consuming data as quickly as we're sending it, and the rate of sends should be slowed down. Web sockets allows implementations to throw an error and close the socket if bufferedAmount gets too big (no limit is specified but it should be fairly large so this doesn't happen casually).  Websockets/XHR queue sent data on the socket transport thread, but I think you ought to be able to do it in your main thread JS library: just have a sendbuffer array, append things to it if the socket would block, and use the eventsink method to know when to retry.  (A poor man's version would just be to set really large buffers on the outputStream and fail the socket if we'd ever block, but that wastes space for every socket that doesn't need large buffers: might be a short-term hack at best).

I'm guessing we want some version of #2.  Hopefully Honza/Patrick/biesi can correct me if I'm wrong on any details here.

> when the stream is opened and there is an error, should we fire both onerror and onclose, or just on error, or even just onclose?

I think you can throw an exception (and no events) in the (host, port) constructor or in open() if an error happens before they return.  Once they've returned successfully you should fire both error and close events if an error occurs.  That's what WS does.

> It seems like the WebSocket API just fire onerror without any data

The WS spec is indeed surprisingly limited in the amount of error data it requires--really just firing onerror.  I'm not sure if we need to do better here.

Comment 34

[Jason Duell]

A note on close() semantics:  unlike websockets, where close() starts a WS-specific protocol shutdown that ends with the server (usually) doing the TCP close, in this API close() shuts down the connection almost immediately (not synchronously, but quickly).  So you could make a case that the CLOSING state isn't terribly useful here, and that we should just go right to CLOSED when close() returns, and not call onclose.  I could be persuaded either way--I'd lean toward keeping the current logic just because it's closer to websockets, but if you get feedback from app programmers that it's useless, we could remove it.

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, we definitely want some variant of #2

Comment 36

[Honza Bambas (:mayhemer)]

Re: buffering/blocking:

Yes, we need to be able to buffer the most possible (reasonable) amount of data.  The send() implementation writes to a large blocking pipe, not to the socket directly.  That is the simplest way.  I already wrote about this in comment 13, btw.

If websockets don't have a way to send a stream, a whole file for instance, then we don't need it here as well.  I would, though, like to have sendStream API ones.


Re: close():

If consumers of websockets are ok with how websockets do it, then let's not try to invent a wheel again..  Are there any spec concerns about wobsocket.close() behavior?

Comment 37

[Honza Bambas (:mayhemer)]

Comment on attachment 604810 [details] [diff] [review]
wip-0.3

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

::: b2g/components/TCPSocket.js
@@ +109,5 @@
> +    let transport = this._transport = createTransport(host, port, this.ssl);
> +    transport.securityCallbacks = new SecurityCallbacks(this);
> +    
> +    this._inputStream = transport.openInputStream(0, 0, 0);
> +    this._outputStream = transport.openOutputStream(1, 65536, 0);

Use nsITransport.OPEN_BLOCKING instead of 1.  

This (65536, 0) limits the output buffer to 1,5 MB.  We should allow larger size (100MB or even more ?).  Since the stream is blocking, we should block as late as possible.

@@ +119,5 @@
> +  },
> +  
> +  close: function ts_close() {
> +    if (this.readyState === CLOSING || this.readyState === CLOSED)
> +      return;

I think (needs to be confirmed!) that if the server shuts the socket down (we are in CLOSED state, the inputstream has closed) we may not be able with this implementation close the socket.  So it may be left hanging in half open state.

@@ +131,5 @@
> +  },
> +
> +  send: function ts_send(data) {
> +    if (this.readyState !== OPEN) {
> +      this.dispatchEvent("onerror", "Socket is not opened");

Is onerror the right thing to do in case of bad state call?

@@ +178,5 @@
> +  },
> +
> +  onDataAvailable: function ts_onDataAvailable(request, context, inputStream, offset, count) {
> +    this._scriptableInputStream.init(inputStream);
> +    this.dispatchEvent("onmessage", this._scriptableInputStream.read(count));

IMO better name is ondata.  This doesn't provide any "message" object or so, just raw data.

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There has been some concerned expressed regarding the fact that websockets simply close the connection if running out of buffer space. However in many cases that has been due to people not understanding that running out of buffer space is basically expected to happen only when running out of memory.

One possible option that has come up is to synchronously fire an even if we run out of buffer space. If .preventDefault() is called on the event we wouldn't close the connection but rather just drop the latest message on the floor. That way pages can do their own error handling if they want to.

Comment 39

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

sec review assigned to :pauljt

Comment 40

[Donovan Preston [:fzzzy]]

Attachment: wip-0.4

wip-0.4; Andreas asked me to work on this patch while Vivien was on vacation. He asked for:

* Change onmessage callback name to ondata
* Non-blocking send
* Ondrain callback which gets called once after send has failed because it would block
* TypedArray support

It would be nice if the lower network layers had apis which took TypedArrays, but it appears they do not yet. In the meantime I implemented conversion to/from TypedArrays in send and before calling ondata.

He also asked for shutdown/half close support and the corresponding callbacks, but the network layer does not yet support that so I left it out for now.

Please review the latest 0.4 wip patch and let me know your comments, thanks.

Comment 41

[Honza Bambas (:mayhemer)]

Comment on attachment 611592 [details] [diff] [review]
wip-0.4

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

::: b2g/components/TCPSocket.js
@@ +129,5 @@
> +    this.host = host;
> +    this.port = port;
> +    if (options !== undefined) {
> +      this.ssl = options.useSSL === true;
> +      this.verifyCert = options.verifyCert === true;

this should default to true I think (don't know what is the policy for options members).

@@ +178,5 @@
> +      // asyncWait will synchronously call the callback before
> +      // send returns!! Force it to wait until the next turn.
> +      let that = this;
> +      next_turn(function() {
> +        that._outputStream.asyncWait(that, 0, 0, null);

Pass nsIThreadManager.currentThread or .mainThread as the last arg.  Then it will always be async and you don't need any hacks.

@@ +212,5 @@
> +  // nsIAsyncOutputStream
> +  onOutputStreamReady: function ts_onOutputStreamReady(stream) {
> +    if (this.readyState === CONNECTING) {    
> +      this.readyState = OPEN;
> +      this.dispatchEvent("onopen");

This won't fire after the actual connection is established.  When you call asyncWait on output stream, that here is a pipe, with space avail in the buffer you always get this callback regardless the TCP socket connection status, i.e. immediately.  That is actually the purpose of the buffering...

You may use onTransportStatus notification from the socket transport (nsISocketTransport, nsITransport.setEventSink) to detect TCP connection (not full SSL session, though) creation.

@@ +275,5 @@
> +SecurityCallbacks.prototype = {
> +  notifySSLError: function sc_notifySSLError(socketInfo, error, targetSite) {
> +    this._socket.dispatchEvent("onerror", "SSL Error: " + error);
> +    // This isn't really the right thing because I think necko still
> +    // closes the connection, this just supresses the error

It is actually ignored: http://hg.mozilla.org/mozilla-central/annotate/869edbbfad81/security/manager/ssl/src/nsNSSIOLayer.cpp#l1592

There is no way currently to pass over cert errors, unless adding an exception.  We can look for a mechanism to rise a dialog that allows that.  It should be possible I think, we already have that dialog.

@@ +295,5 @@
> +    return !this.verifyCert;
> +  },
> +    
> +  getInterface: function sc_getInterface(iid) {
> +    throw Cr.NS_ERROR_NO_INTERFACE;

General proposal: write a test we all can ran :)  At least a simple one.

This is apparently untested.  You have to return |this| here for at least nsISSLErrorListener and nsIInterfaceRequestor.

::: b2g/components/b2g.idl
@@ +58,5 @@
> +  attribute DOMString binaryType;
> +
> +  // event handler attributes
> +  attribute nsIDOMEventListener onopen;
> +  attribute nsIDOMEventListener ondrain;

onwritable ?

Comment 42

[Donovan Preston [:fzzzy]]

Thanks for the review!

(In reply to Honza Bambas (:mayhemer) from comment #41)
> Comment on attachment 611592 [details] [diff] [review]
> wip-0.4
> 
> Review of attachment 611592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/TCPSocket.js
> @@ +129,5 @@
> > +    this.host = host;
> > +    this.port = port;
> > +    if (options !== undefined) {
> > +      this.ssl = options.useSSL === true;
> > +      this.verifyCert = options.verifyCert === true;
> 
> this should default to true I think (don't know what is the policy for
> options members).

Yes, this sounds good. It felt weird to me having the default option be true, and to have to explicitly say false, (simply because I usually consider optional values to default to false) but I think it makes more sense to verify the cert by default.

> > @@ +178,5 @@
> > +      // asyncWait will synchronously call the callback before
> > +      // send returns!! Force it to wait until the next turn.
> > +      let that = this;
> > +      next_turn(function() {
> > +        that._outputStream.asyncWait(that, 0, 0, null);
> 
> Pass nsIThreadManager.currentThread or .mainThread as the last arg.  Then it
> will always be async and you don't need any hacks.

I had already figured out how to pass currentThread after I submitted this patch, but thanks for letting me know this also removes the need for the next turn thing. This also solves another crashing problem I couldn't figure out before, I think...

> > @@ +212,5 @@
> > +  // nsIAsyncOutputStream
> > +  onOutputStreamReady: function ts_onOutputStreamReady(stream) {
> > +    if (this.readyState === CONNECTING) {    
> > +      this.readyState = OPEN;
> > +      this.dispatchEvent("onopen");
> 
> This won't fire after the actual connection is established.  When you call
> asyncWait on output stream, that here is a pipe, with space avail in the
> buffer you always get this callback regardless the TCP socket connection
> status, i.e. immediately.  That is actually the purpose of the buffering...
> 
> You may use onTransportStatus notification from the socket transport
> (nsISocketTransport, nsITransport.setEventSink) to detect TCP connection
> (not full SSL session, though) creation.

Cool, thanks.

> @@ +275,5 @@
> > +SecurityCallbacks.prototype = {
> > +  notifySSLError: function sc_notifySSLError(socketInfo, error, targetSite) {
> > +    this._socket.dispatchEvent("onerror", "SSL Error: " + error);
> > +    // This isn't really the right thing because I think necko still
> > +    // closes the connection, this just supresses the error
> 
> It is actually ignored:
> http://hg.mozilla.org/mozilla-central/annotate/869edbbfad81/security/manager/
> ssl/src/nsNSSIOLayer.cpp#l1592
> 
> There is no way currently to pass over cert errors, unless adding an
> exception.  We can look for a mechanism to rise a dialog that allows that. 
> It should be possible I think, we already have that dialog.

I think there needs to be a webapi for managing security exceptions so that b2g can provide a ui for this.
 
> @@ +295,5 @@
> > +    return !this.verifyCert;
> > +  },
> > +    
> > +  getInterface: function sc_getInterface(iid) {
> > +    throw Cr.NS_ERROR_NO_INTERFACE;
> 
> General proposal: write a test we all can ran :)  At least a simple one.

Yes. Absolutely. I have some code I have been running manually to test, but because b2g is a bit chaotic right now, it's kind of difficult to get an environment together. I will provide test code and instructions with the next version of the patch, which should be ready later today...

> This is apparently untested.  You have to return |this| here for at least
> nsISSLErrorListener and nsIInterfaceRequestor.

Yes, probably untested. Honestly I'm not even sure what that code does, Vivien wrote it originally (I think) and I didn't pay much attention to it. I'll learn what this code is supposed to do so I can write some tests for it.

> ::: b2g/components/b2g.idl
> @@ +58,5 @@
> > +  attribute DOMString binaryType;
> > +
> > +  // event handler attributes
> > +  attribute nsIDOMEventListener onopen;
> > +  attribute nsIDOMEventListener ondrain;
> 
> onwritable ?

ondrain was chosen because that is what node uses, but since this API is not semantically identical to the node api I wouldn't mind a different name.

Although it might be better to provide semantics identical to node...

Comment 43

[Andrew Sutherland [:asuth] (he/him)]

It would be great if the next rev has startTLS support.  The options just needs a way cause createTransport to pass ["starttls"] instead of ["ssl"] and then expose a method (named startTLS?) that does:
  this._transport.securityInfo.QueryInterface(Ci.nsISSLSocketControl)
                              .StartTLS();


Is there a way to surface JS-implemented components like this off the main thread now so it can be used in a background thread (to support the proposed background services mechanism), or will this need to be re-written in C++/XPCOM or C++/JS-API?  I would expect IMAP clients that are trying to be more than a stateless squirrelmail-type UI would want to do the background service thing.


In terms of security exceptions, my restartless jetpack that exposes this API to pages it hosts (strictly for rapid prototyping of IMAP stuff) has door-hangers for permission working reasonably well.  It does a permission lookup when open is called and will asynchronously ask the user if they want to approve the connection.  This introduces an additional readyState of 'authorizing'.  Once the connection is approved, the actual open happens.

It also will detect failed certificates and use a door-hanger to ask if the user wants to see the certificate and potentially add an exception.  That door-hanger is only presented if the connection was explicitly opened with an "allowOverride" option, since bad SSL certificates are probably not desirable in all cases.  One needs to reconnect after the SSL certificate exception is added; the connection is not automatically retried.

A packaged version of the jetpack for those interested can be found here:
https://clicky.visophyte.org/files/labs/jetpack-tcp-imap-demo/jetpack-tcp-imap-demo-0.1.xpi

To see it operate:
1) Open up: about:imap-raw
2) Enter host/port/security/username/password
3) Click connect
4) It will dump the top-level folders
5) It will do a search to get the list of starred/flagged messages in your INBOX
6) It will fetch the headers and bodystructure for the first 10 of those, displaying the actual raw transit data and parsed up data inline.  The headers should get transformed by the node mailparser and libmime modules.

The source is here: https://github.com/asutherland/jetpack-tcp-imap-demo
with the permission bits being in the lib directory.

Comment 44

[Donovan Preston [:fzzzy]]

Thanks for reminding me of the link, Andrew! I'll take a look at the code and make this patch more like yours in api.

Comment 45

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Andrew Sutherland (:asuth) from comment #43)
> Is there a way to surface JS-implemented components like this off the main
> thread now so it can be used in a background thread (to support the proposed
> background services mechanism), or will this need to be re-written in
> C++/XPCOM or C++/JS-API?  I would expect IMAP clients that are trying to be
> more than a stateless squirrelmail-type UI would want to do the background
> service thing.
> 

I'm not sure what background service proposal you're referring to, but being able to access this API from worker threads would be very cool.  AIUI that's blocked on the new DOM bindings work.

Comment 46

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #45)
> I'm not sure what background service proposal you're referring to, but being
> able to access this API from worker threads would be very cool.  AIUI that's
> blocked on the new DOM bindings work.

With the landing of https://hg.mozilla.org/mozilla-central/rev/1bdb337e3136 it looks like the binding work is getting close?

A combination of the "Background Services for Web Applications" thread on dev-webapi you responded to:
http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/3455cb056e40d095/ca05a2d94199c592

And where it intersects with Mike Hanson's "Proposing some social features for Firefox" thread (which gets called out in that thread) and is basically about providing a persistent webpage/JS context that can be communicated with by other webpages:
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/1e4048b16c7a6705

The idea as it relates to an IMAP client would be to have a persistent JS context that has access to IndexedDB (also not yet available on workers, but presumably also gated on the JS bindings), the TCP API, and some communication channel to user-facing webpages/webapps (be it postMessage, message ports, or message channels).

That probably begs the question of whether that use case should be met by workers (possibly a SharedWorker?) or if B2G going multi-process will address the need and there is no need to have this API accessible from workers with any urgency.

Comment 47

[Andrew Sutherland [:asuth] (he/him)]

And I should point out I'm looking to help out (full-time) to facilitate the use-case for B2G, so please don't take this as idle pondering but an opportunity to point me at something! :)

Comment 48

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There's some initial discussion around server push notifications that's ongoing.  Andreas has more details.  That'd be a great place to plug in and help out :).

Comment 49

[Donovan Preston [:fzzzy]]

Attachment: wip-0.5

Hi, here's the next version of the patch. Thanks to Andrew Sutherland for lots of help and encouragement with this version.

This version incorporates as much of the feedback as I could find in the bug and on the mailing list. What's new?

* Tests!
* Services.tm.currentThread is passed to the right places
* TCPSocket is an nsIEventTarget as suggested earlier
* TCPSocketEvent has a socket attribute as suggested
* readyState constants are now strings, not integers
* send will now buffer until memory runs out, but send will
  return true if it buffered, and call ondrain once buffers are empty
  - This gives an easy to use api for sending large things without caring
  - But it also gives a good callback api for doing streaming content generation
* ArrayBuffer support is now lower level and should be much faster.
* onopen is now fired by onTransportStatus so that it is fired at exactly
  the right time, instead of immediately as before
* Connection Refused is now correctly detected and onerror is fired
* TCPSocket.open now only works if the tcp-socket permission has been granted.

Note: Because JavaScript-global-constructor doesn't call init (window),  
  I had to switch to JavaScript-global-property to implement permission
  checking. Because of this, the api is now:
  
"var s = MozTCPSocket.open(...);"

instead of

"var s = new MozTCPSocket(); s.open(...)"

Comment 50

[Donovan Preston [:fzzzy]]

I noticed a small typo, onInputStreamReady is bound to the correct name in the object, but the function object is named ts_onOutputStreamReady.

Comment 51

[Donovan Preston [:fzzzy]]

Attachment: wip-0.6

This newer patch fixes the typo, and makes some changes suggested by Fabrice:

* Move TCPSocket into it's own idl file.
* Switch back to inheriting nsISupports because of the bug fabrice linked.

Comment 52

[[:fabrice] Fabrice Desré]

Comment on attachment 618443 [details] [diff] [review]
wip-0.6

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

Looks like it's going in the right direction! Note that I haven't looked at the network code in details.

::: b2g/components/TCPSocket.idl
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "domstubs.idl"
> +#include "nsIDOMEvent.idl"
> +#include "nsIEventTarget.idl"

You can remove this for now.

@@ +7,5 @@
> +#include "nsIEventTarget.idl"
> +
> +
> +[scriptable, uuid(b82e17da-6476-11e1-8813-57a2ffe9e42c)]
> +interface nsITCPSocket : nsISupports

we usually use nsIDOMSomething for DOM apis.

::: b2g/components/TCPSocket.js
@@ +115,5 @@
> +      return;
> +
> +    this[type].handleEvent(new TCPSocketEvent(this, type, data || ""));
> +  },
> +

You're not really dispatching an event, but rather calling listeners. Can you rename this function

Also, the convention is to use (aType, aData) for parameters name (yes, that means a bunch of changes, sorry)

@@ +121,5 @@
> +    return this._outputStreamPipe.inputStream.available();
> +  },
> +
> +  init: function ts_init(aWindow) {
> +    let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager);

secMan is unused.

@@ +138,5 @@
> +    if (this._hasPrivileges === false) {
> +      LOG("TCPSocket does not have permission in this context.\n");
> +      return;
> +    }
> +    var that = new TCPSocket();

let that = ...

@@ +217,5 @@
> +
> +  startTLS: function() {
> +    this._transport.securityInfo.QueryInterface(
> +      Ci.nsISSLSocketControl
> +    ).StartTLS();

nit: if find this pattern hard to read:
 QueryInterface(
Ci.XXX
)

why not QueryInterface(Ci.XXX) ?

::: b2g/components/b2g.idl
@@ +8,1 @@
>  

probably a remain from git ;)

::: b2g/test/Makefile.in
@@ +32,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

use the new MPL 2 header from http://www.mozilla.org/MPL/headers/

Comment 53

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Will this API work when used from content processes?

Comment 54

[Honza Bambas (:mayhemer)]

Fabrice, I'll start the review on Tuesday.

Comment 55

[Jason Duell]

> Will this API work when used from content processes?

Nope.  This patch expects to be able to access nsISocketTransportService and create a transport from it directly.  That's not going to work under e10s currently.  I expect we'll want to add a C++ e10s layer has a SocketChild/Parent, etc that sends calls to the main process.  With a little luck we can implement that in way so that the JS code so far doesn't need much if any changing. 

Does that sound right to you Honza?

Comment 56

[Honza Bambas (:mayhemer)]

I have to confirm this patch won't work on e10s.

Usual decision: where to cut?

The interface (nsI[DOM]TCPSocket) can be implemented in C++ as a child side.  We only have to do the output buffering logic on the child side too - it's synchronous.  I.e. the child side must have an output buffer it self and asynchronously, or at least in a non-blocking way, pushing it to the parent.  Parent side then must care of the output buffer and communicate with the child side not get overloaded (and OOM).  That might be a bit more complicated but completely doable.

The rest seems to me async.  So, easy to implement for e10s.

Comment 57

[Honza Bambas (:mayhemer)]

Donovan, I started the review, there will be comments.  Please don't update the patch otherwise you will wipe out my splinter comments draft.  Thanks.

Comment 58

[Donovan Preston [:fzzzy]]

No problem, I have just been waiting for your comments before starting on the next version of the patch. :)

Comment 59

[Donovan Preston [:fzzzy]]

Ping Honza, how's that review

Comment 60

[Honza Bambas (:mayhemer)]

(In reply to Donovan Preston from comment #59)
> Ping Honza, how's that review

Right now I'm finishing it! :)

Comment 61

[Honza Bambas (:mayhemer)]

Comment on attachment 618443 [details] [diff] [review]
wip-0.6

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

r-

I didn't check the test.

::: b2g/components/TCPSocket.idl
@@ +22,5 @@
> +  //  arguments when instantiating a TCPSocket object. For example it will
> +  //  be possible to do (similarly to the WebSocket API):
> +  //    var s = new MozTCPSocket(host, port); 
> +  //
> +  nsITCPSocket open(in DOMString host, in unsigned short port, [optional] in jsval options);

Hmm.. so this creates and immediately opens the socket?  It's not a good design, since you may get to a situation that you loose some events before you set them on the object.

IMO the API should copy XHR:

var socket = new TcpSocket();
socket.onopen = function(event) { do something(); }
socket.open(host, port, ssl);

@@ +46,5 @@
> +  attribute nsIDOMEventListener onopen;
> +  attribute nsIDOMEventListener ondrain;
> +  attribute nsIDOMEventListener ondata;
> +  attribute nsIDOMEventListener onerror;
> +  attribute nsIDOMEventListener onclose;

You must add well written doxygen-like (see any other well documented idl file) comments to all public members.

::: b2g/components/TCPSocket.js
@@ +15,5 @@
> +
> +let debug = true;
> +function LOG(msg) {
> +  if (debug)
> +    dump(msg);

Prefix the message with "TCP Socket"

@@ +56,5 @@
> +    optlen = 0;
> +  }
> +  return Cc["@mozilla.org/network/socket-transport-service;1"]
> +           .getService(Ci.nsISocketTransportService)
> +           .createTransport(options, optlen, host, port, null);

Please open a followup to let this work with proxy (socks).

@@ +59,5 @@
> +           .getService(Ci.nsISocketTransportService)
> +           .createTransport(options, optlen, host, port, null);
> +}
> +
> +const MAX_SEND_BUFFER_SIZE_64K_CHUNKS = 128 * 16;

Where this number comes from and what exactly means?

@@ +73,5 @@
> +        "@mozilla.org/binaryinputstream;1", "nsIBinaryInputStream"),
> +      BinaryOutputStream = CC(
> +        "@mozilla.org/binaryoutputstream;1", "nsIBinaryOutputStream", "setOutputStream");
> +
> +const CONNECTING = 'connecting';

kCONNECTING, etc

@@ +90,5 @@
> +
> +  this.binaryType = "string";
> +
> +  this.host = "";
> +  this.port = -1;

Your idl defines this as unsigned int.  You may want to initialize this to 0?

@@ +134,5 @@
> +  // nsITCPSocket
> +  open: function ts_open(host, port, options) {
> +    // in the testing case, init won't be called and
> +    // hasPrivileges will be null. We want to proceed to test.
> +    if (this._hasPrivileges === false) {

Safer is !== true.

@@ +157,5 @@
> +          break;
> +        default:
> +          that.ssl = false;
> +      }
> +      that.verifyCert = options.verifyCert === true;

As said before: verifyCert should be on by default, i.e. that.verifyCert = (options.verifyCert == undefined) ? true : options.verifyCert;

However, this option is IMHO moot.  Why should we ever bypass cert verification?  Also, we actually can't..  I personally don't think the default dialog will ever be put back to the code.

@@ +178,5 @@
> +    that._rawOutputStream = transport.openOutputStream(
> +      Ci.nsITransport.OPEN_UNBUFFERED, 0, 0);
> +
> +    that._outputStreamPipe = Pipe(
> +      true, true, 65536, MAX_SEND_BUFFER_SIZE_64K_CHUNKS, null);

MAX_SEND_BUFFER_SIZE_64K_CHUNKS is on place of |count| of the buffers.  It is the maximum number of buffers the pipe can create.  So the name of the const is probably incorrect.

@@ +198,5 @@
> +    // If the other side closes the connection, we will
> +    // get an onInputStreamReady callback with zero length
> +    // available to indicate the connection was closed.
> +    that._inputStream.asyncWait(
> +      that, that._inputStream.WAIT_CLOSURE_ONLY, 0, Services.tm.currentThread);

As I understand, this is used only for detecting that TCP/SSL connection creation didn't succeed, right?  I somehow don't feel this is the right way to do it...

@@ +211,5 @@
> +    this.readyState = CLOSING;
> +
> +    this._rawOutputStream.close();
> +    this._inputStream.close();
> +    this._transport.close(Cr.NS_OK);

This way you may not send all data out!  Close the pipe input stream, the stream copier will finish push of all buffered data and then it will close the output stream of the socket and also the socket with it.  You however have to close the input stream (that is correct here).

@@ +218,5 @@
> +  startTLS: function() {
> +    this._transport.securityInfo.QueryInterface(
> +      Ci.nsISSLSocketControl
> +    ).StartTLS();
> +  },

Not really correct.

What you do with this code and API:
- open starttls socket, i.e. no TLS at the start, just TCP
- push some amount of data to the pipe's output stream expected to be sent to server over TCP only (no SSL)
- the stream copier slowly copies the data, asynchronously, w/o your control at all
- call startTLS on the socket
-> here is a race condition: nsISSLSocketControl.startTLS() may be called sooner then all data are pushed to the socket, you can send part of it unencrypted and part encrypted, w/o any control

You have to have a separate output stream pipe for unencrypted data (before startTLS has been called) and another output stream pipe for encrypted data.  In call to startTLS(), switch send() to write to the encrypted stream pipe and set some flag you were asked to do startTLS().  You should actually close the output stream of the unencrypted pipe to let the stream copier tell you it has finished async copying of all data.  You have to change the "close stream" arguments when creating the AsyncStreamCopier instance to not close the sink on finish automatically.  You will have to close the socket output stream manually from OnStopRequest called by the AsyncStreamCopier (set via call to asyncCopy() method) then.  After unencrypted data stream copying is done, based on the flag, call startTLS() on the socket and start async copy of the encrypted pipe to the socket.  


I think, since this is quit complex, you may add support for startTLS() in a followup bug.  If not, then please at least separate that support to a "part 2" patch and submit at this bug.  We don't need this support for the first implementation I believe.  But depends on compatibility if not landed on a single train.  cjones?

@@ +222,5 @@
> +  },
> +
> +  send: function ts_send(data) {
> +    if (data === undefined)
> +      return;

return what?


You want to check for the ready state as well.  Calling send() on a closed socket will probably not work.

@@ +231,5 @@
> +      this._binaryOutputStream.writeBytes(data, data.length);
> +    }
> +
> +
> +    if (this.bufferedAmount> 65535) {

Space between bufferedAmount and >

@@ +235,5 @@
> +    if (this.bufferedAmount> 65535) {
> +      if (!this._waitingOnOutput) {
> +        this._waitingOnOutput = true;
> +        this._outputStreamPipe.outputStream.asyncWait(
> +          this, 0, 0, Services.tm.currentThread);

Since you may have a tun of space in the buffer, you will fire ondrain instantly (not synchronously, though).  So I think this algorithm is a bit useless.

@@ +239,5 @@
> +          this, 0, 0, Services.tm.currentThread);
> +      }
> +      return true;      
> +    }
> +    return false;

I don't understand well how this code has to work from the consumer point of view..  Also I completely lack any documentation...  I'd assume you stop accepting new data when there is too much buffered and the |data| don't fit the buffer space left - that causes send() method to block, by the way!  Then don't write, return false and let the consumer wait for ondrain.  

This way of implementation is a bit confusing.  I can write how much I want but from some point send() starts returning true (also depends on how fast the uplink is, though).  The consumer is, however, not obligated to wait for ondrain, as I understand the code.  There can also be confusion when consumer has assigned ondrain event that writes something and also calls send() in a loop or so.  It could happen that ondrain is called in between loop calls to send() and, when wrote something, could break the data chain unexpectedly.  

It's just that I don't feel it's a good API design.  But maybe you have a different opinion and plan, I just don't get it when looking at the code.



The very tricky thing here is to recognize when to call ondrain...  async output stream's AsyncWait method has an argument |requestedCount| that prevents call of OnOutputStreamReady before the requestedCount of bytes is actually available in the output buffer, but that is not implemented.  We could finish that implementation, though, but I would like to avoid that (read further please).



My suggestion would be to stop accepting data when data doesn't fit and close the output stream of the pipe.  Then wait for the copier to finish (i.e. send out all buffered data).  Then create a new pipe and copier, and call ondrain.  That is simple, but is suboptimal - data are not well "pipelined" in the buffer.

You could then have two pipes: when one fills up, do what the above paragraph says but let send() method write to an other, w/o async copying for the moment.  When async copy of the first pipe is done, throw it away and start async copy of the second.  When both pipes gets filled up (network congestion), stop accepting any data and make the consumer wait for ondrain after the first pipe is copied out.

In general, you can have even more smaller pipes like this "chained" one by one (up to e.g. 10).  What I want to achieve with this suggestion is to allow some granularity of notifications to allow ondrain calls sooner then the whole amount of buffered data is sent out.

@@ +263,5 @@
> +      this.dispatchEvent("onopen");
> +
> +      new InputStreamPump(
> +        this._inputStream, -1, -1, 0, 0, false
> +      ).asyncRead(this, null);

I think the InputStreamPump could be potentially eaten by GC.  You should for stability reasons hold ref in the class to it.

@@ +276,5 @@
> +      // Connection refused
> +    }
> +    if (!len) {
> +      LOG("Connection refused\n");
> +      this.dispatchEvent("onerror", "Connection refused");

len == 0 means the TCP connection has been gracefully closed, so that is not an error!

Throw this code away, use just the InputStreamPump, you already have a code to handle connection refusal in OnStopRequest.

@@ +337,5 @@
> +SecurityCallbacks.prototype = {
> +  notifySSLError: function sc_notifySSLError(socketInfo, error, targetSite) {
> +    this._socket.dispatchEvent("onerror", "SSL Error: " + error);
> +    // This isn't really the right thing because I think necko still
> +    // closes the connection, this just supresses the error

And there is no (default) error.  So this has no effect.

::: b2g/test/unit/test_tcpsocket.js
@@ +1,3 @@
> +
> +
> +/**

Remove the leading new lines.

Comment 62

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #61)
> I think, since this is quit complex, you may add support for startTLS() in a
> followup bug.  If not, then please at least separate that support to a "part
> 2" patch and submit at this bug.  We don't need this support for the first
> implementation I believe.  But depends on compatibility if not landed on a
> single train.  cjones?

The e-mail client needs startTLS support.  However, we are capable of avoiding the glitch you are talking about if we wait for an additional network roundtrip.  From RFC 3501, the chat looks like this:

  C: a002 STARTTLS
  S: a002 OK Begin TLS negotiation now
  <TLS negotiation, further commands are under [TLS] layer>

So we can wait on the OK to come back and then initiate TLS.  However, since we want to reduce startup latency as much as possible, it would be preferable to be able to not have to wait for the server to respond.  In the event the server does not want to TLS negotiate, it's a fatal error for security reasons, so there's really no need to check that the server is fine with the upgrade.

Comment 63

[Donovan Preston [:fzzzy]]

Thanks a lot, Honza!

Comment 64

[Jason Duell]

> so this creates and immediately opens the socket?  It's not a good design,
> since you may get to a situation that you loose some events before you set 
>them on the object.

FWIW Web sockets has the exact same sort of API: you create the websocket, and it starts to connect, before you assign event handlers:

  var socket = new WebSocket('ws://game.example.com:12010/updates');
  socket.onopen = function () { ... }

I've always assumed there was some mechanism in our event handling that guarantees there's no race there (main thread won't process event from the network before it exits the current JS control path?)

That said, I don't actually have a strong opinion on whether the WS or XHR model is better here.  But I think either works.

Comment 65

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Javascript is single-threaded (aka run-to-completion). If you have two instructions like:

a = 1;
a++;

There is no way that an event will fire between these two statements.

Many people have had the same reaction that there's somehow a risk that an event handler would run between:

x = new Foo;
x.onsomething = myhandler;

Yet no-one would ever expect myhandler to run in the middle of your javascript program.

It's completely safe to create an API like the above. Apart from WebSocket, WebWorkers and IndexedDB makes frequent use of this pattern.

The XHR model where you first create the object and then start the network connection I think is partially a function of the fact that XHR allows you to set a bunch of meta-data (setRequestHeader) and the fact that it was originally a ActiveX component that Microsoft exposed as-is to the web. It was never designed for javascript.

Comment 66

[Sonny Piers [:sonny]]

http://blog.alexmaccaw.com/chrome-tcp-udp

Comment 67

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

(In reply to Sonny Piers [:sonny] from comment #66)
> http://blog.alexmaccaw.com/chrome-tcp-udp
That is about extensions. TCP/UDP have been available to FF addons forever.
Neither Chrome nor Gecko API for extensions is really good for the web apps, IMO.

Comment 68

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

Comment on attachment 618443 [details] [diff] [review]
wip-0.6

I assume most of this will be re-written in C++, right?
That way you get the right event handling.

interface nsITCPSocket should extend nsIDOMEventTarget

Comment 69

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Olli Pettay [:smaug] from comment #68)
> Comment on attachment 618443 [details] [diff] [review]
> wip-0.6
> 
> I assume most of this will be re-written in C++, right?
> That way you get the right event handling.
> 
> interface nsITCPSocket should extend nsIDOMEventTarget

If bug 731746 is resolved, is there any other reasons to rewrite it in C++?

Comment 70

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

Well, bug 731746 would change only one part of event handling. And I'm not aware of anyone
fixing that bug.

Events itself should still be C++ (unless someone comes up with a solution where
they can be implemented in JS in such way that event handling doesn't slow down.)

Comment 71

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #65)
> Javascript is single-threaded (aka run-to-completion). If you have two
> instructions like:

OK, then let's leave it as it is done in the patch.  I just hope no code spins the main thread event loop between transport.openInputStream() and assignment of the event handler.

Comment 72

[Jason Duell]

Chrome is now exposing a socket API to extensions:

  http://blog.alexmaccaw.com/chrome-tcp-udp
  http://code.google.com/chrome/extensions/experimental.socket.html

(Oddly, the google.com doc says that only 'udp' can be passed to the socket create function: not clear if that's a typo, or they don't support TCP yet).

Could be worth synchronizing our APIs, or at least looking to see if there's lessons to be learned from their choices.   

The way they're handling write semantics looks interesting (you pass a callback to write that gets called when "the write operation completes, the write operation blocked before completion, or an error occurred.")

Comment 73

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, we should definitely reach out to them. Once the System Level API WG has been created we should be able to discuss on that mailing list. Until then we can just use plain ol' emails.

Comment 74

[Donovan Preston [:fzzzy]]

(In reply to Honza Bambas (:mayhemer) from comment #61)
> Comment on attachment 618443 [details] [diff] [review]
> wip-0.6
> 
> Review of attachment 618443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r-
> 
> I didn't check the test.
> 
> ::: b2g/components/TCPSocket.idl
> @@ +22,5 @@
> > +  //  arguments when instantiating a TCPSocket object. For example it will
> > +  //  be possible to do (similarly to the WebSocket API):
> > +  //    var s = new MozTCPSocket(host, port); 
> > +  //
> > +  nsITCPSocket open(in DOMString host, in unsigned short port, [optional] in jsval options);
> 
> Hmm.. so this creates and immediately opens the socket?  It's not a good
> design, since you may get to a situation that you loose some events before
> you set them on the object.

This was already mentioned by other commenters. Since the turn doesn't end
between calling open and setting the event handlers, the event handlers can't
be called and the events won't be lost.

However, I would prefer to have the new TCPSocket(); TCPSocket.open(...)
variant, but because of the way init(window) is called on 
JavaScript-global-property and not JavaScript-global-constructor, I can't
figure out how to implement security checking in the
JavaScript-global-constructor variant (aka "var s = new TCPSocket();", which is 
present in earlier versions of the patch). Any ideas here would be welcome,
otherwise it sounds like this is ok to leave as it is.

> You must add well written doxygen-like (see any other well documented idl
> file) comments to all public members.

> Prefix the message with "TCP Socket"

> Please open a followup to let this work with proxy (socks).

Ok, thanks. Will do.

> @@ +59,5 @@
> > +           .getService(Ci.nsISocketTransportService)
> > +           .createTransport(options, optlen, host, port, null);
> > +}
> > +
> > +const MAX_SEND_BUFFER_SIZE_64K_CHUNKS = 128 * 16;
> 
> Where this number comes from and what exactly means?

It's badly named for sure. I copied it from some other code, I think
it means the max number of 64k chunks to use for the output buffering
is 128 * 16. What should this number be, and what's a good name for
the constant?

> kCONNECTING, etc

> Your idl defines this as unsigned int.  You may want to initialize this to 0?

> Safer is !== true.

Ok, thanks.

> As said before: verifyCert should be on by default, i.e. that.verifyCert =
> (options.verifyCert == undefined) ? true : options.verifyCert;
> 
> However, this option is IMHO moot.  Why should we ever bypass cert
> verification?  Also, we actually can't..  I personally don't think the
> default dialog will ever be put back to the code.

Oh man. I had totally changed this at one point, but my change got reverted.
Working with mercurial patch queues is a little awkward, sorry.

However, I think we should just remove the option to turn off cert validation
if we can't do it... At least for this first version of a patch. IMAP does
have a strong case for allowing connecting to ports with self-signed certs,
since many companies run ssl imap without bothering to buy a real cert, but
it's not essential for the first version. Another bug can be opened to add
the option, and the user interface and api design issues can be worked out
then.

> @@ +198,5 @@
> > +    // If the other side closes the connection, we will
> > +    // get an onInputStreamReady callback with zero length
> > +    // available to indicate the connection was closed.
> > +    that._inputStream.asyncWait(
> > +      that, that._inputStream.WAIT_CLOSURE_ONLY, 0, Services.tm.currentThread);
> 
> As I understand, this is used only for detecting that TCP/SSL connection
> creation didn't succeed, right?  I somehow don't feel this is the right way
> to do it...

I'd love to hear better ideas. I tried a million different ways to get the correct onopen timing (aka not immediately) along with firing onerror on connection refused.

It would be really, really nice if onTransportStatus would fire in the
connection refused case, but it doesn't. It seems like an obvious thing to add. 
If this was added, I wouldn't need this hack.

> @@ +211,5 @@
> > +    this.readyState = CLOSING;
> > +
> > +    this._rawOutputStream.close();
> > +    this._inputStream.close();
> > +    this._transport.close(Cr.NS_OK);
> 
> This way you may not send all data out!  Close the pipe input stream, the
> stream copier will finish push of all buffered data and then it will close
> the output stream of the socket and also the socket with it.  You however
> have to close the input stream (that is correct here).

Thanks, I wasn't sure exactly what to do here. I should have asked.

> @@ +218,5 @@
> > +  startTLS: function() {
> > +    this._transport.securityInfo.QueryInterface(
> > +      Ci.nsISSLSocketControl
> > +    ).StartTLS();
> > +  },
> 
> Not really correct.

[snip] ...

> I think, since this is quit complex, you may add support for startTLS() in a
> followup bug.  If not, then please at least separate that support to a "part
> 2" patch and submit at this bug.  We don't need this support for the first
> implementation I believe.  But depends on compatibility if not landed on a
> single train.  cjones?

I agree, TLS support should be landed in a different patch. I talked to Andrew
about it and he said most people support plain SSL IMAP in addition to TLS,
so leaving TLS out of the first iteration should not be a problem.

> @@ +222,5 @@
> > +  },
> > +
> > +  send: function ts_send(data) {
> > +    if (data === undefined)
> > +      return;
> 
> return what?

Maybe this should be an exception instead? It seems more like an error to
call send without passing anything.

> You want to check for the ready state as well.  Calling send() on a closed
> socket will probably not work.

> Space between bufferedAmount and >

Ok. Thanks.

> @@ +235,5 @@
> > +    if (this.bufferedAmount> 65535) {
> > +      if (!this._waitingOnOutput) {
> > +        this._waitingOnOutput = true;
> > +        this._outputStreamPipe.outputStream.asyncWait(
> > +          this, 0, 0, Services.tm.currentThread);
> 
> Since you may have a tun of space in the buffer, you will fire ondrain
> instantly (not synchronously, though).  So I think this algorithm is a bit
> useless.

I see what you mean. I was working under the assumption that the async event
would fire when the buffers are empty, but if it's a "ready to write" event
and it fires immediately because of the big buffer...

I thought I found a solution just now, that the AsyncStreamCopier
callbacks were the right place to fire ondrain, but it appears these
are only called when the input is closed and has finished being copied.
I need a callback that fires when the buffers have been emptied, every
time the buffers have been emptied.

> @@ +239,5 @@
> > +          this, 0, 0, Services.tm.currentThread);
> > +      }
> > +      return true;      
> > +    }
> > +    return false;
> 
> I don't understand well how this code has to work from the consumer point of
> view..  Also I completely lack any documentation...  I'd assume you stop
> accepting new data when there is too much buffered and the |data| don't fit
> the buffer space left - that causes send() method to block, by the way! 
> Then don't write, return false and let the consumer wait for ondrain.  
> [...]

I thought the idea that was being proposed in much earlier comments on this 
post, before I started working on it, was we would buffer infinitely, thus 
never blocking. I guess that's not really true in the current patch because it 
looks like the max buffer size is 192 meg. ((128 * 16 buffers) * 64 k chunks)

I'd appreciate some help getting the buffering set up properly. If we do want 
to buffer until out of memory and then close the socket and raise an error, as
suggested in an earlier comment on this bug, I'd love some tips on how to
detect the out of memory condition to close the socket.

For more discussion of the API and how it's supposed to work, see the very
bottom of this post. I'll add docstrings for the next version of the patch.

> @@ +263,5 @@
> > +      this.dispatchEvent("onopen");
> > +
> > +      new InputStreamPump(
> > +        this._inputStream, -1, -1, 0, 0, false
> > +      ).asyncRead(this, null);
> 
> I think the InputStreamPump could be potentially eaten by GC.  You should
> for stability reasons hold ref in the class to it.

Thanks, that's a good catch...

> @@ +276,5 @@
> > +      // Connection refused
> > +    }
> > +    if (!len) {
> > +      LOG("Connection refused\n");
> > +      this.dispatchEvent("onerror", "Connection refused");
> 
> len == 0 means the TCP connection has been gracefully closed, so that is not
> an error!
> 
> Throw this code away, use just the InputStreamPump, you already have a code
> to handle connection refusal in onStopRequest.

No, onStopRequest is not called in the Connection Refused case. None of the
InputStreamPump lifetime messages are suitable for getting Connection Refused.
There are onTransportStatus callbacks for all portions of the successful
connection lifecycle, but not Connection Refused.

I'll just get rid of the if (!len) and only fire onerror in the catch block.
The onclose event is fired properly from the onStopRequest callback in the
normal case.

Please, if you can write a code example that uses InputStreamPump and the events
it generates to detect both connection success and connection refused events, I
would love to simplify this code. This is the only way I have found that 
actually works.

> @@ +337,5 @@
> > +SecurityCallbacks.prototype = {
> > +  notifySSLError: function sc_notifySSLError(socketInfo, error, targetSite) {
> > +    this._socket.dispatchEvent("onerror", "SSL Error: " + error);
> > +    // This isn't really the right thing because I think necko still
> > +    // closes the connection, this just supresses the error
> 
> And there is no (default) error.  So this has no effect.

Does this mean the entire SecurityCallbacks implementation is useless, and can
be removed? That would be nice.

At least for the first version of this.

> ::: b2g/test/unit/test_tcpsocket.js
> @@ +1,3 @@
> > +
> > +
> > +/**
> 
> Remove the leading new lines.

No problem.

Thanks for the review!

Now, a discussion about API. Yes, Chrome has just exposed TCP sockets 
to extensions using an interesting API. I don't see anything
wrong with the API proposed by Chrome, and it is nice in certain ways,
but the API embodied by the TCPSocket patch is based on Twisted's socket
implementation and also the Node.js socket implementation. Since it is
quite close to what Node.js uses, it's quite easy to implement Node.js'
socket interfaces on top of it; in fact, that's what Andrew has done
for the Boot to Gecko Mail application. He's using a Node.js IMAP
impelementation ported to TCPSocket with a number of shims and hacks.

var s = TCPSocket.open(...);

A newly created TCPSocket will begin connecting immediately, but cannot
be used to send or receive data until either onopen (success) or onerror
(connection refused).

s.onopen = function(evt) {
  ...
}

After onopen has been called, ondata will be called asynchronously any
time data is available. Pretty simple.

s.ondata = function(evt) {
  console.log(evt.data);
}

Any time after onopen has been called, s.send may be called to send or buffer
data which will be sent in the background to the socket. In the Node.js
api, if send returns true, it means not all of the given data fit in the
OS buffers and Node.js did some in-memory buffering. Once send has
returned true, the 'drain' event is fired once the in-memory buffer has
been fully written to the OS.

function throttle_send(...) {
  if (s.send(...)) {
    s.ondrain = function() {
      s.ondrain = null;
      // update timing calculations to determine actual
      // bitrate and send the next throttled chunk
      throttle_send(...);
    }
  }
}

The TCPSocket patch also has a bufferedAmount variable that can be used
to synchronously check the amount currently buffered, but only the ondrain
approach can be used to stream at the rate of the network connection
without buffering large amounts or polling the bufferedAmount variable.

The Chrome API takes unique callbacks for every single async operation
instead of using on* message handlers. This seems ok, but I believe the
Node.js approach will end up being simpler to use overall. I'd be
interested in a discussion of why the Chrome API was designed to be
the way it is and what the advantages are over the node approach.

Comment 75

[Donovan Preston [:fzzzy]]

(In reply to Honza Bambas (:mayhemer) from comment #61)

> My suggestion would be to stop accepting data when data doesn't fit and
> close the output stream of the pipe.  Then wait for the copier to finish
> (i.e. send out all buffered data).  Then create a new pipe and copier, and
> call ondrain.  That is simple, but is suboptimal - data are not well
> "pipelined" in the buffer.

I have been thinking about this idea a lot, and I tried to implement it, but I got stuck with one thing: If I use this approach, how can I tell the difference between one of the pipes being closed to trigger an ondrain event, and a real, actual, close event from the socket?

Anyway, I think the issue is a red herring, because I think the way the code is now actually does do the right thing. The trick is I am performing the async wait on the non-buffered socket, not the buffered pipe. As the async copier is filling the non-buffered socket, a ready-to-write event will not be fired because the non-buffered socket has been filled by the copier. It is only when the async copier has finished draining the pipe that the output buffer appears to be ready-to-write, the asyncWait callback fires, and the ondrain handler is fired.

I checked for this back when I wrote the patch by sending multi megabyte web requests to a simple web service I wrote. In all cases, when ondrain was fired, bufferedAmount was 0. I can add a test for this.

There may, however, be a problem with raciness depending on how things are actually implemented. If it is possible for the js context to get the ready-to-write notification instead of the async copier, while the pipe is still full, then that would be the race. This would only have the effect of firing ondrain too soon, and if the user then calls send in the ondrain callback, they will simply buffer a little too much data, and wait for the next ondrain again.

The simple workaround for this is to check bufferedAmount before calling ondrain in the asyncWait callback, and if it is > 0, simply call asyncWait again instead of calling ondrain.

Comment 76

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

SecReview Scheduled:
https://mail.mozilla.com/home/[e-mail]/Security%20Review.html?view=month&action=view&invId=110473-110472&pstat=AC&exInvId=110473-179809&useInstance=1&instStartTime=1337803200000&instDuration=3600000

Comment 77

[Donovan Preston [:fzzzy]]

Attachment: wip-0.7

Here is the next version of the patch. The changes are:

* Interface is named nsIDOMTCPSocket
* dispatchEvent renamed callListener
* TCPSocketEvent parameter order is now (aType, aSocket,
  aData)
* Unused secMan variable removed
* Use "let" instead of "var"
* The Makefile.in for the test uses the new MPL2 header
* Extensive documentation comments in the idl file
* Log messages prefixed with TCPSocket
* Confusing MAX_SEND_BUFFER_SIZE_64K_CHUNKS const removed
* Constants are prefixed with k (kCONNECTING, etc)
* this.port is initialized to 0 instead of -1
* The test of this._hasPrivileges is more explicit
* The option to not verify an SSL cert has been removed
  (to be implemented in a follow-up patch, if possible
  and desired)
* The Pipe is created with 65536, 65536 to make it only 
  limited by available memory
* close will now correctly wait until all buffered data has
  been written before closing the socket.
  - I found an interesting bug or feature of Pipe; it will
    only close the underlying socket if the input side of
    the pipe is closed after actually writing some data
    into the Pipe. It will not properly close the socket
    if no data was written.
* startTLS has been removed from this version of the patch,
  to be implemented in a later patch.
* send now raises an exception instead of returning
  undefined if no data was passed or an argument of the
  wrong type was passed.
* Added whitespace in the self.bufferedAmount expression
* The InputStreamPump is now saved in an instance var
* The code for checking connection refused was fixed to
  only handle the connection refused case, and normal
  connection lost is handled by onStopRequest as usual.
* I had accidentally inverted the return value of send
  compared to the convention used by node.js; send now
  returns true as a hint that it's safe to call send again
  immediately without buffering "too much" data, and
  returns false as a hint that the caller may wait until
  the ondrain event handler is called before calling send
  again to prevent excessive buffering, but does not have 
  to.

There are a few things I was not able to address from 
Honza's previous comments.

* I still use onInputStreamReady and check to see if
  available() raises an exception to know when to call the
  onerror handler with a Connection Refused.
  onTransportStatus does not fire an event in this case,
  and onStopRequest is not called, because onStartRequest
  was never called, because the connection was not made.
* I still use _outputStreamPipe.outputStream.asyncWait to
  know when to call ondrain. inputStream and outputStream
  here are names from the perspective of the Pipe, and
  stuff is written into the inputStream and copied to the
  outputStream, which is the underlying unbuffered socket.
  I empirically verified that asyncWait on the 
  outputStream of the pipe does not fire the 
  onOutputStreamReady handler until the Pipe is empty.
  I expanded the tests to buffer much more data and check
  that bufferedAmount is zero in ondrain.

I have no idea what is required for the C++ parts to make
this work with Electrolysis. I could probably learn, but
if someone who knows what they are doing is available to
work on that part of the patch, that would probably make
a lot more sense.

Comment 78

[Andrew Sutherland [:asuth] (he/him)]

Attachment: wip-0.7 with minor glitch fixes

(For the demo tomorrow I want to bounce exactly what I have to my laptop, and make sure everyone is on the same page.  Per debugging with Donovan, 0.7 accidentally moved the event sink assignment which lost some events.  The SSL listener also got removed with the TLS removal, but was still required for SSL.)

Comment 79

[Dietrich Ayala (:dietrich)]

(In reply to Donovan Preston from comment #77)
> I have no idea what is required for the C++ parts to make
> this work with Electrolysis. I could probably learn, but
> if someone who knows what they are doing is available to
> work on that part of the patch, that would probably make
> a lot more sense.

Jonas, Olli, Cjones or anyone else: Who knows the DOM event C++ and e10s well enough to hop in and make this work out-of-process?

This is a platform blocker for landing the B2G email app, so need to some traction on OOPing it asap.

Comment 80

[Andreas Gal :gal]

jlebar, fabrice, maybe gregor

Comment 81

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Let's file a followup for the OOP work.

If this is implemented on top of a necko object that's already remoted, then we get this "for free".  Otherwise we can probably crib off PNecko.

Comment 82

[Donovan Preston [:fzzzy]]

Chris, that's great news that OOP can be done in a followup.

Andrew and I are working through some issues with error handling and SSL sockets, and then there will be another patch for review shortly.

Comment 83

[Donovan Preston [:fzzzy]]

Attachment: wip-0.8

Next patch. Changes:

- SSL Sockets require the SecurityCallbacks, so put them back
- Fix race introduced in 0.7 by calling setEventSink immediately after createTransport
- Correctly call onerror and onclose if the SSL certificate is not valid
- Fix the huge request test to wait for all the data before considering the test passing
- Fix an error with the way binaryType was set in the constructor

Comment 84

[Honza Bambas (:mayhemer)]

I have a different idea on how to implement the synchronous send() method.

Don't write the data you are passing as an arg immediately to the pipe.  Actually, don't have a pipe at all.  Instead, create an input stream (string, or binary - must be buffered! means: it must implement ReadSegments) and initialize it with the data passed as the arg to send.  Then, async copy this stream to the unbuffered socket output stream.  When done, it is your notification of "all has been copied".  (Setup the copier to not close the sink.)

Even better: you can use nsIMultiplexInputStream wrapped over with nsIBufferedInputStream.  Just add your input streams created in send() to nsIMultiplexInputStream and when you are not already async-copying to the socket, start it.  When you get notification copying is done, check whether there is more in nsIMultiplexInputStream (call its available()) and start copying again.  This way you avoid multi-thread race conditions when a notification of finished copying comes after you have added a new stream.  That notification is also your source for notifications of ondrain, even it's not perfect.  We should call ondrain any time we have a reasonable amount of space in our output "buffer" space.  This suggestion may lead from fluent to choppy sending for large amounts, but depends.

You can let send() return false, when nsIMultiplexInputStream.available() returns something over some threshold.


Please consider this suggestion or some of its variant.  Variant could be to not use multiplex stream but have your own queue of streams to copy.  That way you have a better granularity of notifications but you loose a nice feature of coalescing small pieces of data to larger chunks that positively affects network performance.


(please don't obsolete the patch wip-0.8, I started the review and have some splinter comments)

Comment 85

[Honza Bambas (:mayhemer)]

Comment on attachment 631683 [details] [diff] [review]
wip-0.8

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

The patch is become cleaner, that is a good sign!

This is a pre-review this time.

::: b2g/components/TCPSocket.idl
@@ +25,5 @@
> +//  be possible to do (similarly to the WebSocket API):
> +//    var s = new MozTCPSocket(host, port); 
> +
> +[scriptable, uuid(b82e17da-6476-11e1-8813-57a2ffe9e42c)]
> +interface nsIDOMTCPSocket : nsISupports

Rename also the file to nsIDOMTCPSocket

@@ +65,5 @@
> +  /**
> +   * The number of bytes which have previously been buffered by calls to
> +   * send on this socket.
> +   */
> +  readonly attribute unsigned long bufferedAmount;

Hmm... wouldn't be more useful to let the consumer know how much space we actually have in the buffer to let him decide on how much data he may put to send() ?

::: b2g/components/TCPSocket.js
@@ +163,5 @@
> +    transport.setEventSink(that, Services.tm.currentThread);
> +    transport.securityCallbacks = new SecurityCallbacks(that);
> +
> +    that._inputStream = transport.openInputStream(0, 0, 0);
> +    that._unbufferedOutputStream = transport.openOutputStream(

Please rename (there is too much streams in this file):
_inputStream -> _socketInputStream
_unbufferedOutputStream -> _unbufferedSocketOutputStream

@@ +221,5 @@
> +
> +  send: function ts_send(data) {
> +    if (this.readyState !== kOPEN) {
> +      throw new Error("Socket not open.");
> +    } else if (data === undefined) {

Do you really need an else here?

@@ +229,5 @@
> +    if (this.binaryType === "arraybuffer") {
> +      this._binaryOutputStream.writeByteArray(data, data.length);
> +    } else {
> +      this._binaryOutputStream.writeBytes(data, data.length);
> +    }

Since this is non-blocking, you may not send all data or throw NS_ERROR_WOULD_BLOCK when getting to the edge of the buffer, however it is 4GB on size.  Sending 4GB+ large files will be unstable with this API.

@@ +246,5 @@
> +        // and when it is done being copied, we will get an
> +        // onOutputStreamReady event in this thread.
> +        this._waitingOnOutput = true;
> +        this._outputStreamPipe.outputStream.asyncWait(
> +          this, 0, 0, Services.tm.currentThread);

The comment absolutely doesn't reflect reality.  I don't want to repeat my self here, but if you want to know when all buffered data has been sent, wait for the async copier and not for the pipe.  Pipe has to be closed here to make that effect and on next call to send() you have to create a new one and when copying is done, start copy the new pipe.  You need a queue of pipes with your API design since you can get to the buffer exhaustion situation recursively.

Simply: ondrain implemented this way doesn't ensure consistent send of all data out.

@@ +256,5 @@
> +
> +  suspend: function ts_suspend() {
> +    if (this._request) {
> +      this._request.suspend();
> +    }

When there is no request, I'd rather tend to have an internal counter and then carry the number of suspensions to the input pipe when you create it.

@@ +262,5 @@
> +
> +  resume: function ts_resume() {
> +    if (this._request) {
> +      this._request.resume();
> +    }

Same here.

@@ +340,5 @@
> +  this._socket = socket;
> +}
> +SecurityCallbacks.prototype = {
> +  notifySSLError: function sc_notifySSLError(socketInfo, error, targetSite) {
> +    return true;

Please check that if you don't notify onerror from here then the error gets propagated to OnStopRequest of the input pipe.  SSL errors like no cypher overlap etc should be reported as well.

Comment 86

[Donovan Preston [:fzzzy]]

(In reply to Honza Bambas (:mayhemer) from comment #84)
> Even better: you can use nsIMultiplexInputStream wrapped over with
> nsIBufferedInputStream.  Just add your input streams created in send() to
> nsIMultiplexInputStream and when you are not already async-copying to the
> socket, start it.  When you get notification copying is done, check whether
> there is more in nsIMultiplexInputStream (call its available()) and start
> copying again.  This way you avoid multi-thread race conditions when a
> notification of finished copying comes after you have added a new stream. 
> That notification is also your source for notifications of ondrain, even
> it's not perfect.  We should call ondrain any time we have a reasonable
> amount of space in our output "buffer" space.  This suggestion may lead from
> fluent to choppy sending for large amounts, but depends.
> 
> You can let send() return false, when nsIMultiplexInputStream.available()
> returns something over some threshold.

Ok, I will try this approach. Thanks.

Comment 87

[Honza Bambas (:mayhemer)]

Comment on attachment 631683 [details] [diff] [review]
wip-0.8

I'm releasing the r? flag for now just to clear my review queue.  I believe you will have new version of the patch soon anyway.

Comment 88

[Donovan Preston [:fzzzy]]

Attachment: wip-0.9

- idl file renamed nsIDOMTCPSocket.idl
- Buffering is now accomplished with a nsIMultiplexInputStream. 
  ondrain events are now correct, and there is a test to make sure
  two ondrain events can occur on one socket
  - Because of this, the buffer size is limited only by free
    memory
  - How can I catch an out of memory condition in send, and close
    the socket and free all the buffers in this case? This is what
    was proposed in the comments to this bug earlier
- Renamed all the _socketInputStream _socketOutputStream etc
  variables to make reading the code less confusing
- Removed extraneous if data === undefined clause from send
- Added internal counter for calls to suspend/resume made before
  onStartRequest is called, and carried the calls over to the
  request

In regard to notifySSLError, I tested by hand that connecting to
a server with a self-signed certificate correctly calls onerror
followed by onclose. If there are other types of SSL errors I
should be checking for, please tell me how to reproduce those
errors so I can make sure the code does the right thing.

One other thing, I had to convert from arraybuffer to string in
order to be able to use StringInputStream. Is there a better way
to go from an arraybuffer to a stream?

Comment 89

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Donovan Preston from comment #88)
> One other thing, I had to convert from arraybuffer to string in
> order to be able to use StringInputStream. Is there a better way
> to go from an arraybuffer to a stream?

Elaborating in case this is not currently well known, XPConnect has a fast-path for ArrayBuffers where signatures of the type:
  writeByteArray([array, size_is(aLength)] in PRUint8 aBytes, in PRUint32 aLength);
will accept a Uint8Array for aBytes.  There is currently no support for automatic coercion from a JS String to a typed array.

nsIScriptableUnicodeConverter (https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIScriptableUnicodeConverter) can be used to probably more efficiently perform the conversion, but it still introduces an intermediary JS string wrapped by XPConnect.  nsIBinaryOuputStream has such a signature, but it is of the wrong gender...

Comment 90

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andrew Sutherland (:asuth) from comment #89)
> There is currently no support for
> automatic coercion from a JS String to a typed array.

er, to re-state, there is no automatic coercion of typed arrays to XPCOM string types.  (What I said is also true, but not what I meant.)

Comment 91

[Donovan Preston [:fzzzy]]

Attachment: wip-0.9.5

Small update to the last patch.

- Removes obsolete logic from close that was only relevant to the non
  multiplexstream implementation
- Add a bit more detail to comments

Comment 92

[Honza Bambas (:mayhemer)]

Comment on attachment 635473 [details] [diff] [review]
wip-0.9.5

Thanks, this looks better and better!  I'll do a full review on Monday, feel free to update the patch in the meantime, I have no draft comments in splinter.

Comment 93

[Andrew Sutherland [:asuth] (he/him)]

honza, review ping.  It would be great if there's a chance of landing this this week for the B2G work week's demo on Friday.  (Thanks!)

Comment 94

[Andrew Overholt [:overholt]]

Nomming for basecamp.

Comment 95

[Donovan Preston [:fzzzy]]

Review ping #2. I'm going on vacation next week, so it would be nice to wrap this up tighter if possible.

Comment 96

[Honza Bambas (:mayhemer)]

I've started the review.

Comment 97

[Honza Bambas (:mayhemer)]

Attachment: test failure (wip-0.9.5)

After some fight I was able to build B2G on windows (7) as a desktop client.  I did this just to be able to run the test before I gave the review feedback.

The test fails with the attached log, consistently.

Comment 98

[Honza Bambas (:mayhemer)]

Comment on attachment 635473 [details] [diff] [review]
wip-0.9.5

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

According the test, I'm missing a major one: call send in loops with some reasonable amount of data to send, w/o waiting for ondrain (but check ondrain gets called, at least ones)

Your test helper functions for events are a bit confusing.  Some comments what they do would be useful.

Leaving r? according the test failure.

::: b2g/components/B2GComponents.manifest
@@ +32,5 @@
>  
> +# TCPSocket.js
> +component {cda91b22-6472-11e1-aa11-834fec09cd0a} TCPSocket.js
> +contract @mozilla.org/tcp-socket;1 {cda91b22-6472-11e1-aa11-834fec09cd0a}
> +category JavaScript-global-property MozTCPSocket @mozilla.org/tcp-socket;1

I cannot review these pieces.  You need to find reviewer for this code to get this in.

::: b2g/components/TCPSocket.js
@@ +38,5 @@
> +    contractID: "@mozilla.org/tcp-socket-event;1",
> +    classDescription: "TCP Socket Event",
> +    interfaces: [Ci.nsITCPSocketEvent],
> +    flags: Ci.nsIClassInfo.DOM_OBJECT
> +  })

Where is this component registered?  Is there actually a need to have this as a constructable component?

@@ +94,5 @@
> +  this.ssl = false;
> +};
> +
> +
> +function start_copying(that) {

startCopying and move this closer (bellow) createTransport since this is also a helper.

On the other hand, why isn't this a member method?  You have some helper methods in the class anyway.  Just chose one and be consistent.

@@ +133,5 @@
> +  _binaryType: "string",
> +
> +  // Raw socket streams
> +  _transport: null,
> +  _request: null,

_request is result of input stream pipe and is the same as _inputStreamPump.  You don't need this member at all.

@@ +146,5 @@
> +  // Output stream machinery
> +  _outputMultiplexStream: null,
> +  _outputStreamCopier: null,
> +
> +  _waitingOnOutput: false,

According the usage of this flag, please call this _asyncCopierActive or so.

@@ +148,5 @@
> +  _outputStreamCopier: null,
> +
> +  _waitingOnOutput: false,
> +  _waitingForDrain: false,
> +  _earlySuspensions: 0,

Maybe call this _suspendCount, but up to you.

@@ +194,5 @@
> +          that.ssl = false;
> +      }
> +      that.binaryType = options.binaryType || that.binaryType;
> +    } else {
> +      that.ssl = false;

I think this already has a default value.  Would be good to set this to its default only on one place.

@@ +222,5 @@
> +    }
> +
> +    that._outputMultiplexStream = new MultiplexInputStream();
> +
> +    that._outputStreamCopier = AsyncStreamCopier(

Why are you somewhere using |new| and somewhere not?

@@ +229,5 @@
> +      // (nsSocketTransport uses gSocketTransportService)
> +      Cc["@mozilla.org/network/socket-transport-service;1"]
> +        .getService(Ci.nsIEventTarget),
> +      /* source buffered */ true, /* sink buffered */ false,
> +      65536, /* close source*/ false, /* close sink */ false);

Make 65536 a const please.

@@ +265,5 @@
> +    if (!this._waitingOnOutput) {
> +      // If this is the first time send has caused buffering,
> +      // we need to start the async copy.
> +      this._waitingOnOutput = true;
> +      start_copying(this);

I little bit more think these state checks (_waitingOnOutput) should be in the start_copying helper.  And the helper name should be something like "ensure copying" or so.

@@ +268,5 @@
> +      this._waitingOnOutput = true;
> +      start_copying(this);
> +    }
> +
> +    if (this.bufferedAmount > 65535) {

Should be const as well.

@@ +294,5 @@
> +    } else {
> +      this._earlySuspensions--;
> +    }
> +  },
> +  // nsITransportEventSink (Triggered by transport.setEventSink)

Add a blank line before the method comments please to make the code more readable.

@@ +304,5 @@
> +      this.callListener("onopen");
> +
> +      this._inputStreamPump = new InputStreamPump(
> +        this._socketInputStream, -1, -1, 0, 0, false
> +      ).asyncRead(this, null);

You may do the early suspensions right here.  It may give a slightly better performance.

_inputStreamPump is the request you get in OnStartRequest.

@@ +351,5 @@
> +
> +  classInfo: XPCOMUtils.generateCI({
> +    classID: Components.ID("{cda91b22-6472-11e1-aa11-834fec09cd0a}"),
> +    contractID: "@mozilla.org/tcp-socket;1",
> +    classDescription: "TCP Socket Helper",

I don't think this is a "helper" ;)

@@ +376,5 @@
> +    if (status.isDomainMismatch) {
> +      msg = msg + "Domain Mismatch";
> +    } else if (status.isNotValidAtThisTime) {
> +      msg = msg + "Not valid at this time";
> +    } else {

There are more states on the status: http://hg.mozilla.org/mozilla-central/annotate/91d579fd6a5f/security/manager/ssl/public/nsISSLStatus.idl#l19

All should be reflected here and also there may be more then just one set.  Concatenate them in the error message.

@@ +386,5 @@
> +  },
> +
> +  getInterface: function sc_getInterface(iid) {
> +    return this;
> +  }

Please add a QI for this class and let getInterface return result of QI.

@@ +391,5 @@
> +};
> +
> +
> +const NSGetFactory = XPCOMUtils.generateNSGetFactory([TCPSocket]);
> +

Remove the trailing blank line.

::: b2g/components/nsIDOMTCPSocket.idl
@@ +14,5 @@
> + * MozTCPSocket exposes a TCP client socket (no server sockets yet)
> + * to highly privileged apps. It provides a buffered, non-blocking
> + * interface for sending. For receiving, it uses an asynchronous, 
> + * event handler based interface.
> + */

Move this comment right under the license.

@@ +91,5 @@
> +   *             binaryType: "arraybuffer" was passed in the options
> +   *             object, then this object should be an Uint8Array instance.
> +   *             If binaryType: "string" was passed, or if no binaryType
> +   *             option was specified, then this object should be an
> +   *             an ordinary JavaScript string.

typo "an an"

@@ +114,5 @@
> +  readonly attribute DOMString readyState;
> +  readonly attribute DOMString CONNECTING;
> +  readonly attribute DOMString OPEN;
> +  readonly attribute DOMString CLOSING;
> +  readonly attribute DOMString CLOSED;

Not sure how that would work in content JS, but I believe these could be just consts.

@@ +164,5 @@
> +   * If onerror is called before onopen, the error was connection refused,
> +   * and onclose will not be called. If onerror is called after onopen,
> +   * the connection was lost, and onclose will be called after onerror.
> +   */
> +  attribute jsval onerror;

Probably for a followup, and maybe the way this class reports errors is the same as websockets or so, but how will errors coming from the socket be programatically processed by web developers when they want to react to different types of errors differently?  How will errors coming from this class be presented to the user?  The strings we present are not the option at least because they cannot be localized.

@@ +194,5 @@
> +
> +  /**
> +   * The type of this event.
> +   */
> +  readonly attribute DOMString type;

What values this can have?

::: b2g/test/unit/test_tcpsocket.js
@@ +29,5 @@
> +for (var i_big = 0, j_big = 0; i_big < BIG_ARRAY.length; i_big++) {
> +  BIG_ARRAY[i_big] = HELLO_WORLD.charCodeAt(j_big++);
> +  if (j_big >= HELLO_WORLD.length) {
> +    j_big = 0;
> +  }

First, where you, do can use ++var.

Instead of if (j_big >= HELLO_WORLD.length) j_big = 0; you can just do HELLO_WORLD.charCodeAt(i_bug++ % HELLO_WORLD.length).

On the other hand, to make sure all has been sent correctly, BIG_ARRAY should be a random sequence.

@@ +52,5 @@
> +// The "open" method is the constructor-ish method, but for realism, we do not
> +// make it magically happen.
> +const nsIDOMTCPSocket = CC("@mozilla.org/tcp-socket;1",
> +                     "nsIDOMTCPSocket"),
> +      TCPSocket = new nsIDOMTCPSocket();

nsIDOMTCPSocket is not a good name for an object constructor.  Maybe call it just DOMTCPSocket ?

@@ +117,5 @@
> +  },
> +
> +  waitForData: function(expectedData, successFunc) {
> +    this.expectedData = expectedData;
> +  },

What is successFunc for?

@@ +123,5 @@
> +  onStartRequest: function(request, context) {
> +  },
> +
> +  onStopRequest: function(request, context, status) {
> +print('onStopRequest', request, context, status);

Indention and I'm getting errors while executing this test that |print| is not defined.

@@ +140,5 @@
> +  /**
> +   * Forget about the socket we knew about before.
> +   */
> +  reset: function() {
> +    this.binaryInput = this.input = this.binaryOutput = this.output = null;

This really works?  I'd expect that you may need to assign one by one since they are quit different interfaces.

@@ +174,5 @@
> +    }
> +    do_throw('got unexpected: ' + name + ' ' + argstr);
> +  };
> +}
> +function makeExpectData(name, expectedData, fromEvent) {

name seems to be unused.

@@ +232,5 @@
> +  server.onaccept = yayFuncs.serveropen;
> +  server.ondata = makeFailureCase('serverdata');
> +  server.onclose = makeFailureCase('serverclose');
> +}
> +function sendData() {

Blank line before the function and { on the new line.  This applies on many places in this file.

Add a comment before this section that these are the actual tests.  Add a comment over each function what it does and what it expects to happen.

@@ +328,5 @@
> +    sock.close();
> +    yays.ondrain();
> +  }
> +
> +  sock.send(BIG_ARRAY);

You should check this returns false.

@@ +369,5 @@
> +  run_next_test();
> +
> +  do_timeout(10000, function() {
> +    do_throw(
> +      "The test should never take this long unless the system is hosed.");

I think xpcshell harness takes care of this.  This could just collide with that unexpectedly.  But up to you.  If there is a strong reason for having a custom timeout, then please add a comment why you are adding it.

Comment 99

[Honza Bambas (:mayhemer)]

Donovan, will you be able to check on the test failure I'm experiencing or should I do that?

Comment 100

[Donovan Preston [:fzzzy]]

I'm rebuilding against head to see if I can reproduce the problem.

Comment 101

[Donovan Preston [:fzzzy]]

I'm not seeing that failure here. Since I haven't tested on Windows yet (I run on Mac and Andrew tests on Linux) I'm betting there is some windows weirdness there. It seems strange that all the tests before "bufferedClose" would pass, and then that one specifically would time out, though...

Comment 102

[Donovan Preston [:fzzzy]]

Comment on attachment 635473 [details] [diff] [review]
wip-0.9.5

Clearing review flag, will have another incrementally improved patch with this review's comments tomorrow.

Comment 103

[JP Rosevear [:jpr]]

Should block mail app.

Comment 104

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Honza Bambas (:mayhemer) from comment #98)
> 
> ::: b2g/components/B2GComponents.manifest
> @@ +32,5 @@
> >  
> > +# TCPSocket.js
> > +component {cda91b22-6472-11e1-aa11-834fec09cd0a} TCPSocket.js
> > +contract @mozilla.org/tcp-socket;1 {cda91b22-6472-11e1-aa11-834fec09cd0a}
> > +category JavaScript-global-property MozTCPSocket @mozilla.org/tcp-socket;1
> 
> I cannot review these pieces.  You need to find reviewer for this code to
> get this in.
> 
> ::: b2g/components/TCPSocket.js
> @@ +38,5 @@
> > +    contractID: "@mozilla.org/tcp-socket-event;1",
> > +    classDescription: "TCP Socket Event",
> > +    interfaces: [Ci.nsITCPSocketEvent],
> > +    flags: Ci.nsIClassInfo.DOM_OBJECT
> > +  })
>

This pieceis of codes look good but I think the API  should move to dom/socket or something like that instead of living into b2g/.

Donovan, any news with this test failure?

Comment 105

[Donovan Preston [:fzzzy]]

Attachment: wip-0.10

- added global property registration for MozTCPSocketEvent
 - I added this just for consistency, but would also be happy removing it
 - Could be useful for "instanceof"?
- _ensureCopying and _createTransport are now methods instead of just bare
  functions
- Removed unneeded _request variable
- Renamed _waitingOnOutput to _asyncCopierActive
- Renamed _earlySuspensions to _suspendCount
- Removed extraneous double this.ssl initialization
- Use "new" consistently everywhere (new AsyncStreamCopier)
- Made 65536 a const "BUFFER_SIZE"
- Moved more of the state checks into _ensureCopying
- Moved the early suspend calls to immediately after the InputStreamPump has
  been created
- In SecurityCallbacks.notifyCertProblem, pass the "status" object directly
  on to the onerror callback, to allow user code to distinguish between
  various error conditions
- In the idl file the initial comment is now immediately below the license
- Typo "an an" fixed
- I tried to provide richer objects to the onerror callback with the hope
  that by passing the objects instead of strings, the developer will be
  able to differentiate the errors
- Enumerated possibilities for the "type" field of TCPSocketEvent in the
  documentation comment
- Initialized BIG_ARRAY in the test with random data
- In the test, named the constructor TCPSocket instead of nsIDOMTCPSocket
- Removed extraneous successFunc parameter
- Removed extraneous print statements in the tests
- In the test "reset" function, assign all nulls separately
- Use the "name" parameter in makeExpectData to make error messages
  more useful
- Fixed missing blank lines in the tests
- Check that send returns false in the appropriate test
- Added a test that calling "send" twice in a row without waiting for
  ondrain buffers properly, and that ondrain is called eventually

Outstanding issues:

- I still have no idea what the correct thing to return from
  SecurityCallbacks.getInterface is. Someone needs to hold my hand here.
  I can't just guess until it passes review.
- const doesn't seem to work in idl unless I did it wrong somehow, so I left
  them "readonly attribute"
- I removed the manual timeout handling code from the tests but it does not
  appear that the xpcshell harness handles timeouts automatically, so I put
  it back in. More info about the right thing to do here from someone who
  knows would be good. I could have done something wrong.

Comment 106

[Andrew Sutherland [:asuth] (he/him)]

getInterface-wise, this is my sub-expert understanding:

getInterface is part of nsIInterfaceRequester; there's a good blurb on it at https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIInterfaceRequestor but in a nutshell, you just need to return an instance that implements the required interface, and it need not be 'this', but it could be.  Per https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsISocketTransport it sounds like securityCallbacks is expected to implement nsIBadCertListener2 and nsISSLErrorListener.

As such, returning 'this' works, but could confuse code that then tries to getInterface something and gets told a lie.  (The lie won't be fatal though, as XPConnect will simply generate errors whenever the caller tries to call the methods that your implementation does not actually implement.)  It is probably best for the object you are using for securityCallbacks to implement QueryInterface by using XPCOMUtils.generateQI, declaring those interfaces as well as nsIInterfaceRequester and then just have getInterface be the same as the QueryInterface implementation.  (Either by having it be a function that calls QueryInterface, or reusing the exact-same function by copying/whatever.)

Comment 107

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Generally speaking, don't return something from getInterface unless it's actually implementing the requested interface. Additionally, you often need to return the "right" object which implements the requested interface. It's not the case that any object will do.

Consider for example calling docShell.getInterface(nsIDocument). Callers of that definitely expect to get the document currently rendered in that docShell. Even though all document implement nsIDocument, callers are expecting the specific one that is currently being rendered.

Additionally, we have several places in the code which calls getInterface on one object and if that returns null keeps going to call getInterface on another object. So if you are returning 'this' and then not implement the interface, that could mean that you are preventing a better object from being used.

Comment 108

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 643432 [details] [diff] [review]
wip-0.10

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

I think this code should moved to dom/ instead of b2g/. And then you need a new reviewer than me for the Makefile changes in dom/ :)

Comment 109

[Faramarz Rashed]

Vivien, why does this need to move?

Comment 110

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Faramarz (:faramarz) from comment #109)
> Vivien, why does this need to move?

To live next to the other APIs (dom/contacts, dom/apps/, dom/activities, dom/messages, etc...)

This is mostly a story of moving files.

Comment 111

[Honza Bambas (:mayhemer)]

I'll try to take a look tomorrow.

Comment 112

[Andreas Gal :gal]

Can someone else steal the review? This is supposed to land tomorrow.

Comment 113

[Jason Duell]

I don't know the code as well as Honza and have my own bugs that land tomorrow.  I can't think of another reviewer I'd be happy with taking this except mcmanus, and it seems unlikely he'd get to it before honza tomorrow.  Sorry.

Note that this patch won't work in e10s w/o bug 770778--is that part of the deadline too? I can pivot to that once I'm done with cookie and/or appcache-jar stuff...

Comment 114

[Honza Bambas (:mayhemer)]

(In reply to Andreas Gal :gal from comment #112)
> Can someone else steal the review? This is supposed to land tomorrow.

Why is it supposed to land today?  I'll take a look at this now, as the first review.

Comment 115

[Honza Bambas (:mayhemer)]

Comment on attachment 643432 [details] [diff] [review]
wip-0.10

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

I'm definitely for the file move mentioned in comment 108.  Applies to tests as well.

::: b2g/components/TCPSocket.js
@@ +39,5 @@
> +    contractID: "@mozilla.org/tcp-socket-event;1",
> +    classDescription: "TCP Socket Event",
> +    interfaces: [Ci.nsITCPSocketEvent],
> +    flags: Ci.nsIClassInfo.DOM_OBJECT
> +  })

Where are socket, type and data attributes implemented?

Why is this declared as a component but you don't register it at the bottom of this file?  Do we really want this even have an instantiable component?

@@ +92,5 @@
> +  CLOSING: kCLOSING,
> +  CLOSED: kCLOSED,
> +
> +  // The binary type, "string" or "arraybuffer"
> +  binaryType: null,

Hmm.. in the idl this is defined as an r/w attribute.  You have to define getters and setters for it, I believe.  Use get attributeName() {} and set attributeName(value) {} to do it.  http://books.mozdev.org/html/mozilla-chp-8-sect-2.html

The same applies to all other attributes of this interface.

@@ +109,5 @@
> +  _inputStreamScriptable: null,
> +  _inputStreamBinary: null,
> +
> +  // Output stream machinery
> +  _outputMultiplexStream: null,

This is actually an input stream.  Output in the name is missleading.  Just add here more detailed comment on how we are using it.

@@ +130,5 @@
> +             .getService(Ci.nsISocketTransportService)
> +             .createTransport(options, optlen, host, port, null);
> +  },
> +
> +  _ensureCopying: function ts_ensureCopying(that) {

Why using |that| when you have |this| here?

@@ +174,5 @@
> +    // domain. If not, open will refuse to create and open new sockets.
> +    let principal = aWindow.document.nodePrincipal;
> +    this._hasPrivileges = (
> +      Services.perms.testExactPermission(principal.URI, "tcp-socket")
> +      === Ci.nsIPermissionManager.ALLOW_ACTION);

Please use a very new method: testExactPermissionFromPrincipal.

@@ +181,5 @@
> +  // nsIDOMTCPSocket
> +  open: function ts_open(host, port, options) {
> +    // in the testing case, init won't be called and
> +    // hasPrivileges will be null. We want to proceed to test.
> +    if (this._hasPrivileges !== true && this._hasPrivileges !== null) {

Hmm.. could there be some other way?  This is actually a potential hole to bypass permissions.

@@ +200,5 @@
> +          that.ssl = false;
> +      }
> +      that.binaryType = options.binaryType || that.binaryType;
> +    }
> +    that._binaryType = that.binaryType;

This is odd.

@@ +286,5 @@
> +  suspend: function ts_suspend() {
> +    if (this._inputStreamPump) {
> +      this._inputStreamPump.suspend();
> +    } else {
> +      this._suspendCount++;

Do ++this._suspendCount.

@@ +294,5 @@
> +  resume: function ts_resume() {
> +    if (this._inputStreamPump) {
> +      this._inputStreamPump.resume();
> +    } else {
> +      this._suspendCount--;

As well here with --.

@@ +311,5 @@
> +        this._socketInputStream, -1, -1, 0, 0, false
> +      )
> +      while (this._suspendCount) {
> +        this._inputStreamPump.suspend();
> +        this._suspendCount--;

while (this._suspendCount--)
  this._inputStreamPump.suspend();

?

@@ +388,5 @@
> +    return true;
> +  },
> +
> +  getInterface: function sc_getInterface(iid) {
> +    return this;

Oh, please implement this carefully.  Have also QI for this class, gI then can use it.  I think I have mentioned this ones already.

::: b2g/test/unit/test_tcpsocket.js
@@ +8,5 @@
> + *
> + * Future work:
> + * - SSL.  see https://bugzilla.mozilla.org/show_bug.cgi?id=466524
> + *             https://bugzilla.mozilla.org/show_bug.cgi?id=662180
> + *   Alternatively, mochitests could be used.

They should be used primarily.  TCP socket is made for content, so it should be tested by mochi that tests content code.

Also, this test it self should be under dom.  This is a platform class, as I understand.  It is not specific to B2G only, any Gecko application should be able to use it.

@@ +117,5 @@
> +      do_throw('Received ' + count + ' bytes of unexpected data!');
> +    }
> +  },
> +
> +  waitForData: function(expectedData) {

call this "expectData".  Your name may imply the function is actually waiting for something.

This method is unused, though.

@@ +281,5 @@
> +    }
> +  };
> +  sock.ondrain = function(evt) {
> +    if (sock.bufferedAmount) {
> +      do_throw("sock.bufferedAmount was > 0 in ondrain");

IMO this check is too restrictive.  But leave it here for the first test version.

@@ +339,5 @@
> + */
> +
> +function bufferedClose() {
> +  var yays = makeJointSuccess(['serverdata', 'clientclose', 'serverclose']);
> +  server.ondata = yays.serverdata;

You really need to check that all the data got to the server.  Where is it done here?

@@ +353,5 @@
> + */
> + 
> +function badConnect() {
> +  // There's probably nothing listening on tcp port 2.
> +  sock = TCPSocket.open('127.0.0.1', 2);

Unfortunatelly, I don't have right now a better idea how to this, also in a platform independent manner.

@@ +372,5 @@
> +function drainTwice() {
> +  let yays = makeJointSuccess(
> +    ['ondrain', 'ondrain2', 'serverclose', 'clientclose']);
> +
> +  server.ondata = function() {}

You have to check all has arrived to the server.  Where is it done?

@@ +394,5 @@
> +}
> +
> +function cleanup() {
> +  sock.close();
> +  makeSuccessCase('cleanup')();

call run_next_test() directly here and just do_print you are cleaning up?

@@ +406,5 @@
> +function bufferTwice() {
> +  let yays = makeJointSuccess(
> +    ['ondrain', 'serverclose', 'clientclose']);
> +
> +  server.ondata = function() {}

Again, where are the data arriving to the server checked?

@@ +461,5 @@
> +
> +  do_timeout(10000, function() {
> +    do_throw(
> +      "The test should never take this long unless the system is hosed.");
> +  });

No, please remove this or make it just somehow conditional (like having some var debug = false|true global in the test).  xpcshell has a timeout mechanism for sure.  But with a much longer t/o value.

Please make sure, by hand, that do_throw actually works in any of the callbacks you are using it.  It could happen that it might get swallowed.

Comment 116

[Honza Bambas (:mayhemer)]

Comment on attachment 643432 [details] [diff] [review]
wip-0.10

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

::: b2g/components/TCPSocket.js
@@ +263,5 @@
> +      // that took an ArrayBuffer like StringInputStream takes
> +      // a string. There is one, but only in C++ and not exposed
> +      // to js as far as I can tell
> +      data = Array.map(data, function(el, i) {
> +        return String.fromCharCode(el);

On my seper-fast machine this code gets stuck for seconds when executing the test.  CPU is at 100%.  We really must find a different way to convert the data, this is a huge bottle neck.

Comment 117

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #115)
> No, please remove this or make it just somehow conditional (like having some
> var debug = false|true global in the test).  xpcshell has a timeout
> mechanism for sure.  But with a much longer t/o value.
> 
> Please make sure, by hand, that do_throw actually works in any of the
> callbacks you are using it.  It could happen that it might get swallowed.

do_throw sets _passed to false and calls _do_quit, so even if the exception is eaten, the test will fail and the the xpcshell-spun nested-event loop will terminate:

http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#444

Comment 118

[Donovan Preston [:fzzzy]]

(In reply to Honza Bambas (:mayhemer) from comment #116)
> Comment on attachment 643432 [details] [diff] [review]
> wip-0.10
> 
> Review of attachment 643432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/TCPSocket.js
> @@ +263,5 @@
> > +      // that took an ArrayBuffer like StringInputStream takes
> > +      // a string. There is one, but only in C++ and not exposed
> > +      // to js as far as I can tell
> > +      data = Array.map(data, function(el, i) {
> > +        return String.fromCharCode(el);
> 
> On my seper-fast machine this code gets stuck for seconds when executing the
> test.  CPU is at 100%.  We really must find a different way to convert the
> data, this is a huge bottle neck.

Really? That's no good. The entire test run only takes 1.5 seconds on my 2.3 ghz i7 macbook pro.

But yes I agree this code is a bottleneck. The previous version of the code did not have it, because there were apis which took an ArrayBuffer, but once I switched over to multiplexstream I had to put this hack in, because although there is an inputstream that takes an arraybuffer like stringinputstream takes a string, it does not appear to be exposed to javascript.

Comment 119

[Donovan Preston [:fzzzy]]

Argh. At the last minute, I forgot to implement getInterface, even with all the discussion about it. I'm very sorry about that.

Also, I removed the manual timeout code, but the tests just hung forever after I took it out, so I put it back in. Is it just a really, really long timeout value? I'll try again.

Thanks for the review!

Comment 120

[Josh Matthews [:jdm]]

I don't recall if the timeout occurs when running tests locally or not. However, it's two minutes for xpcshell tests, I believe.

Comment 121

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Donovan Preston from comment #119)
> Also, I removed the manual timeout code, but the tests just hung forever
> after I took it out, so I put it back in. Is it just a really, really long
> timeout value? I'll try again.

It sounds like you are saying that successful test runs are depending on that timeout to kill the test?  If so, that suggests a failure?

Comment 122

[Donovan Preston [:fzzzy]]

(In reply to Andrew Sutherland (:asuth) from comment #121)
> (In reply to Donovan Preston from comment #119)
> > Also, I removed the manual timeout code, but the tests just hung forever
> > after I took it out, so I put it back in. Is it just a really, really long
> > timeout value? I'll try again.
> 
> It sounds like you are saying that successful test runs are depending on
> that timeout to kill the test?  If so, that suggests a failure?

No, I removed the manual timeout and made one of the tests failing by causing it to never finish, to see if there was an existing timeout which would get triggered.

I'm not sure if I waited two minutes, though.

Comment 123

[Donovan Preston [:fzzzy]]

(In reply to Josh Matthews [:jdm] from comment #120)
> I don't recall if the timeout occurs when running tests locally or not.
> However, it's two minutes for xpcshell tests, I believe.

No, the timeout does not occur when running the tests locally. I assume "it's two minutes for xpcshell tests" means that there is some sort of external timeout enforced by the automatic test-running infrastructure? That sounds good, if so. I'll remove the manual timeout code.

Comment 124

[Josh Matthews [:jdm]]

(In reply to Donovan Preston from comment #123)
> (In reply to Josh Matthews [:jdm] from comment #120)
> > I don't recall if the timeout occurs when running tests locally or not.
> > However, it's two minutes for xpcshell tests, I believe.
> 
> No, the timeout does not occur when running the tests locally. I assume
> "it's two minutes for xpcshell tests" means that there is some sort of
> external timeout enforced by the automatic test-running infrastructure? That
> sounds good, if so. I'll remove the manual timeout code.

That is correct.

Comment 125

[Andrew Sutherland [:asuth] (he/him)]

I just saw some "can't access dead object" errors from TCPSocket.js:164 in my adb log and I realized the patch never got an inner-window-destroyed observer so it can close connections if the window goes away.  (Like in CameraContent.js and MozKeyboard.js)

Comment 126

[Donovan Preston [:fzzzy]]

Thanks Andrew, that is definitely something that needs to be done. Thank you for the reference to the other pieces of code which do this.

Comment 127

[Donovan Preston [:fzzzy]]

Ok, I think we need to do the inner-window-destroyed stuff in a followup. If the worst thing that happens is some "cannot access dead object" log lines get printed I think that's ok for the first version. I tried implementing observe like CameraContent.js and MozKeyboard.js implement it and I can't get it to fire at all, so I can't clean up those sockets when the window goes away.

Also, I cannot get init to fire at all, which means that the permission code as it is currently implemented actually does not work at all. I know this did work at some distant point in history, because I painstakingly made sure with lots of log statements that the right thing was happening, but it seems not to be called any more.

Finally, I think the problem with ArrayBuffers being extremely slow to convert on Windows needs to be handled in a followup. We're not targeting Windows with this code right now, and the changes required to make this fast probably require C++ changes (someone please correct me if I am wrong here!) so if it's not a problem on the devices we are actually targeting then I think it's safe to say we can do it in a followup.

If it is ok to do these three things in a followup, and vingtetun moves the implementation into the dom directory and converts the xpcshell tests to mochitests, then maybe this can actually be landed sometime this century.

Comment 128

[Donovan Preston [:fzzzy]]

Attachment: wip-0.11.2

0.11.2 changes:

- TCPSocketEvent is no longer an instantiable component
- added accessor functions and __exposedProps__ for all public members
- _outputMultiplexStream renamed _multiplexStream. I did not want to include input in the name because I think that would be even more confusing.
- in _ensureCopying, used 'this' instead of 'that' where appropriate. I still have to use 'that' in onStopRequest because there 'this' is the request instance, not the socket instance
- used testExactPermissionFromPrincipal instead of testExactPermission
- removed odd that._binaryType = that.binaryType line
- changed this._suspendCount++ to ++this._suspendCount and did the same for --
- changed suspend loop in onTransportStatus to "while (this._suspendCount--)"
- implemented getInterface and QueryInterface properly for SecurityCallbacks (finally)
- removed unused waitForData method from test harness
- checked that the data is received correctly on the server in the bufferedClose, drainTwice, and bufferTwice tests
- called run_next_test directly from cleanup
- removed the manual test timeout code

vingtetun can you please help with the following:

~ move into dom/socket
~ switch to mochitests
~ permission code review or new or correct way to do permissions (init is not currently being called, even though it was when I wrote it, therefore the current patch has no permission checking code)

Outstanding issue:

The code which handles ArrayBuffer in send is slow because there is no equivalent setData on BinaryInputStream which takes an ArrayBuffer, analagous to the setData on StringInputStream which takes a string. Thus the code for handling ArrayBuffer in send looks like this after the switch to using the MultiplexStream:

let new_stream = new StringInputStream();
if (this._binaryType === "arraybuffer") {
  data = Array.map(data, function(el, i) {
    return String.fromCharCode(el);
  }).join("");
}
new_stream.setData(data, data.length);

Obviously this is terrible for performance. If someone knows of a way to make StringInputStream.setData take an ArrayBuffer and do the correct thing (right now it works but mangles the data), please enlighten me. Otherwise, I think the best way to fix this is to add a BinaryInputStream.setData which is analagous to StringInputStream.setData.

Comment 129

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 647825 [details] [diff] [review]
wip-0.11.2

Honza I will move the code into dom/ and convert the tests to mochitests but I think the code can be still reviewed before it is done in order to go faster.

Comment 130

[Honza Bambas (:mayhemer)]

(In reply to Vivien Nicolas (:vingtetun) from comment #129)
> Comment on attachment 647825 [details] [diff] [review]
> wip-0.11.2
> 
> Honza I will move the code into dom/ and convert the tests to mochitests but
> I think the code can be still reviewed before it is done in order to go
> faster.

Ok, just please don't obsolete the current wip-0.11.2 patch.  I am starting the review and it would delete the splinter comments draft.

Comment 131

[Honza Bambas (:mayhemer)]

Comment on attachment 647825 [details] [diff] [review]
wip-0.11.2

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

::: b2g/components/TCPSocket.js
@@ +212,5 @@
> +    if (this._asyncCopierActive) {
> +      return;
> +    }
> +    this._asyncCopierActive = true;
> +    this._multiplexStreamCopier.asyncCopy({

do: var self = this; before this line and use instead of |that|.  You then don't need the arg at all.

@@ +215,5 @@
> +    this._asyncCopierActive = true;
> +    this._multiplexStreamCopier.asyncCopy({
> +      onStartRequest: function ts_output_onStartRequest() {
> +      },
> +      onStopRequest: function ts_output_onStopRequest() {

Here, if the status is a failure, you have to call onerror and also close the socket.

@@ +228,5 @@
> +          }
> +          if (that._readyState === kCLOSING) {
> +            that._socketOutputStream.close();
> +            that._readyState = kCLOSED;
> +            that.callListener("onclose");

If these two lines are so needed, how is it that the tests didn't catch they were missing?  If the test is correct, then this is not needed.  It the test is not correct then this code path is not tested and the test then needs to be updated.

@@ +261,5 @@
> +    this.innerWindowID = util.currentInnerWindowID;
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    // TODO why is this code not getting called?

Because you need to register this object to observe it:

Services.obs.addObserver(this, "inner-window-destroyed", true);

If the third arg is true, then it means the observer service just takes a weak ref to your object and you don't need to deregister with removeObserver.  To make this fully work, just add nsISupportsWeakReference to the list of implemented interfaces in QI implementation bellow.

@@ +356,5 @@
> +      // a string. There is one, but only in C++ and not exposed
> +      // to js as far as I can tell
> +      data = Array.map(data, function(el, i) {
> +        return String.fromCharCode(el);
> +      }).join("");

I was talking with few people about how to optimize this.  Best I believe is to implement a C++ platform class that takes arraybuffer and exposes it as nsIInputStream.  A string input stream sister.

Also, would be nice to somehow figure out how to prevent data copy also in case of data being a string.

@@ +436,5 @@
> +      // want to be in the close state until we are done sending
> +      // everything that was buffered. We also don't want to call onclose
> +      // yet.
> +      return;
> +    }

Yes, good change.  But with this code we have to check that when the output stream gets closed too (e.g. by RST) we indicate the error and get to the correct (CLOSED) state.

@@ +483,5 @@
> +
> +SecurityCallbacks.prototype = {
> +  notifySSLError: function sc_notifySSLError(socketInfo, error, targetSite) {
> +    return true;
> +  },

If this method is just empty (the result is now ignored, anyway), do you really need to implement nsISSLErrorListener?  Maybe there is a reason, I just don't remember it.

::: b2g/test/unit/test_tcpsocket.js
@@ +203,5 @@
> +    if (dataBuffer.length >= expectedData.length) {
> +      // check the bytes are equivalent
> +      for (let i = 0; i < expectedData.length; i++) {
> +        if (dataBuffer[i] !== expectedData[i]) {
> +          do_throw(name + ' Received mismatched character at position ' + i);

Not sure, but the previous code seemed to me correct as well.  Up to you.

@@ +384,5 @@
> +      "ondata2", BIG_ARRAY, false, yays.ondata2);
> +
> +    sock.ondrain = yays.ondrain2;
> +
> +    if (sock.send(BIG_ARRAY)) {

It would be great to have a second BIG_ARRAY2 with a different content as an even more prove test.

@@ +432,5 @@
> +
> +  if (sock.send(BIG_ARRAY)) {
> +    throw new Error("sock.send(BIG_ARRAY) did not return false to indicate buffering");
> +  }
> +  if (sock.send(BIG_ARRAY)) {

As well here (BIG_ARRAY2).

Comment 132

[Donovan Preston [:fzzzy]]

(In reply to Honza Bambas (:mayhemer) from comment #131)
> Comment on attachment 647825 [details] [diff] [review]
> wip-0.11.2
> 
> Review of attachment 647825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/TCPSocket.js
> @@ +212,5 @@
> > +    if (this._asyncCopierActive) {
> > +      return;
> > +    }
> > +    this._asyncCopierActive = true;
> > +    this._multiplexStreamCopier.asyncCopy({
> 
> do: var self = this; before this line and use instead of |that|.  You then
> don't need the arg at all.

Ok, sure. The argument was a leftover from when this was a bare function and not a method.

> @@ +215,5 @@
> > +    this._asyncCopierActive = true;
> > +    this._multiplexStreamCopier.asyncCopy({
> > +      onStartRequest: function ts_output_onStartRequest() {
> > +      },
> > +      onStopRequest: function ts_output_onStopRequest() {
> 
> Here, if the status is a failure, you have to call onerror and also close
> the socket.

Ok, thanks.

> @@ +228,5 @@
> > +          }
> > +          if (that._readyState === kCLOSING) {
> > +            that._socketOutputStream.close();
> > +            that._readyState = kCLOSED;
> > +            that.callListener("onclose");
> 
> If these two lines are so needed, how is it that the tests didn't catch they
> were missing?  If the test is correct, then this is not needed.  It the test
> is not correct then this code path is not tested and the test then needs to
> be updated.

These changes were actually exposed by your request that I test that the actual bytes are received by the server in drainTwice. It turned out that there was a subtle state machine problem in the previous version of the patch, where calling send followed by close would get this code into a state where the second buffer to send was never sent and no events were ever fired on that socket again. So thanks for making me write that test. If you want more information I can compare the two versions of the patch and try to remember exactly what was wrong.

> @@ +261,5 @@
> > +    this.innerWindowID = util.currentInnerWindowID;
> > +  },
> > +
> > +  observe: function(aSubject, aTopic, aData) {
> > +    // TODO why is this code not getting called?
> 
> Because you need to register this object to observe it:
> 
> Services.obs.addObserver(this, "inner-window-destroyed", true);
> 
> If the third arg is true, then it means the observer service just takes a
> weak ref to your object and you don't need to deregister with
> removeObserver.  To make this fully work, just add nsISupportsWeakReference
> to the list of implemented interfaces in QI implementation bellow.

Oh awesome, thank you so much.

However it still won't be much use to me because init is not being called for whatever reason, and I need init to be called with the window object to be able to keep track of sockets that are associated with a window and close them when inner-window-destroyed.

> @@ +356,5 @@
> > +      // a string. There is one, but only in C++ and not exposed
> > +      // to js as far as I can tell
> > +      data = Array.map(data, function(el, i) {
> > +        return String.fromCharCode(el);
> > +      }).join("");
> 
> I was talking with few people about how to optimize this.  Best I believe is
> to implement a C++ platform class that takes arraybuffer and exposes it as
> nsIInputStream.  A string input stream sister.

Yes, that would be wonderful. Is this likely to get done soon enough that we should just wait for it, or is it ok to land this with the slow code and switch the patch to use this in a followup, after this new feature has landed?

> Also, would be nice to somehow figure out how to prevent data copy also in
> case of data being a string.

Yes, that would be very nice.

> @@ +436,5 @@
> > +      // want to be in the close state until we are done sending
> > +      // everything that was buffered. We also don't want to call onclose
> > +      // yet.
> > +      return;
> > +    }
> 
> Yes, good change.  But with this code we have to check that when the output
> stream gets closed too (e.g. by RST) we indicate the error and get to the
> correct (CLOSED) state.

Will that be indicated by !status? If so, it falls down below to where it sets close and calls onerror...

> @@ +483,5 @@
> > +
> > +SecurityCallbacks.prototype = {
> > +  notifySSLError: function sc_notifySSLError(socketInfo, error, targetSite) {
> > +    return true;
> > +  },
> 
> If this method is just empty (the result is now ignored, anyway), do you
> really need to implement nsISSLErrorListener?  Maybe there is a reason, I
> just don't remember it.

I'm not sure, I'll try removing it and see what happens.

> ::: b2g/test/unit/test_tcpsocket.js
> @@ +203,5 @@
> > +    if (dataBuffer.length >= expectedData.length) {
> > +      // check the bytes are equivalent
> > +      for (let i = 0; i < expectedData.length; i++) {
> > +        if (dataBuffer[i] !== expectedData[i]) {
> > +          do_throw(name + ' Received mismatched character at position ' + i);
> 
> Not sure, but the previous code seemed to me correct as well.  Up to you.

Yes, the previous code was correct, but it was horribly, horribly slow because it logged every single character. Previously this code was only used to check arrays of a few characters, but once I started using it to check BIG_ARRAY as well, the logspam was unbearable when the test was failing.

> @@ +384,5 @@
> > +      "ondata2", BIG_ARRAY, false, yays.ondata2);
> > +
> > +    sock.ondrain = yays.ondrain2;
> > +
> > +    if (sock.send(BIG_ARRAY)) {
> 
> It would be great to have a second BIG_ARRAY2 with a different content as an
> even more prove test.
> 
> @@ +432,5 @@
> > +
> > +  if (sock.send(BIG_ARRAY)) {
> > +    throw new Error("sock.send(BIG_ARRAY) did not return false to indicate buffering");
> > +  }
> > +  if (sock.send(BIG_ARRAY)) {
> 
> As well here (BIG_ARRAY2).

Yes, sounds good.

Comment 133

[Honza Bambas (:mayhemer)]

Also, I just noticed that you are mixing sock.send(BIG_TYPED_ARRAY) and sock.send(BIG_ARRAY).  As I understand only the former is correct, right?

Comment 134

[Honza Bambas (:mayhemer)]

I have altered your

          data = Array.map(data, function(el, i) {
            return String.fromCharCode(el);
          }).join("");


with

          var dataLen = data.length;
          var offset = 0;
          var result = "";
          while (dataLen) {
            var fragmentLen = dataLen;
            if (fragmentLen > 32768)
              fragmentLen = 32768;
            dataLen -= fragmentLen;

            var fragment = data.subarray(offset, offset + fragmentLen);
            offset += fragmentLen;
            result += String.fromCharCode.apply(null, fragment);
          }
          data = result;


and the test is now about 2-point-something faster (28 seconds).  Still very slow, though.

Comment 135

[Donovan Preston [:fzzzy]]

Yes, best would be a C++ method which takes an ArrayBuffer and produces an InputStream.

Comment 136

[Honza Bambas (:mayhemer)]

(In reply to Donovan Preston from comment #135)
> Yes, best would be a C++ method which takes an ArrayBuffer and produces an
> InputStream.

I'll file a bug on it.

Comment 137

[Donovan Preston [:fzzzy]]

Ok, never mind my previous comments about init not being called. It is being called. So now that I know how to add the inner-window-destroyed observer, I can track the sockets associated with each window and close them on inner-window-destroyed.

Comment 138

[Donovan Preston [:fzzzy]]

(In reply to Honza Bambas (:mayhemer) from comment #133)
> Also, I just noticed that you are mixing sock.send(BIG_TYPED_ARRAY) and
> sock.send(BIG_ARRAY).  As I understand only the former is correct, right?

Whoops, yes, I think you are right. Should send check the type of it's argument and raise if it is not a string or Uint8Array?

Comment 139

[Donovan Preston [:fzzzy]]

> @@ +215,5 @@
> > +    this._asyncCopierActive = true;
> > +    this._multiplexStreamCopier.asyncCopy({
> > +      onStartRequest: function ts_output_onStartRequest() {
> > +      },
> > +      onStopRequest: function ts_output_onStopRequest() {
> 
> Here, if the status is a failure, you have to call onerror and also close
> the socket.

I was hoping to have the next version of the patch done today, but I really need to write a test for this case. I think the test is:

open connection
write data
server closes connection without reading data

> @@ +436,5 @@
> > +      // want to be in the close state until we are done sending
> > +      // everything that was buffered. We also don't want to call onclose
> > +      // yet.
> > +      return;
> > +    }
> 
> Yes, good change.  But with this code we have to check that when the output
> stream gets closed too (e.g. by RST) we indicate the error and get to the
> correct (CLOSED) state.

I need a test for this too. I think the test here is:

open connection
??? server closes connection non-cleanly

Not exactly sure how I can simulate the connection dropping, but I'll poke around and see if I can do it.

Comment 140

[Donovan Preston [:fzzzy]]

(In reply to Donovan Preston from comment #138)
> (In reply to Honza Bambas (:mayhemer) from comment #133)
> > Also, I just noticed that you are mixing sock.send(BIG_TYPED_ARRAY) and
> > sock.send(BIG_ARRAY).  As I understand only the former is correct, right?
> 
> Whoops, yes, I think you are right. Should send check the type of it's
> argument and raise if it is not a string or Uint8Array?

This just happened to work because of the slow code which handles ArrayBuffer using Array.map to convert to a string.

Comment 141

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dom/

This patch is not for review. This is to shared the moves from b2g/ -> dom/ with Donovan.

(In reply to Donovan Preston from comment #128)

> vingtetun can you please help with the following:
> 
> ~ move into dom/socket

Done in the attached wip.

> ~ switch to mochitests

Actually xpcshell-tests are fine. I though you were using the Gaia mocha framework but hopefull I was completely wrong.

> ~ permission code review or new or correct way to do permissions 

Not much to do but I have include what has been done for mozContacts in the code.

Comment 142

[Donovan Preston [:fzzzy]]

Awesome, thank you so much Vivien!

Comment 143

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Also I have sent your patch to the try server and there is an error: 13342 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected interface name in global scope: TCPSocket

You may want to add TCPSocket to http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/test_interfaces.html?force=1

Comment 144

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

TEST-INFO | /Users/cltbld/talos-slave/test/build/xpcshell/tests/dom/network/tests/unit/test_tcpsocket.js | running test ...
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/dom/network/tests/unit/test_tcpsocket.js | test failed (with xpcshell return code: 3), see following log:
>>>>>>>
/Users/cltbld/talos-slave/test/build/xpcshell/tests/dom/network/tests/unit/test_tcpsocket.js:54: NS_ERROR_XPC_BAD_CID: Invalid ClassID or ContractID
<<<<<<<

I wonder if dom_socket.xpt/TCPSocket.js should be added to http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in similarly to b2g/installer/packages-manifest.in

Comment 145

[Honza Bambas (:mayhemer)]

(In reply to Donovan Preston from comment #138)
> Should send check the type of it's
> argument and raise if it is not a string or Uint8Array?

What websockets do?

(In reply to Donovan Preston from comment #139)
> > Here, if the status is a failure, you have to call onerror and also close
> > the socket.
> open connection
> write data
> server closes connection without reading data

Write big amount of data to make highly probable that server close comes sooner then we copy all out to the client socket side.

> > Yes, good change.  But with this code we have to check that when the output
> > stream gets closed too (e.g. by RST) we indicate the error and get to the
> > correct (CLOSED) state.
> 
> I need a test for this too. I think the test here is:
> 
> open connection
> ??? server closes connection non-cleanly
> 
> Not exactly sure how I can simulate the connection dropping, but I'll poke
> around and see if I can do it.

My concern is that when the server shuts its sending side down (here we get OnStop(OK)) that we correctly close our client socket when the server later shuts also its recv side down (which effectively closes the server socket) or just closes directly.

The suggested change in OnStopRequest of the copier is what may handle this well.

I don't think this is testable by Necko since we are just closing our sockets, we don't shut them down in one way and later the other way.  You simply don't have an API for this.


But, completely on the other hand, why don't we want to get onclose called after this point?  And also, since we don't support half-closing on our side, why do we allow send() be called after this point?  And what happens when async send of the buffered data to the socket is done?  

This code can also cause a race conditional behavior: when server shuts send down and we don't have anything buffered, then the socket will close on our side.  But when there is something, socket will hang.

I think it is a gap that client doesn't know the server stopped sending but is still receiving.  Maybe have oneof or so event?  Or call ondata with empty/null data as the usual socket api does (read() == 0 -> EOF) ?

Comment 146

[Honza Bambas (:mayhemer)]

> I think it is a gap that client ...
I think it is a gap that *socket consumer* ...

Comment 147

[Honza Bambas (:mayhemer)]

Re comment 134 in optimized build: original code: 2.5s, my code: 1.8s.  ~30% gain.

Comment 148

[Donovan Preston [:fzzzy]]

(In reply to Honza Bambas (:mayhemer) from comment #147)
> Re comment 134 in optimized build: original code: 2.5s, my code: 1.8s.  ~30%
> gain.

On my machine, before: 1.7 sec. After: 0.9 sec. I put your code in the next version of the patch in case an optimized C++ path is not available.

Comment 149

[Donovan Preston [:fzzzy]]

For the half-close and FIN cases, I'm going to write a little python server that will do half close, and a server that I kill -9 to test the FIN case. I'm a little uncomfortable that I can't write unit tests for these cases.

Comment 150

[Honza Bambas (:mayhemer)]

(In reply to Donovan Preston from comment #149)
> For the half-close and FIN cases, I'm going to write a little python server
> that will do half close, and a server that I kill -9 to test the FIN case.
> I'm a little uncomfortable that I can't write unit tests for these cases.

I think we could potentially integrate it.  

On the other hand, check for node.js and how it is used in mozilla tree for SPDY testing.  I don't say you can use it for testing if TCPSocket in the state it is now, but we could do some tweaks to allow it.  Better use something we already have then writing/adding a new code to the tree.

Comment 151

[Donovan Preston [:fzzzy]]

I wasn't going to add the python server to the test suite, just use it to test manually so I can actually see what happens in those cases.

That's very good to know that there's already node.js in the tree for SPDY testing. That seems like a good approach for testing half-close.

In my last comment, I meant RST, not FIN. I'm not sure how it's possible to test RST even with a node.js server, except by starting the server and then killing it non-cleanly. I'm continuing to investigate how to test that case.

Comment 152

[Robert Kaiser]

Also, as a note, Thunderbird people have this "fakeserver" stuff in their tests, AFAIK also their xpcshell-tests, where they implemented all kinds of email server interaction, I guess you could steal from that.

Comment 153

[Donovan Preston [:fzzzy]]

Ok, I did a bunch of tests of the server doing half-close and the server being killed in the middle of receiving large amounts of data.

If the server does a half-close of either the reading or the writing end, necko gives no notification whatsoever. Therefore TCPSocket will not be usable to talk to any protocols which require half close. This could be fixed later, but will require lower level necko changes.

If the server is killed while reading (the RST case), Honza's suggested change of checking the status in the onStopRequest of _ensureCopying catches the condition and I can call onerror followed by onclose.

Here is the python server I used to test this, to try it first use "easy_install eventlet" to get the eventlet dependency and then run the script:

-----

import socket
import eventlet

server = eventlet.listen(("127.0.0.1", 6942))

print "server started on 6942"

def handle(conn):
  line = ' '

  # This line is used for the half close tests
  #for x in range(3):

  # This line is used to test killing the server while receiving
  while line:

    line = conn.recv(1)
    print line
    conn.send(line)
    eventlet.sleep(2)

  # test half close, close read
  #conn.shutdown(socket.SHUT_RD)

  # test half close, close write
  #conn.shutdown(socket.SHUT_WR)

  # comment out this line for the half close tests
  conn.close()
  print "disconnected"

def run():
  while True:
    (conn, addr) = server.accept()
    print "connected", addr
    eventlet.spawn(handle, conn)

try:
  run()
except KeyboardInterrupt:
  print "exiting"

-----

The client test code was:

-----

var huge = new Array(512345*5);

for (var i = 0; i < huge.length; i++) {
  huge[i] = "a"
}

var sock = MozTCPSocket.open('localhost', 6942);

sock.onopen = function() {
  sock.send(huge.join(""));
}

-----

Comment 154

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Is it possible to do those tests has a followup? The test server infrastructure seems out of the scope of this bug.

Comment 155

[Donovan Preston [:fzzzy]]

I satisfied my own desire to know that the code does the right thing in the RST case by manually testing. It would be nice if I did not have to set up the infrastructure to automatically test this case since it requires some external server. It's up to Honza whether he's comfortable allowing this code without an automated test.

Comment 156

[Andrew Sutherland [:asuth] (he/him)]

Since this is moving to dom/ and it looks like it will be exposed into Firefox's content space too, is there going to be a failure mochitest to ensure that random webpages never have the ability to do random TCP stuff?  Specifically, I am proposing a test where a webpage in an unprivileged context tries to access the TCPSocket API and create a connection.  That would be a ridiculously dangerous regression to ever let happen, so it seems important to have an automated test.

Comment 157

[Donovan Preston [:fzzzy]]

(In reply to Andrew Sutherland (:asuth) from comment #156)
> Since this is moving to dom/ and it looks like it will be exposed into
> Firefox's content space too, is there going to be a failure mochitest to
> ensure that random webpages never have the ability to do random TCP stuff? 
> Specifically, I am proposing a test where a webpage in an unprivileged
> context tries to access the TCPSocket API and create a connection.  That
> would be a ridiculously dangerous regression to ever let happen, so it seems
> important to have an automated test.

Yes, that is an important test to have.

A mochitest would also be required to test the inner-window-destroyed stuff, I think.

Comment 158

[Donovan Preston [:fzzzy]]

Attachment: wip-0.12.2

0.12.2 changes:

- used self = this in _ensureCopying instead of passing in this
- in _ensureCopying onStopRequest, handled status being a failure (this happens when writing data to a socket that is closed uncleanly (RST))
- removed unneeded empty notifySSLError implementation
- passed BIG_TYPED_ARRAY to send in the tests instead of BIG_ARRAY
- made BIG_TYPED_ARRAY_2 and used it when sending twice to make sure the correct data was being sent at the correct time
- implementation files moved from b2g/components to dom/network (thanks vingtetun)
- added TCPSocket to mozilla-central/source/dom/tests/mochitest/general/test_interfaces.html
- added TCPSocket.js and TCPSocket.manifest to mozilla-central/source/browser/installer/package-manifest.in
- used (slightly) faster ArrayBuffer to string conversion code provided by Honza
- used inner-window-destroyed to set all the on* handlers to null and call close on all sockets related to a window to prevent garbage and "Cannot access dead object" errors

Comment 159

[Honza Bambas (:mayhemer)]

(In reply to Donovan Preston from comment #153)
> If the server does a half-close of either the reading or the writing end,
> necko gives no notification whatsoever. Therefore TCPSocket will not be
> usable to talk to any protocols which require half close. This could be
> fixed later, but will require lower level necko changes.

Are you sure it is necko that doesn't process it?  I believe shutting down the send send on the server has to cause read() == 0 on our side.  And the input stream pipe should process that.

(In reply to Donovan Preston from comment #155)
> It's up to Honza whether he's comfortable allowing this
> code without an automated test.

I think I am OK with that.  I wanted to have a basic test.  And we have.  Half-close states are hard to test, and it's better now to open this to the public and see what problems will show up.

Comment 160

[Donovan Preston [:fzzzy]]

(In reply to Honza Bambas (:mayhemer) from comment #159)
> (In reply to Donovan Preston from comment #153)
> > If the server does a half-close of either the reading or the writing end,
> > necko gives no notification whatsoever. Therefore TCPSocket will not be
> > usable to talk to any protocols which require half close. This could be
> > fixed later, but will require lower level necko changes.
> 
> Are you sure it is necko that doesn't process it?  I believe shutting down
> the send send on the server has to cause read() == 0 on our side.  And the
> input stream pipe should process that.

Ok, I will test a little more to see if I can see any event from the server doing half-close of the sending side.

> (In reply to Donovan Preston from comment #155)
> > It's up to Honza whether he's comfortable allowing this
> > code without an automated test.
> 
> I think I am OK with that.  I wanted to have a basic test.  And we have. 
> Half-close states are hard to test, and it's better now to open this to the
> public and see what problems will show up.

Ok, good. :-)

Comment 161

[Honza Bambas (:mayhemer)]

Comment on attachment 648935 [details] [diff] [review]
wip-0.12.2

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

Thanks Donovan.

r=honzab for the networking parts only.  It might still not be perfect, but I believe this is OK to land on m-c now.

You should find a reviewer for the permission checks, window management (call of the init method) and mainly the component integration (TCPSocket.manifest).  Those parts are *not* covered by my review.

Sent to try: https://tbpl.mozilla.org/?tree=Try&rev=a4160f6f110f

::: dom/network/src/Makefile.in
@@ +18,5 @@
> +
> +EXTRA_COMPONENTS = \
> +	TCPSocket.js \
> +  TCPSocket.manifest \
> +  $(NULL)

Check white spaces.

::: dom/network/src/TCPSocket.js
@@ +257,5 @@
> +  },
> +
> +  init: function ts_init(aWindow) {
> +    if (!Services.prefs.getBoolPref("dom.mozTCPSocket.enabled"))
> +      return null;

Maybe it's correct, but why returning null here?

@@ +275,5 @@
> +    ).getInterface(Ci.nsIDOMWindowUtils);
> +
> +    this.innerWindowID = util.currentInnerWindowID;
> +    LOG("window init: " + this.innerWindowID);
> +    Services.obs.addObserver(this, "inner-window-destroyed", true);

I'm no expert to how objects are created and how init() method is called, but I think you can move addObserver call to open() method and let each socket be an observer and let each socket check if (this.winID == destroyedWin.ID) and then delete/close (deref) it self.  Having an array is IMO an overkill.

@@ +557,5 @@
> +  },
> +
> +  QueryInterface: XPCOMUtils.generateQI([
> +    Ci.nsIBadCertListener2,
> +    Ci.nsISSLErrorListener,

nsISSLErrorListener has to be removed, I think.

::: dom/network/tests/unit/test_tcpsocket.js
@@ +403,5 @@
> +  sock.ondrain = yays.ondrain;
> +
> +  if (sock.send(BIG_TYPED_ARRAY_2)) {
> +    throw new Error("sock.send(BIG_TYPED_ARRAY_2) did not return false to indicate buffering");
> +  }

Nit: maybe reverse usage of ARRAY and ARRAY_2 to make the test a bit less confusing (I would expect to first send ARRAY and then ARRAY_2).

Comment 162

[Donovan Preston [:fzzzy]]

Comment on attachment 648935 [details] [diff] [review]
wip-0.12.2

Woohoo! r+! :-)

Fabrice can you review the non-network parts mentioned by Honza? Thanks.

Comment 163

[Honza Bambas (:mayhemer)]

Donovan, please also check results of the try build at https://tbpl.mozilla.org/?tree=Try&rev=a4160f6f110f, the test intermittently fails (send() doesn't always return false).  I think it is too strict condition and really the test just proves it it racy.

Comment 164

[Donovan Preston [:fzzzy]]

Ok. What would you suggest I do to make the test less racy? Increase the size of data being sent?

Comment 165

[Honza Bambas (:mayhemer)]

(In reply to Donovan Preston from comment #164)
> Ok. What would you suggest I do to make the test less racy? Increase the
> size of data being sent?

Check bufferedAmount before you start async copying.  If that doesn't help, then we have a problem.  Personally, as I think of this more, I start thinking of some hidden problem/race condition in lower level (Necko).  Depends, on how likely it is for the necko thread to copy 0.5MB of data to the localhost socket.  Maybe it is incredibly fast and thread scheduling really makes this happen.

I'll reverse the check and send the patch to try again.

Comment 166

[Donovan Preston [:fzzzy]]

Yes, I think checking bufferedAmount + current string size before copying should get rid of the race.

Right now, whether or not send returns true depends on how much the network thread was able to copy out of the multiplex stream between adding the string to the copier and the time that bufferedAmount is checked. If we check bufferedAmount + current string size before starting the current string copying, the tests should not race any more because the string being sent is always bigger than the "buffer size" in those tests.

Thanks

Comment 167

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #165)
> I'll reverse the check and send the patch to try again.

https://tbpl.mozilla.org/?tree=Try&rev=d9c921f0c0be

Comment 168

[Honza Bambas (:mayhemer)]

A new problem: https://tbpl.mozilla.org/php/getParsedLog.php?id=14233081&tree=Try&full=1#error0

Comment 169

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #167)
> (In reply to Honza Bambas (:mayhemer) from comment #165)
> > I'll reverse the check and send the patch to try again.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=d9c921f0c0be

And to refer, the only change made is on these lines (I'm stupid not to create an interdiff by accident, grrr..): https://hg.mozilla.org/try/file/d9c921f0c0be/dom/network/src/TCPSocket.js#l417

Comment 170

[Donovan Preston [:fzzzy]]

I think it would be better to save the "new buffer size" before calling appendStream. It's the lines between the call to appendStream and the check of bufferSize that is the race.

Before line 417:

let newBufferSize = this.bufferedAmount + data.length;

And then in the check that is on line 422 (but will be 423 with the above line added):

if (newBufferSize >= BUFFER_SIZE) {

This way the tests will always pass since data.length is always > BUFFER_SIZE in the tests that check the return value of send.

Comment 171

[Honza Bambas (:mayhemer)]

Both ways are correct.  But the test timeout might be related.  You should try to investigate what went wrong - the test or the code.

Comment 172

[Donovan Preston [:fzzzy]]

I don't think the code as you pushed it to try is correct, because _ensureCopying only starts the copying if it is not already started. In the case where copying is already running, the race still exists between the call to appendStream and the access of bufferedAmount.

It looks like both of these changes should be made, however, because in 0.12.2 _waitingForDrain is set *after* ensureCopying, which is racy as well.

I'm not sure why the timeout is happening.

I need to learn how to push to try so that I can test this myself. I'll whip up a version of the patch with your suggested changes and the newBufferSize implementation, and learn how to push to try.

Comment 173

[[:fabrice] Fabrice Desré]

Comment on attachment 648935 [details] [diff] [review]
wip-0.12.2

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

Mostly nits, apart form the way to observe window destruction. I'd like to see the next version.

::: b2g/chrome/content/shell.js
@@ +55,5 @@
>    let permissions = [
>      'indexedDB-unlimited', 'webapps-manage', 'offline-app', 'pin-app',
>      'websettings-read', 'websettings-readwrite',
>      'content-camera', 'wifi-manage', 'desktop-notification',
> +    'geolocation', 'device-storage', 'alarms', 'tcp-socket'

You'll notice when you'll rebase, but we just got rid of all this permission stuff in shell.js

If a app needs the permission, you need to add it on the gaia side

::: b2g/installer/package-manifest.in
@@ +485,5 @@
>  @BINPATH@/components/ActivityRequestHandler.js
>  @BINPATH@/components/ActivityWrapper.js
>  
> +@BINPATH@/components/TCPSocket.js
> +

You also need the .manifest

::: browser/installer/package-manifest.in
@@ +483,5 @@
>  @BINPATH@/components/ContactManager.manifest
>  @BINPATH@/components/AlarmsManager.js
>  @BINPATH@/components/AlarmsManager.manifest
> +@BINPATH@/components/TCPSocket.js
> +@BINPATH@/components/TCPSocket.manifest

Don't you need the dom_socket.xpt also?

::: dom/network/src/TCPSocket.js
@@ +236,5 @@
> +          self._ensureCopying();
> +        } else {
> +          if (self._waitingForDrain) {
> +            self._waitingForDrain = false;
> +            self.callListener("ondrain");          

nit: whitespace

@@ +257,5 @@
> +  },
> +
> +  init: function ts_init(aWindow) {
> +    if (!Services.prefs.getBoolPref("dom.mozTCPSocket.enabled"))
> +      return null;

Returning null is correct, this is what the backend code expect in such case.

@@ +266,5 @@
> +
> +    let perm = principal == secMan.getSystemPrincipal()
> +                 ? Ci.nsIPermissionManager.ALLOW_ACTION
> +                 : Services.perms.testExactPermissionFromPrincipal(principal, "tcp-socket");
> + 

nit: whitespace

@@ +275,5 @@
> +    ).getInterface(Ci.nsIDOMWindowUtils);
> +
> +    this.innerWindowID = util.currentInnerWindowID;
> +    LOG("window init: " + this.innerWindowID);
> +    Services.obs.addObserver(this, "inner-window-destroyed", true);

I agree with Honza

@@ +459,5 @@
> +      )
> +      while (this._suspendCount--) {
> +        this._inputStreamPump.suspend();
> +      }
> +      

nit: whitespace

::: dom/network/src/TCPSocket.manifest
@@ +1,4 @@
> +# TCPSocket.js
> +component {cda91b22-6472-11e1-aa11-834fec09cd0a} TCPSocket.js
> +contract @mozilla.org/tcp-socket;1 {cda91b22-6472-11e1-aa11-834fec09cd0a}
> +category JavaScript-global-property MozTCPSocket @mozilla.org/tcp-socket;1

change that to JavaScript-navigator-property mozTCPSocket

Comment 174

[Donovan Preston [:fzzzy]]

(In reply to Fabrice Desré [:fabrice] from comment #173)
> Comment on attachment 648935 [details] [diff] [review]
> wip-0.12.2
> 
> Review of attachment 648935 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: b2g/installer/package-manifest.in
> @@ +485,5 @@
> >  @BINPATH@/components/ActivityRequestHandler.js
> >  @BINPATH@/components/ActivityWrapper.js
> >  
> > +@BINPATH@/components/TCPSocket.js
> > +
> 
> You also need the .manifest
> 
> ::: browser/installer/package-manifest.in
> @@ +483,5 @@
> >  @BINPATH@/components/ContactManager.manifest
> >  @BINPATH@/components/AlarmsManager.js
> >  @BINPATH@/components/AlarmsManager.manifest
> > +@BINPATH@/components/TCPSocket.js
> > +@BINPATH@/components/TCPSocket.manifest
> 
> Don't you need the dom_socket.xpt also?

I tried adding this, but this caused the build to fail on the try server. Can someone who knows what they are doing here make the right changes so I don't have to guess?

Alternatively, is there some way for me to test locally that these changes are done correctly, so I can test before blindly sending to the try server and failing?

Comment 175

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Donovan Preston from comment #174)
> Alternatively, is there some way for me to test locally that these changes
> are done correctly, so I can test before blindly sending to the try server
> and failing?

The (full) log files from the run are pretty verbose about what is happening in them.  If you click on a build on https://tbpl.mozilla.org/?tree=Try&rev=42a4d4f2e27b and then one of the failed builds, there will be "view brief log" and "view full log" in the lower left.  If you start from the brief log (only shows error context), there should be a button at the top to upgrade to the full log.

The dividing lines have a lot of equals signs in them, you can search on that to browse around.  In this case, the failures happened for:
========= Started 'make package' failed (results: 2, elapsed: 4 secs) (at 2012-08-10 12:31:01.551031) =========

The other possible permutation with try builds is the setup of the mozconfig.  Near the start of the build process 'cat .mozconfig' gets run, which will show you how the build is configured.  #developers is a reasonable place to ask questions like this if the wiki's aren't proving fruitful.

Comment 176

[Donovan Preston [:fzzzy]]

Thanks for the cat .mozconfig hint. I think if I try doing a local build with that mozconfig, then it should go through the same failing packaging steps locally so I can repro the failure and have a chance of understanding why it's failing.

Comment 177

[Donovan Preston [:fzzzy]]

Attachment: wip-0.13.3

I had asuth run the xpcshell tests three times on try with this latest patch, and the races seem to have been fixed. The 32 bit lunix boxes seem way slow and don't even seem to be starting the test runs though.

https://tbpl.mozilla.org/?tree=Try&rev=606c7668c2fe

wip-0.13.3 changes:

- Fixed race condition by storing bufferedAmount in a local variable
  before calling appendStream.
  - Previously, depending on how quickly the networking thread
    drained the stream between the call to appendStream and the
    check of bufferedAmount on the next line, the tests would race
    occasionally and fail.
- Fixed whitespace in dom/network/src/Makefile.in.
- Changed the way addObserver is called to watch for
  inner-window-destroyed so that addObserver is called for each
  socket, instead of once for each window.
- Removed nsISSLErrorListener.
- Reversed usage of ARRAY and ARRAY_2 in the test to make it less
  confusing.
- Removed fragment that modified obsolete permissions list in
  shell.js
- Messed around with the package-manifest.in files until they were
  correct
- Fixed whitespace in TCPSocket.js
- Changed from JavaScript-global-property MozTCPSocket to
  JavaScript-navigator-property mozTCPSocket
- Added mochitests for the three cases:
  - Ensure that when the dom.mozTCPSocket.enabled preference is
    false, accessing navigator.mozTCPSocket raises an error.
  - Ensure that when the preference is true, but the content does
    not have the tcp-socket permission, calling open raises an
    error.
  - Ensure that when the preference is true, and the content has
    the tcp-socket permission, calling open works.

Comment 178

[[:fabrice] Fabrice Desré]

Comment on attachment 652573 [details] [diff] [review]
wip-0.13.3

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

r=me with nits addressed.

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +4,5 @@
> +
> +/**
> + * MozTCPSocket exposes a TCP client socket (no server sockets yet)
> + * to highly privileged apps. It provides a buffered, non-blocking
> + * interface for sending. For receiving, it uses an asynchronous, 

Nit: whitespace

@@ +211,5 @@
> +   *
> +   * In the onerror callback, data will be a string with a description
> +   * of the error.
> +   *
> +   * In the other callbacks, data will be an empty string. 

Nit: whitespace

::: dom/network/src/TCPSocket.js
@@ +263,5 @@
> +                   .getService(Ci.nsIScriptSecurityManager);
> +
> +    let perm = principal == secMan.getSystemPrincipal()
> +                 ? Ci.nsIPermissionManager.ALLOW_ACTION
> +                 : Services.perms.testExactPermissionFromPrincipal(principal, "tcp-socket");

Note that the permission name is changing to networktcp (see https://bugzilla.mozilla.org/show_bug.cgi?id=783716)

@@ +264,5 @@
> +
> +    let perm = principal == secMan.getSystemPrincipal()
> +                 ? Ci.nsIPermissionManager.ALLOW_ACTION
> +                 : Services.perms.testExactPermissionFromPrincipal(principal, "tcp-socket");
> + 

Nit: whitespace

@@ +448,5 @@
> +
> +      while (this._suspendCount--) {
> +        this._inputStreamPump.suspend();
> +      }
> +      

Nit: whitespace

Comment 179

[Donovan Preston [:fzzzy]]

Attachment: wip-0.13.4

wip-0.13.3 with whitespace nits fixed. I left the permission tcp-socket for now until the discussion in 783716 is resolved.

Comment 180

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c39fdebf386

Comment 181

[Ed Morley [:emorley]]

Backed out for native Android m3 failures in test_tcpsocket_enabled_no_perm.html:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3c39fdebf386

https://hg.mozilla.org/integration/mozilla-inbound/rev/4814d0570ca2

Comment 182

[Donovan Preston [:fzzzy]]

Anyone have any clue why this would fail on Android? It didn't fail when we pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=606c7668c2fe

Comment 183

[[:fabrice] Fabrice Desré]

(In reply to Donovan Preston from comment #182)
> Anyone have any clue why this would fail on Android? It didn't fail when we
> pushed to try:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=606c7668c2fe

I think we forgot to add the .js and manifest to mobile/android/installer/package-manifest.in

Comment 184

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #183)
> (In reply to Donovan Preston from comment #182)
> > Anyone have any clue why this would fail on Android? It didn't fail when we
> > pushed to try:
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=606c7668c2fe
> 
> I think we forgot to add the .js and manifest to
> mobile/android/installer/package-manifest.in

Should it also need to be added to mobile/xul/installer/package-manifest.in ?

Comment 185

[[:fabrice] Fabrice Desré]

(In reply to Vivien Nicolas (:vingtetun) from comment #184)
> (In reply to Fabrice Desré [:fabrice] from comment #183)
> > (In reply to Donovan Preston from comment #182)
> > > Anyone have any clue why this would fail on Android? It didn't fail when we
> > > pushed to try:
> > > 
> > > https://tbpl.mozilla.org/?tree=Try&rev=606c7668c2fe
> > 
> > I think we forgot to add the .js and manifest to
> > mobile/android/installer/package-manifest.in
> 
> Should it also need to be added to mobile/xul/installer/package-manifest.in ?

yes

Comment 186

[Donovan Preston [:fzzzy]]

Oh, good. That makes sense. I'm still confused why it didn't show up in the push to try though.

I'm going to be on a plane tomorrow, so I won't be able to hack on it, but I will tackle it Thursday (if nobody else wants to do it while I am in the air ;-)

Comment 187

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3764a9891e74

Comment 188

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 653364 [details] [diff] [review]
wip-0.13.4

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

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +114,5 @@
> +  readonly attribute DOMString readyState;
> +  readonly attribute DOMString CONNECTING;
> +  readonly attribute DOMString OPEN;
> +  readonly attribute DOMString CLOSING;
> +  readonly attribute DOMString CLOSED;

You need to drop those constant attributes.

Comment 189

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3764a9891e74

Comment 190

[Donovan Preston [:fzzzy]]

(In reply to :Ms2ger from comment #188)
> Comment on attachment 653364 [details] [diff] [review]
> wip-0.13.4
> 
> Review of attachment 653364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/interfaces/nsIDOMTCPSocket.idl
> @@ +114,5 @@
> > +  readonly attribute DOMString readyState;
> > +  readonly attribute DOMString CONNECTING;
> > +  readonly attribute DOMString OPEN;
> > +  readonly attribute DOMString CLOSING;
> > +  readonly attribute DOMString CLOSED;
> 
> You need to drop those constant attributes.

I don't understand, what do you mean? What should be done instead?

Comment 191

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Keep the readyState property, but CONNECTING/OPEN/CLOSING/CLOSED can be removed. People should just write code like |readyState == "connecting"| or |readyState != "open"| instead. This is what javascript normally looks like. Constants like TCPSocket.CONNECTING is more C-like syntax.

Comment 192

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry for not looking at this interface sooner. In general it looks awesome!!

However I don't understand the nsITCPSocketEvent interface. Why aren't we using normal DOM Events?

If the reason is that we can't implement DOMEvents in JS, then we should make sure that nsITCPSocketEvent looks as much as possible as nsIDOMEvent. That would mean renaming "source" to "target", and making sure that the "type" property returns strings like "open", "error", "data", "drain", "close". I.e. the "on"-prefix should not be included in .type.

Should I file a separate bug on this?

Comment 193

[Donovan Preston [:fzzzy]]

(In reply to Jonas Sicking (:sicking) from comment #191)
> Keep the readyState property, but CONNECTING/OPEN/CLOSING/CLOSED can be
> removed. People should just write code like |readyState == "connecting"| or
> |readyState != "open"| instead. This is what javascript normally looks like.
> Constants like TCPSocket.CONNECTING is more C-like syntax.

Hmm, I'm not sure I agree with that. What if someone types "conecting" accidentally? It could be a difficult thing to notice since the code will just not work silently, while TCPSocket.CONECTING will immediately cause an error.

Also, there's a precedent with the way XMLHttpRequest works... I admit that is not necessarily a great precedent to follow, though :-)

Comment 194

[Donovan Preston [:fzzzy]]

(In reply to Jonas Sicking (:sicking) from comment #192)
> Sorry for not looking at this interface sooner. In general it looks awesome!!
> 
> However I don't understand the nsITCPSocketEvent interface. Why aren't we
> using normal DOM Events?
> 
> If the reason is that we can't implement DOMEvents in JS, then we should
> make sure that nsITCPSocketEvent looks as much as possible as nsIDOMEvent.
> That would mean renaming "source" to "target", and making sure that the
> "type" property returns strings like "open", "error", "data", "drain",
> "close". I.e. the "on"-prefix should not be included in .type.
> 
> Should I file a separate bug on this?

Glad you think it looks good! For some reason I thought you had looked at it earlier. I guess I should not assume.

We should absolutely make the events look as much like DOM events as possible. I just didn't think about it before. Yes, please file another bug to do that, along with removing the CONNECTING etc constants if you still think that we should do that.

If we remove CONNECTING etc, should we just remove readyState as well? It's not really needed, the callbacks cover the state space nicely.

Comment 195

[Dave Hylands [:dhylands]]

(In reply to Donovan Preston from comment #193)
> (In reply to Jonas Sicking (:sicking) from comment #191)
> > Keep the readyState property, but CONNECTING/OPEN/CLOSING/CLOSED can be
> > removed. People should just write code like |readyState == "connecting"| or
> > |readyState != "open"| instead. This is what javascript normally looks like.
> > Constants like TCPSocket.CONNECTING is more C-like syntax.
> 
> Hmm, I'm not sure I agree with that. What if someone types "conecting"
> accidentally? It could be a difficult thing to notice since the code will
> just not work silently, while TCPSocket.CONECTING will immediately cause an
> error.
> 
> Also, there's a precedent with the way XMLHttpRequest works... I admit that
> is not necessarily a great precedent to follow, though :-)

I also don't like using hard-coded strings. I was burned by this recently where somebody modified an event changing 'locked' to 'lock' but didn't modify all of the listeners, and my code no longer picked up the event.

If using hard-coded strings/numbers is bad in C++, why shouldn't it be just as bad in JS?

Comment 196

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

As C developers, we can have whatever opinion we want. But the feedback that I always receive from web developers is that string values is preferable to Interface.CONSTANTS

Remember that misspelling an Interface.CONSTANT doesn't actually produce a built-time error. Instead the expression simply returns false and so the effect of

mySocket.readyState == TCPSocket.CONTECTING

is exactly the same as

mySocket.readyState == "conecting"

So you don't actually get any more help from the platform with your debugging.

Comment 197

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

.readyState is still useful though, again, based on feedback from developers. It's useful if you are handed an object and don't know if 'onopen' has fired yet or not.

Comment 198

[David Bruant]

Here is an MDN documentation page:
https://developer.mozilla.org/en-US/docs/DOM/TCPSocket
Can someone please take the time to review it to make sure no important thing is missing or wrong?

Thanks

Comment 199

[Josh Matthews [:jdm]]

It looks complete to me. You may want to mention that resume can throw if used out of order (ie. resume without corresponding  suspend).

Comment 200

[David Bruant]

(In reply to Josh Matthews [:jdm] from comment #199)
> It looks complete to me. You may want to mention that resume can throw if
> used out of order (ie. resume without corresponding  suspend).
Fixed thanks.

I was wondering, there are a couple of comments about improving the API (replacing state constants with strings, making the constructor works, inherit from EventTarget, etc.) Is there a follow-up bug for that?

Comment 201

[Jason Duell]

:jdm:  Can sockets do nested suspend/resume (like most/all nsIChannels)?  If so might be worth adding "suspend may be called multiple times: the socket won't be resumed until the same number of resume calls have been made".

Comment 202

[Josh Matthews [:jdm]]

All is does is call suspend/resume on the input pump, so I presume the answer is yes.

Comment 203

[Donovan Preston [:fzzzy]]

(In reply to David Bruant from comment #198)
> Here is an MDN documentation page:
> https://developer.mozilla.org/en-US/docs/DOM/TCPSocket
> Can someone please take the time to review it to make sure no important
> thing is missing or wrong?
> 
> Thanks

This looks fantastic! Thanks!

I only have one comment: In the documentation for drain, it says "More data can be buffered in the socket." However more data can always be buffered in the socket; drain just lets you know when the previously buffered data has all been written. I think it would be clearer if it said "All previously buffered data has been written to the network" or something similar instead.

Comment 204

[Honza Bambas (:mayhemer)]

(In reply to Donovan Preston from comment #203)
> (In reply to David Bruant from comment #198)
> > Here is an MDN documentation page:
> > https://developer.mozilla.org/en-US/docs/DOM/TCPSocket
> > Can someone please take the time to review it to make sure no important
> > thing is missing or wrong?
> > 
> > Thanks
> 
> This looks fantastic! Thanks!
> 
> I only have one comment: In the documentation for drain, it says "More data
> can be buffered in the socket." However more data can always be buffered in
> the socket; drain just lets you know when the previously buffered data has
> all been written. I think it would be clearer if it said "All previously
> buffered data has been written to the network" or something similar instead.

Yes, that is right.  This event might cause the biggest confusion so we should take care to explain it, maybe like:

Data can be pushed to the socket whenever a consumer wants, they are internally buffered.  However, when sending large amount of data, like video or audio files or any larger content, pushing all data to the socket in a simple loop may cause out-of-memory errors.  To void it, use ondrain event.  The ondrain event is called whenever some data had actually been sent out through the native TCP socket and the socket internal buffered is available to consume more data.  

Example:

const kSegmentSize = 128 * 1024;
var data = myFileStream.read(kSegmentSize);
socket.send(data);
socket.ondrain = function() {
  data = myFileStream.read(kSegmentSize);
  if (data.length)
    socket.send(data);
  else {
    alert("Data send complete!");
    // optionally close the socket
    socket.close();
  }
}


Please fix my bad english and any other aspects ;)

Comment 205

[Jason Duell]

Honza, any advice we can give about how much data is a reasonable/safe amount to send before they should wait for ondrain()?  50K, 5MB?  I assume on phone we don't want large buffering.  But we want to make sure we don't hit network perf by suggesting too small a buffer.

Comment 206

[Honza Bambas (:mayhemer)]

Actually, I forgot about one aspect of the API.

We shouldn't say anything.  The buffer limit can be driven internally and transparently to the consumer by the socket:

The send() method returns whether the buffer is *not* "full":

- when we have buffered more then 64kB send() returns false (i.e. buffer is "full" -> "consumer, stop sending and wait for ondrain!"))
- when less, returns true (i.e. "consumer, feel free to push more now, you won't get any ondrain anyway!")

So, the example, is actually wrong and should be:

const kMyBufferSize = 128 * 1024;
do {
  var data = myStream.read(kMyBufferSize);
} while (socket.send(data));

socket.ondrain = function() {
  var data = myStream.read(kMyBufferSize);
  socket.send(data);
}

This is cumbersome, and I was actually proposing to have a method that takes a some JS content stream API we have (do we actually have something like that?) and copies it to the socket.  It is easy to do with the current implementation, w/o any memory concerns.

However, API "simplicity" won and we have just the send() method returning true/false.

Usually, in EU at least, uploads on ADSL are about 50kB, on 3G (poor provider) are about 40kB.  This means we have to loop the JS code every ~2s with a 128kB buffer limit.

Comment 207

[Mike Habicher [:mikeh] (high bugzilla latency)]

A couple of comments:

(In reply to Honza Bambas (:mayhemer) from comment #204)
> 
> Example:
> 
> const kSegmentSize = 128 * 1024;
> var data = myFileStream.read(kSegmentSize);
> socket.send(data);
> socket.ondrain = function() {
>   data = myFileStream.read(kSegmentSize);
>   if (data.length)
>     socket.send(data);
>   else {
>     alert("Data send complete!");
>     // optionally close the socket
>     socket.close();
>   }
> }

Does this buffering occur only in the underlying socket?  If so, this is a bad usage model if we want to be able to maximize channel throughput.  From what I've read, .ondrain() will trigger when the TCP send buffer is empty, which only happens once the remote peer has ACKed all of our outgoing data.  What this means is that from the time this condition is detected until the time we hit the socket.send() call, the channel is _idle_.

A better approach would be to have an (e.g.) .onavailable() callback that will trigger an event when room becomes available in the TCP send buffer; socket.send() can then grab enough 'data' to refill the buffer to keep the channel running at full capacity.

(In reply to Honza Bambas (:mayhemer) from comment #206)
>
> However, API "simplicity" won and we have just the send() method returning true/false.

It might be more useful to have send() return the number of octets that were accepted into the socket.  That would at least let the client gauge the bandwidth of the channel.

> Usually, in EU at least, uploads on ADSL are about 50kB, on 3G (poor
> provider) are about 40kB.  This means we have to loop the JS code every ~2s
> with a 128kB buffer limit.

We'll empty the buffers a lot faster on WiFi, though.  On 802.11g (maximum goodput of 26Mbps) we'll empty 128KiB in ~50ms.

Comment 208

[Honza Bambas (:mayhemer)]

(In reply to Mike Habicher [:mikeh] from comment #207)
> A couple of comments:
> 
> (In reply to Honza Bambas (:mayhemer) from comment #204)
> > 
> > Example:
> > 
> > const kSegmentSize = 128 * 1024;
> > var data = myFileStream.read(kSegmentSize);
> > socket.send(data);
> > socket.ondrain = function() {
> >   data = myFileStream.read(kSegmentSize);
> >   if (data.length)
> >     socket.send(data);
> >   else {
> >     alert("Data send complete!");
> >     // optionally close the socket
> >     socket.close();
> >   }
> > }
> 
> Does this buffering occur only in the underlying socket?  

If underlying socket == the js TCPSocket, then yes.  It's buffering the data and that buffer is directly put to the OS socket.

> If so, this is a
> bad usage model if we want to be able to maximize channel throughput.  From
> what I've read, .ondrain() will trigger when the TCP send buffer is empty,

Not that exactly.  It is called when one piece of data added to the buffer by send() is sent out.

> which only happens once the remote peer has ACKed all of our outgoing data. 

Not exactly.  Not all data needs to be acked to free some of the OS buffer.  Depends also on how rwin is driven by OS, which is different on every platform.

> What this means is that from the time this condition is detected until the
> time we hit the socket.send() call, the channel is _idle_.

It may not.  We let send() return false (indicating that the buffer is getting filled too much) only after we've buffered some 64k.  We can also make this number larger.  Anyway, when sending out in chunks smaller then 64k, you get a reasonable piplining.  We should document this somewhere.

> 
> A better approach would be to have an (e.g.) .onavailable() callback that
> will trigger an event when room becomes available in the TCP send buffer;
> socket.send() can then grab enough 'data' to refill the buffer to keep the
> channel running at full capacity.

That is what we have mostly.  You don't need to bother with how much is avail in the buffer, you may put whatever amount to send() and it will just tell you whether the buffer is over some reasonable limit or not.

> 
> (In reply to Honza Bambas (:mayhemer) from comment #206)
> >
> > However, API "simplicity" won and we have just the send() method returning true/false.
> 
> It might be more useful to have send() return the number of octets that were
> accepted into the socket.  That would at least let the client gauge the
> bandwidth of the channel.

That is what we didn't want to do, every developer would have to write the complicated handling code again and again and it would be very problematic for less experienced developers that just want to send out and get response.  I was thinking of a fully async way to send data out - like socket.send(callback(socket) { socket.write(data, length); } or register a stream, even async, to send out.  But that is too complicated for sending out just small messages for 1RTT req/resp communication that can be easily buffered.

However, we can have two apis when needed.  I don't see a reason why a js consumer/client should take care of controling bandwidth.

> 
> > Usually, in EU at least, uploads on ADSL are about 50kB, on 3G (poor
> > provider) are about 40kB.  This means we have to loop the JS code every ~2s
> > with a 128kB buffer limit.
> 
> We'll empty the buffers a lot faster on WiFi, though.  On 802.11g (maximum
> goodput of 26Mbps) we'll empty 128KiB in ~50ms.

If it turns out the buffered amount should be larger, we can do it.  It can also be a self-determining heuristically.

Mike - feel free to file bugs.

Comment 209

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, if you think we need abilities to have more fine grained control over sending/receiving data, we need to make sure those capabilities makes it into the standard that's currently being developed based on the current API.

So either reach out to the SysApps WG. Or cc Mounir, Marcos or me and we can help with the outreach.

Discussions on the dev-webapi list is also a good channel.

Comment 210

[Marco Castelluccio [:marco]]

Could I use this API on desktop?

Comment 211

[Donovan Preston [:fzzzy]]

(In reply to Marco Castelluccio [:marco] from comment #210)
> Could I use this API on desktop?

Yes. You need to go to about:config and set dom.mozTCPSocket.enabled to true. Then you also need the tcp-socket permission for the document; see test_tcpsocket_enabled_with_perm.html for details.

Comment 212

[Richard Maher]

Attachment: Tier3Client.js

Bummer! And there's me thinking I was gonna make a lot of money out of this :-(

Is there a MozSocket-Reference somewhere that contrasts this an the BSD interface?

Does the FF API allow multiple tabs to share a Socket?

Ah least we all agree that WebSockets were/are a very bad joke.

Cheers Richard Maher

PS. OUTSTANDING work on the Web App install stuff! You guys/gals are killing it!

Comment 213

[Richard Maher]

Sorry Fabrice, I don't know if your attachment was in response to me or another poster but it appears I stomped on it. Sorry. 

Could you post it again?

Cheers Richard Maher

Comment 214

[[:fabrice] Fabrice Desré]

I just change the mime type on the attachment to let it be readable in bugzilla. We can all move along...