Bug 1369827 - Make proxy port an optional suffix for the host. r=ato

The WebDriver spec has been changed a while ago in regard of how
proxy capabilities are getting specified. It means that the port
is no longer its own key but an optional suffix for each of the
ftpProxy, httpProxy, sslProxy, and socksProxy keys.

MozReview-Commit-ID: zdYnVZSf09

--HG--
extra : rebase_source : c4928e6170b52a0ee247f50861646ec29a56bd34

testing/marionette/driver.js

@@ -772,22 +772,23 @@ GeckoDriver.prototype.newSession = async function(cmd, resp) {
   }
   this.sessionID = element.generateUUID();
   this.newSessionCommandId = cmd.id;
+
   try {
     this.capabilities = session.Capabilities.fromJSON(cmd.parameters);
+
+    if (!this.secureTLS) {
+      logger.warn("TLS certificate errors will be ignored for this session");
+      let acceptAllCerts = new cert.InsecureSweepingOverride();
+      cert.installOverride(acceptAllCerts);
+    }
+
+    if (this.proxy.init()) {
+      logger.info("Proxy settings initialised: " + JSON.stringify(this.proxy));
+    }
   } catch (e) {
     throw new SessionNotCreatedError(e);
   }
 
-  if (!this.secureTLS) {
-    logger.warn("TLS certificate errors will be ignored for this session");
-    let acceptAllCerts = new cert.InsecureSweepingOverride();
-    cert.installOverride(acceptAllCerts);
-  }
-
-  if (this.proxy.init()) {
-    logger.info("Proxy settings initialised: " + JSON.stringify(this.proxy));
-  }
-
   // If we are testing accessibility with marionette, start a11y service in
   // chrome first. This will ensure that we do not have any content-only
   // services hanging around.

testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py

@@ -119,37 +119,6 @@ class TestCapabilityMatching(MarionetteTestCase):
             with self.assertRaisesRegexp(SessionNotCreatedException, "InvalidArgumentError"):
                 self.marionette.start_session({"pageLoadStrategy": value})
 
-    def test_proxy_none_by_default(self):
-        self.marionette.start_session()
-        self.assertNotIn("proxy", self.marionette.session_capabilities)
-
-    def test_invalid_proxy_type(self):
-        with self.assertRaises(SessionNotCreatedException):
-            self.marionette.start_session({"proxy": {"proxyAutoconfigUrl": None}})
-
-        with self.assertRaises(SessionNotCreatedException):
-            self.marionette.start_session({"proxy": {"proxyType": None}})
-
-    def test_proxy_type_pac_invalid_url(self):
-        with self.assertRaises(SessionNotCreatedException):
-            self.marionette.start_session({"proxy": {"proxyType": "pac"}})
-
-        with self.assertRaises(SessionNotCreatedException):
-            self.marionette.start_session({"proxy": {"proxyType": "pac",
-                                                     "proxyAutoconfigUrl": None}})
-
-    def test_proxy_type_direct(self):
-        self.marionette.start_session({"proxy": {"proxyType": "direct"}})
-        self.assertIn("proxy", self.marionette.session_capabilities)
-        self.assertEqual(self.marionette.session_capabilities["proxy"]["proxyType"], "direct")
-        self.assertEqual(self.marionette.get_pref("network.proxy.type"), 0)
-
-    def test_proxy_type_manual(self):
-        self.marionette.start_session({"proxy": {"proxyType": "manual"}})
-        self.assertIn("proxy", self.marionette.session_capabilities)
-        self.assertEqual(self.marionette.session_capabilities["proxy"]["proxyType"], "manual")
-        self.assertEqual(self.marionette.get_pref("network.proxy.type"), 1)
-
     def test_timeouts(self):
         timeouts = {u"implicit": 123, u"pageLoad": 456, u"script": 789}
         caps = {"timeouts": timeouts}

testing/marionette/harness/marionette_harness/tests/unit/test_proxy.py

@@ -2,251 +2,97 @@
 # 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/.
 
-from marionette_driver.errors import InvalidArgumentException
+from marionette_driver import errors
 
 from marionette_harness import MarionetteTestCase
 
 
