Reland "Add reprioritization to SSLConnectJobs"

This reverts commit dc8c60c3.

Reason for revert: Bug causing crashes was fixed in https://crrev.com/c/1333135

Original change's description:
> Revert "Add reprioritization to SSLConnectJobs"
> 
> This reverts commit 8a9302e2.
> 
> Reason for revert: Causing flaky crashes in external/wpt/webusb/idlharness.https.any.worker.html
> 
> Original change's description:
> > Add reprioritization to SSLConnectJobs
> >
> > This change allows the priority of an SSLConnectJob to be changed. If
> > the reprioritization occurs while the job is establishing its underlying
> > connection, the priority change is also passed down to the lower-level
> > socket pool.
> >
> > Bug: 166689
> > Change-Id: I87d1e536846443901cc628423f4615c3ffa7542a
> > Reviewed-on: https://chromium-review.googlesource.com/c/1327423
> > Commit-Queue: Lily Chen <chlily@chromium.org>
> > Reviewed-by: Matt Menke <mmenke@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#606876}
> 
> TBR=mmenke@chromium.org,chlily@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: 166689, 904345
> Change-Id: I6458798e11d210258f2a26330548f1ca33eb6c90
> Reviewed-on: https://chromium-review.googlesource.com/c/1331139
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Commit-Queue: Lily Chen <chlily@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607301}

TBR=mmenke@chromium.org,chlily@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 166689, 904345
Change-Id: I54fe803331be6fe3df534c0f6042bf0fb0057cbb
Reviewed-on: https://chromium-review.googlesource.com/c/1340824Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608950}

net/socket/socket_test_util.cc

@@ -2017,11 +2017,13 @@ MockTransportClientSocketPool::MockConnectJob::MockConnectJob(
     std::unique_ptr<StreamSocket> socket,
     ClientSocketHandle* handle,
     const SocketTag& socket_tag,
-    CompletionOnceCallback callback)
+    CompletionOnceCallback callback,
+    RequestPriority priority)
     : socket_(std::move(socket)),
       handle_(handle),
       socket_tag_(socket_tag),
-      user_callback_(std::move(callback)) {}
+      user_callback_(std::move(callback)),
+      priority_(priority) {}
 
 MockTransportClientSocketPool::MockConnectJob::~MockConnectJob() = default;
 
