Reconcile treating port=0 as valid across GURL and url::SchemeHostPort.

Before this CL:

*) GURL port_zero("http://example.com:0") would be
   considered a valid URL (in conformance with
   https://url.spec.whatwg.org/#port-state which allows port 0).
*) SchemeHostPort constructed out of such GURL would be considered
   invalid.
*) The inconsistency between GURL and SchemeHostPort would lead to
   bizarre results such as
       url::Origin::Resolve(url=http://foo.com:0,
                            base_origin=http://bar.com)
   returning an opaque origin with a precursor origin set
   to http://bar.com.  This in turn would lead to browser process
   crashes via CHECK(...CanAccessDataForOrigin...).

After this CL:

*) SchemeHostPort allows port=0
*) url::Origin::Resolve works as expected (verified by a new unit test)
*) No browser-process crashes (verified by a new browser test)

Also after this CL:

*) Navigating to port=0 still doesn't quite work, because Blink treats
  this as an invalid port (in KURL and SecurityOrigin) which results in
  the port round-tripping to the browser in DidCommitProvisionalLoad IPC
  as port=80 (and the mismatch leads to a renderer kill).

Bug: 1065532
Change-Id: I719fcfb0a027187805c3766d5e4b0365553067f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2128829
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758574}

components/url_formatter/elide_url_unittest.cc

@@ -636,7 +636,7 @@ TEST(TextEliderTest, FormatOriginForSecurityDisplay) {
        L"file://", L"file://", L"file://"},
       {"Invalid scheme 1", "twelve://www.cyber.org/wow.php", L"", L"", L""},
       {"Invalid scheme 2", "://www.cyber.org/wow.php", L"", L"", L""},
-      {"Invalid port 1", "https://173.194.65.103:000", L"", L"", L""},
+      {"Invalid port 1", "https://173.194.65.103:99999", L"", L"", L""},
       {"Invalid port 2", "https://173.194.65.103:gruffle", L"", L"", L""},
       {"Blob URL",
        "blob:http://www.html5rocks.com/4d4ff040-6d61-4446-86d3-13ca07ec9ab9",

content/browser/frame_host/navigation_controller_impl_browsertest.cc

@@ -10474,4 +10474,95 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTestNoServer,
   EXPECT_EQ(PAGE_TYPE_ERROR, entry->GetPageType());
 }
 
+// Test for a navigation that is
+// 1) initiated by a cross-site frame
+// 2) same-document
+// 3) to a http URL with port 0.
+// This is the scenario behind https://crbug.com/1065532.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       SameDocumentNavigationToHttpPortZero) {
+  GURL page_url(embedded_test_server()->GetURL(
+      "foo.com", "/navigation_controller/simple_page_1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), page_url));
+
+  // Inject a HTTP subframe.
+  const char kSubframeScriptTemplate[] = R"(
+      var iframe = document.createElement('iframe');
+      iframe.src = $1;
+      document.body.appendChild(iframe);
+  )";
+  GURL subframe_initial_url =
+      embedded_test_server()->GetURL("another-site.com", "/title2.html");
+  {
+    TestNavigationObserver subframe_injection_observer(shell()->web_contents(),
+                                                       1);
+    ASSERT_TRUE(ExecJs(
+        shell(), JsReplace(kSubframeScriptTemplate, subframe_initial_url)));
+    subframe_injection_observer.WaitForNavigationFinished();
+    ASSERT_TRUE(subframe_injection_observer.last_navigation_succeeded());
+  }
+
+  // From the main page initiate a navigation of the cross-site subframe to a
+  // http URL that has port=0.  Note that this is valid port according to the
+  // URL spec (https://url.spec.whatwg.org/#port-state).
+  //
+  // Before the fix for SchemeHostPort's handling of port=0 the 2nd iteration
+  // below would produce a browser process CHECK/crash:
+  // - Iteration #1: The navigation will error out (because nothing is listening
+  //   on port zero in the test case).  Since
+  //   RenderFrameHostImpl::FailedNavigation doesn't call
+  //   GetOriginForURLLoaderFactory, this iteration wouldn't trigger a crash.
+  // - Iteration #2: The navigation will be treated as a same-document
+  //   navigation, because |old_url| and |new_url| will be considered the same
+  //   when inspected by GetNavigationType in navigation_controller_impl.cc.
+  //   This will trigger a call to GetOriginForURLLoaderFactory which will
+  //   crash if port=0 confuses url::Origin::Resolve.  It is unclear if treating
+  //   the 2nd iteration as same-document should be considered a bug (see also
+  //   https://crbug.com/1065532#c4).
+  //
+  // After the fix for SchemeHostPort's handling of port=0, both iterations
+  // would trigger a CANNOT_COMMIT_ORIGIN kill (see below).
+  GURL::Replacements replace_port_and_ref;
+  replace_port_and_ref.SetPortStr("0");
+  replace_port_and_ref.SetRefStr("someRef");
+  GURL subframe_ref_url =
+      subframe_initial_url.ReplaceComponents(replace_port_and_ref);
+  // Doing 2 iterations, to test the same-document navigation case as outlined
+  // in the big comment above.  This test coverage will be important if we ever
+  // fix the renderer kill that started happening after fixing SchemeHostPort's
+  // handling of port=0..
+  for (int i = 0; i < 2; i++) {
+    SCOPED_TRACE(::testing::Message() << "Navigation #" << i);
+
+    // TODO(lukasza): https://crbug.com/1065532: blink::KURL and
+    // blink::SecurityOrigin treat port=0 as an invalid port, which leads to
+    // committing an origin with port=80 rather than port=0 which leads to a
+    // CANNOT_COMMIT_ORIGIN renderer kill.  This is bad, but not as bad as a
+    // browser crash from the bug, so let's keep things this way going forward.
+    ASSERT_EQ(2u, shell()->web_contents()->GetAllFrames().size());
+    RenderProcessHostBadIpcMessageWaiter bad_ipc_waiter(
+        shell()->web_contents()->GetAllFrames()[1]->GetProcess());
+
+    TestNavigationObserver subframe_ref_observer(shell()->web_contents(), 1);
+    ASSERT_TRUE(
+        ExecJs(shell(), JsReplace("document.querySelector('iframe').src = $1;",
+                                  subframe_ref_url)));
+
+#if 1
+    // TODO(lukasza): https://crbug.com/1065532: blink::KURL and
+    // blink::SecurityOrigin treat port=0 as an invalid port [...]
+    // (see the same comment above).
+    EXPECT_EQ(bad_message::RFH_INVALID_ORIGIN_ON_COMMIT, bad_ipc_waiter.Wait());
+    if (!AreAllSitesIsolatedForTesting()) {
+      // Without site-per-process the main frame and the subframe are hosted in
+      // the same (killed) renderer process, which makes it difficult to
+      // continue the test after the first kill.
+      return;
+    }
+#else
+    subframe_ref_url.WaitForNavigationFinished();
+#endif
+  }
+}
+
 }  // namespace content