-class TestProxy(MarionetteTestCase):
+class TestProxyCapabilities(MarionetteTestCase):
 
     def setUp(self):
-        super(TestProxy, self).setUp()
+        super(TestProxyCapabilities, self).setUp()
+
         self.marionette.delete_session()
 
-    def test_that_we_can_set_a_autodetect_proxy(self):
-        capabilities = {"requiredCapabilities":
-                            {
-                                "proxy":{
-                                    "proxyType": "autodetect",
-                                }
-                            }
-                        }
-        self.marionette.start_session(capabilities)
-        result = None
-        with self.marionette.using_context('chrome'):
-            result = self.marionette.execute_script("""return {
-                "proxyType" : Services.prefs.getIntPref('network.proxy.type'),
-                }
-            """)
-
-        self.assertEqual(result["proxyType"], 4)
-
-    def test_that_capabilities_returned_have_proxy_details(self):
-        capabilities = {"requiredCapabilities":
-                            {
-                                "proxy":{
-                                    "proxyType": "autodetect",
-                                    }
-                            }
-                        }
-        self.marionette.start_session(capabilities)
-        result = self.marionette.session_capabilities
-
-        self.assertEqual(result["proxy"]["proxyType"], "autodetect")
-
-    def test_that_we_can_set_a_system_proxy(self):
-        capabilities = {"requiredCapabilities":
-                            {
-                                "proxy":{
-                                    "proxyType": "system",
-                                    }
-                            }
-                       }
-        self.marionette.start_session(capabilities)
-        result = None
-        with self.marionette.using_context('chrome'):
-            result = self.marionette.execute_script("""return {
-                "proxyType" : Services.prefs.getIntPref('network.proxy.type'),
-                }
-            """)
-
-        self.assertEqual(result["proxyType"], 5)
-
-    def test_we_can_set_a_pac_proxy(self):
-        url = "http://marionette.test"
-        capabilities = {"requiredCapabilities":
-                            {
-                                "proxy":{
-                                    "proxyType": "pac",
-                                    "proxyAutoconfigUrl": url,
-                                }
-                            }
-                        }
-        self.marionette.start_session(capabilities)
-        result = None
-        with self.marionette.using_context('chrome'):
-            result = self.marionette.execute_script("""return {
-                "proxyType" : Services.prefs.getIntPref('network.proxy.type'),
-                "proxyAutoconfigUrl" : Services.prefs.getCharPref('network.proxy.autoconfig_url'),
-                }
-            """)
-
-        self.assertEqual(result["proxyType"], 2)
-        self.assertEqual(result["proxyAutoconfigUrl"], url, 'proxyAutoconfigUrl was not set')
-
-    def test_that_we_can_set_a_manual_proxy(self):
-        port = 4444
-        url = "http://marionette.test"
-        capabilities = {"requiredCapabilities":
-                            {
-                                "proxy":{
-                                    "proxyType": "manual",
-                                    "ftpProxy": url,
-                                    "ftpProxyPort": port,
-                                    "httpProxy": url,
-                                    "httpProxyPort": port,
-                                    "sslProxy": url,
-                                    "sslProxyPort": port,
-                                }
-                            }
-                        }
-        self.marionette.start_session(capabilities)
-        result = None
-        with self.marionette.using_context('chrome'):
-            result = self.marionette.execute_script("""return {
-                "proxyType" : Services.prefs.getIntPref('network.proxy.type'),
-                "httpProxy" : Services.prefs.getCharPref('network.proxy.http'),
-                "httpProxyPort": Services.prefs.getIntPref('network.proxy.http_port'),
-                "sslProxy": Services.prefs.getCharPref('network.proxy.ssl'),
-                "sslProxyPort": Services.prefs.getIntPref('network.proxy.ssl_port'),
-                "ftpProxy": Services.prefs.getCharPref('network.proxy.ftp'),
-                "ftpProxyPort": Services.prefs.getIntPref('network.proxy.ftp_port'),
-                }
-            """)
-
-        self.assertEqual(result["proxyType"], 1)
-        self.assertEqual(result["httpProxy"], url, 'httpProxy was not set')
-        self.assertEqual(result["httpProxyPort"], port, 'httpProxyPort was not set')
-        self.assertEqual(result["sslProxy"], url, 'sslProxy url was not set')
-        self.assertEqual(result["sslProxyPort"], port, 'sslProxyPort was not set')
-        self.assertEqual(result["ftpProxy"], url, 'ftpProxy was not set')
-        self.assertEqual(result["ftpProxyPort"], port, 'ftpProxyPort was not set')
-
-    def test_we_can_set_a_manual_proxy_with_a_socks_proxy_with_socks_version(self):
-        port = 4444
-        url = "http://marionette.test"
-        capabilities = {"requiredCapabilities":
-                            {
-                                "proxy":{
-                                    "proxyType": "manual",
-                                    "socksProxy": url,
-                                    "socksProxyPort": port,
-                                    "socksVersion": 4,
-                                    "socksUsername": "cake",
-                                    "socksPassword": "made with cake"
-                                }
-                            }
-                        }
-        self.marionette.start_session(capabilities)
-        result = None
-        with self.marionette.using_context('chrome'):
-            result = self.marionette.execute_script("""return {
-                "proxyType" : Services.prefs.getIntPref('network.proxy.type'),
-                "socksProxy" : Services.prefs.getCharPref('network.proxy.socks'),
-                "socksProxyPort": Services.prefs.getIntPref('network.proxy.socks_port'),
-                "socksVersion": Services.prefs.getIntPref('network.proxy.socks_version'),
-                }
-            """)
-        self.assertEqual(result["socksProxy"], url, 'socksProxy was not set')
-        self.assertEqual(result["socksProxyPort"], port, 'socksProxyPort was not set')
-        self.assertEqual(result["socksVersion"], 4, 'socksVersion was not set to 4')
-
-    def test_we_can_set_a_manual_proxy_with_a_socks_proxy_with_no_socks_version(self):
-        port = 4444
-        url = "http://marionette.test"
-        capabilities = {"requiredCapabilities":
-                                    {
-                                    "proxy":{
-                                        "proxyType": "manual",
-                                        "socksProxy": url,
-                                        "socksProxyPort": port,
-                                        "socksUsername": "cake",
-                                        "socksPassword": "made with cake"
-                                     }
-                                    }
-        }
-        self.marionette.start_session(capabilities)
-        result = None
-        with self.marionette.using_context('chrome'):
-            result = self.marionette.execute_script("""return {
-                "proxyType" : Services.prefs.getIntPref('network.proxy.type'),
-                "socksProxy" : Services.prefs.getCharPref('network.proxy.socks'),
-                "socksProxyPort": Services.prefs.getIntPref('network.proxy.socks_port'),
-                "socksVersion": Services.prefs.getIntPref('network.proxy.socks_version'),
-
-                }
-            """)
-        self.assertEqual(result["socksProxy"], url, 'socksProxy was not set')
-        self.assertEqual(result["socksProxyPort"], port, 'socksProxyPort was not set')
-        self.assertEqual(result["socksVersion"], 5, 'socksVersion was not set to 5')
-
-    def test_when_not_all_manual_proxy_details_are_in_capabilities(self):
-        port = 4444
-        url = "http://marionette.test"
-        capabilities = {"requiredCapabilities":
-                                    {
-                                    "proxy":{
-                                        "proxyType": "manual",
-                                        "ftpProxy": url,
-                                        "ftpProxyPort": port,
-                                     }
-                                    }
-        }
-        self.marionette.start_session(capabilities)
-        result = None
-        with self.marionette.using_context('chrome'):
-            result = self.marionette.execute_script("""return {
-                "proxyType" : Services.prefs.getIntPref('network.proxy.type'),
-                "httpProxy" : Services.prefs.getCharPref('network.proxy.http'),
-                "httpProxyPort": Services.prefs.getIntPref('network.proxy.http_port'),
-                "sslProxy": Services.prefs.getCharPref('network.proxy.ssl'),
-                "sslProxyPort": Services.prefs.getIntPref('network.proxy.ssl_port'),
-                "ftpProxy": Services.prefs.getCharPref('network.proxy.ftp'),
-                "ftpProxyPort": Services.prefs.getIntPref('network.proxy.ftp_port'),
-                }
-            """)
-
-        self.assertEqual(result["proxyType"], 1)
-        self.assertNotEqual(result["httpProxy"], url,
-                            'httpProxy was set. {}'.format(result["httpProxy"]))
-        self.assertNotEqual(result["httpProxyPort"], port, 'httpProxyPort was set')
-        self.assertNotEqual(result["sslProxy"], url, 'sslProxy url was set')
-        self.assertNotEqual(result["sslProxyPort"], port, 'sslProxyPort was set')
-        self.assertEqual(result["ftpProxy"], url, 'ftpProxy was set')
-        self.assertEqual(result["ftpProxyPort"], port, 'ftpProxyPort was set')
-
-
-
-    def test_proxy_is_a_string_should_throw_invalid_argument(self):
-        capabilities = {"requiredCapabilities":
-                                    {
-                                    "proxy":"I really should be a dictionary"
-                                    }
-                            }
-        try:
-            self.marionette.start_session(capabilities)
-            self.fail("We should have started a session because proxy should be a dict")
-        except InvalidArgumentException as e:
-            assert e.message == "Value of 'proxy' should be an object"
-
-    def test_proxy_is_passed_in_with_no_proxy_doesnt_set_it(self):
-        capabilities = {"requiredCapabilities":
-            {
-                "proxy": {"proxyType": "NOPROXY"},
-            }
-        }
-        self.marionette.start_session(capabilities)
-        result = None
-        with self.marionette.using_context('chrome'):
-            result = self.marionette.execute_script("""return {
-                "proxyType": Services.prefs.getIntPref('network.proxy.type'),
-                };
-            """)
-
-        self.assertEqual(result["proxyType"], 0)
-
     def tearDown(self):
         if not self.marionette.session:
             self.marionette.start_session()
