Disable 0RTT if server and origin have different hosts.

net/quic/quic_stream_factory.cc

 // 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.
 
 #include "net/quic/quic_stream_factory.h"
 
 #include <set>
 
 #include "base/cpu.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 121 matching lines @@
 };
 
 // Responsible for creating a new QUIC session to the specified server, and
 // for notifying any associated requests when complete.
 class QuicStreamFactory::Job {
  public:
   Job(QuicStreamFactory* factory,
       HostResolver* host_resolver,
       const HostPortPair& host_port_pair,
       bool is_https,
-      bool was_alternate_protocol_recently_broken,
+      bool allow_zero_rtt,
       PrivacyMode privacy_mode,
       bool is_post,
       QuicServerInfo* server_info,
       const BoundNetLog& net_log);
 
   // Creates a new job to handle the resumption of for connecting an
   // existing session.
   Job(QuicStreamFactory* factory,
       HostResolver* host_resolver,
       QuicClientSession* session,
@@ skipping 34 matching lines @@
     STATE_CONNECT,
     STATE_RESUME_CONNECT,
     STATE_CONNECT_COMPLETE,
   };
   IoState io_state_;
 
   QuicStreamFactory* factory_;
   SingleRequestHostResolver host_resolver_;
   QuicServerId server_id_;
   bool is_post_;
-  bool was_alternate_protocol_recently_broken_;
+  // True iff server and origin have the same hostname

[Ryan Hamilton, 2015/05/18 14:29:52]

ditto

[Bence, 2015/05/18 15:17:54]

ditto

+  // and alternative service was not recently broken.
+  bool allow_zero_rtt_;

[Ryan Hamilton, 2015/05/18 14:29:52]

I'm not in love with this boolean combining two different ideas. Currently there
are a few reasons that we require confirmation:
* recently broken
* the factory requires it
* the request is a post
And currently, the job receives each of these different conditions and ||s them
together to require confirmation. My inclination would be to continue this
pattern by passing in a new server_and_origin_have_same_host variable.

Alternative, you could do this logic down in QuicStreamRequest::Request and
simply pass in a single require_confirmation boolean.

[Bence, 2015/05/18 15:17:54]

Great idea, following the first suggestion.  Thanks.

   scoped_ptr<QuicServerInfo> server_info_;
   bool started_another_job_;
   const BoundNetLog net_log_;
   QuicClientSession* session_;
   CompletionCallback callback_;
   AddressList address_list_;
   base::TimeTicks dns_resolution_start_time_;
   base::TimeTicks dns_resolution_end_time_;
   base::WeakPtrFactory<Job> weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
 QuicStreamFactory::Job::Job(QuicStreamFactory* factory,
                             HostResolver* host_resolver,
                             const HostPortPair& host_port_pair,
                             bool is_https,
-                            bool was_alternate_protocol_recently_broken,
+                            bool allow_zero_rtt,
                             PrivacyMode privacy_mode,
                             bool is_post,
                             QuicServerInfo* server_info,
                             const BoundNetLog& net_log)
     : io_state_(STATE_RESOLVE_HOST),
       factory_(factory),
       host_resolver_(host_resolver),
       server_id_(host_port_pair, is_https, privacy_mode),
       is_post_(is_post),
-      was_alternate_protocol_recently_broken_(
-          was_alternate_protocol_recently_broken),
+      allow_zero_rtt_(allow_zero_rtt),
       server_info_(server_info),
       started_another_job_(false),
       net_log_(net_log),
       session_(nullptr),
       weak_factory_(this) {
 }
 
 QuicStreamFactory::Job::Job(QuicStreamFactory* factory,
                             HostResolver* host_resolver,
                             QuicClientSession* session,
                             QuicServerId server_id)
     : io_state_(STATE_RESUME_CONNECT),
       factory_(factory),
       host_resolver_(host_resolver),  // unused
       server_id_(server_id),
-      is_post_(false),                                 // unused
-      was_alternate_protocol_recently_broken_(false),  // unused
-      started_another_job_(false),                     // unused
-      net_log_(session->net_log()),                    // unused
+      is_post_(false),               // unused
+      allow_zero_rtt_(true),         // unused
+      started_another_job_(false),   // unused
+      net_log_(session->net_log()),  // unused
       session_(session),
       weak_factory_(this) {
 }
 
 QuicStreamFactory::Job::~Job() {
   // If disk cache has a pending WaitForDataReadyCallback, cancel that callback.
   if (server_info_)
     server_info_->ResetWaitForDataReadyCallback();
 }
 
@@ skipping 175 matching lines @@
 
   if (!session_->connection()->connected()) {
     return ERR_CONNECTION_CLOSED;
   }
 
   session_->StartReading();
   if (!session_->connection()->connected()) {
     return ERR_QUIC_PROTOCOL_ERROR;
   }
   bool require_confirmation =
-      factory_->require_confirmation() || is_post_ ||
-      was_alternate_protocol_recently_broken_;
+      factory_->require_confirmation() || is_post_ || !allow_zero_rtt_;
 
   rv = session_->CryptoConnect(
       require_confirmation,
       base::Bind(&QuicStreamFactory::Job::OnIOComplete, GetWeakPtr()));
   return rv;
 }
 
 int QuicStreamFactory::Job::DoResumeConnect() {
   io_state_ = STATE_CONNECT_COMPLETE;
 
@@ skipping 26 matching lines @@
     : factory_(factory) {}
 
 QuicStreamRequest::~QuicStreamRequest() {
   if (factory_ && !callback_.is_null())
     factory_->CancelRequest(this);
 }
 
 int QuicStreamRequest::Request(const HostPortPair& host_port_pair,
                                bool is_https,
                                PrivacyMode privacy_mode,
+                               base::StringPiece origin_host,
                                base::StringPiece method,
                                const BoundNetLog& net_log,
                                const CompletionCallback& callback) {
   DCHECK(!stream_);
   DCHECK(callback_.is_null());
   DCHECK(factory_);
-  int rv = factory_->Create(host_port_pair, is_https, privacy_mode, method,
-                            net_log, this);
+  origin_host_.assign(origin_host.data(), origin_host.size());

[Ryan Hamilton, 2015/05/18 14:29:52]

It looks like this is the only place that origin_host_ is used. If that's true,
I recommend eliminating the member variable, and removing this line.

[Bence, 2015/05/18 15:17:54]

Oops.  Done.

+  bool server_and_origin_have_same_host = host_port_pair.host() == origin_host;
+  int rv =
+      factory_->Create(host_port_pair, is_https, privacy_mode,
+                       server_and_origin_have_same_host, method, net_log, this);
   if (rv == ERR_IO_PENDING) {
     host_port_pair_ = host_port_pair;
     net_log_ = net_log;
     callback_ = callback;
   } else {
     factory_ = nullptr;
   }
   if (rv == OK)
     DCHECK(stream_);
   return rv;
@@ skipping 102 matching lines @@
   require_confirmation_ = require_confirmation;
   if (http_server_properties_ && (!(local_address_ == IPEndPoint()))) {
     http_server_properties_->SetSupportsQuic(!require_confirmation,
                                              local_address_.address());
   }
 }
 
 int QuicStreamFactory::Create(const HostPortPair& host_port_pair,
                               bool is_https,
                               PrivacyMode privacy_mode,
+                              bool server_and_origin_have_same_host,
                               base::StringPiece method,
                               const BoundNetLog& net_log,
                               QuicStreamRequest* request) {
+  server_and_origin_have_same_host_ = server_and_origin_have_same_host;
   QuicServerId server_id(host_port_pair, is_https, privacy_mode);
   if (HasActiveSession(server_id)) {
     request->set_stream(CreateIfSessionExists(server_id, net_log));
     return OK;
   }
 
   if (HasActiveJob(server_id)) {
     active_requests_[request] = server_id;
     job_requests_map_[server_id].insert(request);
     return ERR_IO_PENDING;
@@ skipping 17 matching lines @@
         // If there is no entry for QUIC, consider that as a new server and
         // don't wait for Cache thread to load the data for that server.
         load_from_disk_cache = false;
       }
     }
     if (load_from_disk_cache && CryptoConfigCacheIsEmpty(server_id)) {
       quic_server_info = quic_server_info_factory_->GetForServer(server_id);
     }
   }
 
