[Cronet] Fix false positive CHECK() failure with socket tagging and a proxy

Cronet was hitting CHECK() crashes when proxying HTTPS traffic through
HTTP proxies when a socket tag was applied.  There was no reason to
crash in these cases, so I'm fixing the CHECK() condition.

Bug: 920592
Change-Id: I0c5abd996d99e495c84aba3843286b96ddba624e
Reviewed-on: https://chromium-review.googlesource.com/c/1407430
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Paul Jensen <pauljensen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#622548}(cherry picked from commit e14a00d50d1ebedb74b842d1f3af22cc6feeb417)
Reviewed-on: https://chromium-review.googlesource.com/c/1489092
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#880}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
diff --git a/net/http/http_proxy_client_socket_pool_unittest.cc b/net/http/http_proxy_client_socket_pool_unittest.cc
index edc4273..b096ab5 100644
--- a/net/http/http_proxy_client_socket_pool_unittest.cc
+++ b/net/http/http_proxy_client_socket_pool_unittest.cc
@@ -378,9 +378,6 @@
   // so we skip this test for SPDY
   if (GetParam() == SPDY)
     return;
-  std::string proxy_host_port = GetParam() == HTTP
-                                    ? (kHttpProxyHost + std::string(":80"))
-                                    : (kHttpsProxyHost + std::string(":443"));
   std::string request =
       "CONNECT www.google.com:443 HTTP/1.1\r\n"
       "Host: www.google.com:443\r\n"
@@ -406,9 +403,6 @@
 }
 
 TEST_P(HttpProxyClientSocketPoolTest, AsyncHaveAuth) {
-  std::string proxy_host_port = GetParam() == HTTP
-                                    ? (kHttpProxyHost + std::string(":80"))
-                                    : (kHttpsProxyHost + std::string(":443"));
   std::string request =
       "CONNECT www.google.com:443 HTTP/1.1\r\n"
       "Host: www.google.com:443\r\n"
@@ -918,6 +912,10 @@
 // returned underlying TCP sockets.
 #if defined(OS_ANDROID)
 TEST_P(HttpProxyClientSocketPoolTest, Tag) {
+  // Socket tagging only supports Android without data reduction proxy, so only
+  // HTTP proxies are supported.
+  if (GetParam() != HTTP)
+    return;
   Initialize(base::span<MockRead>(), base::span<MockWrite>(),
              base::span<MockRead>(), base::span<MockWrite>());
   SocketTag tag1(SocketTag::UNSET_UID, 0x12345678);
@@ -950,6 +948,57 @@
   handle_.socket()->Disconnect();
   handle_.Reset();
 }
+
+TEST_P(HttpProxyClientSocketPoolTest, TagWithProxy) {
+  // Socket tagging only supports Android without data reduction proxy, so only
+  // HTTP proxies are supported.
+  if (GetParam() != HTTP)
+    return;
+  std::string request =
+      "CONNECT www.google.com:443 HTTP/1.1\r\n"
+      "Host: www.google.com:443\r\n"
+      "Proxy-Connection: keep-alive\r\n"
+      "Proxy-Authorization: Basic Zm9vOmJhcg==\r\n\r\n";
+  MockWrite writes[] = {
+      MockWrite(SYNCHRONOUS, 0, request.c_str()),
+  };
+  MockRead reads[] = {
+      MockRead(SYNCHRONOUS, 1, "HTTP/1.1 200 Connection Established\r\n\r\n"),
+  };
+
+  Initialize(reads, writes, base::span<MockRead>(), base::span<MockWrite>());
+  AddAuthToCache();
+
+  SocketTag tag1(SocketTag::UNSET_UID, 0x12345678);
+  SocketTag tag2(getuid(), 0x87654321);
+
+  // Verify requested socket is tagged properly.
+  int rv =
+      handle_.Init("a", CreateTunnelParams(), LOW, tag1,
+                   ClientSocketPool::RespectLimits::ENABLED,
+                   CompletionOnceCallback(), pool_.get(), NetLogWithSource());
+  EXPECT_THAT(rv, IsOk());
+  EXPECT_TRUE(handle_.is_initialized());
+  ASSERT_TRUE(handle_.socket());
+  EXPECT_TRUE(handle_.socket()->IsConnected());
+  EXPECT_EQ(socket_factory()->GetLastProducedTCPSocket()->tag(), tag1);
+  EXPECT_TRUE(
+      socket_factory()->GetLastProducedTCPSocket()->tagged_before_connected());
+
+  // Verify reused socket is retagged properly.
+  StreamSocket* socket = handle_.socket();
+  handle_.Reset();
+  rv = handle_.Init("a", CreateNoTunnelParams(), LOW, tag2,
+                    ClientSocketPool::RespectLimits::ENABLED,
+                    CompletionOnceCallback(), pool_.get(), NetLogWithSource());
+  EXPECT_THAT(rv, IsOk());
+  EXPECT_TRUE(handle_.socket());
+  EXPECT_TRUE(handle_.socket()->IsConnected());
+  EXPECT_EQ(handle_.socket(), socket);
+  EXPECT_EQ(socket_factory()->GetLastProducedTCPSocket()->tag(), tag2);
+  handle_.socket()->Disconnect();
+  handle_.Reset();
+}
 #endif
 
 }  // namespace net
diff --git a/net/http/http_proxy_client_socket_wrapper.cc b/net/http/http_proxy_client_socket_wrapper.cc
index 74bea38..9e19442 100644
--- a/net/http/http_proxy_client_socket_wrapper.cc
+++ b/net/http/http_proxy_client_socket_wrapper.cc
@@ -307,12 +307,25 @@
 }
 
 void HttpProxyClientSocketWrapper::ApplySocketTag(const SocketTag& tag) {
-  // HttpProxyClientSocketPool only tags once connected, when transport_socket_
-  // is set. Socket tagging is not supported with tunneling. Socket tagging is
-  // also not supported with proxy auth so ApplySocketTag() won't be called with
-  // a specific (non-default) tag when transport_socket_ is cleared by
-  // RestartWithAuth().
-  if (tunnel_ || !transport_socket_) {
+  // Applying a socket tag to an HttpProxyClientSocketWrapper is done by simply
+  // applying the socket tag to the underlying socket.
+
+  // In the case of a connection to the proxy using HTTP/2 or HTTP/3 where the
+  // underlying socket may multiplex multiple streams, applying this request's
+  // socket tag to the multiplexed session would incorrectly apply the socket
+  // tag to all mutliplexed streams. In reality this would hit the CHECK(false)
+  // in QuicProxyClientSocket::ApplySocketTag() or
+  // SpdyProxyClientSocket::ApplySocketTag(). Fortunately socket tagging is only
+  // supported on Android without the data reduction proxy, so only simple HTTP
+  // proxies are supported, so proxies won't be using HTTP/2 or HTTP/3. Detect
+  // this case (|ssl_params_| must be set for HTTP/2 and HTTP/3 proxies) and
+  // enforce that a specific (non-default) tag isn't being applied.
+  if (ssl_params_ ||
+      // Android also doesn't support proxy auth, so RestartWithAuth() should't
+      // be called so |transport_socket_| shouldn't be cleared. If
+      // |transport_socket_| is cleared, enforce that a specific (non-default)
+      // tag isn't being applied.
+      !transport_socket_) {
     CHECK(tag == SocketTag());
   } else {
     transport_socket_->ApplySocketTag(tag);