797561 - Expose a server tcp socket API to web applications

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, enhancement)

Description

[Donovan Preston [:fzzzy]]

It should be fairly trivial to add a navigator.mozTCPSocket.listen API which adds a bit of glue to enhance the existing client socket api with server socket support. I have a contributor lined up to write this patch, so I'm opening this bug to track it.

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIServerSocket

Add a "listen(port, [backlog])" method to the TCPSocket object, which creates an nsIServerSocket, calls init on it, and calls asyncListen. The listen method also needs to create and return a new ServerSocket object that has only an "onaccept" callback. Then implement onSocketAccepted, which is called because of asyncListen. In onSocketAccepted, wrap the passed nsISocketTransport object in a TCPSocket object, and pass it to the callback. 

There will also be some additional IPC glue required.

Comment 1

[[:fabrice] Fabrice Desré]

Do we really need such an API? can't we use webRTC data channels instead?

Comment 2

[Honza Bambas (:mayhemer)]

Just a note: you cannot support TLS over TCP, PSM and Necko have no support for SSL server sockets and it is not that simple to add it, also regarding how relatively complicated it is to setup an SSL server certificates and keys.

Comment 3

[Donovan Preston [:fzzzy]]

WebRTC would be great, when is it going to land?

Even if this did not support SSL and is replaced by WebRTC the patch should be simple enough that it would be nice to have. The socket would have exactly the same interface and implementation as the client socket once the connection has been accepted.

Comment 4

[[e-mail]]

If there were no networks,two peers, one is a PC host and the other is a smart phone.
They could communicate with usb cable by creating a TCP client on PC host and a TCP server app on smart phone via "adb", if we had such an api.

Can webRTC do work of this kind?

Comment 5

[Donovan Preston [:fzzzy]]

Are you talking about adb forward? If webRTC was opening a TCP socket on the phone, I think adb forward should be able to work just as well with webRTC as this TCPSocket.listen api.

Someone who knows a bit more about webrtc than me should probably comment, though.

Comment 6

[[e-mail]]

Yes, you are right Donovan, I'm talking about "adb forward". If we can create a local TCP server socket on the phone, all would be OK.
I have no idea whether webRTC could do this, of course, it would be OK by adding the TCPSOcket.listen api.

Comment 7

[[e-mail]]

Hi Donovan, regarding "whether we can create a local TCP server socket with webRTC"
I have chatted with Justin Uberti, his answer is "No". Here is the discussion
https://groups.google.com/forum/?fromgroups=#!topic/discuss-webrtc/wDzxHTgyb1g

Comment 8

[Donovan Preston [:fzzzy]]

Yes, now that I know more about how WebRTC works it is clear that a server-side TCP socket would be significantly more capable. Thanks.

Comment 9

[James Ho]

Donovan, are you going to take ths bug now?

Comment 10

[[:fabrice] Fabrice Desré]

I'm still not convinced we should do that at all. Even if webRTC doesn't offer the same level of control over the server setup, what are the use cases we care with here?

Comment 11

[[e-mail]]

I think it's very important to add such an api. Most Android users in Asia market, they are used to backup contact list, videos, musics etc.. to PC host.

There are many third parties who provide such backup tools for Android. Now, we cooperate with one of them, of course, for Firefox OS.  But, we found out it maybe impossible to do it without such an api.

Comment 12

[Donovan Preston [:fzzzy]]

There are many, many iOS apps which run HTTP servers. Because iOS did not provide for a way for apps to get files on/off the phone, many iOS apps will provide a web interface that can be used to control the application and download large files out of the app or upload large files to the app.

People are already familiar with the concept and it really does make sense.

Comment 13

[[e-mail]]

Donovan, are you going to take it? or maybe I can take a try.

Comment 14

[Andreas Gal :gal]

This is not a blocker. We have more urgent work to work on.

Comment 15

[Donovan Preston [:fzzzy]]

There's a contributor who has already written a patch. I asked him to sign up for bugzilla and take this bug.

Comment 16

[Tomoaki Konno]

Attachment: Patch for creating a TCP server socket

This is a work in progress.
This bugs has been tested on Marionette environment.
But, it is not fully implemented so that TCP server socket can be used on Gaia environment.

I have already started to address an additional implementation for the Gaia environment, and I will also posted the new version here.

Comment 17

[Jason Smith [:jsmith]]

Comment on attachment 685037 [details] [diff] [review]
Patch for creating a TCP server socket

Please don't set flags that don't align with your intentions.

Comment 18

[Honza Bambas (:mayhemer)]

Please add to your .hgrc and resubmit:

[defaults]
diff=-U 8 -p
qdiff=-U 8

[diff]
git=true
showfunc=true
unified=8

Comment 19

[Tomoaki Konno]

Attachment: Patch for creating a TCP server socket

I resubmitted the patch for creating a TCP server socket.
Since I use git repository, the patch file was made with the following command.

