Bug 1433958 - If setting the host succeeds, SetHostPort should not return an error code r=mayhemer
authorValentin Gosu <valentin.gosu@gmail.com>
Mon, 26 Feb 2018 20:43:47 +0100
changeset 405459 2246691a04f845d35b0a87e0040047600df24763
parent 405458 d46afbe90f3c9dbeb0379d40f8292004dc5c6916
child 405460 7ecadae9f269a152ebd9f2f6d0519b15644d9003
push id33519
push useraiakab@mozilla.com
push dateTue, 27 Feb 2018 09:56:16 +0000
treeherdermozilla-central@c4425fcdfb5b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1433958, 65536
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1433958 - If setting the host succeeds, SetHostPort should not return an error code r=mayhemer WebPlatformTests expect that when calling url.host = "host:" // port missing url.host = "host:65536" // port too big url.host = "host:bla" // invalid port that the hostname will be set, but the port will be left unchanged. Since DOM APIs are the only consumers for SetHostPort it means we can change this behaviour to match the WPT expectations. As such, SetHostPort will return NS_OK if setting the host succeded, and will ignore if setting the port failed. MozReview-Commit-ID: LoMw8hCWlCv
netwerk/base/nsIURI.idl
netwerk/base/nsStandardURL.cpp
netwerk/test/unit/test_standardurl.js
--- a/netwerk/base/nsIURI.idl
+++ b/netwerk/base/nsIURI.idl
@@ -149,16 +149,18 @@ interface nsIURI : nsISupports
     attribute AUTF8String username;
     attribute AUTF8String password;
 
     /**
      * The host:port (or simply the host, if port == -1).
      *
      * If this attribute is set to a value that only has a host part, the port
      * will not be reset. To reset the port as well use setHostAndPort.
+     * If setting the host succeeds, this method will return NS_OK, even if
+     * setting the port fails (error in parsing the port, or value out of range)
      */
     attribute AUTF8String hostPort;
 
     /**
      * This function will always set a host and a port. If the port part is
      * empty, the value of the port will be set to the default value.
      */
     void setHostAndPort(in AUTF8String hostport);
--- a/netwerk/base/nsStandardURL.cpp
+++ b/netwerk/base/nsStandardURL.cpp
@@ -1973,35 +1973,35 @@ nsStandardURL::SetHostPort(const nsACStr
             // The format should be [2001::1]:80 where the port is optional
             return NS_ERROR_MALFORMED_URI;
         }
     }
 
     nsresult rv = SetHost(Substring(start, iter));
     NS_ENSURE_SUCCESS(rv, rv);
 
-    // Also set the port if needed.
-    if (iter != end) {
-        iter++;
-        if (iter != end) {
-            nsCString portStr(Substring(iter, end));
-            nsresult rv;
-            int32_t port = portStr.ToInteger(&rv);
-            if (NS_SUCCEEDED(rv)) {
-                rv = SetPort(port);
-                NS_ENSURE_SUCCESS(rv, rv);
-            } else {
-                // Failure parsing port number
-                return NS_ERROR_MALFORMED_URI;
-            }
-        } else {
-            // port number is missing
-            return NS_ERROR_MALFORMED_URI;
-        }
+    if (iter == end) {
+        // does not end in colon
+        return NS_OK;
+    }
+
+    iter++; // advance over the colon
+    if (iter == end) {
+        // port number is missing
+        return NS_OK;
     }
+
+    nsCString portStr(Substring(iter, end));
+    int32_t port = portStr.ToInteger(&rv);
+    if (NS_FAILED(rv)) {
+        // Failure parsing the port number
+        return NS_OK;
+    }
+
+    Unused << SetPort(port);
     return NS_OK;
 }
 
 // This function is different than SetHostPort in that the port number will be
 // reset as well if aValue parameter does not contain a port port number.
 NS_IMETHODIMP
 nsStandardURL::SetHostAndPort(const nsACString &aValue)
 {
--- a/netwerk/test/unit/test_standardurl.js
+++ b/netwerk/test/unit/test_standardurl.js
@@ -203,18 +203,23 @@ add_test(function test_ipv6_fail()
   Assert.throws(() => { url = url.mutate().setHostPort("[2001:1]").finalize(); }, "bad IPv6 address");
   Assert.throws(() => { url = url.mutate().setHostPort("[2001:1]10").finalize(); }, "bad IPv6 address");
   Assert.throws(() => { url = url.mutate().setHostPort("[2001:1]10:20").finalize(); }, "bad IPv6 address");
   Assert.throws(() => { url = url.mutate().setHostPort("[2001:1]:10:20").finalize(); }, "bad IPv6 address");
   Assert.throws(() => { url = url.mutate().setHostPort("[2001:1").finalize(); }, "bad IPv6 address");
   Assert.throws(() => { url = url.mutate().setHostPort("2001]:1").finalize(); }, "bad IPv6 address");
   Assert.throws(() => { url = url.mutate().setHostPort("2001:1]").finalize(); }, "bad IPv6 address");
   Assert.throws(() => { url = url.mutate().setHostPort("").finalize(); }, "Empty hostPort should fail");
-  Assert.throws(() => { url = url.mutate().setHostPort("[2001::1]:").finalize(); }, "missing port number");
-  Assert.throws(() => { url = url.mutate().setHostPort("[2001::1]:bad").finalize(); }, "bad port number");
+
+  // These checks used to fail, but now don't (see bug 1433958 comment 57)
+  url = url.mutate().setHostPort("[2001::1]:").finalize();
+  Assert.equal(url.spec, "http://[2001::1]/");
+  url = url.mutate().setHostPort("[2002::1]:bad").finalize();
+  Assert.equal(url.spec, "http://[2002::1]/");
+
   run_next_test();
 });
 
 add_test(function test_clearedSpec()
 {
   var url = stringToURL("http://example.com/path");
   Assert.throws(() => { url = url.mutate().setSpec("http: example").finalize(); }, "set bad spec");
   Assert.throws(() => { url = url.mutate().setSpec("").finalize(); }, "set empty spec");