Disable 0RTT if server and origin have different hosts.

Disable 0RTT if server and origin have different hosts.

Disable 0RTT if the hostname of the server and origin are different.  Currently
dead code since HttpStreamFactoryImpl::GetAlterantiveServiceFor() refuses to
return an altenative service with hostname different than that of the origin,
but I plan to change this very soon.

While disabling 0RTT in this case is not strictly necessary, it enables us to
move faster with alternative service implementation.  We expect the most
performance gain from pooling to existing connections to different hosts, which
is not hindered by this change.

* Add origin hostname parameter to QuicStreamRequest::Request() method.
* Add server_and_origin_have_same_host parameter to QuicStreamFactory::Create()
  and CreateAuxiliaryJob() methods, and QuicStreamFactory::Job constructor.
* Add server_and_origin_have_same_host_ member to QuicSteamFactory::Job.
* Driveby: rename was_alternate_protocol_recently_broken_ member to
  was_alternative_service_recently_broken_.

BUG=474217
Committed: https://crrev.com/68d401ddf26908913f659ef7d4450df351ae33cb
Cr-Commit-Position: refs/heads/master@{#330402}

[Bence, 2015-05-14 19:50:41]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2015-05-14 19:50:42]

Ryan, PTAL.  Thanks.

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

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:197: // True iff server and origin have the same
hostname
ditto

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:199: bool allow_zero_rtt_;
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.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:495: origin_host_.assign(origin_host.data(),
origin_host.size());
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.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:688: server_and_origin_have_same_host_ &&
!WasQuicRecentlyBroken(server_id),
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.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factory.h
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.h:280: // True iff server and origin have the same
hostname.
nit: iff is not very common in comments. I think "if" is fine.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.h:281: bool server_and_origin_have_same_host_;
Wait, this is surprising. This is a member of the factory, not a job. That
doesn't seem right to me. The factory deal with lots of requests/sessions/jobs
at once. Which does this member apply to?

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory_test.cc:473: Run(true);
How does this test differ from CreateZeroRtt? (I think it basically doesn't?)

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory_test.cc:477: Run(false);
Just inline the run method.

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

Ryan, PTAL.  Thanks.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:197: // True iff server and origin have the same
hostname
On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> ditto

ditto

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:199: bool allow_zero_rtt_;
On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> 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.

Great idea, following the first suggestion.  Thanks.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:495: origin_host_.assign(origin_host.data(),
origin_host.size());
On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> 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.

Oops.  Done.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.cc:688: server_and_origin_have_same_host_ &&
!WasQuicRecentlyBroken(server_id),
On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> 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.

Great idea, thanks.  Done.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factory.h
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.h:280: // True iff server and origin have the same
hostname.
On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> nit: iff is not very common in comments. I think "if" is fine.

I am fine with not using iff if it is not common enough, but in that case, I
prefer to spell out "if and only if" to very specificly convey what I actually
mean.

BTW it is now moot in this file.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.h:281: bool server_and_origin_have_same_host_;
On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> Wait, this is surprising. This is a member of the factory, not a job. That
> doesn't seem right to me. The factory deal with lots of requests/sessions/jobs
> at once. Which does this member apply to?

Good catch, sorry about that.  Done.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory_test.cc:473: Run(true);
On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> How does this test differ from CreateZeroRtt? (I think it basically doesn't?)

I agree, it doesn't.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory_test.cc:477: Run(false);
On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> Just inline the run method.

How about removing the CreateZeroRtt test and keeping these two?  It would be
functionally the same as what you suggest, and would concisely document that the
only difference between the two is the hostname.

[Ryan Hamilton, 2015-05-18 16:21:55]

Looking great. One test-only change and you're good to go.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factory.h
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.h:280: // True iff server and origin have the same
hostname.
On 2015/05/18 15:17:55, Bence wrote:
> On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> > nit: iff is not very common in comments. I think "if" is fine.
> 
> I am fine with not using iff if it is not common enough, but in that case, I
> prefer to spell out "if and only if" to very specificly convey what I actually
> mean.

Yeah, I totally understand the sentiment. It seems, though, that "and only if"
is pretty much implicit in how if is used in comments. For example:

* True if this is a post request
* True if handshake confirmation is required
* True if QUIC is disabled
* True if QUIC was recently broken

All of these are really "iff" but for some reason it seems fine to leave them
out. 

Perhaps this is because programmers are lazy while mathematicians are precise?
:>

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory_test.cc:477: Run(false);
On 2015/05/18 15:17:55, Bence wrote:
> On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> > Just inline the run method.
> 
> How about removing the CreateZeroRtt test and keeping these two?  It would be
> functionally the same as what you suggest, and would concisely document that
the
> only difference between the two is the hostname.

In general, it's preferable to avoid logic in tests because it tends to make
them harder to read/reason about.
(https://docs.google.com/document/d/1VbCdizaXK8xDTKD8Z-IP24BeW8-UYklnuG3leU2M4...
@google.com only) Now, if all of the tests in a file require so much boilerplate
that the test becomes unreadable then it can be ok to add helper methods. But in
this case, I think it just makes these two tests different from the others so I
think it makes sense to just inline Run() in the "different host case".

[Ryan Hamilton, 2015-05-18 16:22:37]

Oh, I think the CL description needs a minor update to match your recent
changes.

[Bence, 2015-05-18 18:12:03]

Ryan, PTAL.  Thanks.

> Oh, I think the CL description needs a minor update to match your recent
> changes.

Done.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factory.h
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory.h:280: // True iff server and origin have the same
hostname.
On 2015/05/18 16:21:54, Ryan Hamilton wrote:
> On 2015/05/18 15:17:55, Bence wrote:
> > On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> > > nit: iff is not very common in comments. I think "if" is fine.
> > 
> > I am fine with not using iff if it is not common enough, but in that case, I
> > prefer to spell out "if and only if" to very specificly convey what I
actually
> > mean.
> 
> Yeah, I totally understand the sentiment. It seems, though, that "and only if"
> is pretty much implicit in how if is used in comments. For example:
> 
> * True if this is a post request
> * True if handshake confirmation is required
> * True if QUIC is disabled
> * True if QUIC was recently broken
> 
> All of these are really "iff" but for some reason it seems fine to leave them
> out. 
> 
> Perhaps this is because programmers are lazy while mathematicians are precise?
> :>

I totally agree.  However, it is not clear to me after all if you mind me
writing "if and only if", so I'll leave it as that for the time being.  Feel
free to ask me to change if you prefer.  Thanks.

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1140153003/diff/1/net/quic/quic_stream_factor...
net/quic/quic_stream_factory_test.cc:477: Run(false);
On 2015/05/18 16:21:55, Ryan Hamilton wrote:
> On 2015/05/18 15:17:55, Bence wrote:
> > On 2015/05/18 14:29:52, Ryan Hamilton wrote:
> > > Just inline the run method.
> > 
> > How about removing the CreateZeroRtt test and keeping these two?  It would
be
> > functionally the same as what you suggest, and would concisely document that
> the
> > only difference between the two is the hostname.
> 
> In general, it's preferable to avoid logic in tests because it tends to make
> them harder to read/reason about.
>
(https://docs.google.com/document/d/1VbCdizaXK8xDTKD8Z-IP24BeW8-UYklnuG3leU2M4...
> @google.com only) Now, if all of the tests in a file require so much
boilerplate
> that the test becomes unreadable then it can be ok to add helper methods. But
in
> this case, I think it just makes these two tests different from the others so
I
> think it makes sense to just inline Run() in the "different host case".

Fair enough.  I sometimes have difficulties finding the right balance,
especially when I feel like it's important to highlight that there is only a
small difference between two tests.

Done.

[Ryan Hamilton, 2015-05-18 18:20:38]

lgtm

Thanks for doing this!

[Bence, 2015-05-18 18:52:26]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-05-18 18:53:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140153003/80001

[commit-bot: I haz the power, 2015-05-18 20:31:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-18 22:42:17]

Patchset 5 (id:??) landed as
https://crrev.com/68d401ddf26908913f659ef7d4450df351ae33cb
Cr-Commit-Position: refs/heads/master@{#330402}