$ git diff -U8 --patience -M -C
(https://wiki.mozilla.org/B2G/Hacking)

Comment 20

[Tomoaki Konno]

Attachment: Patch to use a TCP server socket from GAIA application

Comment 21

[Brian Birtles (:birtles)]

Kanno-san, if you're ready for review you should request a review from a particular person. Donovan, can you suggest a suitable reviewer?

Comment 22

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #21)
> Kanno-san, if you're ready for review you should request a review from a
> particular person. Donovan, can you suggest a suitable reviewer?

Sorry, Konno-san!

Comment 23

[Donovan Preston [:fzzzy]]

I could review it but not until next week or after because of the holidays. If someone else were to review it I would recommend Honza or someone else from the network team

Comment 24

[Tomoaki Konno]

Thanks for your supports, Brian and Donovan.
Could I request a review for this patch from Donovan, Honza or someone else?
I'm glad if I receive a result of the review by early or mid next month.

Comment 25

[Honza Bambas (:mayhemer)]

I can do the review after the new year.

Comment 26

[Tomoaki Konno]

Thanks, Honza.
I'll register your name as a reviewer.

Comment 27

[Donovan Preston [:fzzzy]]

Thanks Honza! I appreciate it.

Comment 28

[Honza Bambas (:mayhemer)]

Comment on attachment 694715 [details] [diff] [review]
Patch to use a TCP server socket from GAIA application

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

Good work, but I'd like some updates.

For now r- just to be able to refer the review comments.

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +34,5 @@
> + *
> + * An interface to a server socket that can accept incoming connections for gaia apps.
> + */
> +[scriptable, uuid(821638a1-5327-416d-8031-668764f2ec04)]
> +interface nsIDOMTCPServerSocket : nsIServerSocket

Do you really need this to derive from nsIServerSocket?  I cannot see the actual need.

@@ +126,5 @@
> +   *                Pass -1 to use the default value.
> +   *
> +   * @return The new TCPServerSocket instance.
> +   */
> +  nsIDOMTCPServerSocket listen(in unsigned short port, [optional] in jsval options, [optional] in unsigned short backlog);

Probably put this under open method declaration.

::: dom/network/interfaces/nsITCPServerSocketParent.idl
@@ +13,5 @@
> +{
> +  // Trigger a callback in the content process for |type|, providing a serialized
> +  // argument of |data|, and update the child's readyState and bufferedAmount values
> +  // with the given values.
> +  [implicit_jscontext] void sendCallbackAccept(in uint16_t port,

Comment is wrong.

::: dom/network/interfaces/nsITCPSocketParent.idl
@@ +38,5 @@
> +              in unsigned short port, in unsigned short backlog,
> +              in DOMString binaryType);
> +
> +  // Post-processing in closing a server socket
> +  void postProcCloseServer(in unsigned short port);

listenCleanup ? up to you.

::: dom/network/src/PTCPServerSocket.ipdl
@@ +24,5 @@
> +  RequestDelete();
> +
> +child:
> +  CallbackAccept(uint16_t port, uint16_t socketID);
> +  //CallbackError(JSError error);

remove commented code.

::: dom/network/src/TCPServerSocketChild.h
@@ +33,5 @@
> +  bool mIPCOpen;
> +};
> +
> +class TCPServerSocketChild : public mozilla::net::PTCPServerSocketChild
> +                           , public TCPServerSocketChildBase

I don't say it's wrong, just want to ask why you need to go through the Base class and directly derive from your interface?

::: dom/network/src/TCPSocket.js
@@ +81,5 @@
>  /*
> + * nsIDOMTCPServerSocket object
> + */
> +
> +function TCPServerSocket() {

Could this live in its own js file?

@@ +103,5 @@
> +  get onaccept() {
> +    return this._onaccept;
> +  },
> +  set onaccept(f) {
> +    this._onaccept=f;

_onaccept = f

@@ +106,5 @@
> +  set onaccept(f) {
> +    this._onaccept=f;
> +  },
> +
> +  _initAcceptedSocket: function tss_initAcceptedSocket(transport, options) {

{ on a new line

@@ +120,5 @@
> +    // If the other side is not listening, we will
> +    // get an onInputStreamReady callback where available
> +    // raises to indicate the connection was refused.
> +    
> +    if (that._binaryType === "arraybuffer") {

From this line and on you are duplicating part of the existing open method.  You should have some openInternal or whatever called method that will let accept and open share these particular lines.

@@ +161,5 @@
> +      this._tcpsocklist.push(tcpsock);
> +      this._onaccept(tcpsock);
> +    }
> +    else {
> +      dump("Received unexpected connection!");

The accepted conn should be closed.

@@ +176,5 @@
> +    that._readyState = kOPEN;
> +    that._socketBridge = Cc["@mozilla.org/tcp-socket-child;1"]
> +                            .createInstance(Ci.nsITCPSocketChild);
> +    that._socketBridge.open(that, "openAccepted:" + socketID, port, false, "",
> +                            this._useWin, that);

|this._useWin, that| are unexpected.

@@ +189,5 @@
> +      this._serverBridge.close();
> +      return;
> +    }
> +
> +    /*Close accepted TCPSockets*/

spaces after /* and before */

@@ +192,5 @@
> +
> +    /*Close accepted TCPSockets*/
> +    for (var i = 0; i < this._tcpsocklist.length; i++) {
> +      this._tcpsocklist[i].close();
> +    }

Are you sure you want to do this?  What if you just want to stop the server from listening but want to let the already established connections alive (if even a bit longer)?

I don't much agree with this.

@@ +201,5 @@
> +  // nsIServerSocketListener (Triggered by _listener.asyncListen)
> +  onSocketAccepted: function tss_onSocketAccepted(server, trans) {
> +    // precondition: this._inChild == false
> +    var tcpsock = this._initAcceptedSocket(trans, this._options);
> +    this._callListenerAcceptCommon(tcpsock)

When any of this throws, you should close the accepted socket.

@@ +216,5 @@
> +    classID: Components.ID("{73065eae-27dc-11e2-895a-000c29987aa2}"),
> +    classDescription: "Server TCP Socket",
> +    interfaces: [
> +      Ci.nsIDOMTCPServerSocket,
> +      Ci.nsIDOMGlobalPropertyInitializer,

Where is this interface implemented?

@@ +217,5 @@
> +    classDescription: "Server TCP Socket",
> +    interfaces: [
> +      Ci.nsIDOMTCPServerSocket,
> +      Ci.nsIDOMGlobalPropertyInitializer,
> +      Ci.nsIObserver,

As well this one?

@@ +609,5 @@
> +
> +      that._serverBridge = Cc["@mozilla.org/tcp-server-socket-child;1"]
> +                             .createInstance(Ci.nsITCPServerSocketChild);
> +      that._serverBridge.listen(that, port, backlog || -1, binaryType);
> +	}

indent

::: dom/network/src/TCPSocketParentIntermediary.js
@@ +22,5 @@
> +
> +    // If aHost is set as 'openAccepted:xxx' instead of host name (e.g. localhost), 
> +    // an accepted socket object created by listen()is returned
> +    //('xxx' is an index of socket list).
> +    if (0 != aHost.indexOf('openAccepted')) {

Hmm.. what happens when I want to connect openAccepted.example.com?

You should at least include the colon, thus search for 'openAccepted:'

However, please, introduce a new method 'accept' on the intermediary class that will handle this.  Pass just the socket id, no magic.

@@ +32,5 @@
> +    }
> +    else {
> +      var socketID = new Number(aHost.split(':')[1]);
> +
> +      socket = this._socket = port2AcceptedSocketList[aPort][socketID];

So, here it is the last place you need the socket mapping in port2AcceptedSocketList.  You should delete it otherwise you will let the socket waste memory even after the actual connection has closed.  The server socket may accept a lot of connections and live a long time.

You should have a unique autoincrementing number that will create the socketID.  Referring by order in the array is anyway not a good practice.

@@ +62,5 @@
> +    if (!serverSocket)
> +      return null;
> +
> +    serverSocket["onaccept"] = function(tcpSocket) {
> +      if (port2AcceptedSocketList[aPort] === undefined) {

Hmm.. what if aPort is -1?  You must use the actual port the server has bound to, right?

@@ +69,5 @@
> +
> +      port2AcceptedSocketList[aPort].push(tcpSocket);
> +
> +      var socketID = port2AcceptedSocketList[aPort].length - 1;
> +      aTCPServerSocket.sendCallbackAccept(aPort, socketID);

better would be to create and init the intermediary on the parent side, it would also remove need for port2AcceptedSocketList array (that I personally don't like very much, but understand well why you need it and have chosen it).  can be done in a followup if worth doing it...

::: dom/network/tests/unit/test_tcpserversocket.js
@@ +1,2 @@
> +/**
> + * Test TCPSocket.js by creating an XPCOM-style server socket, then sending

I didn't check this test deeply, but it seems like it's doing almost the same stuff as test_tcpsocket.js.  Can you fork the file with hg cp rather?  Best would of course be to share the same code, but that might be an overkill.

Also, for me it would be enough to check we accept the connection, send some data first from client, then first from the server and in both situations successfully answer.  After the connection is up, the functionality is well covered by test_tcpsocket.js it self, so IMO no need to retest all again for server socket.  If also makes the test run spend longer time, unnecessarily.

Comment 29

[Donovan Preston [:fzzzy]]

Thanks Honza for the great review! It is much appreciated.

(In reply to Honza Bambas (:mayhemer) from comment #28)
> @@ +192,5 @@
> > +
> > +    /*Close accepted TCPSockets*/
> > +    for (var i = 0; i < this._tcpsocklist.length; i++) {
> > +      this._tcpsocklist[i].close();
> > +    }
> 
> Are you sure you want to do this?  What if you just want to stop the server
> from listening but want to let the already established connections alive (if
> even a bit longer)?
> 
> I don't much agree with this.

I agree with Honza here, to stop the server and keep the existing connections alive is a valid use case. Just leave them open and if the programmer wants to close them they can close them.

Comment 30

[Tomoaki Konno]

Thank you so much, Honza.
I'll modify the codes based on your review.

Comment 31

[Tomoaki Konno]

Honza, I have two questions.
I couldn't partly understand intent of your comments.

> ::: dom/network/src/TCPSocket.js
> @@ +81,5 @@
> >  /*
> > + * nsIDOMTCPServerSocket object
> > + */
> > +
> > +function TCPServerSocket() {
> 
> Could this live in its own js file?

Does your suggestion mean that TCPServerSocket should be separated from TCPSocket.js?


> @@ +69,5 @@
> > +
> > +      port2AcceptedSocketList[aPort].push(tcpSocket);
> > +
> > +      var socketID = port2AcceptedSocketList[aPort].length - 1;
> > +      aTCPServerSocket.sendCallbackAccept(aPort, socketID);
> 
> better would be to create and init the intermediary on the parent side, it
> would also remove need for port2AcceptedSocketList array (that I personally
> don't like very much, but understand well why you need it and have chosen
> it).  can be done in a followup if worth doing it...

Does your suggestion mean that a member variable such as port2AcceptedSocketList array should be added to the TCPSocketParentIntermediary object?
If so, it may be a little bit laborious to cross-refer to the member between TCPSocketParent and TCPServerSocketParent object.
Do you have any good idea?

Comment 32

[Honza Bambas (:mayhemer)]

(In reply to Tomoaki Konno from comment #31)
> Does your suggestion mean that TCPServerSocket should be separated from
> TCPSocket.js?

Yes, but only if it's not an overkill to do and doesn't make things just more complicated.

> > better would be to create and init the intermediary on the parent side, it
> > would also remove need for port2AcceptedSocketList array (that I personally
> > don't like very much, but understand well why you need it and have chosen
> > it).  can be done in a followup if worth doing it...
> 
> Does your suggestion mean that a member variable such as
> port2AcceptedSocketList array should be added to the
> TCPSocketParentIntermediary object?
> If so, it may be a little bit laborious to cross-refer to the member between
> TCPSocketParent and TCPServerSocketParent object.
> Do you have any good idea?

Don't much bother by this suggestion.  The code is written and even I wouldn't probably write it this way, it's OK to land as is.

Now, we create sockets from child to parent (demand to create an outgoing socket is made by content process, and parent side is created as a consequence of creating a child socket side).

Server socket are, however, created on the parent process where we accept connections.  For me it is logical to have also an opposite mechanism: first create a parent side and as a consequence create a child socket side.  I *think* you can send reference to an IPC parent object as an arg of a Send*() event handled by child.  On the child then, you will get the corresponding child object in the corresponding Recv*() event.  Worth to check tho, otherwise you still need some id-mapping.

If you want to do it as part of this patch, feel free, but for me a followup is OK.

Comment 33

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #32)
> (In reply to Tomoaki Konno from comment #31)
> > Does your suggestion mean that TCPServerSocket should be separated from
> > TCPSocket.js?
> 
> Yes, but only if it's not an overkill to do and doesn't make things just
> more complicated.

I see. I'll reconsider whether the separation is appropriate.


> > > better would be to create and init the intermediary on the parent side, it
> > > would also remove need for port2AcceptedSocketList array (that I personally
> > > don't like very much, but understand well why you need it and have chosen
> > > it).  can be done in a followup if worth doing it...
> > 
> > Does your suggestion mean that a member variable such as
> > port2AcceptedSocketList array should be added to the
> > TCPSocketParentIntermediary object?
> > If so, it may be a little bit laborious to cross-refer to the member between
> > TCPSocketParent and TCPServerSocketParent object.
> > Do you have any good idea?
> 
> Don't much bother by this suggestion.  The code is written and even I
> wouldn't probably write it this way, it's OK to land as is.
> 
> Now, we create sockets from child to parent (demand to create an outgoing
> socket is made by content process, and parent side is created as a
> consequence of creating a child socket side).
> 
> Server socket are, however, created on the parent process where we accept
> connections.  For me it is logical to have also an opposite mechanism: first
> create a parent side and as a consequence create a child socket side.  I
> *think* you can send reference to an IPC parent object as an arg of a
> Send*() event handled by child.  On the child then, you will get the
> corresponding child object in the corresponding Recv*() event.  Worth to
> check tho, otherwise you still need some id-mapping.
> 
> If you want to do it as part of this patch, feel free, but for me a followup
> is OK.

Thanks, Honza. 
That's a good idea and I haven't noticed that way.
I have started to modify the code in accordance with your suggestion.

Comment 34

[Tomoaki Konno]

Attachment: A new patch to use TCP Server Socket API

Comment 35

[Tomoaki Konno]

Changes for major comments of the review are as follows.


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

> ::: dom/network/interfaces/nsIDOMTCPSocket.idl
> @@ +34,5 @@
> > + *
> > + * An interface to a server socket that can accept incoming connections for gaia apps.
> > + */
> > +[scriptable, uuid(821638a1-5327-416d-8031-668764f2ec04)]
> > +interface nsIDOMTCPServerSocket : nsIServerSocket
> 
> Do you really need this to derive from nsIServerSocket?  I cannot see the
> actual need.

No. I corrected the idl as bellow.
interface nsIDOMTCPServerSocket : nsISupports
 
> ::: dom/network/interfaces/nsITCPSocketParent.idl
> @@ +38,5 @@
> > +              in unsigned short port, in unsigned short backlog,
> > +              in DOMString binaryType);
> > +
> > +  // Post-processing in closing a server socket
> > +  void postProcCloseServer(in unsigned short port);
> 
> listenCleanup ? up to you.

This post-processing itself was excluded from the codes because it is unnecessary.
 
> ::: dom/network/src/PTCPServerSocket.ipdl
> @@ +24,5 @@
> > +  RequestDelete();
> > +
> > +child:
> > +  CallbackAccept(uint16_t port, uint16_t socketID);
> > +  //CallbackError(JSError error);
> 
> remove commented code.

CallbackError function was added for error handling.
 
> ::: dom/network/src/TCPServerSocketChild.h
> @@ +33,5 @@
> > +  bool mIPCOpen;
> > +};
> > +
> > +class TCPServerSocketChild : public mozilla::net::PTCPServerSocketChild
> > +                           , public TCPServerSocketChildBase
> 
> I don't say it's wrong, just want to ask why you need to go through the Base
> class and directly derive from your interface?

I implemented this in reference to the suggestion for the TCPSocketChildBase in the comment 33 in Bugzilla 770778.
I guess that the TCPServerSocketChildBase class is necessary in order to use a macro function (NS_IMPL_CYCLE_COLLECTING_RELEASE).


> ::: dom/network/src/TCPSocket.js
> @@ +81,5 @@
> >  /*
> > + * nsIDOMTCPServerSocket object
> > + */
> > +
> > +function TCPServerSocket() {
> 
> Could this live in its own js file?

The TCPServerSocket-related code was seperated from TCPSocket.js and nsIDOMTCPSocket.idl.
The following methods were extended so that the TCPSocket and TCPServerSocket object can indirectly cross-refer to a part of members.

　<nsITCPSocketInternal>
   nsIDOMTCPSocket initStream(in nsISocketTransport transport, in DOMString binaryType,in boolean isAccepted)
   nsIDOMTCPSocket initAccepted(in nsITCPSocketChild socketChild, in unsigned short port);
　<nsITCPServerSocketInternal>
   void listen(in unsigned short port, in jsval options, in unsigned short backlog);

> @@ +120,5 @@
> > +    // If the other side is not listening, we will
> > +    // get an onInputStreamReady callback where available
> > +    // raises to indicate the connection was refused.
> > +    
> > +    if (that._binaryType === "arraybuffer") {
> 
> From this line and on you are duplicating part of the existing open method. 
> You should have some openInternal or whatever called method that will let
> accept and open share these particular lines.

The duplicating part was commoditized as an initStream method in the TCPSocket.

> @@ +161,5 @@
> > +      this._tcpsocklist.push(tcpsock);
> > +      this._onaccept(tcpsock);
> > +    }
> > +    else {
> > +      dump("Received unexpected connection!");
> 
> The accepted conn should be closed.

I modified the code to close the accepted conn unless an onaccept handler is set by a user.
 
> @@ +176,5 @@
> > +    that._readyState = kOPEN;
> > +    that._socketBridge = Cc["@mozilla.org/tcp-socket-child;1"]
> > +                            .createInstance(Ci.nsITCPSocketChild);
> > +    that._socketBridge.open(that, "openAccepted:" + socketID, port, false, "",
> > +                            this._useWin, that);
> 
> |this._useWin, that| are unexpected.

This part was excluded from the code due to the modification in this time.
The reason is because sockets are also created from parent to child based on the review.
 
> @@ +192,5 @@
> > +
> > +    /*Close accepted TCPSockets*/
> > +    for (var i = 0; i < this._tcpsocklist.length; i++) {
> > +      this._tcpsocklist[i].close();
> > +    }
> 
> Are you sure you want to do this?  What if you just want to stop the server
> from listening but want to let the already established connections alive (if
> even a bit longer)?
> 
> I don't much agree with this.

I modified the code to keep the tcp socket connections alive even if the tcp server socket is closed.
 
> @@ +201,5 @@
> > +  // nsIServerSocketListener (Triggered by _listener.asyncListen)
> > +  onSocketAccepted: function tss_onSocketAccepted(server, trans) {
> > +    // precondition: this._inChild == false
> > +    var tcpsock = this._initAcceptedSocket(trans, this._options);
> > +    this._callListenerAcceptCommon(tcpsock)
> 
> When any of this throws, you should close the accepted socket.

Exception handling was added.

> ::: dom/network/src/TCPSocketParentIntermediary.js
> @@ +22,5 @@
> > +
> > +    // If aHost is set as 'openAccepted:xxx' instead of host name (e.g. localhost), 
> > +    // an accepted socket object created by listen()is returned
> > +    //('xxx' is an index of socket list).
> > +    if (0 != aHost.indexOf('openAccepted')) {
> 
> Hmm.. what happens when I want to connect openAccepted.example.com?
> 
> You should at least include the colon, thus search for 'openAccepted:'
> 
> However, please, introduce a new method 'accept' on the intermediary class
> that will handle this.  Pass just the socket id, no magic.

This part was excluded from the code due to the modification in this time.
 
> @@ +32,5 @@
> > +    }
> > +    else {
> > +      var socketID = new Number(aHost.split(':')[1]);
> > +
> > +      socket = this._socket = port2AcceptedSocketList[aPort][socketID];
> 
> So, here it is the last place you need the socket mapping in
> port2AcceptedSocketList.  You should delete it otherwise you will let the
> socket waste memory even after the actual connection has closed.  The server
> socket may accept a lot of connections and live a long time.
> 
> You should have a unique autoincrementing number that will create the
> socketID.  Referring by order in the array is anyway not a good practice.

This part was excluded from the code due to the modification in this time.


> @@ +62,5 @@
> > +    if (!serverSocket)
> > +      return null;
> > +
> > +    serverSocket["onaccept"] = function(tcpSocket) {
> > +      if (port2AcceptedSocketList[aPort] === undefined) {
> 
> Hmm.. what if aPort is -1?  You must use the actual port the server has
> bound to, right?

This part was excluded from the code due to the modification in this time.

> @@ +69,5 @@
> > +
> > +      port2AcceptedSocketList[aPort].push(tcpSocket);
> > +
> > +      var socketID = port2AcceptedSocketList[aPort].length - 1;
> > +      aTCPServerSocket.sendCallbackAccept(aPort, socketID);
> 
> better would be to create and init the intermediary on the parent side, it
> would also remove need for port2AcceptedSocketList array (that I personally
> don't like very much, but understand well why you need it and have chosen
> it).  can be done in a followup if worth doing it...

I modified the codes so that sockets are also created from parent to child in line with the review.
I shifted the approach so that sockets can communicate between parent and child without port2AcceptedSocketList.

> ::: dom/network/tests/unit/test_tcpserversocket.js
> @@ +1,2 @@
> > +/**
> > + * Test TCPSocket.js by creating an XPCOM-style server socket, then sending
> 
> I didn't check this test deeply, but it seems like it's doing almost the
> same stuff as test_tcpsocket.js.  Can you fork the file with hg cp rather? 
> Best would of course be to share the same code, but that might be an
> overkill.
> 
> Also, for me it would be enough to check we accept the connection, send some
> data first from client, then first from the server and in both situations
> successfully answer.  After the connection is up, the functionality is well
> covered by test_tcpsocket.js it self, so IMO no need to retest all again for
> server socket.  If also makes the test run spend longer time, unnecessarily.

The excess tests were deleted except for simple tests.

Comment 36

[Honza Bambas (:mayhemer)]

Want to ask for review/feedback on the latest patch or is just a backup/draft?

Comment 37

[Tomoaki Konno]

Could I ask you to review the latest patch again, Honza?

Comment 38

[Honza Bambas (:mayhemer)]

(In reply to Tomoaki Konno from comment #37)
> Could I ask you to review the latest patch again, Honza?

Just set the flag on the patch!

Comment 39

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 715120 [details] [diff] [review]
A new patch to use TCP Server Socket API

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

::: dom/network/interfaces/nsIDOMTCPServerSocket.idl
@@ +58,5 @@
> +   * @param backlog The maximum length the queue of pending connections may grow to.
> +   *                This parameter may be silently limited by the operating system.
> +   *                Pass -1 to use the default value.
> +   */
> +  void listen(in unsigned short port, in jsval options, in unsigned short backlog);

Just curious, is this meant to mirror the POSIX listen() function[1]?  If so, 'port' is usually specified in a separate bind() call[2].  This has the advantage of being able to support additional addressing options[3], including whether or not to listen for incoming connections to a specific address or on a specific interface (something that might be important on a mobile device with multiple active radios or virtual network adapters).

1. http://pubs.opengroup.org/onlinepubs/007904975/functions/listen.html
2. http://pubs.opengroup.org/onlinepubs/007904975/functions/bind.html
3. http://pubs.opengroup.org/onlinepubs/009604599/basedefs/netinet/in.h.html

Comment 40

[Tomoaki Konno]

Thanks, Honza and Jason!


(In reply to Mike Habicher [:mikeh] from comment #39) 

> Just curious, is this meant to mirror the POSIX listen() function[1]?  If
> so, 'port' is usually specified in a separate bind() call[2].  This has the
> advantage of being able to support additional addressing options[3],
> including whether or not to listen for incoming connections to a specific
> address or on a specific interface (something that might be important on a
> mobile device with multiple active radios or virtual network adapters).

No. This is not explicitly done to mirror it.

That's a considerable suggestion.
However, if "port" is separated from listen(), we may need to extend an existing implementation (e.g. "init" or "initWithAddress" in nsServerSocket).
It might have a little impact to some functionality other than TCP Server Socket.

What do you think about this, Honza, Donovan or someone else?

Comment 41

[Honza Bambas (:mayhemer)]

Tomoaki, are you creating your patches that you submit with hg qdiff?  Or, as I do, do you take them directly from .hg/patches?  Problem is that the patches don't show correctly in splinter.  It may indicate there is something wrong with them.

Comment 42

[Honza Bambas (:mayhemer)]

Tomoaki, please refresh the patch to up to date mozilla central and resubmit.  I cannot apply the current patch (rejects) and it also doesn't correctly show in splinter.  Please also set the r? flag.

Some notes:
- remove nsGlobalWindow.cpp changes
- change IID on ifaces you have modified (!)
- hg cp of nsITCPSocketChild.idl to nsITCPServerSocketChild.idl seems to me unneeded, the similarity is low ; just add it as a new file
- make sure the patch is produced with qdiff (or taken refreshed from .hg/patches)

Comment 43

[Tomoaki Konno]

Attachment: A new patch to use TCP Server Socket API

This is a patch created based on diff between my modification and the revision for "d59d2cc" of gecko.

Comment 44

[Tomoaki Konno]

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

Sorry, Honza.
I updated B2G and refreshed the patch.
Can you apply the corrected patch?
I also set the r? flag.

> Some notes:
> - remove nsGlobalWindow.cpp changes
This was removed.

> - change IID on ifaces you have modified (!)
I could not find the overlapped uuid on interfaces. Which part does this point?

> - hg cp of nsITCPSocketChild.idl to nsITCPServerSocketChild.idl seems to me
> unneeded, the similarity is low ; just add it as a new file
This was corrected by changing the option for git diff.
> - make sure the patch is produced with qdiff (or taken refreshed from
> .hg/patches)
This patch was produced with "git diff -U8 --patience".
Is the use of qdiff essential? If so, I'll re-create the new one.

Comment 45

[Honza Bambas (:mayhemer)]

(In reply to Tomoaki Konno from comment #44)
> > - change IID on ifaces you have modified (!)
> I could not find the overlapped uuid on interfaces. Which part does this
> point?

Every interface has the clause at top:

[scriptable, uuid(b82e17da-6476-11e1-8813-57a2ffe9e42c)]

When you change the interface (meaning to add/remove/change signature of a method) you must change the uuid value.  Generate it with some GUID tool (I don't much recommend web tools for this).

> 
> > - make sure the patch is produced with qdiff (or taken refreshed from
> > .hg/patches)
> This patch was produced with "git diff -U8 --patience".
> Is the use of qdiff essential? If so, I'll re-create the new one.

No, if this works, then we are OK.  I just want to make splinter happy to show the patch.

Comment 46

[Tomoaki Konno]

Attachment: A new patch to use TCP Server Socket API

Minor change for the uuid on interfaces.

Comment 47

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #45)
> Every interface has the clause at top:
> 
> [scriptable, uuid(b82e17da-6476-11e1-8813-57a2ffe9e42c)]
> 
> When you change the interface (meaning to add/remove/change signature of a
> method) you must change the uuid value.  Generate it with some GUID tool (I
> don't much recommend web tools for this).
I see. I changed the uuids on interfaces in nsIDOMTCPSocket.idl, nsITCPSocketChild.idl and nsITCPSocketParent.idl I have modified.

> No, if this works, then we are OK.  I just want to make splinter happy to
> show the patch.
I see. If this doesn't work, please let me know.

Comment 48

[Tomoaki Konno]

Attachment: A new patch to use TCP Server Socket API

Minor change to delete the overlapped uuid in the previous patch.

Comment 49

[Honza Bambas (:mayhemer)]

Comment on attachment 719397 [details] [diff] [review]
A new patch to use TCP Server Socket API

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

r-

lot of styling nits.

JS and C++ style is:

function name(args)
{
  body;
}

function name(verylongarg,
              verylongarg2)
{
}

Please check all your newly added code conforms this.

::: dom/network/interfaces/nsIDOMTCPServerSocket.idl
@@ +30,5 @@
> +   * The onerror handler will be called when there is an error. The data
> +   * attribute of the event passed to the onerror handler will have a
> +   * description of the kind of error.
> +   */
> +  attribute jsval onerror;

Can you be more detailed on what errors you trigger this callback?

@@ +42,5 @@
> +/*
> + * Internal interfaces for use in cross-process server-socket implementation.
> +*/
> +[scriptable, uuid(b64b1e68-4efa-497c-b0d8-69f067ad5ec8)]
> +interface nsITCPServerSocketInternal : nsISupports {

{ on a new line

Can you comment more in-detail what is purpose if this interface?  On what process it is being used, by what component it is being implemented (is the server socket, is the parent/child, what?), who are callers.

@@ +61,5 @@
> +   */
> +  void listen(in unsigned short port, in jsval options, in unsigned short backlog);
> +
> +  // Listener for receiving an accepted socket.
> +  void callListenerAccept(in nsITCPSocketChild socketChild);

doxygen style comment please

/**
 *
 */

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +54,5 @@
>     */
>    nsIDOMTCPSocket open(in DOMString host, in unsigned short port, [optional] in jsval options);
>  
> +  /** 
> +   * Listen to a port

Listen on a port?

Whitespace.

@@ +225,5 @@
>    void updateReadyStateAndBuffered(in DOMString readyState, in uint32_t bufferedAmount);
> +
> +  // Initialize an input/output stream.
> +  nsIDOMTCPSocket initStream(in nsISocketTransport transport, in DOMString binaryType,
> +                             in boolean isAccepted);

Could the comment be more detailed?  What are the arguments (where from are taken), what is the result, who are the callers.

should be called initWithTransport

@@ +228,5 @@
> +  nsIDOMTCPSocket initStream(in nsISocketTransport transport, in DOMString binaryType,
> +                             in boolean isAccepted);
> +
> +  // Initialize a child socket object when a parent socket accepts any request.
> +  nsIDOMTCPSocket initAccepted(in nsITCPSocketChild socketChild, in unsigned short port);

As well here.

and should be called initWithAcceptedChild

::: dom/network/interfaces/nsITCPServerSocketChild.idl
@@ +16,5 @@
> +  void listen(in nsITCPServerSocketInternal serverSocket, in unsigned short port,
> +              in unsigned short backlog, in DOMString binaryType);
> +
> +  // Tell the chrome process to perform equivalent operations to all following methods
> +  void close();

/**
 */ style comments

Be more detailed about how this interface is used, what are the args, what is their source.

::: dom/network/interfaces/nsITCPServerSocketParent.idl
@@ +18,5 @@
> +  // Trigger a callback in the content process when an error occurs.
> +  [implicit_jscontext] void sendCallbackError(in DOMString message,
> +                                              in DOMString filename,
> +                                              in uint32_t lineNumber,
> +                                              in uint32_t columnNumber);

/** */, details on how the interfaces is used, details about the arguments.

::: dom/network/interfaces/nsITCPSocketChild.idl
@@ +23,5 @@
>    void suspend();
>    void close();
> +
> +  // Initialize a child socket object.
> +  void setTCPSocket(in nsITCPSocketInternal socket, in jsval socketVal);

more comments, where from called, where the socket is taken from, what is its state, actually all args need to have a comment

::: dom/network/interfaces/nsITCPSocketParent.idl
@@ +25,5 @@
>                                           in uint32_t bufferedAmount);
> +
> +  // Initialize a parent socket object.
> +  [implicit_jscontext] void setTCPSocket(in nsIDOMTCPSocket socket,
> +                                         in nsITCPSocketIntermediary intermediary);

Call this setSocketAndIntermediary

@@ +40,5 @@
>  
> +  // Listen on a port
> +  nsIDOMTCPServerSocket listen(in nsITCPServerSocketParent parent,
> +              in unsigned short port, in unsigned short backlog,
> +              in DOMString binaryType);

indention

::: dom/network/src/PTCPSocket.ipdl
@@ +39,5 @@
>    manager PNecko;
>  
>  parent:
> +  Open(nsString host, uint16_t port, bool useSSL, nsString binaryType,
> +             nullable PBrowser browser);

indention

::: dom/network/src/TCPServerSocket.js
@@ +67,5 @@
> +  },
> +
> +  _callListenerAcceptCommon: function tss_callListenerAcceptCommon(socket) {
> +    if (this._onaccept) {
> +      this["onaccept"].call(null, socket);

You should catch exceptions from this call and close the socket.

@@ +74,5 @@
> +      socket.close();
> +      dump("Received unexpected connection!");
> +    }
> +  },
> +  

while space

@@ +76,5 @@
> +    }
> +  },
> +  
> +  /* nsITCPServerSocketInternal method */
> +  listen: function tss_listen(port, options, backlog) {

Should throw when already listening.  I prefer it because it's quit confusing having listen on TCPSocket as well as TCPServerSocket.

@@ +86,5 @@
> +                             .createInstance(Ci.nsITCPServerSocketChild);
> +      this._serverBridge.listen(this, port, backlog, options.binaryType);
> +    }
> +    else {
> +      this._listener = new ServerSocket(port, false, backlog);

_listener should be simply called neckoTCPServerSocket

@@ +105,5 @@
> +      var type = "error";
> +      var error = new Error(message, filename, lineNumber, columnNumber);
> +    
> +      this["onerror"].call(null, new TCPSocketEvent(type, this, error));
> +    }    

whitespaces (check splinter in general)

@@ +133,5 @@
> +  },
> +
> +  // nsIServerSocketListener (Triggered by _listener.asyncListen)
> +  onStopListening: function tss_onStopListening(server, status) {
> +    LOG("onStopListening was called.");

maybe throw the necko server socket away?

::: dom/network/src/TCPServerSocketChild.cpp
@@ +86,5 @@
> +  TCPSocketChild* socket = static_cast<TCPSocketChild*>(psocket);
> +
> +  socket->AddIPDLReference();
> +  nsresult rv = mServerSocket->CallListenerAccept(static_cast<nsITCPSocketChild*>(socket));
> +  NS_ENSURE_SUCCESS(rv, true);

Just return true always.  Log the error only (as NS_ERROR or better as NS_WARNING only)

@@ +92,5 @@
> +}
> +
> +bool
> +TCPServerSocketChild::RecvCallbackError(const nsString& aMessage, const nsString& aFilename,
> +                             const uint32_t& aLineNumber, const uint32_t& aColumnNumber)

indention

@@ +96,5 @@
> +                             const uint32_t& aLineNumber, const uint32_t& aColumnNumber)
> +{
> +  nsresult rv = mServerSocket->CallListenerError(aMessage, aFilename,
> +                                                 aLineNumber, aColumnNumber);
> +  NS_ENSURE_SUCCESS(rv, true);

as above

::: dom/network/src/TCPServerSocketParent.cpp
@@ +17,5 @@
> +FireInteralError(mozilla::net::PTCPServerSocketParent* aActor, uint32_t aLineNo)
> +{
> +  mozilla::unused <<
> +      aActor->SendCallbackError(NS_LITERAL_STRING("Internal error"),
> +                          NS_LITERAL_STRING(__FILE__), aLineNo, 0);

indent

@@ +52,5 @@
> +}
> +
> +NS_IMETHODIMP
> +TCPServerSocketParent::SendCallbackAccept(nsITCPSocketParent *socket,
> +  JSContext* cx)

indention

what's the cx for?

@@ +57,5 @@
> +{
> +  TCPSocketParent* _socket = static_cast<TCPSocketParent*>(socket);
> +  PTCPSocketParent* _psocket = static_cast<PTCPSocketParent*>(_socket);
> +
> +  mozilla::unused << mNeckoParent->SendPTCPSocketConstructor(_psocket);

Null check for mNeckoParent

@@ +83,5 @@
> +  NS_ENSURE_TRUE(mServerSocket, true);
> +  rv = mServerSocket->Close();
> +  NS_ENSURE_SUCCESS(rv, true);
> +  rv = mServerSocket->GetPort(&aPort);
> +  NS_ENSURE_SUCCESS(rv, true);

what's getting the port for?
don't return false, just return true + IMO those are not critical, no need to bloat console

@@ +96,5 @@
> +  if (mServerSocket) {
> +    mServerSocket->Close();
> +  }
> +  mNeckoParent = nullptr;
> +  mServerSocket = nullptr;

move to the if

::: dom/network/src/TCPServerSocketParent.h
@@ +39,5 @@
> +
> +  PNeckoParent* mNeckoParent;
> +  nsCOMPtr<nsITCPSocketIntermediary> mIntermediary;
> +  nsCOMPtr<nsIDOMTCPServerSocket> mServerSocket;
> +  JSObject* mIntermediaryObj;

This seems unused.  Is it intended to be used as rooting?

@@ +40,5 @@
> +  PNeckoParent* mNeckoParent;
> +  nsCOMPtr<nsITCPSocketIntermediary> mIntermediary;
> +  nsCOMPtr<nsIDOMTCPServerSocket> mServerSocket;
> +  JSObject* mIntermediaryObj;
> +  bool mIPCOpen;

I think this class should also have the ipdl referencing functions.

::: dom/network/src/TCPSocket.js
@@ +111,5 @@
>      close: 'r',
>      send: 'r',
>      readyState: 'r',
>      binaryType: 'r',
> +    listen: 'r',

do you really need this?

@@ +286,5 @@
>      this._bufferedAmount = bufferedAmount;
>    },
> +
> +  initStream: function ts_initStream(transport, binaryType, isAccepted) {
> +    let that = isAccepted ? new TCPSocket() : this;

I a bit don't understand this.  It's kinda confusing.  There shouldn't be any isAccepted, the method should always work with |this|.  It's up to the caller to create the socket if a new is demanded.

@@ +320,5 @@
> +    if (isAccepted) {
> +      /* ReadyState is kOpen since accepted transport stream has already been connected */
> +      that._readyState = kOPEN;
> +      that._inputStreamPump = new InputStreamPump(that._socketInputStream, -1, -1, 0, 0, false);
> +      that._inputStreamPump.asyncRead(that, null);

Also should be shared.  I'd rather let the caller do this (call the shared method) then play with the isAccepted flag.  Also the caller can very well create the new socket it self.  This flag is confusing.

@@ +323,5 @@
> +      that._inputStreamPump = new InputStreamPump(that._socketInputStream, -1, -1, 0, 0, false);
> +      that._inputStreamPump.asyncRead(that, null);
> +    }
> +
> +    return that;

when returning a new object (and not even always - how confising!) this is not "initXXX" method but "createXXX" or "openXXX" method.  Up to you to decide on semantic and the appropriate name according it.

@@ +335,5 @@
> +    that._readyState = kOPEN;
> +    socketChild.setTCPSocket(that, that);
> +    that._socketBridge = socketChild;
> +
> +    return that;

the same applies here.

@@ +440,5 @@
>  
>      let transport = that._transport = this._createTransport(host, port, that._ssl);
>      transport.setEventSink(that, Services.tm.currentThread);
>      transport.securityCallbacks = new SecurityCallbacks(that);
> +    that.initStream(transport, that._binaryType, false);

|that| already know the transport in its _transport property.  Why are you passing it again?  Why cannot this method assign that property?  Confusing.

@@ +460,3 @@
>  
> +    that.onaccept = function() {
> +      LOG("Default onaccept handler(set by TCPSocket.listen())");

So, when consumers don't set onaccept you just silently let the accepted sockets hang? No..

either close the socket or let this handler throw and let the server socket implementation handle it by closing of the socket

::: dom/network/src/TCPSocketParent.cpp
@@ +82,1 @@
>                        const nsString& aBinaryType, PBrowserParent* aBrowser)

indent

@@ +261,5 @@
> +                              JSContext* cx)
> +{
> +  mSocket = socket;
> +  mIntermediary = intermediary;
> +  AddIPDLReference();

I don't think this is the right place to add ipdl ref, no logical reference to ipdl here, in this method.

It's better to keep call to these methods in the class however.

nsITCPSocketParent should have method InitIPC or so (not sure about the name) that will send the constructor and add ipdl ref.  Now this happens in TCPServerSocketParent::SendCallbackAccept.

For simplicity, the AddIPDLReference() call could be moved there, it'd be acceptable.

::: dom/network/src/TCPSocketParent.h
@@ +50,2 @@
>              const bool& useSSL, const nsString& aBinaryType,
>              PBrowserParent* aBrowser);

indent

::: dom/network/src/TCPSocketParentIntermediary.js
@@ +41,4 @@
>      return socket;
>    },
>  
> +  listen: function(aTCPServerSocket, aPort, aBacklog, aBinaryType) {

s/aTCPServerSocket/aTCPServerSocketParent/

@@ +46,5 @@
> +    let serverSocket = baseSocket.listen(aPort, { binaryType: aBinaryType }, aBacklog);
> +    let port = serverSocket.port;
> +
> +    if (!serverSocket)
> +      return null;

Should be above the previous line?

@@ +55,5 @@
> +      var intermediary = new TCPSocketParentIntermediary();
> +
> +      intermediary._socket = socket;
> +      intermediary._setCallbacks(socketParent, socket);
> +      socketParent.setTCPSocket(socket, intermediary);

This is a horrible triangle... could it be somehow restructulized or at least armed with comments to make this better readable?

@@ +56,5 @@
> +
> +      intermediary._socket = socket;
> +      intermediary._setCallbacks(socketParent, socket);
> +      socketParent.setTCPSocket(socket, intermediary);
> +      aTCPServerSocket.sendCallbackAccept(socketParent);

blank line above this line

@@ +58,5 @@
> +      intermediary._setCallbacks(socketParent, socket);
> +      socketParent.setTCPSocket(socket, intermediary);
> +      aTCPServerSocket.sendCallbackAccept(socketParent);
> +    }
> +    serverSocket["onerror"] = function(data) {

blank line above this line too

::: dom/network/tests/unit/test_tcpserversocket.js
@@ +222,5 @@
> +
> +function cleanup() {
> +  do_print("Cleaning up");
> +  sock.close();
> +  sock_2.close();

what is sock_2 ?

@@ +236,5 @@
> +add_test(connectSock);
> +add_test(sendData);
> +add_test(receiveData);
> +// - server closes on us
> +add_test(serverCloses);

I'd add one more round of send and recv data tests after this one to check and demonstrate closing the server doesn't close existing connections.

Also may be good to check that closing the server actually prevents acception of connections (client connection has to fail after the server is closed).

I'd like to see both ways data exchange.  I can see you only check client send + server response.  Would be good to open a new socket and check server hallo first + client answer.

::: netwerk/ipc/NeckoChild.cpp
@@ +159,3 @@
>  {
> +  TCPSocketChild* p = new TCPSocketChild();
> +  p->AddRef();

should be ipdl ref add, no?

@@ +178,5 @@
>  
> +PTCPServerSocketChild*
> +NeckoChild::AllocPTCPServerSocket(const uint16_t& aPort,
> +                            const uint16_t& aBacklog,
> +                            const nsString& aBinaryType)

indent

::: netwerk/ipc/NeckoChild.h
@@ +38,5 @@
>    virtual bool DeallocPFTPChannel(PFTPChannelChild*);
>    virtual PWebSocketChild* AllocPWebSocket(PBrowserChild*, const SerializedLoadContext&);
>    virtual bool DeallocPWebSocket(PWebSocketChild*);
> +  virtual PTCPSocketChild* AllocPTCPSocket();
> +  virtual bool RecvPTCPSocketConstructor(PTCPSocketChild*);

is constructor actually needed?

@@ +43,4 @@
>    virtual bool DeallocPTCPSocket(PTCPSocketChild*);
> +  virtual PTCPServerSocketChild* AllocPTCPServerSocket(const uint16_t& aPort,
> +                                           const uint16_t& aBacklog,
> +                                           const nsString& aBinaryType);

indent

::: netwerk/ipc/NeckoParent.cpp
@@ +288,5 @@
> +
> +PTCPServerSocketParent*
> +NeckoParent::AllocPTCPServerSocket(const uint16_t& aPort,
> +                             const uint16_t& aBacklog,
> +                             const nsString& aBinaryType)

indent

@@ +291,5 @@
> +                             const uint16_t& aBacklog,
> +                             const nsString& aBinaryType)
> +{
> +  TCPServerSocketParent* p = new TCPServerSocketParent();
> +  p->AddRef();

should be AddIPDLref?

@@ +299,5 @@
> +bool
> +NeckoParent::RecvPTCPServerSocketConstructor(PTCPServerSocketParent* aActor,
> +                                       const uint16_t& aPort,
> +                                       const uint16_t& aBacklog,
> +                                       const nsString& aBinaryType)

indent

::: netwerk/ipc/NeckoParent.h
@@ +77,5 @@
> +                                            const nsString& aBinaryType);
> +  virtual bool RecvPTCPServerSocketConstructor(PTCPServerSocketParent*,
> +                                         const uint16_t& aPort,
> +                                         const uint16_t& aBacklog,
> +                                         const nsString& aBinaryType);

Indentions

::: netwerk/ipc/PNecko.ipdl
@@ +35,5 @@
>    manages PFTPChannel;
>    manages PWebSocket;
>    manages PTCPSocket;
>    manages PRemoteOpenFile;
> +  manages PTCPServerSocket;

under PTCPSocket

Comment 50

[Tomoaki Konno]

Thanks, Honza.
I'll modify them and resubmit the revised patch.

Comment 51

[Tomoaki Konno]

Attachment: A new patch to use TCP Server Socket API

This is a patch created based on diff between my modification and the revision for "14864f" of gecko.
This patch was produced with "git diff -U8 --patience".

Comment 52

[Tomoaki Konno]

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

I basically modified the codes along Honza's comments.
  Corrected JS and C++ style.
  Added more comments for IDL.
  Renamed methods properly.

As for the rest, the details where I modified are as follows.

 
> @@ +225,5 @@
> >    void updateReadyStateAndBuffered(in DOMString readyState, in uint32_t bufferedAmount);
> > +
> > +  // Initialize an input/output stream.
> > +  nsIDOMTCPSocket initStream(in nsISocketTransport transport, in DOMString binaryType,
> > +                             in boolean isAccepted);
> 
> Could the comment be more detailed?  What are the arguments (where from are
> taken), what is the result, who are the callers.
> 
> should be called initWithTransport

The interace is changed in reference to the later modification of codes in TCPSocket.js.


> @@ +228,5 @@
> > +  nsIDOMTCPSocket initStream(in nsISocketTransport transport, in DOMString binaryType,
> > +                             in boolean isAccepted);
> > +
> > +  // Initialize a child socket object when a parent socket accepts any request.
> > +  nsIDOMTCPSocket initAccepted(in nsITCPSocketChild socketChild, in unsigned short port);
> 
> As well here.
> 
> and should be called initWithAcceptedChild
> 

The interace is changed in reference to the later modification of codes in TCPSocket.js.

> ::: dom/network/src/TCPSocket.js
> @@ +111,5 @@
> >      close: 'r',
> >      send: 'r',
> >      readyState: 'r',
> >      binaryType: 'r',
> > +    listen: 'r',
> 
> do you really need this?

I think it is necessary. Gaia application should explicitly allow to read the listen property, but should not allow to write it.


 
> @@ +286,5 @@
> >      this._bufferedAmount = bufferedAmount;
> >    },
> > +
> > +  initStream: function ts_initStream(transport, binaryType, isAccepted) {
> > +    let that = isAccepted ? new TCPSocket() : this;
> 
> I a bit don't understand this.  It's kinda confusing.  There shouldn't be
> any isAccepted, the method should always work with |this|.  It's up to the
> caller to create the socket if a new is demanded.

"initStream" was changed to "_initStream" as the local member.
"isAccepted" and "transport" were deleted.


> @@ +320,5 @@
> > +    if (isAccepted) {
> > +      /* ReadyState is kOpen since accepted transport stream has already been connected */
> > +      that._readyState = kOPEN;
> > +      that._inputStreamPump = new InputStreamPump(that._socketInputStream, -1, -1, 0, 0, false);
> > +      that._inputStreamPump.asyncRead(that, null);
> 
> Also should be shared.  I'd rather let the caller do this (call the shared
> method) then play with the isAccepted flag.  Also the caller can very well
> create the new socket it self.  This flag is confusing.

"isAccepted" was deleted.
This part was included in a "createAcceptedParent" method, which was newly defined.

> @@ +323,5 @@
> > +      that._inputStreamPump = new InputStreamPump(that._socketInputStream, -1, -1, 0, 0, false);
> > +      that._inputStreamPump.asyncRead(that, null);
> > +    }
> > +
> > +    return that;
> 
> when returning a new object (and not even always - how confising!) this is
> not "initXXX" method but "createXXX" or "openXXX" method.  Up to you to
> decide on semantic and the appropriate name according it.

"initStream" and "initAccepted" were deleted.
Instead, "createAcceptedParent" and "createAcceptedChild" were newly defined.
The "_initStream" are called in the createAcceptedParent. 

> @@ +335,5 @@
> > +    that._readyState = kOPEN;
> > +    socketChild.setTCPSocket(that, that);
> > +    that._socketBridge = socketChild;
> > +
> > +    return that;
> 
> the same applies here.

Please see above.

> @@ +440,5 @@
> >  
> >      let transport = that._transport = this._createTransport(host, port, that._ssl);
> >      transport.setEventSink(that, Services.tm.currentThread);
> >      transport.securityCallbacks = new SecurityCallbacks(that);
> > +    that.initStream(transport, that._binaryType, false);
> 
> |that| already know the transport in its _transport property.  Why are you
> passing it again?  Why cannot this method assign that property?  Confusing.

The "transport" arg was excluded because the TCPSocket object has a "_transport" member.

> @@ +460,3 @@
> >  
> > +    that.onaccept = function() {
> > +      LOG("Default onaccept handler(set by TCPSocket.listen())");
> 
> So, when consumers don't set onaccept you just silently let the accepted
> sockets hang? No..
> 
> either close the socket or let this handler throw and let the server socket
> implementation handle it by closing of the socket

Default handler was deleted because the code has checked whether "_onaccept" is set in the "_callListenerAccepteCommon" method for TCPServerSocket.js.
 

> ::: dom/network/src/TCPSocketParentIntermediary.js
> @@ +55,5 @@
> > +      var intermediary = new TCPSocketParentIntermediary();
> > +
> > +      intermediary._socket = socket;
> > +      intermediary._setCallbacks(socketParent, socket);
> > +      socketParent.setTCPSocket(socket, intermediary);
> 
> This is a horrible triangle... could it be somehow restructulized or at
> least armed with comments to make this better readable?

I changed the set of "socket" to be conducted in the "_setCallbacks" method for TCPSocketParentIntermediary.
Since it was difficult to modify the cross structure, the comments was added.

> @@ +236,5 @@
> > +add_test(connectSock);
> > +add_test(sendData);
> > +add_test(receiveData);
> > +// - server closes on us
> > +add_test(serverCloses);
> 
> I'd add one more round of send and recv data tests after this one to check
> and demonstrate closing the server doesn't close existing connections.
> 
> Also may be good to check that closing the server actually prevents
> acception of connections (client connection has to fail after the server is
> closed).
> 
> I'd like to see both ways data exchange.  I can see you only check client
> send + server response.  Would be good to open a new socket and check server
> hallo first + client answer.

Three tests were added.
(1) Test to send/recv data after closing the server.
(2) Test to open from client after closing the server.
(3) Test to first send from the server, then response from client.

Comment 53

[Honza Bambas (:mayhemer)]

Comment on attachment 731610 [details] [diff] [review]
A new patch to use TCP Server Socket API

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

r=honzab but still few things to address:

- Before this lands we need to push to try.  Can you do it or should i do it?

- You need sr for dom/network changes.  I'm not a dom peer.  Jonas, can you please check or delegate?

- Why did you remove run of the test in child?  This is extremely important to run in IPC.

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +227,5 @@
> +  // Create a socket object on the parent side.
> +  // This is called in accepting any open request on the parent side.
> +  // 
> +  // @param transport
> +  //        The connected socket transport. See nsISocketTransport.

"Accepted" transport rather?  Please remove the note about "See nsISocketTransport".

@@ +233,5 @@
> +  //        "arraybuffer" to use UInt8 array instances or "string" to use String.
> +  nsIDOMTCPSocket createAcceptedParent(in nsISocketTransport transport,
> +                                       in DOMString binaryType);
> +
> +  // Create a socket object on the child side

"Create a DOM socket ..."

::: dom/network/interfaces/nsITCPServerSocketParent.idl
@@ +37,5 @@
> +   */
> +  void sendCallbackError(in DOMString message,
> +                                              in DOMString filename,
> +                                              in uint32_t lineNumber,
> +                                              in uint32_t columnNumber);

indention

::: dom/network/interfaces/nsITCPSocketChild.idl
@@ +24,5 @@
>    void close();
> +
> +  // Initialize a child socket object. It is called from the child side socket,
> +  // which is generated in receiving a notification of accepting any open request
> +  // on the parent side. The socket after being initialized will be established.

what is "The socket" ?  What is "being initialized" and what means "will be established" ?

It's unclear at all what this method is supposed to do.

@@ +30,5 @@
> +  // @param socket
> +  //        The socket on the child side.
> +  // @param socketVal
> +  //        The socket on the child side for deserialization 
> +  //        from an other implementation format to a javascript format.

No idea what this is.

@@ +31,5 @@
> +  //        The socket on the child side.
> +  // @param socketVal
> +  //        The socket on the child side for deserialization 
> +  //        from an other implementation format to a javascript format.
> +  void setSocket(in nsITCPSocketInternal socket, in jsval socketVal);

Doxygen style comments

::: dom/network/src/TCPServerSocket.js
@@ +149,5 @@
> +  // nsIServerSocketListener (Triggered by _neckoTCPServerSocket.asyncListen)
> +  onStopListening: function tss_onStopListening(server, status) {
> +    if (status != NS_BINDING_ABORTED) {
> +      throw new Error("Server socket was closed by unexpected reason.");
> +    }

I meant to:

this._neckoTCPServerSocket = null;

::: dom/network/src/TCPServerSocketChild.cpp
@@ +84,5 @@
> +TCPServerSocketChild::RecvCallbackAccept(PTCPSocketChild *psocket)
> +{
> +  TCPSocketChild* socket = static_cast<TCPSocketChild*>(psocket);
> +
> +  socket->AddIPDLReference();

I believe this is done in NeckoChild::AllocPTCPSocket()  So, this should actually cause a leak and should be detected by tests, if I'm correct.

@@ +87,5 @@
> +
> +  socket->AddIPDLReference();
> +  nsresult rv = mServerSocket->CallListenerAccept(static_cast<nsITCPSocketChild*>(socket));
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("CallListenerAccept was failed.");

Instead "was failed" may be better to log "threw exception".

::: dom/network/src/TCPServerSocketParent.cpp
@@ +81,5 @@
> +  }
> +  else {
> +    NS_ERROR("The member value for NeckoParent is wrong.");
> +  }
> +  mozilla::unused << PTCPServerSocketParent::SendCallbackAccept(_psocket);

When you don't construct the IPC bridge with Send*Constructor, this is probably going to fail.

@@ +100,5 @@
> +
> +bool
> +TCPServerSocketParent::RecvClose()
> +{
> +  NS_ENSURE_TRUE(mServerSocket, false);

true

::: dom/network/src/TCPSocket.js
@@ +334,5 @@
> +  createAcceptedChild: function ts_createAcceptedChild(socketChild, port) {
> +    let that = new TCPSocket();
> +
> +    that._inChild = true;
> +    that._port = port;

Are you sure the port assignment is correct?  

As I understand TCPSocket.prototype._port is the remote port number.  Not local.  Anyway, I believe this can be left 0 or -1 (default).  the value is small and implementation hard.

::: dom/network/src/TCPSocketParentIntermediary.js
@@ +70,5 @@
> +    serverSocket["onerror"] = function(data) {
> +        var error = data.data;
> +
> +        aTCPServerSocketParent.sendCallbackError(error.message, error.filename,
> +                                           error.lineNumber, error.columnNumber);

indention

::: dom/network/tests/unit/test_tcpserversocket.js
@@ +40,5 @@
> +  BIG_ARRAY[i_big] = Math.floor(Math.random() * 256);
> +}
> +
> +const BIG_TYPED_ARRAY = new Uint8Array(BIG_ARRAY);
> +      

ws

@@ +175,5 @@
> +  sock.onclose = makeFailureCase('close');  
> +}
> +
> +/**
> + * Connect the socket to the server in closing server.

Please rephrase.

@@ +179,5 @@
> + * Connect the socket to the server in closing server.
> + * This test is added after closing server test.
> + */
> +function openSockInClosingServer() {
> +  var success = makeSuccessCase('clientopen');

'clientnotopen' ?

::: netwerk/ipc/NeckoParent.cpp
@@ -271,5 @@
> -  if (UsingNeckoIPCSecurity() && !aBrowser) {
> -    printf_stderr("NeckoParent::AllocPTCPSocket: FATAL error: no browser present \
> -                   KILLING CHILD PROCESS\n");
> -    return nullptr;
> -  }

Why this has been removed? (Maybe already discussed, just don't remember)

Comment 54

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #53)
> Comment on attachment 731610 [details] [diff] [review]
> A new patch to use TCP Server Socket API
> 
> Review of attachment 731610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> - Before this lands we need to push to try.  Can you do it or should i do it?
I will be able to "push to try".
It might take a little time to do it, because I'm not familiar with Mercurial and "push to try".
 
> - You need sr for dom/network changes.
I see.
 
> - Why did you remove run of the test in child?  This is extremely important
> to run in IPC.
I just forgot to add the test to this patch. I'll add it.

I'll also modify the code along your other comments.

Comment 55

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

Comment on attachment 731610 [details] [diff] [review]
A new patch to use TCP Server Socket API

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

sr=me with those things fixed or answered.

Though I only looked at the public API, let me know if you want sr on the implementation too.

::: dom/network/interfaces/nsIDOMTCPServerSocket.idl
@@ +23,5 @@
> +   * The onaccept event handler is called when a client connection is accepted.
> +   * The data attribute of the event passed to the onaccept handler will be a TCPSocket
> +   * instance, which is used for communication between client and server. 
> +   */
> +  attribute jsval onaccept;

In the current spec drafts, this event is called "connection" rather than "accept". Do you think we should push for a spec change, or should we use "connection" instead?

http://raw-sockets.sysapps.org/#interface-tcpserversocket

@@ +35,5 @@
> +
> +  /**
> +   * Close the server socket.
> +   */
> +  void close();

The spec calls this one closeServer(), but I think simply calling it close() is better so I'll suggest a spec change.

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +71,5 @@
> +   *
> +   * @return The new TCPServerSocket instance.
> +   */
> +  nsIDOMTCPServerSocket listen(in unsigned short port, [optional] in jsval options,
> +                               [optional] in unsigned short backlog);

This should really be a constructor rather than a function on TCPSocket.

I don't think the binaryType is needed. We should always return incoming data as ArrayBuffers. The spec is going to change to remove the binaryType.

The backlog argument should be a property on the "options" object argument.

Comment 56

[Josh Matthews [:jdm]]

Please hold off landing this until after bug 831107 is finished. I apologize for the bitrot it will cause.

Comment 57

[Tomoaki Konno]

Thanks, Jonas.
I answer as below, and I will fix the codes based on Jonas and Honza's comments.
After that, I would like to ask you to do sr.

Also, I should merge the change of the implementation for TCP Socket from Josh before push to try.


(In reply to Jonas Sicking (:sicking) from comment #55) 
> ::: dom/network/interfaces/nsIDOMTCPServerSocket.idl
> @@ +23,5 @@
> > +   * The onaccept event handler is called when a client connection is accepted.
> > +   * The data attribute of the event passed to the onaccept handler will be a TCPSocket
> > +   * instance, which is used for communication between client and server. 
> > +   */
> > +  attribute jsval onaccept;
> 
> In the current spec drafts, this event is called "connection" rather than
> "accept". Do you think we should push for a spec change, or should we use
> "connection" instead?
> 
> http://raw-sockets.sysapps.org/#interface-tcpserversocket

I think "accept" is better than "connection".
It is not intuitive for me whether "connection" means "connecting" or "connected".
And also, "connect" has been used as the function at the client side in POSIX.
http://pubs.opengroup.org/onlinepubs/009695399/functions/connect.html 

So, I hope the spec change.



> @@ +35,5 @@
> > +
> > +  /**
> > +   * Close the server socket.
> > +   */
> > +  void close();
> 
> The spec calls this one closeServer(), but I think simply calling it close()
> is better so I'll suggest a spec change.

I agree with you.
I think it is explicit that the method is to close a server socket.
 
> ::: dom/network/interfaces/nsIDOMTCPSocket.idl
> @@ +71,5 @@
> > +   *
> > +   * @return The new TCPServerSocket instance.
> > +   */
> > +  nsIDOMTCPServerSocket listen(in unsigned short port, [optional] in jsval options,
> > +                               [optional] in unsigned short backlog);
> 
> This should really be a constructor rather than a function on TCPSocket.
> 
> I don't think the binaryType is needed. We should always return incoming
> data as ArrayBuffers. The spec is going to change to remove the binaryType.
> 
> The backlog argument should be a property on the "options" object argument.

That may be better if there is no ploblem to create stuff like mozTCPServerSocket.
I'll modify "listen" as a function on TCPServerSocket.
I also remove the binaryType option and add a backlog argument to "options"

Comment 58

[Tomoaki Konno]

Could you vouch in this bug, Jonas or Honza?
I would like to start preparing for conducting a test of TCP Server Socket on try
server.

https://bugzilla.mozilla.org/show_bug.cgi?id=862638

Comment 59

[Tomoaki Konno]

(In reply to Tomoaki Konno from comment #58)
> Could you vouch in this bug, Jonas or Honza?
> I would like to start preparing for conducting a test of TCP Server Socket
> on try
> server.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=862638
Thanks, Brian.
Brian has vouched for me.

Comment 60

[Donovan Preston [:fzzzy]]

(In reply to Jonas Sicking (:sicking) from comment #55)
> This should really be a constructor rather than a function on TCPSocket.

If I recall correctly, at the time the original TCPSocket patch was written, there were issues with constructors implemented in javascript not being able to take arguments. This is why the original api ended up being TCPSocket.connect. If there are no longer issues with constructors implemented in javascript being able to take arguments, then perhaps both the client socket and server socket should change to be constructors. (TCPClientSocket and TCPServerSocket?)

Comment 61

[Donovan Preston [:fzzzy]]

(In reply to Jonas Sicking (:sicking) from comment #55)
> I don't think the binaryType is needed. We should always return incoming
> data as ArrayBuffers. The spec is going to change to remove the binaryType.

For now wouldn't it be better to keep the binaryType to match the current client socket implementation, and then change both the client socket and server socket to not have binaryType in another, future patch?

Comment 62

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

It doesn't seem better to me to match the other API if we know we'll have to change it. Changing APIs carry a much higher cost than inconsistent APIs.

I'd rather create an inconsistency now and make people happy later by fixing the inconsistency by making TCPSocket follow the standardized API.

FWIW, I checked what others at sysapps thought about "accept" as event name. There didn't seem to be any terribly strong opinions but the new proposal was to use "connect" as name since that matches what shared workers use for incoming connections

http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#shared-workers-and-the-sharedworkerglobalscope-interface

Let me know if that's an acceptable name.


I don't know if we support constructors yet. Mrbkap might.

Comment 63

[Tomoaki Konno]

Attachment: A new patch to use TCP Server Socket API

This is a patch created by diff between my modification and the revision for "76e1987" of gecko based on Honza's comments.
This patch was produced with "git diff -U8 --patience".

This has not been modified based on sr and 
not been merged with the patch of Bug 831107 yet.

Comment 64

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #53)
> Comment on attachment 731610 [details] [diff] [review]
 
> - Why did you remove run of the test in child?  This is extremely important
> to run in IPC.
This test has been reverted.
 
> ::: dom/network/interfaces/nsITCPSocketChild.idl
> @@ +24,5 @@
> >    void close();
> > +
> > +  // Initialize a child socket object. It is called from the child side socket,
> > +  // which is generated in receiving a notification of accepting any open request
> > +  // on the parent side. The socket after being initialized will be established.
> 
> what is "The socket" ?  What is "being initialized" and what means "will be
> established" ?
> 
> It's unclear at all what this method is supposed to do.
> 
> @@ +30,5 @@
> > +  // @param socket
> > +  //        The socket on the child side.
> > +  // @param socketVal
> > +  //        The socket on the child side for deserialization 
> > +  //        from an other implementation format to a javascript format.
> 
> No idea what this is.

I modified the comment.
Let me know if it is not still unclear.
Possibly, Josh might understand the detail of this implementation well.

> ::: netwerk/ipc/NeckoParent.cpp
> @@ -271,5 @@
> > -  if (UsingNeckoIPCSecurity() && !aBrowser) {
> > -    printf_stderr("NeckoParent::AllocPTCPSocket: FATAL error: no browser present \
> > -                   KILLING CHILD PROCESS\n");
> > -    return nullptr;
> > -  }
> 
> Why this has been removed? (Maybe already discussed, just don't remember)

This was removed because "aBrowser" has already been removed.

Comment 65

[Tomoaki Konno]

(In reply to Jonas Sicking (:sicking) from comment #62) 
> Let me know if that's an acceptable name.
That name is acceptable for me.
I understood your direction about changing APIs and the background of naming.

Comment 66

[Honza Bambas (:mayhemer)]

Comment on attachment 742336 [details] [diff] [review]
A new patch to use TCP Server Socket API

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

When can I expect patch for full review?

::: dom/network/interfaces/nsITCPSocketChild.idl
@@ +12,1 @@
>  interface nsITCPSocketChild : nsISupports

Would be good to express what exactly implements this interface.

@@ +22,5 @@
>    void resume();
>    void suspend();
>    void close();
> +
> +  /** Initialize a child socket object for IPC. It is called from the child side,

/**
 * Initiaze ...

@@ +25,5 @@
> +
> +  /** Initialize a child socket object for IPC. It is called from the child side,
> +   * which is generated in receiving a notification of accepting any open request
> +   * on the parent side. The child socket object will interact
> +   * with the corresponding parent socket object through the IPC.

The child socket object will interact
		* with the corresponding parent socket object through the IPC.

This sentence just confuses.  This is obvious.

@@ +28,5 @@
> +   * on the parent side. The child socket object will interact
> +   * with the corresponding parent socket object through the IPC.
> +   *
> +   * @param socket
> +   *        The child socket object to interact

"The TCP socket on the child side." seems more explanatory.

It should be clear whether you are connecting the child IPC side of the IPC bridge to this instance or this instance is the IPC bridge child side.

That is still not clear at all from the comments.

Comment 67

[Blake Kaplan (:mrbkap) (inactive)]

I'm pretty sure that constructors can take arguments now. nsIDOMGlobalObjectConstructor certainly seems to take arguments.

Comment 68

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #66)
> Comment on attachment 742336 [details] [diff] [review]
> A new patch to use TCP Server Socket API
> 
> Review of attachment 742336 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When can I expect patch for full review?

Sorry. I'd like to ask you to fully review the patch next week.

Comment 69

[DongShengXue]

Now I used the last patch. When I make a connection from a socket client to to server using binaryType "arraybuffer" and read a data from the socket in server, I am unable to read the buffer. Specifically, 

  socket.ondata = function(event) {
    var receivedData=event.data;
    var recvLength = receivedData.byteLength !== undefined ? receivedData.byteLength : receivedData.length;
    var recvBufferView = new Uint8Array(receivedData);
  }

shows that receivedData.byteLength is (e.g.) 4.  But var recvBufferView = new Uint8Array(receivedData); failed: Error: Permission denied to access object. Why ? And How can I use receivedData? Thanks.

Comment 70

[Tomoaki Konno]

(In reply to DongShengXue from comment #69)

"event.data" is Uint8Array object because this patch hasn't reflected Bug 831107
and Bug 865652 yet.
So, you don't have to reconvert "receivedData" to Uint8Array object.
And also, "recvLength" would be "receivedData.length" (not "receivedData.byteLength).

But I'm not sure whether that is the cause of the error (Permision denied ...).
Thanks.

Comment 71

[Josh Matthews [:jdm]]

It's almost certainly caused by createAcceptedChild passing in a chrome socket object, which is used later by TCPSocketChild as a basis for creating new JS values. You'll need to have code like http://mxr.mozilla.org/mozilla-central/source/dom/network/src/TCPSocketChild.cpp#86, and pass this._useWin (like http://mxr.mozilla.org/mozilla-central/source/dom/network/src/TCPSocket.js#389).

Comment 72

[DongShengXue]

(In reply to Tomoaki Konno from comment #70)
Thank you for your reply. In previous versions "event.data" really is Uint8Array object. But the last version, the send data and received data type are all ArrayBuffer objects. So the even.data has no  attribute named length, system will be prompted to type is  undefined.

Comment 73

[DongShengXue]

(In reply to Josh Matthews from comment #71)
Thank you for your reply. Sorry I can't clearly understand your suggestion. I create a socket server in phone side by window.navigator.mozTCPSocket.listen(this.PORT, this.OPTIONS, this.BACKLOG); When I accept a connection from client, I set ondata method to my function. Behind what happened as before said（comment 69），I saw the test_tcpsocket.js and test_tcpserversocket.js, the code is similar with mine. My service can work in the old version, I am not sure what can I do now.

Comment 74

[Tomoaki Konno]

(In reply to DongShengXue from comment #72)

To reconfirm, do the previous versions and the last version mean the version of gecko or patch?
If gecko, since that doesn't properly work, please wait for our new patch that will support　ArrayBuffer object along with Bug 831107 and Bug 865652 (or please reverse your gecko version previous to "76e1987").
Conversely, if patch, I haven't figured out what's behind yet. It seems to receive Uint8Array object in "event.data" on my environment.

Comment 75

[DongShengXue]

(In reply to  Tomoaki Konno from comment #74)

Thanks a lot. My gecko version is new. I got it.

Comment 76

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

Here's the latest spec for the TCPServerSocket API:

http://raw-sockets.sysapps.org/#interface-tcpserversocket

The main differences appear to be:

Uses "localPort" rather than "port". I don't have a strong opinion on this, but it does make it more consistent to the TCPSocket and UDPSocket APIs. Let me know what you think.

Supports passing a "localAddress" as well as port. Unless we have use cases for this, I don't think we should implement this part yet. This seems much more useful for servers.

Uses "connect" rather than "accept" as event name.

Supports "suspend()" and "resume()". Again, unless there are use cases for this I don't know that we need to be in a hurry to implement it. This does seem more useful though. But it obviously doesn't need to be in the initial version.

Supports SSL connections. Again, this seems useful, but probably better to wait for a followup patch.

Uses a constructor dictionary. Would be great to do this. Even if we only support "port" for now.

Comment 77

[Donovan Preston [:fzzzy]]

(In reply to Jonas Sicking (:sicking) from comment #76)
> Here's the latest spec for the TCPServerSocket API:
> 
> http://raw-sockets.sysapps.org/#interface-tcpserversocket
> 
> The main differences appear to be:
> 
> Uses "localPort" rather than "port". I don't have a strong opinion on this,
> but it does make it more consistent to the TCPSocket and UDPSocket APIs. Let
> me know what you think.

I think localPort is a good name for it. Might as well be overly specific about which port it is.

> Supports passing a "localAddress" as well as port. Unless we have use cases
> for this, I don't think we should implement this part yet. This seems much
> more useful for servers.

Yes, I think having a localAddress is a good idea just for completeness. I can't imagine the phones are going to have more than one interface and tcp address, but it could be possible? The one use case that springs to mind that is immediately useful is binding to "127.0.0.1" rather than "0.0.0.0" to only allow connections to be made from the phone and not from any other interfaces present on the phone.

> Uses "connect" rather than "accept" as event name.

Personally I think accept is a better name, but I don't care too much. connect would be ok I guess.

> Supports "suspend()" and "resume()". Again, unless there are use cases for
> this I don't know that we need to be in a hurry to implement it. This does
> seem more useful though. But it obviously doesn't need to be in the initial
> version.

Are you talking about suspend() and resume() on the listen socket? So after calling suspend the socket will no longer accept new connections until resume is called? I'm not sure I can think of a use case for this, but it may be useful for something...

If it's suspend and resume on the accepted socket you are talking about, that should already work since client sockets have suspend and resume and once a connection has been accepted the responsibility is handed over to the client socket implementation.

> Supports SSL connections. Again, this seems useful, but probably better to
> wait for a followup patch.

Yes this seems useful, but I wonder where the certificate would come from. Stored on the sd card somewhere? Seems like this needs more thought.

> Uses a constructor dictionary. Would be great to do this. Even if we only
> support "port" for now.

Agreed, a constructor dict would be good.

Comment 78

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

(In reply to Donovan Preston from comment #77)
> > Supports passing a "localAddress" as well as port. Unless we have use cases
> > for this, I don't think we should implement this part yet. This seems much
> > more useful for servers.
> 
> Yes, I think having a localAddress is a good idea just for completeness. I
> can't imagine the phones are going to have more than one interface and tcp
> address, but it could be possible? The one use case that springs to mind
> that is immediately useful is binding to "127.0.0.1" rather than "0.0.0.0"
> to only allow connections to be made from the phone and not from any other
> interfaces present on the phone.

Let's not implement this for now. We can re-evaluate if people are asking for it.

> > Supports "suspend()" and "resume()". Again, unless there are use cases for
> > this I don't know that we need to be in a hurry to implement it. This does
> > seem more useful though. But it obviously doesn't need to be in the initial
> > version.
> 
> Are you talking about suspend() and resume() on the listen socket? So after
> calling suspend the socket will no longer accept new connections until
> resume is called?

Yes

> I'm not sure I can think of a use case for this, but it
> may be useful for something...

I think the idea is to limit to only having N open connections. By using suspend/resume you can .suspend() once you have N open connections and .resume() once one of those connections are closed.

My assumption is that this behaves differently from simply calling .close() and reopening a new server as needed. Maybe it still occupies the port so that no-one else can't open a server to the same port?

> > Supports SSL connections. Again, this seems useful, but probably better to
> > wait for a followup patch.
> 
> Yes this seems useful, but I wonder where the certificate would come from.
> Stored on the sd card somewhere? Seems like this needs more thought.

Agreed.

Comment 79

[Donovan Preston [:fzzzy]]

(In reply to Jonas Sicking (:sicking) from comment #78)
> I think the idea is to limit to only having N open connections. By using
> suspend/resume you can .suspend() once you have N open connections and
> .resume() once one of those connections are closed.
> 
> My assumption is that this behaves differently from simply calling .close()
> and reopening a new server as needed. Maybe it still occupies the port so
> that no-one else can't open a server to the same port?

Ah yes, that makes a lot of sense. Yes, it would still occupy the port so that no one else can take the port. Sounds good.

Comment 80

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Jonas Sicking (:sicking) from comment #78)
> (In reply to Donovan Preston from comment #77)
> > 
> > Yes, I think having a localAddress is a good idea just for completeness. I
> > can't imagine the phones are going to have more than one interface and tcp
> > address, but it could be possible? The one use case that springs to mind
> > that is immediately useful is binding to "127.0.0.1" rather than "0.0.0.0"
> > to only allow connections to be made from the phone and not from any other
> > interfaces present on the phone.
> 
> Let's not implement this for now. We can re-evaluate if people are asking
> for it.

A phone with an active WiFi connection and a cellular connection will most definitely have two interfaces; a further complication is that both networks may get the same (private) IP address.

If we support Bluetooth or USB networking, we may get additional interfaces and addresses from those as well.

Comment 81

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

I agree that having multiple IP numbers for a single device might not be rare. But what are the use cases for opening a TCP server to a specific IP number on an end user device? Rather than opening it on the "default" internet-connected interface?

Comment 82

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Jonas Sicking (:sicking) from comment #81)
>
> I agree that having multiple IP numbers for a single device might not be
> rare. But what are the use cases for opening a TCP server to a specific IP
> number on an end user device? Rather than opening it on the "default"
> internet-connected interface?

In my experience, the only real cases are services that _must_ go over the cellular network, because they're handled internally by the carrier's private network. My memory is a bit fuzzy, but I think there was a use-case where MMS objects were transferred from the phone to the carrier network using a network-originated TCP connection. (i.e. the handset opens a server socket on the cellular interface, then sends an SMS to the network saying "I've got something for you", which prompts the network to open a new TCP connection to the handset.) If we have an in-house MMS expert, s/he would know more.

I guess a flip-side could be if someone wanted to provide a LAN-only service (e.g. media streaming, local real-time multiplayer games), in which case being able to force/limit connections to the WiFi interface would be useful.

Comment 83

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

Do we keep the 3G data connection enabled when the user connects to WiFi? In any case, we'd need a way to enumerate the IP of the various interfaces. So this sounds like fodder for a followup bug.

Comment 84

[Donovan Preston [:fzzzy]]

That's funny, because I have on my todo list "Talk to Jonas about API for getting local IP address" so now I can check it off :-)

Comment 85

[Honza Bambas (:mayhemer)]

(In reply to Donovan Preston from comment #79)
> (In reply to Jonas Sicking (:sicking) from comment #78)
> > I think the idea is to limit to only having N open connections. By using
> > suspend/resume you can .suspend() once you have N open connections and
> > .resume() once one of those connections are closed.
> > 
> > My assumption is that this behaves differently from simply calling .close()
> > and reopening a new server as needed. Maybe it still occupies the port so
> > that no-one else can't open a server to the same port?
> 
> Ah yes, that makes a lot of sense. Yes, it would still occupy the port so
> that no one else can take the port. Sounds good.

(Haven't read the spec...)  I wonder how we should behave here regarding clients.  If we leave the socket listening but stop polling it and call accept on it, then it depends on a platform how connections will be handled.  E.g. windows will accept the connection internally regardless call to accept.  Then the client will just hang there with an open TCP conn, sending data to our OS buffer and wasting our (and its) resources anyway.

I would more tend to the following implementation: accept() and immediately close() when in suspended mode.

But I have to think of this more.

Comment 86

[Florian Bender]

(In reply to Jonas Sicking (:sicking) from comment #76)
> Supports passing a "localAddress" as well as port. Unless we have use cases
> for this, I don't think we should implement this part yet. This seems much
> more useful for servers.

Binding to specific ports on the local host does have some possible use cases, e. g. having a DB service running (though this is probably not an immediate use case for plain Firefox OS). Port binding also allows for e. g. easy Port Forwarding when Firefox runs inside a VM (i. e. a "real" simulator). 

Are those use cases you are interested in?

Comment 87

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

I was talking specifically about the address part, not the port. Specifying ports is supported by the current patch.

If someone is interested to keep discussing the ability to specify a local address, then it's probably a better to file a separate bug for that.

Comment 88

[Tomoaki Konno]

Attachment: A new patch to use TCP Server Socket API

This is a patch created by diff between my modification and the revision for "76e1987" (NOT the latest version) of gecko based on Honza's comments with "git diff -U8 --patience".
Also, the name of "accept" has been changed to "connect" based on Jonas's comment and the following spec.
http://raw-sockets.sysapps.org/#interface-tcpserversocket 

But, this has NOT been modified along with the patch of Bug 831107 (that is supported on the latest gecko) and NOT supported for the constructor dict yet.
I'll modify these soon.

Comment 89

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #66)
> 
> ::: dom/network/interfaces/nsITCPSocketChild.idl
> @@ +12,1 @@
> >  interface nsITCPSocketChild : nsISupports
> 
> Would be good to express what exactly implements this interface.

Do you mean that the more detailed information such as “TCPSocketChild.cpp” should be expressed? If so, I don't think so...
In addition, although this explanation was originally written in Bug 770778,
can we modify it?
 
> @@ +25,5 @@
> > +
> > +  /** Initialize a child socket object for IPC. It is called from the child side,
> > +   * which is generated in receiving a notification of accepting any open request
> > +   * on the parent side. The child socket object will interact
> > +   * with the corresponding parent socket object through the IPC.
> 
> The child socket object will interact
> 		* with the corresponding parent socket object through the IPC.
> 
> This sentence just confuses.  This is obvious.

This part was removed from the comment.
 
> @@ +28,5 @@
> > +   * on the parent side. The child socket object will interact
> > +   * with the corresponding parent socket object through the IPC.
> > +   *
> > +   * @param socket
> > +   *        The child socket object to interact
> 
> "The TCP socket on the child side." seems more explanatory.
> 
> It should be clear whether you are connecting the child IPC side of the IPC
> bridge to this instance or this instance is the IPC bridge child side.
> 
> That is still not clear at all from the comments.

I intend to connect the child IPC side of the IPC bridge to this instance (TCP socket).

Comment 90

[Honza Bambas (:mayhemer)]

(In reply to Tomoaki Konno from comment #89)
> Do you mean that the more detailed information such as “TCPSocketChild.cpp”
> should be expressed? If so, I don't think so...
> In addition, although this explanation was originally written in Bug 770778,
> can we modify it?

It's just hard to figure out what this is used for, what is the role of this interface.

This code is overcomplicated (not your fault!) and thus needs to be very well documented.

Let's don't bother with comments in the code, it seems to be hard to explain it there.

Tomoaki, please, rather create a wiki page that will outline this IPC beast implementation overview, how stuff interacts, what has what interface, what class has what role, how one stuff refers other stuff and how events and objects are flowing.  Then you may put a link to this bug.  Thanks.

> I intend to connect the child IPC side of the IPC bridge to this instance
> (TCP socket).

Then write it to the comment not in the bug ;)

Comment 91

[Honza Bambas (:mayhemer)]

Comment on attachment 751651 [details] [diff] [review]
A new patch to use TCP Server Socket API

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

r=honzab

I haven't seen the try run.  Had it been pushed?
Landing this is conditioned by that.

::: dom/network/interfaces/nsITCPSocketChild.idl
@@ +28,5 @@
> +   * which is generated in receiving a notification of accepting any open request
> +   * on the parent side.
> +   *
> +   * @param socket
> +   *        The TCP socket on the child side.

You may want to note here (if I am correct!) that: 

We use single implementation that works on a child process as well as in the single process model.

Comment 92

[Tomoaki Konno]

Attachment: A new patch to use TCP Server Socket API

This is a patch created by diff between my modification and the revision for "489c6ee" of gecko with "git diff -U8 --patience".
This has supported the modification in Bug 831107, Bug 861196 and so on, so that it can be applied to the latest gecko.
And also, the name of "port" for the tcp server socket has been changed to "localPort" along with the following spec.
http://raw-sockets.sysapps.org/#interface-tcpserversocket 

But, this has NOT changed for supporting the constructor dict yet.
I'll modify it soon.

Comment 93

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #90)
> Let's don't bother with comments in the code, it seems to be hard to explain
> it there.
I agree with you.
 
> Tomoaki, please, rather create a wiki page that will outline this IPC beast
> implementation overview, how stuff interacts, what has what interface, what
> class has what role, how one stuff refers other stuff and how events and
> objects are flowing.  Then you may put a link to this bug.  Thanks.
I see. I would be glad to get the cooperation from other persons such as Josh for creating the wiki page.

Comment 94

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #91)
> I haven't seen the try run.  Had it been pushed?
> Landing this is conditioned by that.
Sorry to be late. I'll do the try in about a week.
After the try test, I'd like to ask you to fully review my patch.
Thanks.

Comment 95

[Honza Bambas (:mayhemer)]

Comment on attachment 756515 [details] [diff] [review]
A new patch to use TCP Server Socket API

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

Sorry for delay, I'm rewriting HTTP cache.  If there will be need for other review round (I sincerely hope not!) push that time more to get this done asap.

Do the try run.  Ask others for help if needed.

This was only a light review, I rely on you to do proper testing (run your test locally, on more then one machine only if possible) and do the try run.

r=honzab

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +71,5 @@
> +   *
> +   * @return The new TCPServerSocket instance.
> +   */
> +  nsIDOMTCPServerSocket listen(in unsigned short localPort, [optional] in jsval options,
> +                               [optional] in unsigned short backlog);

I think this is change in nsITCPSocketInternal interface, so it needs to change iid.

::: dom/network/src/TCPSocket.js
@@ +344,5 @@
> +    that._binaryType = binaryType;
> +    that._inChild = true;
> +    that._readyState = kOPEN;
> +    that.useWin = aWindow;
> +    socketChild.setSocket(that, that.useWin);

According:

 * @param socketVal
		* The TCP socket jsval on the child side to create data
		* as "jsval" for deserialization.
		*/
		[implicit_jscontext]
		void setSocket(in nsITCPSocketInternal socket, in jsval socketVal);

I think the second arg is wrong, or do I miss something?

Note: I'm no expert to the "jsval" stuff at all, maybe someone else should check on that then me.

::: dom/network/src/TCPSocket.manifest
@@ +8,5 @@
>  contract @mozilla.org/tcp-socket-intermediary;1 {afa42841-a6cb-4a91-912f-93099f6a3d18}
> +
> +# TCPServerSocket.js
> +component {73065eae-27dc-11e2-895a-000c29987aa2} TCPServerSocket.js
> +contract @mozilla.org/tcp-server-socket;1 {73065eae-27dc-11e2-895a-000c29987aa2}
\ No newline at end of file

Add new line at the end of this file.

::: dom/network/tests/unit/xpcshell.ini
@@ +3,5 @@
>  tail =
>  
>  [test_tcpsocket.js]
> +[test_multisend.js]
> +[test_tcpserversocket.js]
\ No newline at end of file

Again, new line at the end of the file.

::: layout/build/nsLayoutModule.cpp.orig
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Please remove this .orig file...

Comment 96

[Josh Matthews [:jdm]]

(In reply to Honza Bambas (:mayhemer) from comment #95) 
> ::: dom/network/src/TCPSocket.js
> @@ +344,5 @@
> > +    that._binaryType = binaryType;
> > +    that._inChild = true;
> > +    that._readyState = kOPEN;
> > +    that.useWin = aWindow;
> > +    socketChild.setSocket(that, that.useWin);
> 
> According:
> 
>  * @param socketVal
> 		* The TCP socket jsval on the child side to create data
> 		* as "jsval" for deserialization.
> 		*/
> 		[implicit_jscontext]
> 		void setSocket(in nsITCPSocketInternal socket, in jsval socketVal);
> 
> I think the second arg is wrong, or do I miss something?
> 
> Note: I'm no expert to the "jsval" stuff at all, maybe someone else should
> check on that then me.

The documentation (and argument name) is wrong here; we're passing a jsval so we can use its compartment, so we want the content window object here.

Comment 97

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #95)
> Comment on attachment 756515 [details] [diff] [review]
> A new patch to use TCP Server Socket API
> 
> Review of attachment 756515 [details] [diff] [review]:
> ----------------------------------------------------------------- 
> Do the try run.  Ask others for help if needed.
> 
> This was only a light review, I rely on you to do proper testing (run your
> test locally, on more then one machine only if possible) and do the try run.

I conducted the test with try server as bellow.
While the test on B2G platform was successful, 
the test on other platform was not successful.

https://tbpl.mozilla.org/?tree=Try&rev=a430ac2b0135

Is it essential to pass all tests on all platforms for check-in to B2G repo?
Sorry, since I'm not familiar with try server test...

Comment 98

[[:fabrice] Fabrice Desré]

(In reply to Tomoaki Konno from comment #97)

> 
> I conducted the test with try server as bellow.
> While the test on B2G platform was successful, 
> the test on other platform was not successful.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=a430ac2b0135
> 
> Is it essential to pass all tests on all platforms for check-in to B2G repo?
> Sorry, since I'm not familiar with try server test...

yes, builds and tests need to be green on all platforms, since this is a single repository.

Some build errors like https://tbpl.mozilla.org/php/getParsedLog.php?id=23993845&tree=Try should not be hard to fix.

The M3 failures should be fixed by adding TCPServerSocket to https://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/test_interfaces.html?force=1#487

Comment 99

[Tomoaki Konno]

(In reply to Fabrice Desré [:fabrice] from comment #98)
Thanks for your early response. I see.

Comment 100

[Kai-Chih Hu [:khu]]

Trying to explore the possibility to mark it as koi+. Or, also fine to land it to later versions.

Comment 101

[[:fabrice] Fabrice Desré]

Tomoaki, will you have time to fix the test errors?

Comment 102

[Tomoaki Konno]

(In reply to Fabrice Desré [:fabrice] from comment #101)
Yes. I'm correcting the error and I will, though it is taking time to fix it. 
Sorry for delay.

Comment 103

[Tomoaki Konno]

Attachment: A patch to use TCP Server Socket API

This is a patch created by diff between my modification and the revision for "136989:5e66712edb18" of mozilla-central.

The patch was tested on Try server.
I guess that errors in the test result might be caused by the other bugs.
https://tbpl.mozilla.org/?tree=Try&rev=3fd2aabbed7f

But, this has NOT changed for supporting the constructor dict yet.

Comment 104

[Honza Bambas (:mayhemer)]

(In reply to Tomoaki Konno from comment #103)
> The patch was tested on Try server.
> I guess that errors in the test result might be caused by the other bugs.
> https://tbpl.mozilla.org/?tree=Try&rev=3fd2aabbed7f

This must run mainly on B2G.  And the try run doesn't show B2G run.  I'm not sure why...  But I use just "try: -a" when I want the patch go a complete coverage.  Your "-p all" may not do it.

Comment 105

[Honza Bambas (:mayhemer)]

Comment on attachment 777599 [details] [diff] [review]
A patch to use TCP Server Socket API

I just quickly went through the patch. 

Please push again to try with "try: -a" to get B2G results.  If OK, land it.

Comment 106

[Tomoaki Konno]

(In reply to Honza Bambas (:mayhemer) from comment #105)
> Comment on attachment 777599 [details] [diff] [review]

Thanks, Honza.
The try run with "try: -a" was conducted.
https://tbpl.mozilla.org/?tree=Try&rev=b1aedc435e74

Since there have seemed to remain some errors in the patch,
I'll correct them.
I would appreciate it if anyone give any ideas or support to fix it.

Comment 107

[Josh Matthews [:jdm]]

You're missing the mobile manifest: http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in

Comment 108

[Tomoaki Konno]

Attachment: A patch to use TCP Server Socket API

This patch is tweaked from a previous patch for just memo.
This is a patch created by diff between my modification and the revision for "138805:d5469522e728" of mozilla-inbound (NOT mozilla-central).

The patch was tested on Try server with "try:-a".
There have seemed to remain some errors in the patch.
https://tbpl.mozilla.org/?tree=Try&rev=47fc0118352e

But, this has NOT changed for supporting the constructor dict yet.

Comment 109

[Tomoaki Konno]

(In reply to Josh Matthews [:jdm] from comment #107)
> You're missing the mobile manifest:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/
> package-manifest.in
Thanks, Josh.
The manifest was added in the above patch (and also changed to mozilla-inbound).
https://tbpl.mozilla.org/?tree=Try&rev=47fc0118352e

Comment 110

[Tomoaki Konno]

Attachment: A patch to use TCP Server Socket API

This is a patch created by diff between my modification and the revision for "139942:973361ec4fb5" of mozilla-inbound (NOT mozilla-central).

The patch was tested on Try server with "try:-a".
https://tbpl.mozilla.org/?tree=Try&rev=c7ff0c225ce5


Although there have remained 4 errors in the test result,
the errors may be caused by the other bugs. But, I'm not sure.
Is this result not sufficient for landing?

(this has NOT changed for supporting the constructor dict yet.)

Comment 111

[[:fabrice] Fabrice Desré]

Yes, the tests failures are unrelated oranges. Are there any differences between this patch and the one that already got r+ from Honza?

Comment 112

[Tomoaki Konno]

There are few differences.
The base repo was changed to mozilla-inbound.
The mobile manifest file was modified based on the above Josh's comment.

Comment 113

[[:fabrice] Fabrice Desré]

(In reply to Tomoaki Konno from comment #112)
> There are few differences.
> The base repo was changed to mozilla-inbound.
> The mobile manifest file was modified based on the above Josh's comment.

Ok, thanks!

Honza, do you object carrying your r+ forward?

Comment 114

[Masatoshi Kimura [:emk]]

Comment on attachment 782091 [details] [diff] [review]
A patch to use TCP Server Socket API

>--- a/dom/tests/mochitest/general/test_interfaces.html
>+++ b/dom/tests/mochitest/general/test_interfaces.html
>@@ -469,16 +469,17 @@ var interfaceNamesInGlobalScope =
>     "TCPSocket",
>+    "TCPServerSocket",
>     "Text",

Do you really need to expose "TCPServerSocket" on the global object of all pages (including non-B2G)? If not, please consider renaming the interface class name to "nsITCPServerSocket".

Comment 115

[Honza Bambas (:mayhemer)]

(In reply to Fabrice Desré [:fabrice] from comment #113)
> (In reply to Tomoaki Konno from comment #112)
> > There are few differences.
> > The base repo was changed to mozilla-inbound.
> > The mobile manifest file was modified based on the above Josh's comment.
> 
> Ok, thanks!
> 
> Honza, do you object carrying your r+ forward?

None of the changes sound like major modifications that would need me to look over.  Please land.

Comment 116

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/birch/rev/22f3531d579f

Comment 117

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22f3531d579f

Comment 118

[Honza Bambas (:mayhemer)]

Comment on attachment 782091 [details] [diff] [review]
A patch to use TCP Server Socket API

apparently r+

Comment 119

[Jeremie Patonnier :Jeremie]

Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/API/TCPSocket.listen
https://developer.mozilla.org/en-US/docs/Web/API/TCPServerSocket

(feel free to review it)

Comment 120

[Tomoaki Konno]

Jeremie, I greatly appreciate your support.

Comment 121

[Andrew Overholt [:overholt]]

Removing koi? since this landed on central and will be in 1.2 by virtue of being in 26.