-        else:
-            self.marionette.restart(clean=True)
+
+        with self.marionette.using_context("chrome"):
+            self.marionette.execute_script("""
+                Cu.import("resource://gre/modules/Preferences.jsm");
+                Preferences.resetBranch("network.proxy");
+            """)
+
+        super(TestProxyCapabilities, self).tearDown()
+
+    def test_proxy_object_none_by_default(self):
+        self.marionette.start_session()
+        self.assertNotIn("proxy", self.marionette.session_capabilities)
+
+    def test_proxy_object_in_returned_capabilities(self):
+        capabilities = {"proxy": {"proxyType": "system"}}
+
+        self.marionette.start_session(capabilities)
+        self.assertEqual(self.marionette.session_capabilities["proxy"],
+                         capabilities["proxy"])
+
+    def test_proxy_type_autodetect(self):
+        capabilities = {"proxy": {"proxyType": "autodetect"}}
+
+        self.marionette.start_session(capabilities)
+        self.assertEqual(self.marionette.session_capabilities["proxy"],
+                         capabilities["proxy"])
+
+    def test_proxy_type_direct(self):
+        capabilities = {"proxy": {"proxyType": "direct"}}
+
+        self.marionette.start_session(capabilities)
+        self.assertEqual(self.marionette.session_capabilities["proxy"],
+                         capabilities["proxy"])
+
+    def test_proxy_type_manual_without_port(self):
+        proxy_hostname = "marionette.test"
+        capabilities = {"proxy": {
+            "proxyType": "manual",
+            "ftpProxy": "{}:21".format(proxy_hostname),
+            "httpProxy": "{}:80".format(proxy_hostname),
+            "sslProxy": "{}:443".format(proxy_hostname),
+    def test_proxy_type_pac(self):
+        pac_url = "http://marionette.test"
+        capabilities = {"proxy": {"proxyType": "pac", "proxyAutoconfigUrl": pac_url}}
+
+        self.marionette.start_session(capabilities)
+        self.assertEqual(self.marionette.session_capabilities["proxy"],
+                         capabilities["proxy"])
+
+    def test_proxy_type_system(self):
+        capabilities = {"proxy": {"proxyType": "system"}}
+
+        self.marionette.start_session(capabilities)
+        self.assertEqual(self.marionette.session_capabilities["proxy"],
+                         capabilities["proxy"])
+
+    def test_invalid_proxy_object(self):
+        capabilities = {"proxy": "I really should be a dictionary"}
+
+        with self.assertRaises(errors.SessionNotCreatedException):
+            self.marionette.start_session(capabilities)
+
+    def test_missing_proxy_type(self):
+        with self.assertRaises(errors.SessionNotCreatedException):
+            self.marionette.start_session({"proxy": {"proxyAutoconfigUrl": "foobar"}})
+
+    def test_invalid_proxy_type(self):
+        capabilities = {"proxy": {"proxyType": "NOPROXY"}}
+
+        with self.assertRaises(errors.SessionNotCreatedException):
+            self.marionette.start_session(capabilities)
+
+    def test_invalid_autoconfig_url_for_pac(self):
+        with self.assertRaises(errors.SessionNotCreatedException):
+            self.marionette.start_session({"proxy": {"proxyType": "pac"}})
+
+        with self.assertRaises(errors.SessionNotCreatedException):
+            self.marionette.start_session({"proxy": {"proxyType": "pac",
+                                                     "proxyAutoconfigUrl": None}})

testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini

@@ -5,6 +5,7 @@ skip-if = manage_instance == false || appname == 'fennec' # Bug 1298921 and bug
 [test_data_driven.py]
 [test_session.py]
 [test_capabilities.py]
+[test_proxy.py]
 [test_accessibility.py]
 [test_expectedfail.py]
 expected = fail

testing/marionette/session.js

@@ -6,6 +6,8 @@
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
+Cu.importGlobalProperties(["URL"]);
+
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
@@ -108,12 +110,12 @@ session.Proxy = class {
   /** @class */
   constructor() {
     this.proxyType = null;
+    this.ftpProxy = null;
+    this.ftpProxyPort = null;
     this.httpProxy = null;
     this.httpProxyPort = null;
     this.sslProxy = null;
     this.sslProxyPort = null;
-    this.ftpProxy = null;
-    this.ftpProxyPort = null;
     this.socksProxy = null;
     this.socksProxyPort = null;
     this.socksVersion = null;
@@ -140,21 +142,33 @@ session.Proxy = class {
 
       case "manual":
         Preferences.set("network.proxy.type", 1);
-        if (this.httpProxy && this.httpProxyPort) {
-          Preferences.set("network.proxy.http", this.httpProxy);
-          Preferences.set("network.proxy.http_port", this.httpProxyPort);
-        }
-        if (this.sslProxy && this.sslProxyPort) {
-          Preferences.set("network.proxy.ssl", this.sslProxy);
-          Preferences.set("network.proxy.ssl_port", this.sslProxyPort);
-        }
-        if (this.ftpProxy && this.ftpProxyPort) {
+
+        if (this.ftpProxy) {
           Preferences.set("network.proxy.ftp", this.ftpProxy);
-          Preferences.set("network.proxy.ftp_port", this.ftpProxyPort);
+          if (Number.isInteger(this.ftpProxyPort)) {
+            Preferences.set("network.proxy.ftp_port", this.ftpProxyPort);
+          }
         }
+
+        if (this.httpProxy) {
+          Preferences.set("network.proxy.http", this.httpProxy);
+          if (Number.isInteger(this.httpProxyPort)) {
+            Preferences.set("network.proxy.http_port", this.httpProxyPort);
+          }
+        }
+
+        if (this.sslProxy) {
+          Preferences.set("network.proxy.ssl", this.sslProxy);
+          if (Number.isInteger(this.sslProxyPort)) {
+            Preferences.set("network.proxy.ssl_port", this.sslProxyPort);
+          }
+        }
+
         if (this.socksProxy) {
           Preferences.set("network.proxy.socks", this.socksProxy);
-          Preferences.set("network.proxy.socks_port", this.socksProxyPort);
+          if (Number.isInteger(this.socksProxyPort)) {
+            Preferences.set("network.proxy.socks_port", this.socksProxyPort);
+          }
           if (this.socksVersion) {
             Preferences.set("network.proxy.socks_version", this.socksVersion);
           }
@@ -176,33 +190,60 @@ session.Proxy = class {
     }
   }
 
-  toString() { return "[object session.Proxy]"; }
-
-  /**
-   * @return {Object.<string, (number|string)>}
-   *     JSON serialisation of proxy object.
-   */
-  toJSON() {
-    return marshal({
-      proxyType: this.proxyType,
-      httpProxy: this.httpProxy,
-      httpProxyPort: this.httpProxyPort,
-      sslProxy: this.sslProxy,
-      sslProxyPort: this.sslProxyPort,
-      ftpProxy: this.ftpProxy,
-      ftpProxyPort: this.ftpProxyPort,
-      socksProxy: this.socksProxy,
-      socksProxyPort: this.socksProxyPort,
-      socksProxyVersion: this.socksProxyVersion,
-      proxyAutoconfigUrl: this.proxyAutoconfigUrl,
-    });
-  }
-
   /**
    * @param {Object.<string, ?>} json
    *     JSON Object to unmarshal.
+   *
+   * @throws {InvalidArgumentError}
+   *     When proxy configuration is invalid.
    */
   static fromJSON(json) {
+    // Parse hostname and optional port from host
+    function fromHost(scheme, host) {
+      assert.string(host);
+
+      if (host.includes("://")) {
+        throw new InvalidArgumentError(`${host} contains a scheme`);
+      }
+
+      let url;
+      try {
+        // To parse the host a scheme has to be added temporarily.
+        // If the returned value for the port is an empty string it
+        // could mean no port or the default port for this scheme was
+        // specified. In such a case parse again with a different
+        // scheme to ensure we filter out the default port.
+        url = new URL("http://" + host);
+        if (url.port == "") {
+          url = new URL("https://" + host);
+        }
+      } catch (e) {
+        throw new InvalidArgumentError(e.message);
+      }
+
+      // If the port hasn't been set, use the default port of
+      // the selected scheme (except for socks which doesn't have one).
+      let port = parseInt(url.port);
+      if (!Number.isInteger(port)) {
+        if (scheme === "socks") {
+          port = null;
+        } else {
+          port = Services.io.getProtocolHandler(scheme).defaultPort;
+        }
+      }
+
+      if (url.username != "" ||
+          url.password != "" ||
+          url.pathname != "/" ||
+          url.search != "" ||
+          url.hash != "") {
+        throw new InvalidArgumentError(
+            `${host} was not of the form host[:port]`);
+      }
+
+      return [url.hostname, port];
+    }
+
     let p = new session.Proxy();
     if (typeof json == "undefined" || json === null) {
       return p;
@@ -214,38 +255,69 @@ session.Proxy = class {
     p.proxyType = assert.string(json.proxyType);
 
     switch (p.proxyType) {
-      case "manual":
-        if (typeof json.httpProxy != "undefined") {
-          p.httpProxy = assert.string(json.httpProxy);
-          p.httpProxyPort = assert.positiveInteger(json.httpProxyPort);
-        }
-
-        if (typeof json.sslProxy != "undefined") {
-          p.sslProxy = assert.string(json.sslProxy);
-          p.sslProxyPort = assert.positiveInteger(json.sslProxyPort);
-        }
-
-        if (typeof json.ftpProxy != "undefined") {
-          p.ftpProxy = assert.string(json.ftpProxy);
-          p.ftpProxyPort = assert.positiveInteger(json.ftpProxyPort);
-        }
-
-        if (typeof json.socksProxy != "undefined") {
-          p.socksProxy = assert.string(json.socksProxy);
-          p.socksProxyPort = assert.positiveInteger(json.socksProxyPort);
-          p.socksProxyVersion = assert.positiveInteger(
-              json.socksProxyVersion);
-        }
-
+      case "autodetect":
+      case "direct":
+      case "system":
         break;
 
       case "pac":
         p.proxyAutoconfigUrl = assert.string(json.proxyAutoconfigUrl);
         break;
+
+      case "manual":
+        if (typeof json.ftpProxy != "undefined") {
+          [p.ftpProxy, p.ftpProxyPort] = fromHost("ftp", json.ftpProxy);
+        }
+        if (typeof json.httpProxy != "undefined") {
+          [p.httpProxy, p.httpProxyPort] = fromHost("http", json.httpProxy);
+        }
+        if (typeof json.sslProxy != "undefined") {
+          [p.sslProxy, p.sslProxyPort] = fromHost("https", json.sslProxy);
+        }
+        if (typeof json.socksProxy != "undefined") {
+          [p.socksProxy, p.socksProxyPort] = fromHost("socks", json.socksProxy);
+          p.socksProxyVersion = assert.positiveInteger(
+              json.socksProxyVersion);
+        }
+        break;
+
+      default:
+        throw new InvalidArgumentError(
+            `Invalid type of proxy: ${p.proxyType}`);
     }
 
     return p;
   }
+
+  /**
+   * @return {Object.<string, (number|string)>}
+   *     JSON serialisation of proxy object.
+   */
+  toJSON() {
+    function toHost(hostname, port) {
+      if (!hostname) {
+        return null;
+      }
+
+      if (port != null) {
+        return `${hostname}:${port}`;
+      }
+
+      return hostname;
+    }
+
+    return marshal({
+      proxyType: this.proxyType,
+      ftpProxy: toHost(this.ftpProxy, this.ftpProxyPort),
+      httpProxy: toHost(this.httpProxy, this.httpProxyPort),
+      sslProxy: toHost(this.sslProxy, this.sslProxyPort),
+      socksProxy: toHost(this.socksProxy, this.socksProxyPort),
+      socksProxyVersion: this.socksProxyVersion,
+      proxyAutoconfigUrl: this.proxyAutoconfigUrl,
+    });
+  }
+
+  toString() { return "[object session.Proxy]"; }
 };
 
 /** WebDriver session capabilities representation. */

testing/marionette/test_session.js

@@ -98,13 +98,9 @@ add_test(function test_Proxy_ctor() {
   let props = [
     "proxyType",
     "httpProxy",
-    "httpProxyPort",
     "sslProxy",
-    "sslProxyPort",
     "ftpProxy",
-    "ftpProxyPort",
     "socksProxy",
-    "socksProxyPort",
     "socksVersion",
     "proxyAutoconfigUrl",
   ];
@@ -132,6 +128,12 @@ add_test(function test_Proxy_init() {
   equal(Preferences.get("network.proxy.autoconfig_url"),
       "http://localhost:1234");
 
+  // direct
+  p = new session.Proxy();
+  p.proxyType = "direct";
+  ok(p.init());
+  equal(Preferences.get("network.proxy.type"), 0);
+
   // autodetect
   p = new session.Proxy();
   p.proxyType = "autodetect";
@@ -144,11 +146,24 @@ add_test(function test_Proxy_init() {
   ok(p.init());
   equal(Preferences.get("network.proxy.type"), 5);
 
-  // direct
-  p = new session.Proxy();
-  p.proxyType = "direct";
-  ok(p.init());
-  equal(Preferences.get("network.proxy.type"), 0);
+  // manual
+  for (let proxy of ["ftp", "http", "ssl", "socks"]) {
+    p = new session.Proxy();
+    p.proxyType = "manual";
+    p[`${proxy}Proxy`] = "foo";
+    p[`${proxy}ProxyPort`] = 42;
+    if (proxy === "socks") {
+      p[`${proxy}Version`] = 4;
+    }
+
+    ok(p.init());
+    equal(Preferences.get("network.proxy.type"), 1);
+    equal(Preferences.get(`network.proxy.${proxy}`), "foo");
+    equal(Preferences.get(`network.proxy.${proxy}_port`), 42);
+    if (proxy === "socks") {
+      equal(Preferences.get(`network.proxy.${proxy}_version`), 4);
+    }
+  }
 
   run_next_test();
 });
@@ -163,10 +178,33 @@ add_test(function test_Proxy_toJSON() {
   let p = new session.Proxy();
   deepEqual(p.toJSON(), {});
 
+  // manual
   p = new session.Proxy();
   p.proxyType = "manual";
   deepEqual(p.toJSON(), {proxyType: "manual"});
 
+  for (let proxy of ["ftpProxy", "httpProxy", "sslProxy", "socksProxy"]) {
+    let expected = {proxyType: "manual"}
+
+    let manual = new session.Proxy();
+    manual.proxyType = "manual";
+
+    // without port
+    manual[proxy] = "foo";
+    expected[proxy] = "foo"
+    deepEqual(manual.toJSON(), expected);
+
+    // with port
+    manual[proxy] = "foo";
+    manual[`${proxy}Port`] = 0;
+    expected[proxy] = "foo:0";
+    deepEqual(manual.toJSON(), expected);
+
+    manual[`${proxy}Port`] = 42;
+    expected[proxy] = "foo:42"
+    deepEqual(manual.toJSON(), expected);
+  }
+
   run_next_test();
 });
 
@@ -178,10 +216,10 @@ add_test(function test_Proxy_fromJSON() {
     Assert.throws(() => session.Proxy.fromJSON(typ), InvalidArgumentError);
   }
 
-  // must contain proxyType
+  // must contain a valid proxyType
   Assert.throws(() => session.Proxy.fromJSON({}), InvalidArgumentError);
-  deepEqual({proxyType: "foo"},
-      session.Proxy.fromJSON({proxyType: "foo"}).toJSON());
+  Assert.throws(() => session.Proxy.fromJSON({proxyType: "foo"}),
+      InvalidArgumentError);
 
   // manual
   session.Proxy.fromJSON({proxyType: "manual"});
@@ -189,40 +227,50 @@ add_test(function test_Proxy_fromJSON() {
   for (let proxy of ["httpProxy", "sslProxy", "ftpProxy", "socksProxy"]) {
     let manual = {proxyType: "manual"};
 
-    for (let typ of [true, 42, [], {}, null]) {
-      manual[proxy] = typ;
+    // invalid hosts
+    for (let host of [true, 42, [], {}, null, "http://foo",
+        "foo:-1", "foo:65536", "foo/test", "foo#42", "foo?foo=bar",
+        "2001:db8::1"]) {
+      manual[proxy] = host;
       Assert.throws(() => session.Proxy.fromJSON(manual),
           InvalidArgumentError);
     }
 
-    manual[proxy] = "foo";
-    Assert.throws(() => session.Proxy.fromJSON(manual),
-        InvalidArgumentError);
-
-    for (let typ of ["bar", true, [], {}, null, undefined]) {
-      manual[proxy + "Port"] = typ;
-      Assert.throws(() => session.Proxy.fromJSON(manual),
-          InvalidArgumentError);
-    }
-
-    manual[proxy] = "foo";
-    manual[proxy + "Port"] = 1234;
-
-    let expected = {
-      "proxyType": "manual",
-      [proxy]: "foo",
-      [proxy + "Port"]: 1234,
-    };
-
+    let expected = {"proxyType": "manual"};
     if (proxy == "socksProxy") {
-      manual.socksProxyVersion = 42;
-      expected.socksProxyVersion = 42;
+      manual.socksProxyVersion = 5;
+      expected.socksProxyVersion = 5;
+    }
+
+    // valid proxy hosts with port
+    for (let host of ["foo:1", "foo:80", "foo:443", "foo:65535",
+        "127.0.0.1:42", "[2001:db8::1]:42"]) {
+      manual[proxy] = host;
+      expected[proxy] = host;
+
+      deepEqual(expected, session.Proxy.fromJSON(manual).toJSON());
+    }
+
+    // Without a port the default port of the scheme is used
+    for (let host of ["foo", "foo:"]) {
+      manual[proxy] = host;
+
+      // For socks no default port is available
+      if (proxy === "socksProxy") {
+        expected[proxy] = `foo`;
+      } else {
+        let default_ports = {"ftpProxy": 21, "httpProxy": 80,
+           "sslProxy": 443};
+
+        expected[proxy] = `foo:${default_ports[proxy]}`;
+      }
+      deepEqual(expected, session.Proxy.fromJSON(manual).toJSON());
     }
-    deepEqual(expected, session.Proxy.fromJSON(manual).toJSON());
   }
 
+  // missing required socks version
   Assert.throws(() => session.Proxy.fromJSON(
-      {proxyType: "manual", socksProxy: "foo", socksProxyPort: 1234}),
+      {proxyType: "manual", socksProxy: "foo:1234"}),
       InvalidArgumentError);
 
   run_next_test();