1120308 - [Presentation WebAPI] control protocol establishment and offer-answer exchange

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Junior [inactive]]

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

Comment 1

[Junior [inactive]]

Attachment: tcp-control-channel

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|

Comment 2

[Junior [inactive]]

Attachment: tcp-control-channel

Rename to TCPPresentationServer.js

Comment 3

[Junior [inactive]]

Attachment: tcp-control-channel

Return the correct port in TCPPresentationServer.

Comment 4

[Junior [inactive]]

Attachment: tcp-control-channel

tcp device and control channel for presentation api

Modify the comment in the idl

Comment 5

[Junior [inactive]]

Attachment: tcp-control-channel

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

Comment 6

[Junior [inactive]]

Attachment: interdiff

attach the interdiff of the last two patches

Comment 7

[[:fabrice] Fabrice Desré]

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.

Comment 8

[Junior [inactive]]

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.

Comment 9

[Junior [inactive]]

Attachment: tcp-control-channel

WIP patch
modify by Fabrice's review comment.

Comment 10

[Junior [inactive]]

Attachment: interdiff

interdiff

Comment 11

[Gary Chen [:xeonchen]]

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|.

Comment 12

[Junior [inactive]]

(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.

Comment 13

[Junior [inactive]]

Attachment: tcp-control-channel

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?

Comment 14

[Junior [inactive]]

Attachment: tcp-control-channel

package-manifest.in for browser/android installer

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f8a9f313f90

Comment 15

[[:fabrice] Fabrice Desré]

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().

Comment 16

[Junior [inactive]]

(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.

Comment 17

[Junior [inactive]]

(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.

Comment 18

[Junior [inactive]]

Attachment: tcp-control-channel

Modify by comment 15 and comment 17

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

Comment 19

[[:fabrice] Fabrice Desré]

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 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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?

Comment 21

[Junior [inactive]]

Attachment: tcp-control-channel

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.

Comment 22

[Junior [inactive]]

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

Comment 23

[[:fabrice] Fabrice Desré]

Comment on attachment 8582315 [details] [diff] [review]
tcp-control-channel

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

r=me

Comment 24

[Carsten Book [:Tomcat]]

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!

Comment 25

[Junior [inactive]]

Attachment: tcp-control-channel

rebase, carry f+ and r+

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11960a87b918

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11960a87b918