We need a default control channel, a plain TCP socket, for mDNS/DNS-SD service provider.

tcp device and control channel for presentation api Not in this patch: 1. If a ua not in devices list connect to this server (|TCPPresentationServer.onSessionRequest|, should have a way to force discovery or do something. 2. timeout/keepalive for |establishControlChannel|

Rename to TCPPresentationServer.js

Return the correct port in TCPPresentationServer.

tcp device and control channel for presentation api Modify the comment in the idl

Add the attribute |id| to make the server more elastic to satisfy the mdns use case. Fabrice and S.C., Sorry for spam a lot, but I think the patch is ready to review. May I have your review and feedback on this? xeonchen, could you check if the interface satisfies your need? Last but not least, I'd like to file follow-up bugs for: 1. The first point mentioned in comment 1 2. SSL/TLS control channel

attach the interdiff of the last two patches

Comment on attachment 8560367 [details] [diff] [review] tcp-control-channel Review of attachment 8560367 [details] [diff] [review]: ----------------------------------------------------------------- I did a first (quick) pass. I don't think it's ready yet. It looks like some parts of the implementation don't match the IDL. Also please fix all the style nits, that may sound picky but that helps when reviewing large files (proper and consistent indentation, spaces after |if| and |for|, etc). That would also help to have comments that explain the overall setup, the flow of the code and the format of the messages. ::: dom/presentation/interfaces/nsITCPPresentationServer.idl @@ +11,5 @@ > +{ > + /** > + * Callback while the server socket stops listening. > + * @param aReason > + * The reason of the socket close What are the possible reasons? @@ +26,5 @@ > + /** > + * This method initializes a TCP presentation server. > + * @param aId > + * The unique Id for the device within the discovery scope. If aId > + * is null or opt-out, the TCP presenetation server should not work nit: s/presenetation/presentation ::: dom/presentation/provider/TCPPresentationServer.js @@ +46,5 @@ > + init: function(aId, aPort) { > + log("TCPPresentationServer - init id: " + aId); > + this.id = aId; > + // 0 indicates opt-out parameter, and a port will be selected automatically. > + this._port = (aPort != 0) ? aPort : -1; what if aPort is undefined? @@ +51,5 @@ > + > + if(this._serverSocket) { > + log("TCPPresentationServer - server socket has been inited"); > + throw Cr.NS_ERROR_FAILURE; > + } Shouldn't this be before even setting this._port ? @@ +60,5 @@ > + this._serverSocket.init(this._port, false, -1); > + this._serverSocket.asyncListen(this); > + this._port = this._serverSocket.port; > + } catch (e) { > + //NS_ERROR_SOCKET_ADDRESS_IN_USE nit: space after // @@ +72,5 @@ > + }, > + > + set id(aId) { > + if(this._id) { > + throw Cr.NS_ERROR_FAILURE; maybe don't throw is aId === this._id ? @@ +74,5 @@ > + set id(aId) { > + if(this._id) { > + throw Cr.NS_ERROR_FAILURE; > + } > + this._id = aId; from the idl "the TCP presentation server should not work until the |id| is set.". But here we're not starting the server? @@ +90,5 @@ > + return this._listener; > + }, > + > + get _isInit() { > + return this._id!==null; nit: spaces around !== @@ +118,5 @@ > + > + getTCPDevice: function(aId) { > + if(!this._devices[aId]) { > + throw Cr.NS_ERROR_NOT_AVAILABLE; > + } I think that returning null is fine too. @@ +132,5 @@ > + }, > + > + requestSession: function(aDevice, aUrl, aPresentationId) { > + if(!this._isInit) > + { nit: here and later, |if (...) {| @@ +145,5 @@ > + 0, > + aDevice.host, > + aDevice.port, > + null); > + what is that fails? @@ +153,5 @@ > + aPresentationId, > + 'sender', > + aUrl); > + > + return controlChannel; just |return new TCPControlChannel(...);| @@ +170,5 @@ > + null, // presentation ID > + 'receiver', > + null // url > + ); > + return controlChannel; here too @@ +282,5 @@ > + > + this._presentationServer = presentationServer; > + > + let currentThread = Cc["@mozilla.org/thread-manager;1"] > + .getService().currentThread; nit: you can use Services.tm.currentThread; @@ +364,5 @@ > + //XXX End line for seperating the concatenated json in tcp stream > + let message = JSON.stringify(aMsg) + '\n'; > + let rawMessage = converter.convertToByteArray(message); > + try { > + this._output.write(message, message.length); hm, you convert message into rawMessage but then you don't use it? If you don't need it, you don't need |converter| either. @@ +415,5 @@ > + let data = NetUtil.readInputStreamToString(aInputStream, aInputStream.available()); > + log('TCPControlChannel - onDataAvailable: ' + data); > + > + //XXX To solve concatenated JSON from TCP streaming > + let jsonArray = data.split('\n'); what if the json message is actually multiline? @@ +462,5 @@ > + } > + } > + }, > + > + nit: too many blank lines @@ +465,5 @@ > + > + > + get listener() { > + return this._listener; > + }, nit: missing blank line! @@ +508,5 @@ > + return; > + } > + log("TCPControlChannel - notify opened with role: " + this._direction); > + this._listener.notifyOpened(); > + }, please add blank lines between the functions, and some comments to help understanding what's happening. @@ +554,5 @@ > + > + this._listener.notifyClosed(aReason); > + }, > + > + _convertToJson: function(aDescription) { rename to that doesn't need to be a method on this object. Make it a global and rename to something like descriptionAsJSON() ::: dom/presentation/provider/moz.build @@ +12,5 @@ > +FAIL_ON_WARNINGS = True > + > +include('/ipc/chromium/chromium-config.mozbuild') > + > +FINAL_LIBRARY = 'xul' you don't need these 3 lines.

Fabrice, thanks for your review. Will fix all the nits in next patch. (In reply to Fabrice Desré [:fabrice] from comment #7) > ::: dom/presentation/provider/TCPPresentationServer.js > @@ +46,5 @@ > > + init: function(aId, aPort) { > > + log("TCPPresentationServer - init id: " + aId); > > + this.id = aId; > > + // 0 indicates opt-out parameter, and a port will be selected automatically. > > + this._port = (aPort != 0) ? aPort : -1; > > what if aPort is undefined? |init| is for the idl and should not be called by direct function call. Will throw in next patch. > @@ +74,5 @@ > > + set id(aId) { > > + if(this._id) { > > + throw Cr.NS_ERROR_FAILURE; > > + } > > + this._id = aId; > > from the idl "the TCP presentation server should not work until the |id| is > set.". But here we're not starting the server? This is kinda complicated. The thing is mdns needs the port to register mDNSResponder to get the |id|, and nsITCPPresentationServer need |id| to establish control channel. The flow may like: TCPPresentationServer.init(); mdns.register(TCPPresentationServer.port); TCPPresentationServer.id = mdns.id; Hence, I init the server socket first to get port, and check |id| for the control channel establishment by the guarder |_isInit:this._id !== null| But I should this to save resource: Init the server socket and get the available port while init and leave |asyncListen| when the |id| is appropriately set. > @@ +145,5 @@ > > + 0, > > + aDevice.host, > > + aDevice.port, > > + null); > > + > > what is that fails? > > If throw, it should pop the exception to |TCPDevice.establishControlChannel|, and I should add comment to this function and |nsIPresentationDevice.idl|. But I found lots of kinds exception may be throw in |createTransport|. This makes me hard to add comment in idl since the comment style is |@throws NS_XXX_FAILURE| Thoughts? > @@ +415,5 @@ > > + let data = NetUtil.readInputStreamToString(aInputStream, aInputStream.available()); > > + log('TCPControlChannel - onDataAvailable: ' + data); > > + > > + //XXX To solve concatenated JSON from TCP streaming > > + let jsonArray = data.split('\n'); > > what if the json message is actually multiline? > Will remove the line breaks in the json while sending packet. > @@ +508,5 @@ > > + return; > > + } > > + log("TCPControlChannel - notify opened with role: " + this._direction); > > + this._listener.notifyOpened(); > > + }, > > please add blank lines between the functions, and some comments to help > understanding what's happening. > For the functions, I rename _doOpen/_doClose to _notifyOpened/_notifyClosed to match the idl of the listener and add comment.

WIP patch modify by Fabrice's review comment.

interdiff

Comment on attachment 8561333 [details] [diff] [review] tcp-control-channel Review of attachment 8561333 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/presentation/provider/TCPPresentationServer.js @@ +71,5 @@ > + return this._id; > + }, > + > + set id(aId) { > + if (!aId || aId === this._id) { In C++ implementation, aId is |const nsACString& aId| and therefore |if (!aId...) {| will always be |false|.

(In reply to Liang-Heng Chen [:xeonchen] from comment #11) > Comment on attachment 8561333 [details] [diff] [review] > tcp-control-channel > > Review of attachment 8561333 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/presentation/provider/TCPPresentationServer.js > @@ +71,5 @@ > > + return this._id; > > + }, > > + > > + set id(aId) { > > + if (!aId || aId === this._id) { > > In C++ implementation, aId is |const nsACString& aId| and therefore |if > (!aId...) {| will always be |false|. Thanks. But we still need a way to let cpp user delay id setting, I'll provide |aId.length == 0| for this purpose.

1. modify by comment 8 and comment 12 2. add error handling for sendOffer/sendAnswer/_sendInit 3. For |createTransport| in |TCPDevice.establishControlChannel|, I just absorb the error and throw Cr.NS_ERROR_FAILURE, similiar with 2. I also notice that I have lots of |JSON.stringify| for debug logging. It is painful for performance but needed for debugging. Any suggestion?

package-manifest.in for browser/android installer try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f8a9f313f90

Comment on attachment 8564890 [details] [diff] [review] tcp-control-channel Review of attachment 8564890 [details] [diff] [review]: ----------------------------------------------------------------- We're getting close! I'd like to understand why the code in onSocketAccepted() is correct though. Also, I feel that we have fairly minimal test coverage, but it's ok to add more in a followup. ::: dom/presentation/provider/TCPPresentationServer.js @@ +6,5 @@ > +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > +const { NetUtil } = Cu.import("resource://gre/modules/NetUtil.jsm", {}); Any reason to not just do Cu.import("resource://gre/modules/NetUtil.jsm") ? @@ +135,5 @@ > + }, > + > + requestSession: function (aDevice, aUrl, aPresentationId) { > + if (!this._isInit()) > + { nit: { on the previous line. @@ +161,5 @@ > + return new TCPControlChannel(this, > + socketTransport, > + aDevice, > + aPresentationId, > + 'sender', nit: here and everywhere else, use double quotes for strings. @@ +167,5 @@ > + }, > + > + responseSession: function (aDevice, aSocketTransport) { > + if (!this._isInit()) > + { nit: { on the previous line. @@ +186,5 @@ > + // Triggered by TCPControlChannel > + onSessionRequest: function (aId, aUrl, aPresentationId, aControlChannel) { > + let device = this._devices[aId]; > + if (!device) { > + //XXX should have a way to recovery please file a bug to add the bug number to this comment. @@ +202,5 @@ > + onSocketAccepted: function (aServerSocket, aClientSocket) { > + log("TCPPresentationServer - onSocketAccepted: " + aClientSocket.host + > + ":" + aClientSocket.port); > + let device = new TCPDeviceInfo(aClientSocket.host, aClientSocket.port); > + this.responseSession(device, aClientSocket); What is the object lifetime here? we don't hold on the result of this.responseSession() and |device| is scoped to the function. @@ +406,5 @@ > + throw e.result; > + } > + this._sendingMessage = 'answer'; > + }, > + It looks like we can share a lot of code in _sendInit(), sendOffer() and sendAnswer().

(In reply to Fabrice Desré [:fabrice] from comment #15) > @@ +202,5 @@ > > + onSocketAccepted: function (aServerSocket, aClientSocket) { > > + log("TCPPresentationServer - onSocketAccepted: " + aClientSocket.host + > > + ":" + aClientSocket.port); > > + let device = new TCPDeviceInfo(aClientSocket.host, aClientSocket.port); > > + this.responseSession(device, aClientSocket); > > What is the object lifetime here? we don't hold on the result of > this.responseSession() and |device| is scoped to the function. > ::: dom/presentation/provider/TCPPresentationServer.js @@ +317,5 @@ + this._presentationServer = presentationServer; + + let currentThread = Services.tm.currentThread; + transport.setEventSink(this, currentThread); + I'm not sure. |setEventSink| holds a nsCOMPtr. I guess here's a circular reference of transport and TCPControlChannel, thus causing the return value of this.responseSession never released.

(In reply to Junior Hsu [:juniorhsu] from comment #16) > (In reply to Fabrice Desré [:fabrice] from comment #15) > > @@ +202,5 @@ > > > + onSocketAccepted: function (aServerSocket, aClientSocket) { > > > + log("TCPPresentationServer - onSocketAccepted: " + aClientSocket.host + > > > + ":" + aClientSocket.port); > > > + let device = new TCPDeviceInfo(aClientSocket.host, aClientSocket.port); > > > + this.responseSession(device, aClientSocket); > > > > What is the object lifetime here? we don't hold on the result of > > this.responseSession() and |device| is scoped to the function. > > > > ::: dom/presentation/provider/TCPPresentationServer.js > @@ +317,5 @@ > + this._presentationServer = presentationServer; > + > + let currentThread = Services.tm.currentThread; > + transport.setEventSink(this, currentThread); > + > > I'm not sure. > |setEventSink| holds a nsCOMPtr. > I guess here's a circular reference of transport and TCPControlChannel, thus > causing the return value of this.responseSession never released. If a user agent connects to the server, we create a control channel but hand it to |TCPDevice.listener| when the initial information exchange finishes. Therefore, we hold the control channels in this period in next patch. Also |setEventSink(null, null)| in |close| to avoid circular reference.

Modify by comment 15 and comment 17 Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a59b86ed421

Comment on attachment 8573012 [details] [diff] [review] tcp-control-channel Review of attachment 8573012 [details] [diff] [review]: ----------------------------------------------------------------- Looks really good Junior, thanks! I'd just like to take a quick look at the (hopefully final) version. ::: dom/presentation/interfaces/nsITCPPresentationServer.idl @@ +18,5 @@ > + void onClose(in nsresult aReason); > +}; > + > +/** > + * TCP presentation server which can be utilized by discovery services nit: s/utilized/used and add a full stop at the end. ::: dom/presentation/provider/TCPPresentationServer.js @@ +10,5 @@ > +Cu.import("resource://gre/modules/NetUtil.jsm"); > + > +function log(aMsg) { > + //dump("-*- TCPPresentationServer.js: " + aMsg + "\n"); > +} can you disable logging with const instead, and use it to prevent evaluation of the log() parameters when not logging: const DEBUG = false; function log(aMsg) { ... } DEBUG && log("Something went wrong: " + e); @@ +27,5 @@ > + this._serverSocket = null; > +} > + > +TCPPresentationServer.prototype = { > + _devices: {}, // id -> device could you use a Map() instead? @@ +35,5 @@ > + * finishes. Therefore, we hold the control channels in this period. > + */ > + _controlChannels: [], > + > + init: function (aId, aPort) { here and everywhere else: function(...) @@ +37,5 @@ > + _controlChannels: [], > + > + init: function (aId, aPort) { > + if (this._isInit()) { > + log("TCPPresentationServer - server socket has been inited"); nit: s/inited/initialized @@ +49,5 @@ > + > + log("TCPPresentationServer - init id: " + aId + " port: " + aPort); > + > + /** > + * 0 or undifined indicates opt-out parameter, and a port will be selected s/undifined/undefined @@ +84,5 @@ > + throw Cr.NS_ERROR_FAILURE; > + } > + this._id = aId; > + > + if(this._serverSocket) { nit: if (...) @@ +116,5 @@ > + }, > + > + createTCPDevice: function (aId, aName, aType, aHost, aPort) { > + if (this._devices[aId]) { > + throw Cr.NS_ERROR_FAILURE; I would rather throw NS_ERROR_INVALID_ARG @@ +133,5 @@ > + }, > + > + removeTCPDevice: function (aId) { > + if (!this._devices[aId]) { > + throw Cr.NS_ERROR_NOT_AVAILABLE; I would rather throw NS_ERROR_INVALID_ARG @@ +214,5 @@ > + holdControlChannel: function (aControlChannel) { > + this._controlChannels.push(aControlChannel); > + }, > + > + unholdControlChannel: function (aControlChannel) { nit: maybe rename to releaseControlChannel ? @@ +216,5 @@ > + }, > + > + unholdControlChannel: function (aControlChannel) { > + let index = this._controlChannels.indexOf(aControlChannel); > + if(index > -1) { nit: !== -1 instead @@ +360,5 @@ > + _pendingClose: null, > + _pendingCloseReason: null, > + _sendingMessageType: null, > + > + _sendMessage: function (type, JSONData, onThrow) { nit: aType, aJSONData, aOnThrow @@ +422,5 @@ > + log("TCPControlChannel - Send: " + JSON.stringify(aMsg, null, 2)); > + > + /** > + * XXX Following by a line break for seperating the concatenated json in tcp > + * stream. Therefore, need to remove the line breaks for |aMsg|. this comment is not really understandable. ::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js @@ +13,5 @@ > + > +// Call |run_next_test| if all functions in |names| are called > +function makeJointSuccess(names) { > + let funcs = {}, successCount = 0; > + names.forEach(function (name) { nit: function(name)

Comment on attachment 8573012 [details] [diff] [review] tcp-control-channel Review of attachment 8573012 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/presentation/interfaces/nsIPresentationControlChannel.idl @@ +36,5 @@ > interface nsIPresentationControlChannelListener: nsISupports > { > /* > * Callback for receiving offer from remote endpoint. > + * @param offer The received offer. There are several unnecessary changes in these idl files. Please remove it from your patch. ::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js @@ +75,5 @@ > + > + let offer = aOffer.QueryInterface(Ci.nsIPresentationChannelDescription); > + Assert.strictEqual(offer.tcpAddress.queryElementAt(0,Ci.nsISupportsString).data, > + '192.168.123.123', > + 'expected offer address array'); nit: alignment @@ +79,5 @@ > + 'expected offer address array'); > + Assert.equal(offer.tcpPort, 123, 'expected offer port'); > + try { > + let tcpType = Ci.nsIPresentationChannelDescription.TYPE_TCP; > + let offer = new TestDescription(tcpType, ['192.168.321.321'], 321); There are many fake ports and addresses scattered in the entire file. Can we make them const variables?

Modify by comment 19 and comment 20, carry f+ A minor change is: In nsITCPPresentationServer, |GetTCPDevice| throws NS_ERROR_INVALID_ARG if a TCPDevice with |aId| does not exist. It returns |undefined| in previous patch, and gives cpp user an nsCOMPtr whose raw pointer is null, which is not intuitive.

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7088cb4b074

Comment on attachment 8582315 [details] [diff] [review] tcp-control-channel Review of attachment 8582315 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Hi, this failed to apply: renamed 1120308 -> tcp-control-channel applying tcp-control-channel patching file mobile/android/installer/package-manifest.in Hunk #1 FAILED at 453 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/installer/package-manifest.in.rej could you take a look, thanks!

rebase, carry f+ and r+