1197749 - [TV][2.6] Receive keyboard/pointer event from client in remote control service

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::TV::System, defect, P1)

Description

[Eric Tsai (no more review request accepted)]

We need to enable WebSocket in httpd.js for remote control sending key/touch event to TV.

Comment 1

[[:fabrice] Fabrice Desré]

Why don't you use a webrtc datachannel?

Comment 2

[Eric Tsai (no more review request accepted)]

Hi Fabrice, consider compatibility, and we want mobile client connect to TV server directly, we choose WebSocket in the beginning. But now, it seems we can't make WebSocket works on httpd.js. Now we will move to ajax.

Comment 3

[Eric Tsai (no more review request accepted)]

Using httpd.js as http server to receive keyboard/pointer event, there is some modifications:
1) Create some other entry point to start http server
2) httpd.js only works for localhost when start. A better way is using primary identity to start server.
3) Using sjs extension to handle ajax, I get an exception NS_ERROR_ILLEGAL_VALUE when create sandbox. I use nsIPrincipal to replace gGlobalObject can solve but need to find a formal way.

Comment 4

[Eric Tsai (no more review request accepted)]

May need more discussion on WebSocket support for production and relative works.

Comment 5

[Eric Tsai (no more review request accepted)]

Attachment: 0001-package-httpd-server.patch

Package httpd.js to b2g build

Comment 6

[Eric Tsai (no more review request accepted)]

Attachment: 0002-httpd-js-bind-any-ipaddr.patch

Comment 7

[Josh Cheng [:josh]]

Hi Eric,
Could you provide a target milestone for this bug? Thanks!

Comment 8

[Eric Tsai (no more review request accepted)]

The two patches provide http server for receiving events. Does it means target landing time or ?

Comment 9

[Josh Cheng [:josh]]

(In reply to Eric Tsai from comment #8)
> The two patches provide http server for receiving events. Does it means
> target landing time or ?

Yes, the planned landing date after the patch been reviewed. Thanks!

Comment 10

[Eric Tsai (no more review request accepted)]

Attachment: Add RemoteControlService and for remote control requests

Hi Fabrice, please help to review the WIP of RemoteControlService, which serves static file server script(.sjs) request.

Comment 11

[Eric Tsai (no more review request accepted)]

Attachment: Add RemoteControlService and for remote control requests

Hi Fabrice, please help to review the WIP of RemoteControlService, which serves static file server script(.sjs) request. Fix a bug when initialize.

Comment 12

[Eric Tsai (no more review request accepted)]

Hi Fabrice, since you are busy in other 2.5 bugs. Is there any reviewer you suggest? :schien is not available these two weeks :(

Comment 13

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

Comment on attachment 8674749 [details] [diff] [review]
Add RemoteControlService and for remote control requests

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

I can give some basic feedback before reviewer is decided.

Thanks @etsai! What a great amount of work!
Here is the summary of my feedback:

0. Need architecture diagram for reviewer to understand the control flow and code location.
1. Lazy initialize RemoteControlService.jsm
2. unify the debugging function. (debug/dump/dumpn/...)
3. We put server script in gecko and static file in Gaia, not sure if this is a right architecture. need second eyes on it.

::: b2g/chrome/content/shell.js
@@ +34,4 @@
>  Cu.import('resource://gre/modules/MobileIdentityManager.jsm');
>  Cu.import('resource://gre/modules/PresentationDeviceInfoManager.jsm');
>  Cu.import('resource://gre/modules/AboutServiceWorkers.jsm');
> +Cu.import('resource://gre/modules/RemoteControlService.jsm');

lazy initialize this module.

::: b2g/components/RemoteControlService.jsm
@@ +13,5 @@
> + *
> + * The communication mechanism is based in mozFxAccountsContentEvent (for
> + * messages coming from the UI) and mozFxAccountsChromeEvent (for messages
> + * sent from the chrome side) custom events.
> + */

The entire section of comment is copied from FxAccountsMgmtService.jsm. Please remove it or replace with something related to RemoteControlService.

@@ +18,5 @@
> +
> +"use strict";
> +
> +/* static functions */
> +const DEBUG = true;

Should turn off debugging by default.

@@ +21,5 @@
> +/* static functions */
> +const DEBUG = true;
> +
> +function debug(aStr) {
> +  DEBUG && dump("RemoteControlService: " + aStr + "\n");

If you really want to save the execution time in non-debug environment, you can follow the convention in TCPPresentationServer.js [1]. You'll need to put the short-circuit statement on each |debug| invocation.

https://dxr.mozilla.org/mozilla-central/source/dom/presentation/provider/TCPPresentationServer.js#39

@@ +30,5 @@
> +const { classes: Cc, interfaces: Ci, utils: Cu, Constructor: CC } = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +const { SystemAppProxy } = Cu.import("resource://gre/modules/SystemAppProxy.jsm");

prefer lazy initialization for SystemAppProxy.jsm.

XPCOMUtils.defineLazyModuleGetter(this, "SystemAppProxy",
                                  "resource://gre/modules/SystemAppProxy.jsm");

@@ +43,5 @@
> +                          "nsIBinaryOutputStream",
> +                          "setOutputStream");
> +const ScriptableInputStream = CC("@mozilla.org/scriptableinputstream;1",
> +                          "nsIScriptableInputStream",
> +                          "init");

nit: identation

@@ +48,5 @@
> +
> +var gThreadManager = null;
> +
> +const TVRC_STATIC_BLACKLIST = ['/client.html', '/pairing.html'];
> +const TVRC_SJS = ['/client.sjs', '/pairing.sjs'];

What's the purpose of this black list? I thought we only need a white list and response 404 error for all other accesses.
In addition, I'll prefer this kind of configuration being stored in either gecko preference or some ini file.

@@ +51,5 @@
> +const TVRC_STATIC_BLACKLIST = ['/client.html', '/pairing.html'];
> +const TVRC_SJS = ['/client.sjs', '/pairing.sjs'];
> +
> +// For b2g-destkop, you need to fill ip address here to start http server correctly
> +const DEFAULT_IP_ADDR = "10.247.26.32";

We definitely need rewrite this to be landed.

@@ +57,5 @@
> +
> +const RC_SETTINGS_DEVICES = 'remote-control.authorized-devices';
> +const RC_SETTINGS_ENABLED = 'remote-control.enabled';
> +const RC_SETTINGS_SERVERIP = 'remote-control.server-ip';
> +const RC_SETTINGS_PAIRING = 'remote-control.pairing-required';

not sure if all four values will be used in Gaia as well, might need to remove some of them from mozSettings to gecko preference.

@@ +59,5 @@
> +const RC_SETTINGS_ENABLED = 'remote-control.enabled';
> +const RC_SETTINGS_SERVERIP = 'remote-control.server-ip';
> +const RC_SETTINGS_PAIRING = 'remote-control.pairing-required';
> +
> +const DEVICE_EXPIRE_TIME = 90*24*60*60*1000;

Put DEFAULT_PORT and DEVICE_EXPIRE_TIME in gecko preference.

@@ +65,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "SettingsService",
> +                                   "@mozilla.org/settingsService;1",
> +                                   "nsISettingsService");
> +
> +function NS_ASSERT(cond, msg)

You can import NS_ASSERT from resource://gre/modules/debug.js.

@@ +92,5 @@
> +    debug ("init");
> +
> +    this._httpServer = Components.classes["@mozilla.org/server/jshttp;1"]
> +                           .createInstance(Components.interfaces.nsIHttpServer);
> +    gThreadManager = Cc["@mozilla.org/thread-manager;1"].getService();

Use |Services.tm| instead of creating a global variable.

@@ +147,5 @@
> +      lock.set(RC_SETTINGS_SERVERIP, ipaddr, null, null);
> +    } else {
> +      var nm;
> +      try {
> +        nm = Cc["@mozilla.org/network/manager;1"].getService(Ci.nsINetworkManager);

You can use |if (Ci.nsINetworkManager)| to test if on real device instead of using the expansive try-catch.

@@ +163,5 @@
> +          lock.set(RC_SETTINGS_SERVERIP, ipAddresses["value"] + ":" + _port, null, null);
> +        } else {
> +          var observerService = Components.classes["@mozilla.org/observer-service;1"]
> +                      .getService(Components.interfaces.nsIObserverService);
> +          observerService.addObserver (this, "network-active-changed", false);

Use |Services.obs.addObserver| instead.

@@ +173,5 @@
> +        this._httpServer.identity.add ("http", _ipaddr, _port);
> +        this._httpServer.start(_port);
> +        lock.set(RC_SETTINGS_SERVERIP, "http://" + _ipaddr + ":" + _port, null, null);
> +      }
> +    }

You can create a |getSelfIPAddress()| function so that you can avoid duplicating httpServer setting code and settings modification.

@@ +227,5 @@
> +    }
> +
> +    switch (detail.type) {
> +      case "control-mode-changed":
> +        this._setSharedState("isCursorMode", detail.detail.cursor.toString());

Is this for switching between cursor mode and gesture mode in control page? Need more explanation on this interaction.

@@ +267,5 @@
> +  },
> +
> +  _generateUUID: function() {
> +    var uuidGenerator = Components.classes["@mozilla.org/uuid-generator;1"]
> +                    .getService(Components.interfaces.nsIUUIDGenerator);

use lazy getter like at the begin of JSM file:

XPCOMUtils.defineLazyServiceGetter(this, "uuidgen",
                                   "@mozilla.org/uuid-generator;1",
                                   "nsIUUIDGenerator");

@@ +337,5 @@
> +  },
> +
> +  _clearPIN: function() {
> +    this._pin = null;
> +  },

Suppose these functions are for Bug 1207996. Please split these code to another patch. It'll make reviewer's life easier. :)