-  scoped_ptr<Job> job(new Job(this, host_resolver_, host_port_pair, is_https,
-                              WasQuicRecentlyBroken(server_id), privacy_mode,
-                              method == "POST" /* is_post */, quic_server_info,
-                              net_log));
+  scoped_ptr<Job> job(new Job(
+      this, host_resolver_, host_port_pair, is_https,
+      server_and_origin_have_same_host && !WasQuicRecentlyBroken(server_id),
+      privacy_mode, method == "POST" /* is_post */, quic_server_info, net_log));
   int rv = job->Run(base::Bind(&QuicStreamFactory::OnJobComplete,
                                base::Unretained(this), job.get()));
   if (rv == ERR_IO_PENDING) {
     active_requests_[request] = server_id;
     job_requests_map_[server_id].insert(request);
     active_jobs_[server_id].insert(job.release());
     return rv;
   }
   if (rv == OK) {
     DCHECK(HasActiveSession(server_id));
     request->set_stream(CreateIfSessionExists(server_id, net_log));
   }
   return rv;
 }
 
 void QuicStreamFactory::CreateAuxilaryJob(const QuicServerId server_id,
                                           bool is_post,
                                           const BoundNetLog& net_log) {
-  Job* aux_job = new Job(this, host_resolver_, server_id.host_port_pair(),
-                         server_id.is_https(), WasQuicRecentlyBroken(server_id),
-                         server_id.privacy_mode(), is_post, nullptr, net_log);
+  Job* aux_job = new Job(
+      this, host_resolver_, server_id.host_port_pair(), server_id.is_https(),
+      server_and_origin_have_same_host_ && !WasQuicRecentlyBroken(server_id),

[Ryan Hamilton, 2015/05/18 14:29:52]

It looks like this method is only called from the job. instead of storing
server_and_origin_have_same_host_ in the factory (which seems like a bug) I
would store this member in the job, and pass it in to this method.

[Bence, 2015/05/18 15:17:55]

Great idea, thanks.  Done.

+      server_id.privacy_mode(), is_post, nullptr, net_log);
   active_jobs_[server_id].insert(aux_job);
   task_runner_->PostTask(FROM_HERE,
                          base::Bind(&QuicStreamFactory::Job::RunAuxilaryJob,
                                     aux_job->GetWeakPtr()));
 }
 
 bool QuicStreamFactory::OnResolution(
     const QuicServerId& server_id,
     const AddressList& address_list) {
   DCHECK(!HasActiveSession(server_id));
@@ skipping 517 matching lines @@
   // Since the session was active, there's no longer an
   // HttpStreamFactoryImpl::Job running which can mark it broken, unless the TCP
   // job also fails. So to avoid not using QUIC when we otherwise could, we mark
   // it as recently broken, which means that 0-RTT will be disabled but we'll
   // still race.
   http_server_properties_->MarkAlternativeServiceRecentlyBroken(
       alternative_service);
 }
 
 }  // namespace net