@@ -2107,8 +2109,8 @@ int MockTransportClientSocketPool::RequestSocket(
   std::unique_ptr<StreamSocket> socket =
       client_socket_factory_->CreateTransportClientSocket(
           AddressList(), NULL, net_log.net_log(), NetLogSource());
-  MockConnectJob* job = new MockConnectJob(std::move(socket), handle,
-                                           socket_tag, std::move(callback));
+  MockConnectJob* job = new MockConnectJob(
+      std::move(socket), handle, socket_tag, std::move(callback), priority);
   job_list_.push_back(base::WrapUnique(job));
   handle->set_pool_id(1);
   return job->Connect();
@@ -2117,7 +2119,13 @@ int MockTransportClientSocketPool::RequestSocket(
 void MockTransportClientSocketPool::SetPriority(const std::string& group_name,
                                                 ClientSocketHandle* handle,
                                                 RequestPriority priority) {
-  // TODO: Implement.
+  for (auto& job : job_list_) {
+    if (job->handle() == handle) {
+      job->set_priority(priority);
+      return;
+    }
+  }
+  NOTREACHED();
 }
 
 void MockTransportClientSocketPool::CancelRequest(const std::string& group_name,

net/socket/socket_test_util.h

@@ -1177,12 +1177,18 @@ class MockTransportClientSocketPool : public TransportClientSocketPool {
     MockConnectJob(std::unique_ptr<StreamSocket> socket,
                    ClientSocketHandle* handle,
                    const SocketTag& socket_tag,
-                   CompletionOnceCallback callback);
+                   CompletionOnceCallback callback,
+                   RequestPriority priority);
     ~MockConnectJob();
 
     int Connect();
     bool CancelHandle(const ClientSocketHandle* handle);
 
+    ClientSocketHandle* handle() const { return handle_; }
+
+    RequestPriority priority() const { return priority_; }
+    void set_priority(RequestPriority priority) { priority_ = priority; }
+
    private:
     void OnConnect(int rv);
 
@@ -1190,6 +1196,7 @@ class MockTransportClientSocketPool : public TransportClientSocketPool {
     ClientSocketHandle* handle_;
     const SocketTag socket_tag_;
     CompletionOnceCallback user_callback_;
+    RequestPriority priority_;
 
     DISALLOW_COPY_AND_ASSIGN(MockConnectJob);
   };
@@ -1203,6 +1210,11 @@ class MockTransportClientSocketPool : public TransportClientSocketPool {
   RequestPriority last_request_priority() const {
     return last_request_priority_;
   }
+
+  const std::vector<std::unique_ptr<MockConnectJob>>& requests() const {
+    return job_list_;
+  }
+
   int release_count() const { return release_count_; }
   int cancel_count() const { return cancel_count_; }
 

net/socket/ssl_client_socket_pool.cc

@@ -429,6 +429,11 @@ int SSLConnectJob::ConnectInternal() {
   return DoLoop(OK);
 }
 
+void SSLConnectJob::ChangePriorityInternal(RequestPriority priority) {
+  if (transport_socket_handle_)
+    transport_socket_handle_->SetPriority(priority);
+}
+
 SSLClientSocketPool::SSLConnectJobFactory::SSLConnectJobFactory(
     TransportClientSocketPool* transport_pool,
     SOCKSClientSocketPool* socks_pool,

net/socket/ssl_client_socket_pool.h

@@ -151,6 +151,8 @@ class SSLConnectJob : public ConnectJob {
   // Otherwise, it returns a net error code.
   int ConnectInternal() override;
 
+  void ChangePriorityInternal(RequestPriority priority) override;
+
   scoped_refptr<SSLSocketParams> params_;
   TransportClientSocketPool* const transport_pool_;
   SOCKSClientSocketPool* const socks_pool_;

net/socket/ssl_client_socket_pool_unittest.cc

@@ -285,6 +285,7 @@ TEST_F(SSLClientSocketPoolTest, BasicDirect) {
   EXPECT_THAT(rv, IsOk());
   EXPECT_TRUE(handle.is_initialized());
   EXPECT_TRUE(handle.socket());
+  EXPECT_EQ(MEDIUM, transport_socket_pool_.requests()[0]->priority());
   TestLoadTimingInfo(handle);
   EXPECT_EQ(0u, handle.connection_attempts().size());
 }
@@ -310,10 +311,40 @@ TEST_F(SSLClientSocketPoolTest, SetSocketRequestPriorityOnInitDirect) {
                         ClientSocketPool::RespectLimits::ENABLED,
                         callback.callback(), pool_.get(), NetLogWithSource()));
     EXPECT_EQ(priority, transport_socket_pool_.last_request_priority());
+    EXPECT_EQ(priority, transport_socket_pool_.requests()[i]->priority());
     handle.socket()->Disconnect();
   }
 }
 
+// Test that the SSLConnectJob passes priority changes down to the transport
+// socket pool (for the DIRECT case).
+TEST_F(SSLClientSocketPoolTest, SetSocketRequestPriorityDirect) {
+  StaticSocketDataProvider data;
+  socket_factory_.AddSocketDataProvider(&data);
+  SSLSocketDataProvider ssl(ASYNC, OK);
+  socket_factory_.AddSSLSocketDataProvider(&ssl);
+
+  CreatePool(true /* tcp pool */, false, false);
+  scoped_refptr<SSLSocketParams> params = SSLParams(ProxyServer::SCHEME_DIRECT);
+
+  ClientSocketHandle handle;
+  TestCompletionCallback callback;
+  int rv = handle.Init(kGroupName, params, MEDIUM, SocketTag(),
+                       ClientSocketPool::RespectLimits::ENABLED,
+                       callback.callback(), pool_.get(), NetLogWithSource());
+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+  EXPECT_FALSE(handle.is_initialized());
+  EXPECT_FALSE(handle.socket());
+  EXPECT_EQ(MEDIUM, transport_socket_pool_.requests()[0]->priority());
+
+  pool_->SetPriority(kGroupName, &handle, LOWEST);
+  EXPECT_EQ(LOWEST, transport_socket_pool_.requests()[0]->priority());
+
+  EXPECT_THAT(callback.WaitForResult(), IsOk());
+  EXPECT_TRUE(handle.is_initialized());
+  EXPECT_TRUE(handle.socket());
+}
+
 TEST_F(SSLClientSocketPoolTest, BasicDirectAsync) {
   StaticSocketDataProvider data;
   socket_factory_.AddSocketDataProvider(&data);
@@ -524,6 +555,36 @@ TEST_F(SSLClientSocketPoolTest, SetTransportPriorityOnInitSOCKS) {
                         ClientSocketPool::RespectLimits::ENABLED,
                         callback.callback(), pool_.get(), NetLogWithSource()));
   EXPECT_EQ(HIGHEST, transport_socket_pool_.last_request_priority());
+  EXPECT_EQ(HIGHEST, transport_socket_pool_.requests()[0]->priority());
+}
+
+// Test that the SSLConnectJob passes priority changes down to the transport
+// socket pool (for the SOCKS_PROXY case).
+TEST_F(SSLClientSocketPoolTest, SetTransportPrioritySOCKS) {
+  StaticSocketDataProvider data;
+  socket_factory_.AddSocketDataProvider(&data);
+  SSLSocketDataProvider ssl(ASYNC, OK);
+  socket_factory_.AddSSLSocketDataProvider(&ssl);
+
+  CreatePool(false, true /* http proxy pool */, true /* socks pool */);
+  scoped_refptr<SSLSocketParams> params = SSLParams(ProxyServer::SCHEME_SOCKS5);
+
+  ClientSocketHandle handle;
+  TestCompletionCallback callback;
+  int rv = handle.Init(kGroupName, params, MEDIUM, SocketTag(),
+                       ClientSocketPool::RespectLimits::ENABLED,
+                       callback.callback(), pool_.get(), NetLogWithSource());
+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+  EXPECT_FALSE(handle.is_initialized());
+  EXPECT_FALSE(handle.socket());
+  EXPECT_EQ(MEDIUM, transport_socket_pool_.requests()[0]->priority());
+
+  pool_->SetPriority(kGroupName, &handle, LOWEST);
+  EXPECT_EQ(LOWEST, transport_socket_pool_.requests()[0]->priority());
+
+  EXPECT_THAT(callback.WaitForResult(), IsOk());
+  EXPECT_TRUE(handle.is_initialized());
+  EXPECT_TRUE(handle.socket());
 }
 
 TEST_F(SSLClientSocketPoolTest, SOCKSBasicAsync) {
@@ -656,8 +717,13 @@ TEST_F(SSLClientSocketPoolTest, SetTransportPriorityOnInitHTTP) {
                         ClientSocketPool::RespectLimits::ENABLED,
                         callback.callback(), pool_.get(), NetLogWithSource()));
   EXPECT_EQ(HIGHEST, transport_socket_pool_.last_request_priority());
+  EXPECT_EQ(HIGHEST, transport_socket_pool_.requests()[0]->priority());
 }
 
+// TODO(chlily): Test that the SSLConnectJob passes priority changes down to the
+// transport socket pool (for the HTTP_PROXY case), once change priority is
+// implemented for HttpProxyClientSocketPool.
+
 TEST_F(SSLClientSocketPoolTest, HttpProxyBasicAsync) {
   MockWrite writes[] = {
       MockWrite(