@@ +348,5 @@
> +
> +    // Using channel.open to check if static file exists
> +    try {
> +      var ioService = Components.classes["@mozilla.org/network/io-service;1"]
> +                    .getService(Components.interfaces.nsIIOService);

Use |Services.io| instead.

@@ +349,5 @@
> +    // Using channel.open to check if static file exists
> +    try {
> +      var ioService = Components.classes["@mozilla.org/network/io-service;1"]
> +                    .getService(Components.interfaces.nsIIOService);
> +      var channel = ioService.newChannel("app://remote-control-client.gaiamobile.org" + path, null, null);

We don't hard code app URL in gecko source code. Please put it in gecko preference and follow the convention in [1] for file path generation.

[1] https://dxr.mozilla.org/mozilla-central/source/b2g/components/B2GAboutRedirector.js#15

@@ +373,5 @@
> +  _checkPathFromCookie: function(request) {
> +    /*
> +      if (request.path=='/' && request.hasHeader("Cookie")) {
> +         return this._isValidUUID (request.getHeader("Cookie")) ? "/client.html" : "/pairing.html";
> +    */

nit: clean up unused code.

@@ +476,5 @@
> +  _handleSJSRequest: function(request, response)
> +  {
> +    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
> +                    .getService(Components.interfaces.nsIIOService);
> +    var channel = ioService.newChannel("resource://gre/res/remotecontrol" + request.path, null, null);

Follow the convention for generating app URL.

@@ +545,5 @@
> +    }
> +  },
> +};
> +
> +RemoteControlService.init();

Try to make RemoteControlService lazy initialized.

::: b2g/installer/package-manifest.in
@@ +1003,5 @@
>  @RESPATH@/components/utils.manifest
> +
> +@RESPATH@/components/httpd.manifest
> +@RESPATH@/components/httpd.js
> +@RESPATH@/components/test_necko.xpt
\ No newline at end of file

These files only exist in test/debug build.

::: netwerk/test/httpserver/httpd.js
@@ +507,4 @@
>    //
>    start: function(port)
>    {
> +    this._start(port, "0.0.0.0")

Modifying httpd.js in netwerk/test folder is likely to break our automation test. you'll need to copy one for our need.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8674749 [details] [diff] [review]
Add RemoteControlService and for remote control requests

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

Eric, can you find someone else to review that?

Comment 15

[Eric Tsai (no more review request accepted)]

Fabrice, @schien had feedbacked this patch and I'm working on update patch now. However, we still need someone to give the final review+, @schien may not be the one. I know you are busy on other 2.5 bugs, is there anyone you suggest to help on this bug?

Comment 16

[Eric Tsai (no more review request accepted)]

Attachment: RemoteControlService, update by schien's feedback

Comment 17

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

Comment on attachment 8682988 [details] [diff] [review]
RemoteControlService, update by schien's feedback

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

This for the update and here is my feedback.

::: b2g/app/b2g.js
@@ +1147,5 @@
>  pref("dom.performance.enable_notify_performance_timing", true);
> +
> +// Remote Control URL, for static file or sjs file
> +pref("remotecontrol.client_page.prepath", "app://remote-control-client.gaiamobile.org");
> +pref("remotecontrol.client_page.blacklist", "/client.html,/pairing.html");

move those two prefs to https://github.com/mozilla-b2g/gaia/blob/master/build/config/tv/custom-prefs.js

::: b2g/chrome/content/settings.js
@@ +614,5 @@
> +      Services.prefs.setBoolPref('remotecontrol.service.pairing_required', value);
> +  });
> +
> +  setupRemoteContol();
> +})();

This is something I'll do for bootstrapping remote control service: https://gist.github.com/schien/c2607b6e4a68d76f51a8

::: b2g/components/RemoteControlHttpd.jsm
@@ +5322,5 @@
> + *   "/home/jwalden/" then a request to /index.html will load
> + *   /home/jwalden/index.html); if this is omitted, only the default URLs in
> + *   this server implementation will be functional
> + */
> +function server(port, basePath)

Should be able to remove this entire function.

::: b2g/components/RemoteControlService.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * RemoteControlService.jsm is the entry point of remote control function.
> + * The service initializes web server (httpd.js), handle request from user,

"httpd.js" should be changed to "HttpServer" or "RemoteControlHttpd.jsm".

@@ +114,5 @@
> +
> +  start: function(ipaddr, port) {
> +    DEBUG && debug ("start");
> +    var _ipaddr = null;
> +    var _port = port ? port : Services.prefs.getIntPref("remotecontrol.default_server_port");

a lot of prefs in this file can be read only once.

@@ +157,5 @@
> +  },
> +
> +  observe: function(subject, topic, data) {
> +    // Receive network connected, start server
> +    if (topic == "network-active-changed" && subject) {

You can reference to https://dxr.mozilla.org/mozilla-central/source/dom/presentation/provider/TCPPresentationServer.js#197 on how to check network status event correctly.

Comment 18

[Eric Tsai (no more review request accepted)]

Attachment: Update feedback from attachment 8682988

Comment 19

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

Comment on attachment 8685891 [details] [diff] [review]
Update feedback from attachment 8682988 [details] [diff] [review]

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

Most of "var" can be replaced with "let". I didn't pick up all the nits but please fix all of them.
We could enter the formal review process in your next upload.

::: b2g/components/RemoteControlService.jsm
@@ +8,5 @@
> + * pass to static file or server script (sjs), send response to user.
> + *
> + *     sjs (gecko) <--   RemoteControlService  --> static file (gaia, app://remote-control-client.gaiamobile.org/)
> + *
> + *           user <-->  httpd.js             Settings DB <--> gaia remote-control app

replace "httpd.js" here with "RemoteControlHttpd.jsm"?

@@ +151,5 @@
> +      } else {
> +        // in b2g-desktop, use dns reverse lookup local ip address. Modify /etc/hosts if necessary
> +        var dns = Components.classes["@mozilla.org/network/dns-service;1"]
> +                       .getService(Components.interfaces.nsIDNSService);
> +        this._activeServerAddress = dns.resolve(dns.myHostName, 0).getNextAddrAsString();

Open a follow-up bug for Stingray TV to make sure we address this issue appropriately on production device.

@@ +202,5 @@
> +        this._activeServerAddress = ipAddresses["value"];
> +        this._httpServer.identity.add ("http", this._activeServerAddress, this._activeServerPort);
> +        lock.set(RC_SETTINGS_SERVERIP, this._activeServerAddress + ":" + this._activeServerPort, null, null);
> +      }
> +    }

Need to update IP address for b2g-desktop as well?

@@ +207,5 @@
> +  },
> +
> +  // nsIObserver
> +  observe: function(subject, topic, data) {
> +    // Receive network connected, start server

nit: move this comment to somewhere more precisely.

@@ +230,5 @@
> +          this._resume();
> +        }
> +
> +        break;
> +      }

We can have more precise logic for the network status:

1. status change to offline -> pause
2. status change to online -> resume
3. interface change while online -> resume

Other cases should be no-op.