url/gurl_unittest.cc

@@ -5,9 +5,11 @@
 #include <stddef.h>
 
 #include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
+#include "url/origin.h"
 #include "url/url_canon.h"
 #include "url/url_test_utils.h"
 
@@ -223,29 +225,32 @@ TEST(GURLTest, CopyFileSystem) {
 
 TEST(GURLTest, IsValid) {
   const char* valid_cases[] = {
-    "http://google.com",
-    "unknown://google.com",
-    "http://user:pass@google.com",
-    "http://google.com:12345",
-    "http://google.com/path",
-    "http://google.com//path",
-    "http://google.com?k=v#fragment",
-    "http://user:pass@google.com:12345/path?k=v#fragment",
-    "http:/path",
-    "http:path",
+      "http://google.com",
+      "unknown://google.com",
+      "http://user:pass@google.com",
+      "http://google.com:12345",
+      "http://google.com:0",  // 0 is a valid port
+      "http://google.com/path",
+      "http://google.com//path",
+      "http://google.com?k=v#fragment",
+      "http://user:pass@google.com:12345/path?k=v#fragment",
+      "http:/path",
+      "http:path",
   };
   for (size_t i = 0; i < base::size(valid_cases); i++) {
     EXPECT_TRUE(GURL(valid_cases[i]).is_valid())
         << "Case: " << valid_cases[i];
   }
 
   const char* invalid_cases[] = {
-    "http://?k=v",
-    "http:://google.com",
-    "http//google.com",
-    "http://google.com:12three45",
-    "://google.com",
-    "path",
+      "http://?k=v",
+      "http:://google.com",
+      "http//google.com",
+      "http://google.com:12three45",
+      "file://server:123",  // file: URLs cannot have a port
+      "file://server:0",
+      "://google.com",
+      "path",
   };
   for (size_t i = 0; i < base::size(invalid_cases); i++) {
     EXPECT_FALSE(GURL(invalid_cases[i]).is_valid())
@@ -979,4 +984,43 @@ TEST(GURLTest, DebugAlias) {
   EXPECT_STREQ("https://foo.com/bar", url_debug_alias);
 }
 
+TEST(GURLTest, PortZero) {
+  GURL port_zero_url("http://127.0.0.1:0/blah");
+
+  // https://url.spec.whatwg.org/#port-state says that the port 1) consists of
+  // ASCII digits (this excludes negative numbers) and 2) cannot be greater than
+  // 2^16-1.  This means that port=0 should be valid.
+  EXPECT_TRUE(port_zero_url.is_valid());
+  EXPECT_EQ("0", port_zero_url.port());
+  EXPECT_EQ("127.0.0.1", port_zero_url.host());
+  EXPECT_EQ("http", port_zero_url.scheme());
+
+  // https://crbug.com/1065532: SchemeHostPort would previously incorrectly
+  // consider port=0 to be invalid.
+  SchemeHostPort scheme_host_port(port_zero_url);
+  EXPECT_TRUE(scheme_host_port.IsValid());
+  EXPECT_EQ(port_zero_url.scheme(), scheme_host_port.scheme());
+  EXPECT_EQ(port_zero_url.host(), scheme_host_port.host());
+  EXPECT_EQ(port_zero_url.port(),
+            base::NumberToString(scheme_host_port.port()));
+
+  // https://crbug.com/1065532: The SchemeHostPort problem above would lead to
+  // bizarre results below - resolved origin would incorrectly be returned as an
+  // opaque origin derived from |another_origin|.
+  url::Origin another_origin = url::Origin::Create(GURL("http://other.com"));
+  url::Origin resolved_origin =
+      url::Origin::Resolve(port_zero_url, another_origin);
+  EXPECT_FALSE(resolved_origin.opaque());
+  EXPECT_EQ(port_zero_url.scheme(), resolved_origin.scheme());
+  EXPECT_EQ(port_zero_url.host(), resolved_origin.host());
+  EXPECT_EQ(port_zero_url.port(), base::NumberToString(resolved_origin.port()));
+
+  // port=0 and default HTTP port are different.
+  GURL default_port("http://127.0.0.1/foo");
+  EXPECT_EQ(0, SchemeHostPort(port_zero_url).port());
+  EXPECT_EQ(80, SchemeHostPort(default_port).port());
+  url::Origin default_port_origin = url::Origin::Create(default_port);
+  EXPECT_FALSE(default_port_origin.IsSameOriginWith(resolved_origin));
+}
+
 }  // namespace url

url/origin_unittest.cc

@@ -483,7 +483,8 @@ TEST_F(OriginTest, UnsafelyCreate) {
       {"http", "example.com", 123},
       {"https", "example.com", 443},
       {"https", "example.com", 123},
-      {"file", "", 0},
+      {"http", "example.com", 0},  // 0 is a valid port for http.
+      {"file", "", 0},             // 0 indicates "no port" for file: scheme.
       {"file", "example.com", 0},
   };
 
@@ -539,12 +540,10 @@ TEST_F(OriginTest, UnsafelyCreateUniqueOnInvalidInput) {
                {"http", "example.com\rnot-example.com"},
                {"http", "example.com\n"},
                {"http", "example.com\r"},
-               {"http", "example.com", 0},
                {"unknown-scheme", "example.com"},
                {"host-only", "\r", 0},
                {"host-only", "example.com", 22},
-               {"host-port-only", "example.com", 0},
-               {"file", ""}};
+               {"file", "", 123}};  // file: shouldn't have a port.
 
   for (const auto& test : cases) {
     SCOPED_TRACE(testing::Message()

url/scheme_host_port.cc

@@ -79,10 +79,12 @@ bool IsValidInput(const base::StringPiece& scheme,
   switch (scheme_type) {
     case SCHEME_WITH_HOST_AND_PORT:
     case SCHEME_WITH_HOST_PORT_AND_USER_INFORMATION:
-      // A URL with |scheme| is required to have the host and port (may be
-      // omitted in a serialization if it's the same as the default value).
-      // Return an invalid instance if either of them is not given.
-      if (host.empty() || port == 0)
+      // A URL with |scheme| is required to have the host and port, so return an
+      // invalid instance if host is not given.  Note that a valid port is
+      // always provided by SchemeHostPort(const GURL&) constructor (a missing
+      // port is replaced with a default port if needed by
+      // GURL::EffectiveIntPort()).
+      if (host.empty())
         return false;
 
       // Don't do an expensive canonicalization if the host is already

url/scheme_host_port.h

@@ -80,8 +80,8 @@ class COMPONENT_EXPORT(URL) SchemeHostPort {
 
   // Creates a (scheme, host, port) tuple. |host| must be a canonicalized
   // A-label (that is, '☃.net' must be provided as 'xn--n3h.net'). |scheme|
-  // must be a standard scheme. |port| must not be 0, unless |scheme| does not
-  // support ports (e.g. 'file'). In that case, |port| must be 0.
+  // must be a standard scheme. |port| must be 0 if |scheme| does not support
+  // ports (e.g. 'file').
   //
   // Copies the data in |scheme| and |host|.
   SchemeHostPort(base::StringPiece scheme,

url/scheme_host_port_unittest.cc

@@ -91,9 +91,10 @@ TEST_F(SchemeHostPortTest, ExplicitConstruction) {
   } cases[] = {
       {"http", "example.com", 80},
       {"http", "example.com", 123},
+      {"http", "example.com", 0},  // 0 is a valid port for http.
       {"https", "example.com", 443},
       {"https", "example.com", 123},
-      {"file", "", 0},
+      {"file", "", 0},  // 0 indicates "no port" for file: scheme.
       {"file", "example.com", 0},
   };
 
@@ -130,8 +131,7 @@ TEST_F(SchemeHostPortTest, InvalidConstruction) {
                {"http", "example.com\rnot-example.com", 80},
                {"http", "example.com\n", 80},
                {"http", "example.com\r", 80},
-               {"http", "example.com", 0},
-               {"file", "", 80}};
+               {"file", "", 80}};  // Can''t have a port for file: scheme.
 
   for (const auto& test : cases) {
     SCOPED_TRACE(testing::Message() << test.scheme << "://" << test.host << ":"