Fix a bug where an alternate protocol request to an unsafe port

would hang indefinitely, and a logging bug from the same original
CL, add unit test.

Buggy commit:  http://codereview.chromium.org/8898008/

R=wtc@chromium.org
BUG=93326

Review URL: http://codereview.chromium.org/9240012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@118210 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 94 insertions, 26 deletions

diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index cca2f8e..f0dad75 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -6817,6 +6817,60 @@ TEST_F(HttpNetworkTransactionTest, AlternateProtocolPortUnrestrictedAllowed2) {
   HttpStreamFactory::set_use_alternate_protocols(false);
 }
 
+TEST_F(HttpNetworkTransactionTest, AlternateProtocolUnsafeBlocked) {
+  // Ensure that we're not allowed to redirect traffic via an alternate
+  // protocol to an unsafe port, and that we resume the second
+  // HttpStreamFactoryImpl::Job once the alternate protocol request fails.
+  HttpStreamFactory::set_use_alternate_protocols(true);
+  SessionDependencies session_deps;
+
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("http://www.google.com/");
+  request.load_flags = 0;
+
+  // The alternate protocol request will error out before we attempt to connect,
+  // so only the standard HTTP request will try to connect.
+  MockRead data_reads[] = {
+    MockRead("HTTP/1.1 200 OK\r\n\r\n"),
+    MockRead("hello world"),
+    MockRead(true, OK),
+  };
+  StaticSocketDataProvider data(
+      data_reads, arraysize(data_reads), NULL, 0);
+  session_deps.socket_factory.AddSocketDataProvider(&data);
+
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
+
+  HttpServerProperties* http_server_properties =
+      session->http_server_properties();
+  const int kUnsafePort = 7;
+  http_server_properties->SetAlternateProtocol(
+      HostPortPair::FromURL(request.url),
+      kUnsafePort,
+      NPN_SPDY_2);
+
+  scoped_ptr<HttpTransaction> trans(new HttpNetworkTransaction(session));
+  TestCompletionCallback callback;
+
+  int rv = trans->Start(&request, callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  // The HTTP request should succeed.
+  EXPECT_EQ(OK, callback.WaitForResult());
+
+  // Disable alternate protocol before the asserts.
+  HttpStreamFactory::set_use_alternate_protocols(false);
+
+  const HttpResponseInfo* response = trans->GetResponseInfo();
+  ASSERT_TRUE(response != NULL);
+  ASSERT_TRUE(response->headers != NULL);
+  EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine());
+
+  std::string response_data;
+  ASSERT_EQ(OK, ReadTransaction(trans.get(), &response_data));
+  EXPECT_EQ("hello world", response_data);
+}
+
 TEST_F(HttpNetworkTransactionTest, UseAlternateProtocolForNpnSpdy) {
   HttpStreamFactory::set_use_alternate_protocols(true);
   HttpStreamFactory::set_next_protos(SpdyNextProtos());
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index f401db6..9c5820c 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -123,7 +123,7 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
       next_state_(STATE_NONE),
       pac_request_(NULL),
       blocking_job_(NULL),
-      dependent_job_(NULL),
+      waiting_job_(NULL),
       using_ssl_(false),
       using_spdy_(false),
       force_spdy_always_(HttpStreamFactory::force_spdy_always()),
@@ -211,9 +211,9 @@ void HttpStreamFactoryImpl::Job::WaitFor(Job* job) {
   DCHECK_EQ(STATE_NONE, next_state_);
   DCHECK_EQ(STATE_NONE, job->next_state_);
   DCHECK(!blocking_job_);
-  DCHECK(!job->dependent_job_);
+  DCHECK(!job->waiting_job_);
   blocking_job_ = job;
-  job->dependent_job_ = this;
+  job->waiting_job_ = this;
 }
 
 void HttpStreamFactoryImpl::Job::Resume(Job* job) {
@@ -236,8 +236,8 @@ void HttpStreamFactoryImpl::Job::Orphan(const Request* request) {
   // We've been orphaned, but there's a job we're blocked on. Don't bother
   // racing, just cancel ourself.
   if (blocking_job_) {
-    DCHECK(blocking_job_->dependent_job_);
-    blocking_job_->dependent_job_ = NULL;
+    DCHECK(blocking_job_->waiting_job_);
+    blocking_job_->waiting_job_ = NULL;
     blocking_job_ = NULL;
     stream_factory_->OnOrphanedJobComplete(this);
   }
@@ -386,6 +386,10 @@ int HttpStreamFactoryImpl::Job::RunLoop(int result) {
   if (result == ERR_IO_PENDING)
     return result;
 
+  // If there was an error, we should have already resumed the |waiting_job_|,
+  // if there was one.
+  DCHECK(result == OK || waiting_job_ == NULL);
+
   if (IsPreconnecting()) {
     MessageLoop::current()->PostTask(
         FROM_HERE,
@@ -549,24 +553,30 @@ int HttpStreamFactoryImpl::Job::DoLoop(int result) {
 int HttpStreamFactoryImpl::Job::StartInternal() {
   CHECK_EQ(STATE_NONE, next_state_);
   next_state_ = STATE_START;
-  net_log_.BeginEvent(NetLog::TYPE_HTTP_STREAM_JOB,
-                      HttpStreamJobParameters::Create(request_info_.url,
-                                                      origin_url_));
   int rv = RunLoop(OK);
   DCHECK_EQ(ERR_IO_PENDING, rv);
   return rv;
 }
 
 int HttpStreamFactoryImpl::Job::DoStart() {
-  // Don't connect to restricted ports.
   int port = request_info_.url.EffectiveIntPort();
-  if (!IsPortAllowedByDefault(port) && !IsPortAllowedByOverride(port))
-    return ERR_UNSAFE_PORT;
-
   origin_ = HostPortPair(request_info_.url.HostNoBrackets(), port);
   origin_url_ = HttpStreamFactory::ApplyHostMappingRules(
       request_info_.url, &origin_);
 
+  net_log_.BeginEvent(NetLog::TYPE_HTTP_STREAM_JOB,
+                      HttpStreamJobParameters::Create(request_info_.url,
+                                                      origin_url_));
+
+  // Don't connect to restricted ports.
+  if (!IsPortAllowedByDefault(port) && !IsPortAllowedByOverride(port)) {
+    if (waiting_job_) {
+      waiting_job_->Resume(this);
+      waiting_job_ = NULL;
+    }
+    return ERR_UNSAFE_PORT;
+  }
+
   next_state_ = STATE_RESOLVE_PROXY;
   return OK;
 }
@@ -603,8 +613,10 @@ int HttpStreamFactoryImpl::Job::DoResolveProxyComplete(int result) {
   }
 
   if (result != OK) {
-    if (dependent_job_)
-      dependent_job_->Resume(this);
+    if (waiting_job_) {
+      waiting_job_->Resume(this);
+      waiting_job_ = NULL;
+    }
     return result;
   }
 
@@ -684,12 +696,12 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
     }
   }
 
-  // OK, there's no available SPDY session. Let |dependent_job_| resume if it's
+  // OK, there's no available SPDY session. Let |waiting_job_| resume if it's
   // paused.
 
-  if (dependent_job_) {
-    dependent_job_->Resume(this);
-    dependent_job_ = NULL;
+  if (waiting_job_) {
+    waiting_job_->Resume(this);
+    waiting_job_ = NULL;
   }
 
   if (proxy_info_.is_http() || proxy_info_.is_https())
@@ -739,9 +751,9 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
 
   // TODO(willchan): Make this a bit more exact. Maybe there are recoverable
   // errors, such as ignoring certificate errors for Alternate-Protocol.
-  if (result < 0 && dependent_job_) {
-    dependent_job_->Resume(this);
-    dependent_job_ = NULL;
+  if (result < 0 && waiting_job_) {
+    waiting_job_->Resume(this);
+    waiting_job_ = NULL;
   }
 
   // |result| may be the result of any of the stacked pools. The following
diff --git a/net/http/http_stream_factory_impl_job.h b/net/http/http_stream_factory_impl_job.h
index e93b111..492a317 100644
--- a/net/http/http_stream_factory_impl_job.h
+++ b/net/http/http_stream_factory_impl_job.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -232,8 +232,10 @@ class HttpStreamFactoryImpl::Job {
   // proceed.
   Job* blocking_job_;
 
-  // |dependent_job_| is dependent on |this|. Notify it when it's ok to proceed.
-  Job* dependent_job_;
+  // |waiting_job_| is a Job waiting to see if |this| can reuse a connection.
+  // If |this| is unable to do so, we'll notify |waiting_job_| that it's ok to
+  // proceed and then race the two Jobs.
+  Job* waiting_job_;
 
   // True if handling a HTTPS request, or using SPDY with SSL
   bool using_ssl_;