Fix regression in the HTTP request line

Adds a test that demonstrates a regression from M in
the HTTP request line when a proxied connection
is made. It only affects URLs with empty paths (i.e. ones
without even a '/' after the host / authority). For
example, on M the request line for a request to
new URL("http://myhost").openConnection() would be:

GET http://myhost HTTP/1.1

but on N, without this change, it would be:

GET http://myhost/ HTTP/1.1

This change reverts to the M behavior.

Bug: 29983827
Test: CtsLibcoreOkHttpTestCases and CtsLibcoreTestCases
Change-Id: I117bca1371e5311b85e0cee0d3d2528191ade182
(cherry picked from commit cd57d9eb83acc7a13ee576bfa22d4c490316ac18)

4 files changed, 199 insertions, 18 deletions

diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java
index 71deb6c..d45ac10 100644
--- a/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java
+++ b/okhttp-tests/src/test/java/com/squareup/okhttp/HttpUrlTest.java
@@ -29,6 +29,7 @@ import org.junit.Test;
 
 import static java.util.Collections.singletonList;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.fail;
 
@@ -110,7 +111,11 @@ public final class HttpUrlTest {
 
   @Test public void resolveNoScheme() throws Exception {
     HttpUrl base = HttpUrl.parse("http://host/a/b");
-    assertEquals(HttpUrl.parse("http://host2/"), base.resolve("//host2"));
+    // ANDROID-BEGIN: http://b/29983827
+    // assertEquals(HttpUrl.parse("http://host2/"), base.resolve("//host2"));
+    assertEquals(HttpUrl.parse("http://host2"), base.resolve("//host2"));
+    // ANDROID-END: http://b/29983827
+    assertEquals(HttpUrl.parse("http://host2"), base.resolve("//host2"));
     assertEquals(HttpUrl.parse("http://host/path"), base.resolve("/path"));
     assertEquals(HttpUrl.parse("http://host/a/path"), base.resolve("path"));
     assertEquals(HttpUrl.parse("http://host/a/b?query"), base.resolve("?query"));
@@ -119,6 +124,98 @@ public final class HttpUrlTest {
     assertEquals(HttpUrl.parse("http://host/path"), base.resolve("\\path"));
   }
 
+  // ANDROID-BEGIN: http://b/29983827
+  @Test public void encodedPath_pathVariations() throws Exception {
+    assertEquals("", HttpUrl.parse("http://example.com").encodedPath());
+    assertEquals("", HttpUrl.parse("http://example.com#fragment").encodedPath());
+    assertEquals("", HttpUrl.parse("http://example.com?arg=value").encodedPath());
+    assertEquals("/", HttpUrl.parse("http://example.com/").encodedPath());
+    assertEquals("/", HttpUrl.parse("http://example.com/#fragment").encodedPath());
+    assertEquals("/", HttpUrl.parse("http://example.com/?arg=value").encodedPath());
+    assertEquals("/foo", HttpUrl.parse("http://example.com/foo").encodedPath());
+    assertEquals("/foo/", HttpUrl.parse("http://example.com/foo/").encodedPath());
+  }
+
+  @Test public void newBuilder_pathVariations() throws Exception {
+    assertNewBuilderRoundtrip("http://example.com");
+    assertNewBuilderRoundtrip("http://example.com/");
+    assertNewBuilderRoundtrip("http://example.com/foo");
+    assertNewBuilderRoundtrip("http://example.com/foo/");
+  }
+
+  private void assertNewBuilderRoundtrip(String urlString) {
+    HttpUrl url = HttpUrl.parse(urlString);
+    assertEquals(url, url.newBuilder().build());
+  }
+
+  @Test public void equals_emptyPathNotSameAsSlash() throws Exception {
+    assertFalse(HttpUrl.parse("http://example.com").equals(HttpUrl.parse("http://example.com/")));
+  }
+
+  @Test public void parse_pathVariations() throws Exception {
+    parseThenAssertToStringEquals("http://example.com");
+    parseThenAssertToStringEquals("https://example.com");
+    parseThenAssertToStringEquals("http://example.com?value=42");
+    parseThenAssertToStringEquals("http://example.com:3434");
+    parseThenAssertToStringEquals("http://example.com#hello");
+
+    parseThenAssertToStringEquals("http://example.com/");
+    parseThenAssertToStringEquals("https://example.com/");
+    parseThenAssertToStringEquals("http://example.com/foo/bar");
+    parseThenAssertToStringEquals("http://example.com/foo/bar/");
+    parseThenAssertToStringEquals("http://example.com/foo/bar?value=100");
+    parseThenAssertToStringEquals("http://example.com/?value=200");
+
+    {
+      HttpUrl httpUrl = HttpUrl.parse("http://example.com/foo/..");
+      assertEquals("http://example.com/", httpUrl.toString());
+    }
+
+    {
+      HttpUrl httpUrl = HttpUrl.parse("http://example.com/..");
+      assertEquals("http://example.com/", httpUrl.toString());
+    }
+  }
+
+  private static void parseThenAssertToStringEquals(String url) {
+    HttpUrl httpUrl = HttpUrl.parse(url);
+    assertEquals(url, httpUrl.toString());
+  }
+
+  @Test public void resolve_pathVariations() throws Exception {
+    HttpUrl baseWithoutSlash = HttpUrl.parse("http://example.com");
+    assertResolveResult("http://example.com#section", baseWithoutSlash, "#section");
+    assertResolveResult("http://example.com?attitude=friendly", baseWithoutSlash,
+            "?attitude=friendly");
+    assertResolveResult("http://example.com", baseWithoutSlash, "");
+    assertResolveResult("http://example.com/", baseWithoutSlash, "/");
+    assertResolveResult("http://example.com/foo", baseWithoutSlash, "/foo");
+    assertResolveResult("http://example.com/foo", baseWithoutSlash, "foo");
+
+    assertResolveResult("http://other.com", baseWithoutSlash, "http://other.com");
+    assertResolveResult("http://other.com/", baseWithoutSlash, "http://other.com/");
+    assertResolveResult("http://other.com/foo", baseWithoutSlash, "http://other.com/foo");
+
+    HttpUrl baseWithSlash = HttpUrl.parse("http://example.com/");
+    assertResolveResult("http://example.com/#section", baseWithSlash, "#section");
+    assertResolveResult("http://example.com/?attitude=friendly", baseWithSlash,
+            "?attitude=friendly");
+    assertResolveResult("http://example.com/", baseWithSlash, "");
+    assertResolveResult("http://example.com/", baseWithSlash, "/");
+    assertResolveResult("http://example.com/foo", baseWithSlash, "/foo");
+    assertResolveResult("http://example.com/foo", baseWithSlash, "foo");
+
+    assertResolveResult("http://other.com", baseWithSlash, "http://other.com");
+    assertResolveResult("http://other.com/", baseWithSlash, "http://other.com/");
+    assertResolveResult("http://other.com/foo", baseWithSlash, "http://other.com/foo");
+  }
+
+  private void assertResolveResult(String expected, HttpUrl base, String link) {
+    assertEquals(HttpUrl.parse(expected), base.resolve(link));
+    assertEquals(expected, base.resolve(link).toString());
+  }
+  // ANDROID-END: http://b/29983827
+
   @Test public void resolveUnsupportedScheme() throws Exception {
     HttpUrl base = HttpUrl.parse("http://a/");
     assertEquals(null, base.resolve("ftp://b"));
@@ -939,8 +1036,12 @@ public final class HttpUrlTest {
         .removePathSegment(0)
         .removePathSegment(0)
         .build();
-    assertEquals(Arrays.asList(""), url.pathSegments());
-    assertEquals("/", url.encodedPath());
+    // ANDROID-BEGIN: http://b/29983827 Behavior changed. Test name is now incorrect.
+    // assertEquals(Arrays.asList(""), url.pathSegments());
+    // assertEquals("/", url.encodedPath());
+    assertEquals(Collections.emptyList(), url.pathSegments());
+    assertEquals("http://host", url.toString());
+    // ANDROID-END: http://b/29983827
   }
 
   @Test public void removePathSegmentOutOfBounds() throws Exception {
diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java
index 3598da0..7c948b8 100644
--- a/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java
+++ b/okhttp-tests/src/test/java/com/squareup/okhttp/URLConnectionTest.java
@@ -680,19 +680,29 @@ public final class URLConnectionTest {
     testConnectViaProxy(ProxyConfig.HTTP_PROXY_SYSTEM_PROPERTY);
   }
 
+  // ANDROID-BEGIN: http://b/29983827
   private void testConnectViaProxy(ProxyConfig proxyConfig) throws Exception {
+    testConnectViaProxy(proxyConfig, "http://android.com/foo", "android.com");
+    testConnectViaProxy(proxyConfig, "http://android.com/", "android.com");
+    testConnectViaProxy(proxyConfig, "http://android.com", "android.com");
+    testConnectViaProxy(proxyConfig, "http://mms", "mms");
+  }
+
+  private void testConnectViaProxy(ProxyConfig proxyConfig, String urlString, String expectedHost)
+          throws Exception {
     MockResponse mockResponse = new MockResponse().setBody("this response comes via a proxy");
     server.enqueue(mockResponse);
 
-    URL url = new URL("http://android.com/foo");
+    URL url = new URL(urlString);
     connection = proxyConfig.connect(server, client, url);
     assertContent("this response comes via a proxy", connection);
     assertTrue(connection.usingProxy());
 
     RecordedRequest request = server.takeRequest();
-    assertEquals("GET http://android.com/foo HTTP/1.1", request.getRequestLine());
-    assertEquals("android.com", request.getHeader("Host"));
+    assertEquals("GET " + urlString + " HTTP/1.1", request.getRequestLine());
+    assertEquals(expectedHost, request.getHeader("Host"));
   }
+  // ANDROID-END: http://b/29983827
 
   @Test public void contentDisagreesWithContentLengthHeaderBodyTooLong() throws IOException {
     server.enqueue(new MockResponse().setBody("abc\r\nYOU SHOULD NOT SEE THIS")
diff --git a/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java b/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java
index beabeca..8e75e3f 100644
--- a/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java
+++ b/okhttp/src/main/java/com/squareup/okhttp/HttpUrl.java
@@ -442,11 +442,18 @@ public final class HttpUrl {
   }
 
   /**
-   * Returns the entire path of this URL, encoded for use in HTTP resource resolution. The
-   * returned path is always nonempty and is prefixed with {@code /}.
+   * Returns the entire path of this URL, encoded for use in HTTP resource resolution.
+   // ANDROID-BEGIN: http://b/29983827
+   //   * The returned path is always nonempty and is prefixed with {@code /}.
+   // ANDROID-END: http://b/29983827
    */
   public String encodedPath() {
     int pathStart = url.indexOf('/', scheme.length() + 3); // "://".length() == 3.
+    // ANDROID-BEGIN: http://b/29983827
+    if (pathStart == -1) {
+      return "";
+    }
+    // ANDROID-END: http://b/29983827
     int pathEnd = delimiterOffset(url, pathStart, url.length(), "?#");
     return url.substring(pathStart, pathEnd);
   }
@@ -460,6 +467,12 @@ public final class HttpUrl {
 
   public List<String> encodedPathSegments() {
     int pathStart = url.indexOf('/', scheme.length() + 3);
+    // ANDROID-BEGIN: http://b/29983827
+    if (pathStart == -1) {
+      return new ArrayList<>();
+    }
+    // ANDROID-END: http://b/29983827
+
     int pathEnd = delimiterOffset(url, pathStart, url.length(), "?#");
     List<String> result = new ArrayList<>();
     for (int i = pathStart; i < pathEnd; ) {
@@ -590,13 +603,19 @@ public final class HttpUrl {
 
   /** Returns the URL that would be retrieved by following {@code link} from this URL. */
   public HttpUrl resolve(String link) {
-    Builder builder = new Builder();
+    // ANDROID-BEGIN: http://b/29983827
+    // Builder builder = new Builder();
+    Builder builder = new Builder(false);
+    // ANDROID-END: http://b/29983827
     Builder.ParseResult result = builder.parse(this, link);
     return result == Builder.ParseResult.SUCCESS ? builder.build() : null;
   }
 
   public Builder newBuilder() {
-    Builder result = new Builder();
+    // ANDROID-BEGIN: http://b/29983827
+    // Builder builder = new Builder();
+    Builder result = new Builder(false);
+    // ANDROID-END: http://b/29983827
     result.scheme = scheme;
     result.encodedUsername = encodedUsername();
     result.encodedPassword = encodedPassword();
@@ -615,7 +634,10 @@ public final class HttpUrl {
    * URL, or null if it isn't.
    */
   public static HttpUrl parse(String url) {
-    Builder builder = new Builder();
+    // ANDROID-BEGIN: http://b/29983827
+    // Builder builder = new Builder();
+    Builder builder = new Builder(false);
+    // ANDROID-END: http://b/29983827
     Builder.ParseResult result = builder.parse(null, url);
     return result == Builder.ParseResult.SUCCESS ? builder.build() : null;
   }
@@ -636,7 +658,10 @@ public final class HttpUrl {
    * @throws UnknownHostException if the host was invalid
    */
   static HttpUrl getChecked(String url) throws MalformedURLException, UnknownHostException {
-    Builder builder = new Builder();
+    // ANDROID-END: http://b/29983827
+    // Builder builder = new Builder();
+    Builder builder = new Builder(false);
+    // ANDROID-END: http://b/29983827
     Builder.ParseResult result = builder.parse(null, url);
     switch (result) {
       case SUCCESS:
@@ -677,10 +702,22 @@ public final class HttpUrl {
     List<String> encodedQueryNamesAndValues;
     String encodedFragment;
 
+    // ANDROID-BEGIN: http://b/29983827
+    // public Builder() {
+    //   encodedPathSegments.add(""); // The default path is '/' which needs a trailing space.
+    // }
+
     public Builder() {
-      encodedPathSegments.add(""); // The default path is '/' which needs a trailing space.
+      this(true); // // The default path is '/' which needs a trailing space.
     }
 
+    private Builder(boolean startWithSlash) {
+      if (startWithSlash) {
+        encodedPathSegments.add("");
+      }
+    }
+    // ANDROID-END: http://b/29983827
+
     public Builder scheme(String scheme) {
       if (scheme == null) {
         throw new IllegalArgumentException("scheme == null");
@@ -782,9 +819,12 @@ public final class HttpUrl {
 
     public Builder removePathSegment(int index) {
       encodedPathSegments.remove(index);
-      if (encodedPathSegments.isEmpty()) {
-        encodedPathSegments.add(""); // Always leave at least one '/'.
-      }
+      // ANDROID-BEGIN: http://b/29983827. Note this method only used from tests.
+      // Only changed for consistency.
+      //      if (encodedPathSegments.isEmpty()) {
+      //        encodedPathSegments.add(""); // Always leave at least one '/'.
+      //      }
+      // ANDROID-END: http://b/29983827 - only used from tests
       return this;
     }
 
@@ -1112,8 +1152,14 @@ public final class HttpUrl {
         encodedPathSegments.add("");
         pos++;
       } else {
-        // Relative path: clear everything after the last '/'.
-        encodedPathSegments.set(encodedPathSegments.size() - 1, "");
+        // ANDROID-BEGIN: http://b/29983827
+        // // Relative path: clear everything after the last '/'.
+        // encodedPathSegments.set(encodedPathSegments.size() - 1, "");
+        // Relative path: clear everything after the last '/' (if there is one).
+        if (!encodedPathSegments.isEmpty()) {
+          encodedPathSegments.set(encodedPathSegments.size() - 1, "");
+        }
+        // ANDROID-END: http://b/29983827
       }
 
       // Read path segments.
@@ -1138,6 +1184,15 @@ public final class HttpUrl {
         pop();
         return;
       }
+
+      // ANDROID-BEGIN: http://b/29983827
+      // If the encodedPathSegments doesn't even include "/" then add the leading "/" before
+      // pushing more segments or modifying existing segments.
+      if (encodedPathSegments.isEmpty()) {
+        encodedPathSegments.add("");
+      }
+      // ANDROID-END: http://b/29983827
+
       if (encodedPathSegments.get(encodedPathSegments.size() - 1).isEmpty()) {
         encodedPathSegments.set(encodedPathSegments.size() - 1, segment);
       } else {
@@ -1170,6 +1225,14 @@ public final class HttpUrl {
      * to ["a", "b", ""].
      */
     private void pop() {
+      // ANDROID-BEGIN: http://b/29983827
+      // Cannot pop() if there isn't even a "/". Leave the path as is. This method is only used
+      // from push(). push() handles the empty case explicitly.
+      if (encodedPathSegments.isEmpty()) {
+        return;
+      }
+      // ANDROID-END: http://b/29983827
+
       String removed = encodedPathSegments.remove(encodedPathSegments.size() - 1);
 
       // Make sure the path ends with a '/' by either adding an empty string or clearing a segment.
diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/RequestLine.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/RequestLine.java
index d22be27..89a3922 100644
--- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/RequestLine.java
+++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/RequestLine.java
@@ -46,6 +46,13 @@ public final class RequestLine {
    */
   public static String requestPath(HttpUrl url) {
     String path = url.encodedPath();
+    // ANDROID-BEGIN: http://b/29983827 - Now path can be empty, which is forbidden in relative
+    // paths so we must handle it here.
+    if (path.isEmpty()) {
+      path = "/";
+    }
+    // ANDROID-END: http://b/29983827
+
     String query = url.encodedQuery();
     return query != null ? (path + '?' + query) : path;
   }