@@ +240,5 @@
> +
> +        switch (subject["key"]) {
> +          case  RC_SETTINGS_DEVICES:
> +            this._uuids = JSON.parse(subject["value"]);
> +              break;

nit: indentation.

@@ +259,5 @@
> +      return;
> +    }
> +
> +    switch (detail.type) {
> +      case "control-mode-changed":

IIRC we plan to general support cursor/directional-key mode switch in Gaia app. I think this mozContentEvent might change to something more formal at that time. Please add a bug reference in comment.

@@ +376,5 @@
> +    // Handle static files request
> +    else if (this._isValidPath(request.path))
> +    {  this._handleStaticRequest(request, response); }
> +    else
> +    { throw HttpServer.HTTP_500; }

nit: fix if-else coding style.

if (...) {
  ...
} else if (...) {
  ...
} else {
  ...
}

@@ +380,5 @@
> +    { throw HttpServer.HTTP_500; }
> +  },
> +
> +  _checkPathFromCookie: function(request) {
> +     if (request.path=='/' ) {

nit: space between operator and operand.

@@ +381,5 @@
> +  },
> +
> +  _checkPathFromCookie: function(request) {
> +     if (request.path=='/' ) {
> +         return "/client.html";

nit: indentation

@@ +435,5 @@
> +
> +    var input = new BinaryInputStream(fis);
> +    var output = new BinaryOutputStream(response.bodyOutputStream);
> +    var writeData =
> +      {

nit: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_objects

@@ +501,5 @@
> +      // keys initially corresponding to the empty string.
> +      var self = RemoteControlService;
> +      var path = request.path;
> +      s.importFunction(function getState(k)
> +      {

nit: remove the line break.

Comment 20

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1197749-Add-RemoteControlService-for-events.patch

Hi Fabrice, schein helps to do feedbacks. After discuss with schien, we would like to know if he could help review this patch in b2g's part. He will invite necko expert to review RemoteControlHttpd.jsm (originally from httpd.js, necko test module) and other network related function. Is that OK for you or any other suggestion?

Comment 21

[Eric Tsai (no more review request accepted)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #19)

Others are updated in attachment 8685891 [details] [diff] [review]

> @@ +202,5 @@
> > +        this._activeServerAddress = ipAddresses["value"];
> > +        this._httpServer.identity.add ("http", this._activeServerAddress, this._activeServerPort);
> > +        lock.set(RC_SETTINGS_SERVERIP, this._activeServerAddress + ":" + this._activeServerPort, null, null);
> > +      }
> > +    }
> 
> Need to update IP address for b2g-desktop as well?
> 

In b2g-desktop, DNS reverse lookup is a tricky way to get local IP address. NetworkLinkService can tell network status change but there is no interaction between NetworkLinkService and DNS reverse lookup - get IP from DNS might be old one. Maybe need some input from necko expert to help. It's why I don't have update IP address for b2g-desktop now.

Comment 22

[[:fabrice] Fabrice Desré]

Comment on attachment 8686619 [details] [diff] [review]
0001-Bug-1197749-Add-RemoteControlService-for-events.patch

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

Sorry for taking a long time to look at that. I have some concerns:
- httpd.js was never meant to be a production http server, it was written for the test harness. Did you go through a security review?
- that's a lot of code, and if it's only usable on tv, I would rather only ship it on tv builds, not simply disable it.
- some files like the .sjs ones are not in this patch. Is that expected?

::: b2g/components/RemoteControlHttpd.jsm
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * Bug 1197749, this module is for RemoteControlService, from netwerk/test/httpserver/httpd.js

nit: remove the "Bug 1197749" part.

::: b2g/components/RemoteControlService.jsm
@@ +88,5 @@
> +    this._httpServer = new HttpServer();
> +    this._uuids = {};
> +
> +    // Initial member variables from Gecko preferences
> +    this._client_page_prepath = Services.prefs.getCharPref("remotecontrol.client_page.prepath");

where is this pref defined?

@@ +96,5 @@
> +    this._activeServerPort = this._default_port = Services.prefs.getIntPref("remotecontrol.default_server_port");
> +    this._UUID_expire_days = Services.prefs.getIntPref("remotecontrol.UUID_expire_days");
> +
> +    // Listen UUID changes from gaia
> +    Services.obs.addObserver (this, "mozsettings-changed", false);

Do we have to use settings because this is not managed by the system app?

@@ +112,5 @@
> +    let settingsCallback = {
> +      handle: function(name, result) {
> +        switch (name) {
> +          case RC_SETTINGS_DEVICES:
> +            RemoteControlService._uuids = JSON.parse (result);

nit: no space after JSON.parse
But you should not even have to parse, and just use an object as a setting. I don't understand why you stringify uuids.

@@ +120,5 @@
> +      handleError: function(name) { },
> +    };
> +
> +    let lock = SettingsService.createLock();
> +    lock.get (RC_SETTINGS_DEVICES, settingsCallback);

nit: extra space after |get|

@@ +123,5 @@
> +    let lock = SettingsService.createLock();
> +    lock.get (RC_SETTINGS_DEVICES, settingsCallback);
> +  },
> +
> +  start: function(ipaddr, port) {

You need to protect yourself against multiple calls to start() without stop() in between.

@@ +151,5 @@
> +        Services.obs.addObserver(this, "network:offline-status-changed", false);
> +      } else {
> +        // In b2g-desktop, use dns reverse lookup local ip address. Modify /etc/hosts if necessary
> +        let dns = Components.classes["@mozilla.org/network/dns-service;1"]
> +                       .getService(Components.interfaces.nsIDNSService);

nit: s/Components.classes/Cc and s/Components.interfaces/Ci

@@ +152,5 @@
> +      } else {
> +        // In b2g-desktop, use dns reverse lookup local ip address. Modify /etc/hosts if necessary
> +        let dns = Components.classes["@mozilla.org/network/dns-service;1"]
> +                       .getService(Components.interfaces.nsIDNSService);
> +        this._activeServerAddress = dns.resolve(dns.myHostName, 0).getNextAddrAsString();

this is blocking the main thread synchronously. Please switch to asyncResolve() instead.

@@ +159,5 @@
> +
> +    if (!(this._activeServerAddress === null)) {
> +      let lock = SettingsService.createLock();
> +
> +      this._httpServer.identity.add ("http", this._activeServerAddress, this._activeServerPort);

nit: extra space after |add|

@@ +183,5 @@
> +  _pause: function() {
> +    // While network disconnected, remove registered active IP address
> +    let lock = SettingsService.createLock();
> +
> +    this._httpServer.identity.remove ("http", this._activeServerAddress, this._activeServerPort);

nit: extra space after |remove|

@@ +189,5 @@
> +  },
> +
> +  _resume: function() {
> +    // While network connected, register to accept connections from active IP address
> +    if (Ci.nsINetworkManager) {

instead, do an early return:
if (!Ci.nsINetworkManager) {
  return;
}

@@ +196,5 @@
> +
> +      // in b2g, if connected, use activeNetworkInfo
> +      if (nm.activeNetworkInfo) {
> +        let ipAddresses = {};
> +        let prefixs = {};

nit: s/prefixs/prefixes

@@ +197,5 @@
> +      // in b2g, if connected, use activeNetworkInfo
> +      if (nm.activeNetworkInfo) {
> +        let ipAddresses = {};
> +        let prefixs = {};
> +        let numOfIpAddresses = activeNetwork.getAddresses (ipAddresses, prefixs);

nit: extra space

@@ +199,5 @@
> +        let ipAddresses = {};
> +        let prefixs = {};
> +        let numOfIpAddresses = activeNetwork.getAddresses (ipAddresses, prefixs);
> +
> +        this._activeServerAddress = ipAddresses["value"];

nit: ipAddresses.value

@@ +201,5 @@
> +        let numOfIpAddresses = activeNetwork.getAddresses (ipAddresses, prefixs);
> +
> +        this._activeServerAddress = ipAddresses["value"];
> +        this._httpServer.identity.add ("http", this._activeServerAddress, this._activeServerPort);
> +        lock.set(RC_SETTINGS_SERVERIP, this._activeServerAddress + ":" + this._activeServerPort, null, null);

create the lock just before set.

@@ +245,5 @@
> +        switch (subject["key"]) {
> +          case  RC_SETTINGS_DEVICES:
> +            this._uuids = JSON.parse(subject["value"]);
> +            break;
> +        }

the switch is a bit overkill here (at least for now).
That could just be:
if (subject.key == RC_SETTINGS_DEVICES) {
  this._uuids = subject.value;
}

@@ +252,5 @@
> +      }
> +    }
> +  },
> +
> +  handleEvent: function UP_handleEvent(evt) {

remove UP_handleEvent

@@ +268,5 @@
> +        // System App updates current application control mode
> +        // Cursor mode for browser uses cursor
> +        // Gesture mode for TV app uses spatial navigation
> +        // Save for server script when dispatch event to gecko
> +        // Bug 1224118 is follow-up bug for formal cursor mode change

I don't really understand this comment...

@@ +275,5 @@
> +    }
> +  },
> +
> +  // Clone get/set state/sharedState from httpd.js for runtime state
> +  _getState: function(path, k)

k? Please use something a bit more meaningful.

@@ +276,5 @@
> +  },
> +
> +  // Clone get/set state/sharedState from httpd.js for runtime state
> +  _getState: function(path, k)
> +  {

that should not be on its own line (in this function and the following ones, that look copy/pasted from somewhere).

@@ +283,5 @@
> +      return state[path][k];
> +    return "";
> +  },
> +
> +  _setState: function(path, k, v)

idem with k & v

@@ +311,5 @@
> +
> +  // Generate UUID and expire timestamp
> +  _generateUUID: function() {
> +    let uuid = UUIDGenerator.generateUUID();
> +    let uuidString = uuid.toString();

let uuidString = UUIDGenerator.generateUUID().toString();

@@ +319,5 @@
> +    let uuids = this._uuids;
> +
> +    dic[uuidString] = timeStamp;
> +    uuids[uuidString] = timeStamp;
> +    lock.set(RC_SETTINGS_DEVICES, JSON.stringify(uuids), null, null);

No need to stringify

@@ +339,5 @@
> +    if (uuid in this._uuids) {
> +      let lock = SettingsService.createLock();
> +
> +      delete this._uuids[uuid];
> +      lock.set(RC_SETTINGS_DEVICES, JSON.stringify(this._uuids), null, null);

no need to stringify

@@ +347,5 @@
> +  _clearAllUUID: function() {
> +    let lock = SettingsService.createLock();
> +
> +    this._uuids = {};
> +    lock.set(RC_SETTINGS_DEVICES, JSON.stringify(this._uuids), null, null);

same here.

@@ +353,5 @@
> +
> +  // Check incoming path is valid or not
> +  _isValidPath: function(path) {
> +    // '/' is always valid, will redirect to client.html or pairing.html
> +    if (path == '/') return true;

if (path == "/") {
  return true;
}

@@ +355,5 @@
> +  _isValidPath: function(path) {
> +    // '/' is always valid, will redirect to client.html or pairing.html
> +    if (path == '/') return true;
> +    // Block any invalid access to file system
> +    if (path.indexOf("..") > -1) return false;

idem here, use { } even for single line if's

@@ +357,5 @@
> +    if (path == '/') return true;
> +    // Block any invalid access to file system
> +    if (path.indexOf("..") > -1) return false;
> +
> +    // Using channel.open to check if static file exists

can't we get the file path and use nsIFile.exists() instead?

@@ +386,5 @@
> +    }
> +  },
> +
> +  _checkPathFromCookie: function(request) {
> +     if (request.path == '/' ) {

nit: double quotes for strings

@@ +396,5 @@
> +
> +  // Clone from _writeFileResponse in httpd.js and split to two part:
> +  // handleStaticRequest and handleStaticSJSRequest
> +  // Modify to nsIURI and nsIChannel to access file in remotecontrol client app
> +  _handleStaticRequest: function(request, response)

please fix the style in this method: { not on their own line, and { } even for single expression if's

@@ +409,5 @@
> +
> +    let offset = 0;
> +    let count = fis.available();
> +
> +    if (path.endsWith("css"))

{ }

@@ +416,5 @@
> +      response.setHeader("Content-Type", "text/html;charset=utf-8", false);
> +    response.setHeader("Content-Length", "" + count, false);
> +
> +    try
> +    {

try {

@@ +418,5 @@
> +
> +    try
> +    {
> +      if (offset !== 0)
> +      {

if (offset !== 0) {

@@ +485,5 @@
> +    // Now that we know copying will start, flag the response as async.
> +    response.processAsync();
> +  },
> +
> +  _handleSJSRequest: function(request, response)

fix this method's style too.

Comment 23

[[:fabrice] Fabrice Desré]

Also, there are no tests... We absolutely need some.

Comment 24

[Eric Tsai (no more review request accepted)]

Hi Fabrice, I will provide next patch for nit improvement, TV build and test case. Reply others inline. 

> Sorry for taking a long time to look at that. I have some concerns:
> - httpd.js was never meant to be a production http server, it was written
> for the test harness. Did you go through a security review?

As discussed with :schien, we will invite netwerk team to review httpd.js.
For connection between TV and client, bug 1215457 provides secure connection and it's on going.
I'm not sure security review means we need a feature security review or these 2 is enough?

> - some files like the .sjs ones are not in this patch. Is that expected?

Originally the patch is from prototyping bug, .sjs from is in bug 1197751 and 1207996. This bug only provides the service and http server.

> ::: b2g/components/RemoteControlService.jsm
> @@ +88,5 @@
> > +    this._httpServer = new HttpServer();
> > +    this._uuids = {};
> > +
> > +    // Initial member variables from Gecko preferences
> > +    this._client_page_prepath = Services.prefs.getCharPref("remotecontrol.client_page.prepath");
> 
> where is this pref defined?
> 

Defined in gaia/build/config/tv/custom-prefs.js, these pages are packaged in a TV app so we move pref to Gaia.

> @@ +96,5 @@
> > +    this._activeServerPort = this._default_port = Services.prefs.getIntPref("remotecontrol.default_server_port");
> > +    this._UUID_expire_days = Services.prefs.getIntPref("remotecontrol.UUID_expire_days");
> > +
> > +    // Listen UUID changes from gaia
> > +    Services.obs.addObserver (this, "mozsettings-changed", false);
> 
> Do we have to use settings because this is not managed by the system app?
> 

Yes, a TV app will use the mozsettings.

> 
> @@ +357,5 @@
> > +    if (path == '/') return true;
> > +    // Block any invalid access to file system
> > +    if (path.indexOf("..") > -1) return false;
> > +
> > +    // Using channel.open to check if static file exists
> 
> can't we get the file path and use nsIFile.exists() instead?
> 

For static file packaged in tv app will be: app://remotecontrol-client.gaiamobile.org/<PATH_TO_STATIC_FILE>, I think nsIFile.exists() doesn't work. That is why I use channel.open().

Comment 25

[Eric Tsai (no more review request accepted)]

Hi Jason, as comment 24, could you suggest any expert to verify using httpd.js in production?

Comment 26

[[:fabrice] Fabrice Desré]

Not sure why I'm ni?

Comment 27

[Jason Duell]

OK, there's a bunch of possible options here. 

1) We could do a security audit of httpd.js and use that.  As Fabrice mentioned, httpd.js hasn't previously been considered as a production HTTP server. It's also been underowned for quite a while (:waldo was the main driver many years ago, most people have wandered in just to fix little bugs).  That's the "bad news". The good news is that Waldo tells me that he "tried to write/architect httpd.js *as if* it were receiving untrusted input" and that "the idea of shipping it does not completely fill me with horror" :)  So that's not too terrible of a start.  That said, httpd.js is 5000+ lines of JS code, which is a fairly complicated piece of software.  The only piece of it that Waldo can think of that is a definitive security risk is all the code related to the "/trace" logic: "As far as holes go, definitely the default "/trace" path would be one -- that was a mostly-diagnostic way of dumping any incoming request which happens to be bad, in the general case, for the same reason TRACE is considered a bad HTTP method now."  It looks like we could remove that code (the only test case I can find that uses it is test_bug477578.js, which seems to use it just to test all POST/GET/etc methods.  So we could probably strip that out.

Waldo also mentions a couple of other missing features of httpd.js. 1) we have no request timeout functionality at the moment; 2) we're missing the capability to make any response at all until the entirety of the request head (GET ... and all headers) has been processed, which may or may not matter to you (I suspect it won't matter for this use case at all).  3) Our solution for using https (SSLTunnel) is not safe to use in production: from the start of ssltunnel.cpp: "WARNING: DO NOT USE THIS CODE IN PRODUCTION SYSTEMS.  It is highly likely to be plagued with the usual problems endemic to C (buffer overflows and the like).  We don't especially care here (but would accept patches!) because this is only intended for use in our test harnesses in controlled situations where input is guaranteed not to be malicious."

2) We could write JS server that's similar to httpd.js, but simpler (and easier to audit).  How simple this could be might depend on what your communication requirements are.  Do you need a full HTTP stack?  If you're just sending some GET or POSTs, you might be able to write (and/or cut-and-paste from httpd.js) just the bits that you actually need.  

It sounds like this is what we did on Android a while ago: see bug 860599 comment 1.

3) Like #2, but we write a websockets server.  I'm not clear on what your communication requirements are.  Websockets gives you easier server-push capability, but Walso does tell me that httpd.js can do AJAX-style requests if needed (the default mode is for server handlers to reply synchronously, but there's an opt-in mode that allows you to call response.processAsync() and reply later without blocker further requests).  There's at least one existing JS implementation of websockets which could be a base here: https://github.com/websockets/ws

What's the security model for our TV/remote control?  I get the impression from Fabrice that we'll just be opening up port 80 (or whatever) on a wifi network, which both the TV and remote will be on.  Do we have any way of locking down that network from other devices?  What are our security needs here? If we need to implement HTTPS this will be a lot more work (but if we don't I'm not sure how prevent other devices from masquerading or otherwise interfering with the TV).  Some idea of the threat model here would be great to have.

Comment 28

[Jason Duell]

Seeing if cpearce has any thoughts/suggestions from our Android http server implementation that are relevant here.

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

If you're just doing HTTP for use over a local, then you could extract the C++ HTTP server we wrote in bug 860599. It was designed for a very specific use case; exposing an HTTP interface to a presumably non-hostile local consumer (the Android platform video streaming playback libraries), so you may find you'll have to add capabilities to it for your purposes. It's not designed to be highly scale-able, and you'd need to figure out how to expose it. It doesn't do HTTPS.

Comment 30

[Eric Tsai (no more review request accepted)]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #27)

Thanks Jason, after internal discussion today, we would like to request network team's help to do security audit of httpd.js (or RemoteControlHttpd.jsm), here are the reasons:

1. In this case, our usage is simple. HTTP server provides these paths only: a) / , b) sjs file for AJAX request in a white list only, c) static files in a gaia app package. Other request will throw HTTP 500 directly. In attachment 8686619 [details] [diff] [review], I use registerPrefixHandler to handle all request in prefix "/":

+    this._httpServer.registerPrefixHandler("/", function(request, response) {
+      RemoteControlService._handleRequest(request, response);
+    });

The "/trace" logic doesn't work in our usage now, but I agree it's necessary to remove these holes for production.

2. Thanks Waldo but I'm not sure if we need all missing features now.

3. EPM Josh's concern is to deliver to partner in time. Using native porting is time consuming and we don't need a high performance and scale-able one. So we think it's ok to keep and audit httpd.js.


> 1) We could do a security audit of httpd.js and use that.  As Fabrice
> mentioned, httpd.js hasn't previously been considered as a production HTTP
> server. It's also been underowned for quite a while (:waldo was the main
> driver many years ago, most people have wandered in just to fix little
> bugs).  That's the "bad news". The good news is that Waldo tells me that he
> "tried to write/architect httpd.js *as if* it were receiving untrusted
> input" and that "the idea of shipping it does not completely fill me with
> horror" :)  So that's not too terrible of a start.  That said, httpd.js is
> 5000+ lines of JS code, which is a fairly complicated piece of software. 
> The only piece of it that Waldo can think of that is a definitive security
> risk is all the code related to the "/trace" logic: "As far as holes go,
> definitely the default "/trace" path would be one -- that was a
> mostly-diagnostic way of dumping any incoming request which happens to be
> bad, in the general case, for the same reason TRACE is considered a bad HTTP
> method now."  It looks like we could remove that code (the only test case I
> can find that uses it is test_bug477578.js, which seems to use it just to
> test all POST/GET/etc methods.  So we could probably strip that out.
> 
> Waldo also mentions a couple of other missing features of httpd.js. 1) we
> have no request timeout functionality at the moment; 2) we're missing the
> capability to make any response at all until the entirety of the request
> head (GET ... and all headers) has been processed, which may or may not
> matter to you (I suspect it won't matter for this use case at all).  3) Our
> solution for using https (SSLTunnel) is not safe to use in production: from
> the start of ssltunnel.cpp: "WARNING: DO NOT USE THIS CODE IN PRODUCTION
> SYSTEMS.  It is highly likely to be plagued with the usual problems endemic
> to C (buffer overflows and the like).  We don't especially care here (but
> would accept patches!) because this is only intended for use in our test
> harnesses in controlled situations where input is guaranteed not to be
> malicious."
> 

Yes, we just sending GET, no POST. Not sure if we strip code not used in httpd.js or copy and paste will be better?

> 2) We could write JS server that's similar to httpd.js, but simpler (and
> easier to audit).  How simple this could be might depend on what your
> communication requirements are.  Do you need a full HTTP stack?  If you're
> just sending some GET or POSTs, you might be able to write (and/or
> cut-and-paste from httpd.js) just the bits that you actually need.  
> 
> It sounds like this is what we did on Android a while ago: see bug 860599
> comment 1.
> 

For https, we have another topic about secure connection in bug 1215457. We will based on http server to enhance security. Paul from security team already gave us many useful suggestion.

> 
> What's the security model for our TV/remote control?  I get the impression
> from Fabrice that we'll just be opening up port 80 (or whatever) on a wifi
> network, which both the TV and remote will be on.  Do we have any way of
> locking down that network from other devices?  What are our security needs
> here? If we need to implement HTTPS this will be a lot more work (but if we
> don't I'm not sure how prevent other devices from masquerading or otherwise
> interfering with the TV).  Some idea of the threat model here would be great
> to have.

Comment 31

[Eric Tsai (no more review request accepted)]

(In reply to [:fabrice] Fabrice Desré from comment #26)
> Not sure why I'm ni?

After asking Josh, he ni you because need your feedback for comment 24.

Comment 32

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1197749-Add-RemoteControlService-for-events.patch

Hi Fabrice, please help to review the patch based on your last comment. I will update another attachment for unit test only.
For httpd.js, Jason's team could help to do security audit, is that ok for security review or you have other concern on this path?

Comment 33

[Eric Tsai (no more review request accepted)]

Attachment: Unit test for RemoteControlService

Comment 34

[[:fabrice] Fabrice Desré]

Comment on attachment 8692810 [details] [diff] [review]
0001-Bug-1197749-Add-RemoteControlService-for-events.patch

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

We're getting close!

::: b2g/chrome/content/settings.js
@@ +688,5 @@
> +  'remote-control.pairing-required': {
> +    prefName: 'remotecontrol.service.pairing_required',
> +    resetToPref: true
> +  },
> +#endif

I'd rather not add any new #ifdefs in chrome JS files. Can you instead add support for MOZ_B2G_REMOTECONTROLSERVICE in AppConstants.jsm and use it there?

::: b2g/components/RemoteControlService.jsm
@@ +20,5 @@
> + * gecko/b2g/components/RemoteControlHttpd.js
> + * gaia/tv_apps/remote-control - remote control app
> + * gaia/tv_apps/remote-control-client - remote control client page static files
> + *
> + * For more detail, please visit: https://wiki.mozilla.org/Firefox_OS/Remote_Control

nit: s/detail/details
But it's great to have comments and docs!

@@ +47,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "HttpServer",
> +                          "resource://gre/modules/RemoteControlHttpd.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "UUIDGenerator",
> +                          '@mozilla.org/uuid-generator;1', 'nsIUUIDGenerator');

double quotes everywhere please.

@@ +65,5 @@
> +// For bi-direction share with Gaia remote-control app, use mozSettings here, not Gecko preference
> +// Ex. the service adds authorized devices, app can revoke all
> +// We may switch to a new API, like FlyWeb, which can achieve but not using mozSettings
> +const RC_SETTINGS_DEVICES = 'remote-control.authorized-devices';
> +const RC_SETTINGS_SERVERIP = 'remote-control.server-ip';

double quotes here too.

@@ +71,5 @@
> +const SERVER_STATUS = {
> +  STOP: {value: 0},
> +  STARTING: {value: 1},
> +  START: {value: 2}
> +};

I don't understand why this would not work as well:
const SERVER_STATUS = {
  STOP: 0,
  STARTING: 1,
  START: 2
}
Also, this should be STOPPED, STARTING, STARTED

@@ +89,5 @@
> +  _default_port: null,
> +  _UUID_expire_days: null,
> +
> +  init: function() {
> +    DEBUG && debug ("init");

nit: debug("init");

@@ +134,5 @@
> +  start: function(ipaddr, port) {
> +    if (this._serverStatus != SERVER_STATUS.STOP) {
> +      return;
> +    }
> +    DEBUG && debug ("start");

debug("start");

@@ +151,5 @@
> +        // in b2g, if connected, use activeNetworkInfo
> +        if (nm.activeNetworkInfo) {
> +          let ipAddresses = {};
> +          let prefixes = {};
> +          let numOfIpAddresses = nm.activeNetworkInfo.getAddresses (ipAddresses, prefixes);

getAddresses(...)

@@ +158,5 @@
> +        }
> +
> +        // Monitor network status to pause/restart service
> +        Services.obs.addObserver (this, "network-active-changed", false);
> +        Services.obs.addObserver(this, "network:offline-status-changed", false);

Nothing guarantees that we remove these observers. You should always observe xpcom-shutdown and clean up there if the state is not STOPPED

@@ +163,5 @@
> +      } else {
> +        // In b2g-desktop, use dns reverse lookup local ip address. Modify /etc/hosts if necessary
> +        let dns = Cc["@mozilla.org/network/dns-service;1"]
> +                       .getService(Ci.nsIDNSService);
> +        //this._activeServerAddress = dns.resolve(dns.myHostName, 0).getNextAddrAsString();

remove commented code.

@@ +179,5 @@
> +        dns.asyncResolve(dns.myHostName, 0, activeServerAddressDNSListener, Services.tm.mainThread);
> +      }
> +    }
> +
> +    if (!(this._activeServerAddress === null)) {

if (this._activeServerAddress !== null) instead?

@@ +192,5 @@
> +    this._serverStatus = SERVER_STATUS.START;
> +  },
> +
> +  stop: function() {
> +    if (this._serverStatus != SERVER_STATUS.START) {

You add the observers when setting _serverStatus to STARTING, so this will leak observers.

@@ +304,5 @@
> +  // Clone get/set state/sharedState from httpd.js for runtime state
> +  _getState: function(path, key) {
> +    let state = this._state;
> +    if (path in state && key in state[path])
> +      return state[path][key];

here and in other _XXXState methods: use { } even for single line ifs.

@@ +409,5 @@
> +      throw HttpServer.HTTP_500;
> +    }
> +  },
> +
> +  _checkPathFromCookie: function(request) {

I don't understand why this method is named like that. That's not doing anything cookie related...

@@ +432,5 @@
> +
> +    let offset = 0;
> +    let count = fis.available();
> +
> +    if (path.endsWith("css")) {

use ".css" instead of "css".

@@ +505,5 @@
> +    let fis = channel.open();
> +
> +    try {
> +      let sis = new ScriptableInputStream(fis);
> +      let s = Cu.Sandbox (Cc["@mozilla.org/systemprincipal;1"].createInstance(Ci.nsIPrincipal));

Cu.Sandbox(...)

@@ +525,5 @@
> +        return self._getSharedState(k);
> +      });
> +      s.importFunction(function setSharedState(k, v) {
> +        self._setSharedState(k, v);
> +      });

update all these methods with something more meaningful than k & v

Comment 35

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. RemoteControlHttpd.jsm, js http server

Hi Jason, I made RemoteControlHttpd.jsm as a patch, please help to give us security audit.

Comment 36

[Eric Tsai (no more review request accepted)]

Attachment: Patch 2. RemoteControlService.jsm, service, preference and settings

Hi Fabrice, I've updated based on your comment for attachment 8692810 [details] [diff] [review]. This patch only contains RemoteControlService. Http server is in another patch and request security audit from Jason's team.

Comment 37

[Eric Tsai (no more review request accepted)]

Attachment: Patch 3. Unit test for RemoteControlService

Comment 38

[[:fabrice] Fabrice Desré]

Comment on attachment 8695876 [details] [diff] [review]
Patch 2. RemoteControlService.jsm, service, preference and settings

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

This looks fine to me, except that I really want the situation when calling stop() in the STARTING state to be clarified. Thanks!

::: b2g/components/RemoteControlService.jsm
@@ +63,5 @@
> +                          "nsIScriptableInputStream", "init");
> +
> +// For bi-direction share with Gaia remote-control app, use mozSettings here, not Gecko preference
> +// Ex. the service adds authorized devices, app can revoke all
> +// We may switch to a new API, like FlyWeb, which can achieve but not using mozSettings

remove this last comment line.

@@ +200,5 @@
> +
> +  // Stop http server and clean up registered observers
> +  // Return false if server not started, stop failed
> +  stop: function() {
> +    if (this._serverStatus != SERVER_STATUS.STARTED) {

I still think this is not right when _serverStatus = STARTING. In this case you will not remove the observers you added. Or am I missing something?

Comment 39

[[:fabrice] Fabrice Desré]

Comment on attachment 8695885 [details] [diff] [review]
Patch 3. Unit test for RemoteControlService

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

::: b2g/components/test/unit/test_remotecontrolservice.js
@@ +17,5 @@
> +                          "nsIBinaryInputStream", "setInputStream");
> +
> +function getRandomPort()
> +{
> +  return 1024 + Math.floor(Math.random() * (65535-1024));

nit: spaces around '-'

@@ +24,5 @@
> +function run_test() {
> +  let resourcePath = do_get_file("data/");
> +
> +  // Setup preferences, originally in b2g.js or custom_pref.js (gaia)
> +  Services.prefs.setCharPref("remotecontrol.client_page.prepath", 'file://' + resourcePath.path);

nit: double quotes around file:// and on the following lines.

@@ +49,5 @@
> +  let serverPort = getRandomPort();
> +  Cu.import("resource://gre/modules/RemoteControlService.jsm", testStartScope);
> +  ok(testStartScope.RemoteControlService.start("127.0.0.1", serverPort));
> +  // Use server status to check if service started
> +  deepEqual(testStartScope.RemoteControlService._serverStatus, SERVER_STATUS.STARTED, "RemoteControlService started");

you don't need deepEqual here, _serverStatus is just an integer

@@ +50,5 @@
> +  Cu.import("resource://gre/modules/RemoteControlService.jsm", testStartScope);
> +  ok(testStartScope.RemoteControlService.start("127.0.0.1", serverPort));
> +  // Use server status to check if service started
> +  deepEqual(testStartScope.RemoteControlService._serverStatus, SERVER_STATUS.STARTED, "RemoteControlService started");
> +  testStartScope.RemoteControlService.stop();

test _serverStatus again here (it should be STOPPED)

@@ +82,5 @@
> +  Cu.import("resource://gre/modules/RemoteControlService.jsm", testSJSScope);
> +  testSJSScope.RemoteControlService.start("127.0.0.1", serverPort);
> +
> +  // Test sync XHR sending
> +  var sync = createXHR(false, serverPort);

why use a sync xhr? This is really not recommended.

@@ +89,5 @@
> +  testSJSScope.RemoteControlService.stop();
> +  run_next_test();
> +});
> +
> +// Start then stop RemoteControlService and check server status is set to STOP

not sure why you need a new test for that.

Comment 40

[Eric Tsai (no more review request accepted)]

(In reply to [:fabrice] Fabrice Desré from comment #38)

> 
> @@ +200,5 @@
> > +
> > +  // Stop http server and clean up registered observers
> > +  // Return false if server not started, stop failed
> > +  stop: function() {
> > +    if (this._serverStatus != SERVER_STATUS.STARTED) {
> 
> I still think this is not right when _serverStatus = STARTING. In this case
> you will not remove the observers you added. Or am I missing something?

My idea is when server is not completely started, server should not be stopped. When server is starting, stop server will return false. Caller can re-stop server again when get false.

Comment 41

[[:fabrice] Fabrice Desré]

Hm, but start() is actually synchronous. It will never return with _serverStatus being STARTING right? So this state is not giving us anything. And in the desktop case, we do an async DNS resolution so we potentially race. It seems to me that this would be more robust to have start() return a promise that resolve when everything is set up properly (including settings). What do you think?

Comment 42

[Eric Tsai (no more review request accepted)]

(In reply to [:fabrice] Fabrice Desré from comment #41)
> Hm, but start() is actually synchronous. It will never return with
> _serverStatus being STARTING right? So this state is not giving us anything.

Yes, start() is synchronous. Using starting is to prevent stop() when service is starting in multithreading? I'm not sure it's possible case or not. If it doesn't happen, I think server only need to provide started and stopped status.

> And in the desktop case, we do an async DNS resolution so we potentially
> race. It seems to me that this would be more robust to have start() return a
> promise that resolve when everything is set up properly (including
> settings). What do you think?

For async DNS resolution on desktop, if use promise is used to notify set up. In b2g, I use network manager to observe network change, it's also need to use promise as well? Because not in call case active network info is available when start() is called.

Comment 43

[[:fabrice] Fabrice Desré]

(In reply to Eric Tsai from comment #42)
> (In reply to [:fabrice] Fabrice Desré from comment #41)
> > Hm, but start() is actually synchronous. It will never return with
> > _serverStatus being STARTING right? So this state is not giving us anything.
> 
> Yes, start() is synchronous. Using starting is to prevent stop() when
> service is starting in multithreading? I'm not sure it's possible case or
> not. If it doesn't happen, I think server only need to provide started and
> stopped status.

The "run to completion" guarantee in JS prevents this kind of thing to happen, so it really seems we don't need this state.

> > And in the desktop case, we do an async DNS resolution so we potentially
> > race. It seems to me that this would be more robust to have start() return a
> > promise that resolve when everything is set up properly (including
> > settings). What do you think?
> 
> For async DNS resolution on desktop, if use promise is used to notify set
> up. In b2g, I use network manager to observe network change, it's also need
> to use promise as well? Because not in call case active network info is
> available when start() is called.

You don't want start() to return a different kind of result on different platforms. Also, are you sure you don't want to wait for lock.set() to complete the transaction?

Comment 44

[Jason Duell]

I asked some security folks at Mozilla (amuntner, dvedtiz) about this, and they are not very happy at the idea of shipping a full HTTP server that's been used only for testing, just to receive remote control clicks.  They're wondering why we need a server at all.  How does the remote control for the TV work?  Does it use Infrared? Bluetooth? If so I'd expect there's some native API for receiving events from the remote on the TV, and we could write some webapi code to access it. (Or is the only way to communicate between the remote/TV via wifi?).

Come grab me at Orlando (or we can set up a time to meet). I understand you're on a schedule and would like a fix here ASAP.

Comment 45

[Eric Tsai (no more review request accepted)]

Hi Jason, per-discussed, remote control related Gecko bugs are 1197749 1197751 and 1207996. I also discussed with amuntner on 12/10 afternoon, he will also help for TV product review.

Comment 46

[Eric Tsai (no more review request accepted)]

Attachment: Patch 2. RemoteControlService.jsm, service, preference and settings

Update start() with promise to notify complete or fail

Comment 47

[Jason Duell]

Comment on attachment 8695871 [details] [diff] [review]
Patch 1. RemoteControlHttpd.jsm, js http server

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

Consensus among security experts @ mozilla is that we can't ship httpd.js safely in this time frame, and that we should either write a small webserver (possibly cut-and-paste from httpd.js) with security audit, or use an existing server that's known to have security (node.js for example).   Sorry for the delay in deciding this--I know you are on a tight schedule.

Comment 48

[[:fabrice] Fabrice Desré]

Comment on attachment 8697709 [details] [diff] [review]
Patch 2. RemoteControlService.jsm, service, preference and settings

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

r=me with nits addressed.

::: b2g/components/RemoteControlService.jsm
@@ +129,5 @@
> +    lock.get(RC_SETTINGS_DEVICES, settingsCallback);
> +  },
> +
> +  // Start http server and register observers.
> +  // Return false if server not in stopped status, start failed

nit: update the comment to say that it's returning a promise and what it resolves/rejects to.

@@ +132,5 @@
> +  // Start http server and register observers.
> +  // Return false if server not in stopped status, start failed
> +  start: function(ipaddr, port) {
> +    if (this._serverStatus == SERVER_STATUS.STARTED) {
> +      return Promise.reject("Start fail: already started");

We usually don't reject with plain English sentences. Use reject("AlreadyStarted") instead.

@@ +177,5 @@
> +              let self = RemoteControlService;
> +              let lock = SettingsService.createLock();
> +              let settingsCallback = {
> +                handle: function(name, result) {
> +                  aResolve("Start complete");

aResolve() since there is nothing useful in this parameter.

@@ +180,5 @@
> +                handle: function(name, result) {
> +                  aResolve("Start complete");
> +                },
> +                handleError: function(name) {
> +                  aReject("Start fail: set URL fail");

aReject("SettingsFailure")

@@ +188,5 @@
> +              self._activeServerAddress = aRecord.getNextAddrAsString();
> +              self._httpServer.identity.add("http", self._activeServerAddress, self._activeServerPort);
> +              lock.set(RC_SETTINGS_SERVERIP, self._activeServerAddress + ":" + self._activeServerPort, settingsCallback);
> +            } else {
> +              aReject("Start fail: DNS lookup failed");

aReject("DnsLookupFailure")

@@ +200,5 @@
> +    if (this._activeServerAddress !== null) {
> +      let lock = SettingsService.createLock();
> +      let settingsCallback = {
> +        handle: function(name, result) {
> +          aResolve("Start complete");

aResolve();

@@ +203,5 @@
> +        handle: function(name, result) {
> +          aResolve("Start complete");
> +        },
> +        handleError: function(name) {
> +          aReject("Start fail: set URL fail");

aReject("SettingsFailure")

@@ +211,5 @@
> +      this._httpServer.identity.add("http", this._activeServerAddress, this._activeServerPort);
> +      lock.set(RC_SETTINGS_SERVERIP, this._activeServerAddress + ":" + this._activeServerPort, settingsCallback);
> +    } else if (Ci.nsINetworkManager){
> +      // For b2g but there no IP address, reject promise
> +      aReject("Start incomplete: only loopback IP address");

aReject("NoIpAddress")

Comment 49

[Eric Tsai (no more review request accepted)]

Attachment: Patch 2. RemoteControlService.jsm, service, preference and settings

Fix promise related nit

Comment 50

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. RemoteControlHttpd.jsm, js http server

Paul, could you help to review if there is any possible secure issue in this version? After your review, I may invite :jduell to review for http server functionality. Is it ok for you?

Comment 51

[Eric Tsai (no more review request accepted)]

Attachment: Patch 2. RemoteControlService.jsm, service, preference and settings

Update RemoteControlService.jsm by modification of RemoteControlHttpd.jsm

Comment 52

[Eric Tsai (no more review request accepted)]

Comment on attachment 8712057 [details] [diff] [review]
Patch 1. RemoteControlHttpd.jsm, js http server

Hi Jason, this patch mainly contains simplified httpd.js for remote control's usage. Julian is helping on security review and he shared a google doc for notes:
https://docs.google.com/document/d/1QrAmRpdJMYHP42tezPAM2ajF-da_vbThhX4QH41pjHc/edit
If reviewer needs any help, please let me know. I will provide response ASAP.

Comment 53

[Jason Duell]

Eric: The google doc seems to imply that :ted looked at the httpd.js code already.  Is that true?  (It's still worth having someone on necko look at it, but it'd be good to know who's looked over this so far).

Comment 54

[Jason Duell]

Comment on attachment 8712057 [details] [diff] [review]
Patch 1. RemoteControlHttpd.jsm, js http server

Nick.  I'm giving this to you.  Let me know if you need help with it, or if there are bits you think someone else is better placed to review.

Comment 55

[Eric Tsai (no more review request accepted)]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #53)
> Eric: The google doc seems to imply that :ted looked at the httpd.js code
> already.  Is that true?  (It's still worth having someone on necko look at
> it, but it'd be good to know who's looked over this so far).

Yes, :ted helps on security review and we need to know what's the gap to production server from necko team's perspective.

Comment 56

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. RemoteControlHttpd.jsm, js http server

Minor fix for unit test

Comment 57

[Eric Tsai (no more review request accepted)]

Attachment: Patch 2. RemoteControlService.jsm, service, preference and settings

Minor fix for removing identity

Comment 58

[Eric Tsai (no more review request accepted)]

Attachment: Patch 3. Unit test for RemoteControlService

Fix comment from https://bugzilla.mozilla.org/attachment.cgi?id=8695885 and new http server architecture

Comment 59

[u408661]

Comment on attachment 8721913 [details] [diff] [review]
Patch 1. RemoteControlHttpd.jsm, js http server

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

So there are two big things for me here - there is a lot of code that is unused, still (mostly for allowing state to be shared between requests), which should be removed - we should not ship what we aren't using. I called out some of this in my inline comments, but I can't guarantee I caught all the unused bits.

The other, and significantly more important item, is that there is no TLS/SSL support anywhere to be seen here. We absolutely should not be shipping anything that speaks plaintext - not only is it not good to enable passive MITM, it would also be hypocritical given our stance on TLS everywhere. The only work I see related to this is bug 1235013, which hasn't been touched in a month, and appears (from my quick glance at the patch) to be going down the "roll your own crypto scheme" path instead of using TLS to secure the connection. We might have to go through some sort of TOFU scheme with TLS, since it's going to be basically impossible to ensure hostname matches even if we had a properly signed cert, but that's still better than no crypto at all.

::: b2g/components/RemoteControlHttpd.jsm
@@ +136,5 @@
> +/** An object (hash) whose fields are the numbers of all HTTP error codes. */
> +const HTTP_ERROR_CODES = array2obj(range(400, 417).concat(range(500, 505)));
> +
> +/** Type used to denote SJS scripts for CGI-like functionality. */
> +const SJS_TYPE = "sjs";

Make this ".sjs"

@@ +167,5 @@
> +  }
> +}
> +
> +/** Dumps the current JS stack if DEBUG. */
> +function dumpStack() {

Put an "if (DEBUG)" in here to avoid all the work in non-DEBUG cases

@@ +440,5 @@
> +    this._doQuit = false;
> +  },
> +
> +  /**
> +   * Registers a path to channdl handler.

s/channdl/channel/

@@ +465,5 @@
> +   * Retrieves the string associated with the given key in this, for the given
> +   * path's saved state.  All keys are initially associated with the empty
> +   * string.
> +   */
> +  getState: function(path, key) {

It looks to me like this is unused - let's get rid of it (and other things that are unused - I may not catch all of them during this review) in the interest of reducing complexity as much as possible. (If it's still used and I just missed it, then leave it, of course.)

@@ +473,5 @@
> +  /**
> +   * Sets the string associated with the given key in this, for the given path's
> +   * saved state.
> +   */
> +  setState: function(path, key, value) {

Unused?

@@ +482,5 @@
> +   * Retrieves the string associated with the given key in this, in
> +   * entire-server saved state.  All keys are initially associated with the
> +   * empty string.
> +   */
> +  getSharedState: function(key) {

Unused?

@@ +490,5 @@
> +  /**
> +   * Sets the string associated with the given key in this, in entire-server
> +   * saved state.
> +   */
> +  setSharedState: function(key, value) {

Unused?

@@ +498,5 @@
> +  /**
> +   * Retrieves the object associated with the given key in this in
> +   * object-valued saved state.  All keys are initially associated with null.
> +   */
> +  getObjectState: function(key) {

Unused?

@@ +506,5 @@
> +  /**
> +   * Sets the object associated with the given key in this in object-valued
> +   * saved state.  The value may be null.
> +   */
> +  setObjectState: function(key, value) {

Unused?

@@ +515,5 @@
> +   * Returns true iff this server is not running (and is not in the process of
> +   * serving any requests still to be processed when the server was last
> +   * stopped after being run).
> +   */
> +  isStopped: function() {

Unused?

@@ +621,5 @@
> +
> +    // Bug 508125: Add a GC here else we'll use gigabytes of memory running
> +    // mochitests. We can't rely on xpcshell doing an automated GC, as that
> +    // would interfere with testing GC stuff...
> +    Cu.forceGC();

I'm not sure we actually want to force a GC here - according to the bug (which is actually bug 508128, *not* 508125 (which I don't even have access to)), this looks like it was designed to fix an issue with test machines running out of memory because we don't have any kind of decent gc strategy in xpcshell. However, I could be totally wrong in my reading of that (exceedingly long) bug. I'll ni? some people who may, in fact, be better able to answer this.

This is my long way of saying, if we get confirmation that forcing GC here is unnecessary, we should not do that in production code.

@@ +1073,5 @@
> +        // scheme to use to look up the correct port here, in general.  Since
> +        // the HTTPS case requires a tunnel/proxy and thus requires that the
> +        // requested URI be absolute (and thus contain the necessary
> +        // information), let's assume HTTP will prevail and use that.
> +        port = +port || 80;

This will need to change for HTTPS

@@ +1192,5 @@
> +        port = uri.port;
> +        if (port === -1) {
> +          if (scheme === "http") {
> +            port = 80;
> +          } else {

Again, needs changing for HTTPS

@@ +1713,5 @@
> +   *   the key whose corresponding value is to be returned
> +   * @returns string
> +   *   the corresponding value, which is initially the empty string
> +   */
> +  _getState: function(path, key) {

Not sure any of this state management stuff is used, either. Probably worth getting rid of (also the registrations of it in sjs setup above)

Comment 60

[Eric Tsai (no more review request accepted)]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #59)
> Comment on attachment 8721913 [details] [diff] [review]
> Patch 1. RemoteControlHttpd.jsm, js http server
> 
> Review of attachment 8721913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So there are two big things for me here - there is a lot of code that is
> unused, still (mostly for allowing state to be shared between requests),
> which should be removed - we should not ship what we aren't using. I called
> out some of this in my inline comments, but I can't guarantee I caught all
> the unused bits.
> 

I will catch all unused code and remove in next patch.

> The other, and significantly more important item, is that there is no
> TLS/SSL support anywhere to be seen here. We absolutely should not be
> shipping anything that speaks plaintext - not only is it not good to enable
> passive MITM, it would also be hypocritical given our stance on TLS
> everywhere. The only work I see related to this is bug 1235013, which hasn't
> been touched in a month, and appears (from my quick glance at the patch) to
> be going down the "roll your own crypto scheme" path instead of using TLS to
> secure the connection. We might have to go through some sort of TOFU scheme
> with TLS, since it's going to be basically impossible to ensure hostname
> matches even if we had a properly signed cert, but that's still better than
> no crypto at all.
> 

Yes, bug 1235013 is part of the feature. We implement non-encrypted version then add encryption.
We adopt this not SSL/TLS because we want to reduce the impact on UX when user see warning on browser by self-signed certification everytime.
Could you provide more information about how "some sort of TOFU scheme" works? If user prompt only shows at the first time, I think it works our case.

> @@ +1073,5 @@
> > +        // scheme to use to look up the correct port here, in general.  Since
> > +        // the HTTPS case requires a tunnel/proxy and thus requires that the
> > +        // requested URI be absolute (and thus contain the necessary
> > +        // information), let's assume HTTP will prevail and use that.
> > +        port = +port || 80;
> 
> This will need to change for HTTPS
> 
> @@ +1192,5 @@
> > +        port = uri.port;
> > +        if (port === -1) {
> > +          if (scheme === "http") {
> > +            port = 80;
> > +          } else {
> 
> Again, needs changing for HTTPS
> 

Not sure "TOFU scheme" uses HTTPS or not. If not, could we just keep HTTP only because HTTPS is also unused for now?

Comment 61

[u408661]

(In reply to Eric Tsai from comment #60)
> (In reply to Nicholas Hurley [:nwgh][:hurley] from comment #59)
> > Comment on attachment 8721913 [details] [diff] [review]
> > Patch 1. RemoteControlHttpd.jsm, js http server
> > 
> > Review of attachment 8721913 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > So there are two big things for me here - there is a lot of code that is
> > unused, still (mostly for allowing state to be shared between requests),
> > which should be removed - we should not ship what we aren't using. I called
> > out some of this in my inline comments, but I can't guarantee I caught all
> > the unused bits.
> > 
> 
> I will catch all unused code and remove in next patch.
> 
> > The other, and significantly more important item, is that there is no
> > TLS/SSL support anywhere to be seen here. We absolutely should not be
> > shipping anything that speaks plaintext - not only is it not good to enable
> > passive MITM, it would also be hypocritical given our stance on TLS
> > everywhere. The only work I see related to this is bug 1235013, which hasn't
> > been touched in a month, and appears (from my quick glance at the patch) to
> > be going down the "roll your own crypto scheme" path instead of using TLS to
> > secure the connection. We might have to go through some sort of TOFU scheme
> > with TLS, since it's going to be basically impossible to ensure hostname
> > matches even if we had a properly signed cert, but that's still better than
> > no crypto at all.
> > 
> 
> Yes, bug 1235013 is part of the feature. We implement non-encrypted version
> then add encryption.
> We adopt this not SSL/TLS because we want to reduce the impact on UX when
> user see warning on browser by self-signed certification everytime.
> Could you provide more information about how "some sort of TOFU scheme"
> works? If user prompt only shows at the first time, I think it works our
> case.

TOFU = Trust On First Use. Basically, the first time we see a cert (client-side), we trust it for that hostname and treat it as trusted forever (like the "add exception" page for certs). So, things would go something like this:

1. First time the TV boots up, it generates a TLS certificate to be presented by the remote control web server. This certificate is valid in all ways except it is self-signed (since there's no root authority to sign it).
2. When the remote control app on the phone (or wherever) discovers the TV, it makes a connection to it and puts the self-signed cert into its own trust store (whether this happens automatically or with user intervention is a question for people more paranoid than I)
3. From this point on, all connections to the TV are considered normally-authenticated TLS connections, so nothing special happens in the future.

The one question is how to handle the TVs IP address changing (given this is likely on a home network, and home DHCP servers sometimes like to hand out changed IPs for no good reason), but I think that's an easily-solvable problem (share some kind of secret between the TV and remote, when the IP changes, the remote asks for some proof of ownership of the secret from the TV, when that proof is received, the cert is trusted for the new IP without any user intervention regardless of whether we have user intervention in the first pairing or not).

> > @@ +1192,5 @@
> > > +        port = uri.port;
> > > +        if (port === -1) {
> > > +          if (scheme === "http") {
> > > +            port = 80;
> > > +          } else {
> > 
> > Again, needs changing for HTTPS
> > 
> 
> Not sure "TOFU scheme" uses HTTPS or not. If not, could we just keep HTTP
> only because HTTPS is also unused for now?

Yes, TOFU implies HTTPS. I'm fine keeping HTTP in there, but unless I get some hard assurances that we won't ship this, even to a nightly-style population, without TLS, I'll likely never r+ this patch until TLS is supported.

Comment 62

[Eric Tsai (no more review request accepted)]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #61)

> TOFU = Trust On First Use. Basically, the first time we see a cert
> (client-side), we trust it for that hostname and treat it as trusted forever
> (like the "add exception" page for certs). So, things would go something
> like this:
> 
> 1. First time the TV boots up, it generates a TLS certificate to be
> presented by the remote control web server. This certificate is valid in all
> ways except it is self-signed (since there's no root authority to sign it).
> 2. When the remote control app on the phone (or wherever) discovers the TV,
> it makes a connection to it and puts the self-signed cert into its own trust
> store (whether this happens automatically or with user intervention is a
> question for people more paranoid than I)

In our case, we use browser(firefox, chrome, safari, etc.) as client connects to remote control web server on TV. Does browser support TOFU by default or it only works for app? We tried to implement HTTPS but we can't overcome how to avoid self-signed warning page. Finally we fall back to HTTP with our own crypto mechanism.

> 3. From this point on, all connections to the TV are considered
> normally-authenticated TLS connections, so nothing special happens in the
> future.
> 

The reason to use browser is let user can easily visit remote control web server without installing native app.

Comment 63

[u408661]

(In reply to Eric Tsai from comment #62)
> In our case, we use browser(firefox, chrome, safari, etc.) as client
> connects to remote control web server on TV. Does browser support TOFU by
> default or it only works for app? We tried to implement HTTPS but we can't
> overcome how to avoid self-signed warning page. Finally we fall back to HTTP
> with our own crypto mechanism.

TOFU is not a particular technology, it is just the idea that the first time you see a certificate, you trust it as authoritative for that host (regardless of the existing trust database). An example of this is, in fact, the self-signed warning page (where you can add a permanent exception - that's what TOFU is). I'm not suggesting we use the existing self-signed warning, it's horrible UX for something like this. We'll either need to override that page for this use case (seems hard?) or automate the initial trusting of the certificate (which can be done in chrome script, netwerk/test/unit/test_http2.js has an example of this). Yes, all of these options are harder than either plaintext or a roll-your-own crypto mechanism, but these options are also significantly more correct that plaintext or rolling your own. I'm not sure that the security people would sign off on rolling our own crypto-over-http scheme (nor do I think that they should!) - down that path lies madness and subtle security bugs that no one wants. All of this can be done either in the browser or an app, it doesn't matter which.

I do want to back up for a moment, though, and ask (again) why we're using an HTTPD that bridges to gecko? The only use of gecko internals I see in the patches on this bug are to generate a UUID - something that is easily available from any language, including nodejs or python. I also see some mention of getting at things in an app:// URI - surely we could just go straight to disk for those? Is there any use of gecko internals *not* in this patch that necessitates the use of an httpd with direct access to gecko? (For example, how does the remote control web page change volume? Is there some gecko API that allows this? I don't see the httpd or other JSMs in any of the patches on this bug that use that kind of API, so I'm trying to get the full picture). If there is no reason for the httpd for this work to have access to gecko internals, I have to ask again why we're using httpd.js to begin with - there are plenty of http servers out there that have actually been tested in the real world that we could be using.

Comment 64

[u408661]

Oh, and I forgot to mention (though perhaps you already know this) - we have nsITLSServerSocket, which is an extension of nsIServerSocket, so changing the httpd.js code to serve over TLS should be a relatively straightforward exercise.

Comment 65

[Eric Tsai (no more review request accepted)]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #63)

This is the first bug for the feature. The other 2 bugs for non-encryption is 1197751 and 1207996. For You can refer to https://wiki.mozilla.org/Firefox_OS/Remote_Control for current architecture design.

> TOFU is not a particular technology, it is just the idea that the first time
> you see a certificate, you trust it as authoritative for that host
> (regardless of the existing trust database). An example of this is, in fact,
> the self-signed warning page (where you can add a permanent exception -
> that's what TOFU is). I'm not suggesting we use the existing self-signed
> warning, it's horrible UX for something like this. We'll either need to
> override that page for this use case (seems hard?) or automate the initial
> trusting of the certificate (which can be done in chrome script,
> netwerk/test/unit/test_http2.js has an example of this). Yes, all of these
> options are harder than either plaintext or a roll-your-own crypto
> mechanism, but these options are also significantly more correct that
> plaintext or rolling your own. I'm not sure that the security people would
> sign off on rolling our own crypto-over-http scheme (nor do I think that
> they should!) - down that path lies madness and subtle security bugs that no
> one wants. All of this can be done either in the browser or an app, it
> doesn't matter which.
> 

I agree with TLS is more secure then plaintext and our-own crypto. Currently we don't figure out how to override the page in all browsers(yes, we don't provide app for user. That could make things easier.) by easy way. Not sure if secure team will give us other feedback to improve this.

> I do want to back up for a moment, though, and ask (again) why we're using
> an HTTPD that bridges to gecko? The only use of gecko internals I see in the
> patches on this bug are to generate a UUID - something that is easily
> available from any language, including nodejs or python. I also see some
> mention of getting at things in an app:// URI - surely we could just go
> straight to disk for those? Is there any use of gecko internals *not* in
> this patch that necessitates the use of an httpd with direct access to
> gecko? (For example, how does the remote control web page change volume? Is
> there some gecko API that allows this? I don't see the httpd or other JSMs
> in any of the patches on this bug that use that kind of API, so I'm trying
> to get the full picture). If there is no reason for the httpd for this work
> to have access to gecko internals, I have to ask again why we're using
> httpd.js to begin with - there are plenty of http servers out there that
> have actually been tested in the real world that we could be using.

We use HTTPD because it receives control messages from client and "simulate" user actions like press home key, move mouse cursor by using XPCOM interface in sandbox. We use ".sjs" file in "resource://gre/res/remotecontrol" because they runs chrome privilege.

Other assets are packaged in app://remote-control-client.gaiamobile.org. I hope the wiki document can let you know more why have to design like this. Also I will try nsITLSServerSocket and let you know the result.

Comment 66

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #64)
> Oh, and I forgot to mention (though perhaps you already know this) - we have
> nsITLSServerSocket, which is an extension of nsIServerSocket, so changing
> the httpd.js code to serve over TLS should be a relatively straightforward
> exercise.

I don't think using an invalid (self-signed) certificate is an option here, unless we are ok with users having to click through certificate warnings. The use case here is with client devices outside of our control, I'm not sure how we can do anything to deal with the existing certificate warning? I agree with the TOFU approach in general but I just don't know of any option to deal with certificate warnings in a user-friendly way (nevermind the anti-pattern of training users to click through certificates).


I made an alternative suggestion via email which I'll summarize here. Any thoughts on this approach?

1. Ship the remote control code via a static website on a internet-hosted webserver (https://remotecontrol.foo.bar) so that we can know we trust the client code.

2. Use some kind of PAKE ( J-PAKE maybe?) to agree on a key based on a passcode shared visually between TV & client. 

3. Strip down/replace the httpd.js code with a more simple mechanism that just processes commands sent from the trusted client code to TV (either via websockets or XHR. Websockets might be out if https requires all websockets to be secure)

What do you think?

Comment 67

[u408661]

(In reply to Paul Theriault [:pauljt] from comment #66)
> I don't think using an invalid (self-signed) certificate is an option here,
> unless we are ok with users having to click through certificate warnings.
> The use case here is with client devices outside of our control, I'm not
> sure how we can do anything to deal with the existing certificate warning? I
> agree with the TOFU approach in general but I just don't know of any option
> to deal with certificate warnings in a user-friendly way (nevermind the
> anti-pattern of training users to click through certificates).

Yeah, this is true. Ugh. (Aside: now I want to go write a spec for handling TOFU well in cases like this, and get everyone to implement it. Probably not enough to actually do it, though.)

Anyway, I think your plan as outlined both here and (slightly more fleshed-out) in email makes a lot of sense. Let's do that :)

Comment 68

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #67)
> (In reply to Paul Theriault [:pauljt] from comment #66)
> > I don't think using an invalid (self-signed) certificate is an option here,
> > unless we are ok with users having to click through certificate warnings.
> > The use case here is with client devices outside of our control, I'm not
> > sure how we can do anything to deal with the existing certificate warning? I
> > agree with the TOFU approach in general but I just don't know of any option
> > to deal with certificate warnings in a user-friendly way (nevermind the
> > anti-pattern of training users to click through certificates).
> 
> Yeah, this is true. Ugh. (Aside: now I want to go write a spec for handling
> TOFU well in cases like this, and get everyone to implement it. Probably not
> enough to actually do it, though.)
> 
> Anyway, I think your plan as outlined both here and (slightly more
> fleshed-out) in email makes a lot of sense. Let's do that :)

So a flaw in my cunning plan is mixed content blocking. HTTPS can't make XHR requests to http due to FF blocking this as part of mixed content blocking. [1] Back to the drawing board :(

[1]https://developer.mozilla.org/en-US/docs/Security/Mixed_content#Mixed_active_content

Comment 69

[Josh Cheng [:josh]]

Hi Eric,
Could you revise the title to TCP socket?
Thanks

Comment 70

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. RemoteControlService.jsm, service, preference and settings

Hi :schien, could you please help to provide feedback of RemoteContorlService based on TLSSocketServer. This version only start service, bring up TLSSocketServer if dom.presentation.discoverable is true.

Comment 71

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

Comment on attachment 8746461 [details] [diff] [review]
Patch 1. RemoteControlService.jsm, service, preference and settings

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

can we default turn on remote control?

::: b2g/app/b2g.js
@@ +1156,5 @@
>  #endif
> +
> +#ifdef MOZ_B2G_REMOTECONTROLSERVICE
> +// Remote Control default TLS socket server port
> +pref("remotecontrol.default_server_port", 8080);

can we use random port?

::: b2g/components/RemoteControlService.jsm
@@ +64,5 @@
> +  _socket: null, // The socket associated with this
> +  _doQuit: false, // Indicates when the service is to be shut down at the end of the request.
> +  _socketClosed: true, // True if the socket in this is closed, false otherwise.
> +  _connectionGen: 0, // Used for tracking existing connections
> +  _connections: {}, // Hash of all open connections, indexed by connection number

use Map for '_connections', it'll provide more utility functions (such as forEach). https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

@@ +200,5 @@
> +          // connections, plus a safety margin in case some other process is
> +          // talking to the server as well.
> +          let maxConnections = 5 + Math.max(
> +            Services.prefs.getIntPref("network.http.max-persistent-connections-per-server"),
> +            Services.prefs.getIntPref("network.http.max-persistent-connections-per-proxy"));

maximum number of connection is for HTTP server, do we still need this for TLS connection?

@@ +206,5 @@
> +          try {
> +            // When automatically selecting a port, sometimes the chosen port is
> +            // "blocked" from clients. So, we simply keep trying to to
> +            // get a server socket until a valid port is obtained. We limit
> +            // ourselves to finite attempts just so we don't loop forever.

We can simply use random port since we'll broadcast port information via mDNS.

@@ +337,5 @@
> +  this._connection = connection;
> +
> +  this._output = Components.classes["@mozilla.org/intl/converter-output-stream;1"]
> +                           .createInstance(Components.interfaces.nsIConverterOutputStream);
> +  this._output.init(this._connection.output, "UTF-8", 0, 0x0000);

We should leverage https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/io/text-streams.js for handling of the text input/output on IO stream, since the code is currently being used by addon SDK. Copy the necessary code to here should be enough.

@@ +350,5 @@
> +    try {
> +      let incomingMessage = bin2String(readBytes(input, input.available()));
> +      try {
> +        // Parse JSON string to event object
> +        let event = JSON.parse(incomingMessage);

you might read partial string or concatenated string so JSON.parse might fail. You'll need manually split the string into multiple JSON string (search for '}{' pattern) and preserve the incomplete incoming message.

Comment 72

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. RemoteControlService.jsm, service

Comment 73

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

Comment on attachment 8749059 [details] [review]
Patch 1. RemoteControlService.jsm, service

please see my comment on pull request

Comment 74

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

Comment on attachment 8749059 [details] [review]
Patch 1. RemoteControlService.jsm, service

looks good mostly, please see my additional comments on pull request.

Comment 75

[Eric Tsai (no more review request accepted)]

Comment on attachment 8749059 [details] [review]
Patch 1. RemoteControlService.jsm, service

Hi Nicolas, according to security review, we've change architecture to provide connection on TLS socket server, use JPAKE as authentication.

This patch is about connection on TLS socket server, may need your help to review.

For more architecture design detail, you could refer to https://wiki.mozilla.org/Firefox_OS/Remote_Control

Comment 76

[u408661]

Would it be possible to get the patch attached as a regular patch? Github has (IMO) found a way to make code review even worse than splinter.

Comment 77

[u408661]

Using mozreview would be fine, too. Just not github (please)

Comment 78

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. RemoteControlService.jsm, service, preference

Hi Nicolas, this is the same patch but I convert to hg patch file. I use github it's because Firefox TV 2.6 use github as version control tool. I will sync the change from review to github. Once the review process done, could you also give plus to the github PR?

Comment 79

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. RemoteControlService.jsm, service, preference

Hi Nicolas, this is the same patch but I convert to hg patch file. I use github it's because Firefox TV 2.6 use github as version control tool. I will sync the change from review to github. Once the review process done, could you also give plus to the github PR?

Comment 80

[u408661]

Comment on attachment 8751175 [details] [diff] [review]
Patch 1. RemoteControlService.jsm, service, preference

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

I have a couple concerns, plus a few grammar nits in comments that can be fixed up.

::: b2g/components/RemoteControlService.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * RemoteControlService.jsm is the entry point of remote control function.
> + * The service initializes a TLS socket server to receives events from user.

"which receives" or "to receive", not "to receives"

@@ +9,5 @@
> + *               RemoteControlService <-- Gecko Preference
> + *
> + *     user -->  nsITLSSocketServer --> script (gecko)
> + *
> + * Events from user are in JSON format. After parsed to control command,

"After they are parsed into control commands"

@@ +10,5 @@
> + *
> + *     user -->  nsITLSSocketServer --> script (gecko)
> + *
> + * Events from user are in JSON format. After parsed to control command,
> + * these events are passed to script (js), runs in sandbox,

"run in sandbox"

@@ +175,5 @@
> +    // Monitor xpcom-shutdown to stop service and clean up
> +    Services.obs.addObserver(this, "xpcom-shutdown", false);
> +
> +    // Start TLSSocketServer with self-signed certification
> +    Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);

This line does not appear particularly useful. If it's required for the cert service to work properly, please add a comment saying so (or for whatever reason it's required, please add a comment saying why). If it's not required, just get rid of it.

@@ +196,5 @@
> +              if (!allowed) {
> +                DEBUG && debug("Warning: obtained TLSServerSocket listens on a blocked port: " + temp.port);
> +              }
> +
> +              if (!allowed && self._port == -1) {

Should this be "!allowed || self._port == -1"?

@@ +214,5 @@
> +            DEBUG && debug("Listen on port " + socket.port + ", " + MAX_CLIENT_CONNECTIONS + " pending connections");
> +
> +            socket.serverCert = cert;
> +            socket.setSessionCache(false);
> +            socket.setSessionTickets(false);

Why are you disabling the session cache and session tickets? These should be decent perf improvements for re-connections from clients who have closed their sockets and re-connect after a short time.

@@ +391,5 @@
> +}
> +EventHandler.prototype = {
> +  // PUBLIC FUNCTIONS
> +  handleEvent: function(aEvent) {
> +    // Implement JPAKE pairing and control command dispatch here

What's the status of this? Seems pretty critical to me to have this implemented before reviews continue.

Comment 81

[Eric Tsai (no more review request accepted)]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #80)

Thanks Nicholas, will fix grammar nits and other comments. Please refer my answer inline:

> @@ +196,5 @@
> > +              if (!allowed) {
> > +                DEBUG && debug("Warning: obtained TLSServerSocket listens on a blocked port: " + temp.port);
> > +              }
> > +
> > +              if (!allowed && self._port == -1) {
> 
> Should this be "!allowed || self._port == -1"?
> 

If we use port = -1 but get a blocked random port, then we will retry (100 times) to get an allowed port.
If we use fixed port but it's blocked, then we stop trying then quit.

"!allowed || self._port == -1" then we always retry (self._port = socket.port is in line 224) until loop ends.
Do you think this make sense?

Comment 82

[Eric Tsai (no more review request accepted)]

Attachment: Patch 1. RemoteControlService.jsm, service, preference

Fix grammar, add more comment for reviewer

Comment 84

[Josh Cheng [:josh]]

Hi Nicholas,

This is TV EPM Josh. We would like this feature to be landed before London workweek. Could you please kindly help to review the patch from Eric when you are available? :)

Thank you so much for the help!

Comment 85

[u408661]

(In reply to Eric Tsai from comment #81)
> (In reply to Nicholas Hurley [:nwgh][:hurley] from comment #80)
> 
> Thanks Nicholas, will fix grammar nits and other comments. Please refer my
> answer inline:
> 
> > @@ +196,5 @@
> > > +              if (!allowed) {
> > > +                DEBUG && debug("Warning: obtained TLSServerSocket listens on a blocked port: " + temp.port);
> > > +              }
> > > +
> > > +              if (!allowed && self._port == -1) {
> > 
> > Should this be "!allowed || self._port == -1"?
> > 
> 
> If we use port = -1 but get a blocked random port, then we will retry (100
> times) to get an allowed port.
> If we use fixed port but it's blocked, then we stop trying then quit.
> 
> "!allowed || self._port == -1" then we always retry (self._port =
> socket.port is in line 224) until loop ends.
> Do you think this make sense?

Ah, yes, that makes sense - I was interpreting the -1 as an error return, forgetting that self._port isn't set until later (and also apparently forgetting that this is not C++, so self._port being passed isn't a reference that can be overwritten by the callee)

Comment 86

[u408661]

Comment on attachment 8751571 [details] [diff] [review]
Patch 1. RemoteControlService.jsm, service, preference

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

LGTM, I'll figure out how to r+ the github thing shortly

Comment 87

[u408661]

Comment on attachment 8749059 [details] [review]
Patch 1. RemoteControlService.jsm, service

r=me (assuming the github PR has the fixes I r+'d in the splinter patch). If I need to do something directly on github to r+ this, let me know

Comment 88

[Eric Tsai (no more review request accepted)]

Comment on attachment 8749059 [details] [review]
Patch 1. RemoteControlService.jsm, service

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1197749
User impact if declined: No remote control feature
Testing completed: Self tested
Risk to taking this patch (and alternatives if risky): low, individual feature
String or UUID changes made by this patch: N/A

Comment 89

[Josh Cheng [:josh]]

Comment on attachment 8749059 [details] [review]
Patch 1. RemoteControlService.jsm, service

Approved for TV 2.6.

Comment 90

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/425aaa9763aef80ec1f7e78079e410ee1ccc9e32

Comment 91

[Eric Tsai (no more review request accepted)]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #80)
> Comment on attachment 8751175 [details] [diff] [review]
> @@ +214,5 @@
> > +            DEBUG && debug("Listen on port " + socket.port + ", " + MAX_CLIENT_CONNECTIONS + " pending connections");
> > +
> > +            socket.serverCert = cert;
> > +            socket.setSessionCache(false);
> > +            socket.setSessionTickets(false);
> 

Hi Nicholas, I met problems when set session cache and tickets to true. Now I runs the service on nesux player based on branch gecko 2.6.

When I set session cache to true, when client connects and sends something, onInputStreamReady is called but input.available() is 0, looks like client disconnects.

When I set session tickets to true, when client connects, b2g crashes on https://github.com/mozilla-b2g/gecko-b2g/blob/b2g48_v2_6/security/nss/lib/ssl/ssl3ext.c#L194

Not sure if you have any idea about this? I found devtool also use TLSSocketServer, and set these two to false, not sure it's because we use self-signed certificate and client will override to trust server cert?

Comment 92

[u408661]

Eric - see bug 1273677 - it would appear that there is a bug around session caches in TLSSocketServer. It looks like that's about ready to land in nightly, but I'm not sure if it will be uplifted to b2g48 or not, so it's probably best to disable session caches in b2g48 until the bug gets fixed there (if it gets fixed there at all).