Add cross origin to Blink-driven preconnect

Add cross origin to Blink-driven preconnect

In order to support <link rel=preconnect> for both anonymous and non-anonymous connection pools we need to add support for preconnect requests coming from Blink.

BUG=468005
Committed: https://crrev.com/9131644005a8e069c128512f5674e53fa4d651ba
Cr-Commit-Position: refs/heads/master@{#340854}

[Yoav Weiss, 2015-05-11 06:02:02]

yoav@yoav.ws changed reviewers:
+ mkwst@chromium.org

[Yoav Weiss, 2015-05-11 06:03:51]

Hey Mike!

Not a very exciting CL for your jet lagged return to our glorious time zone, but
can you take a look? :D

[Mike West, 2015-05-11 06:36:45]

On 2015/05/11 at 06:03:51, yoav wrote:
> Hey Mike!
> 
> Not a very exciting CL for your jet lagged return to our glorious time zone,
but can you take a look? :D

Could `crossOrigin` be something more specific than a string? An enum, perhaps?
(That probably means it would make more sense to do the Blink side first, unless
we already have such an enum lying around)

[Yoav Weiss, 2015-05-11 07:08:26]

On 2015/05/11 06:36:45, Mike West (traveling. slow.) wrote:
> On 2015/05/11 at 06:03:51, yoav wrote:
> > Hey Mike!
> > 
> > Not a very exciting CL for your jet lagged return to our glorious time zone,
> but can you take a look? :D
> 
> Could `crossOrigin` be something more specific than a string? An enum,
perhaps?
> (That probably means it would make more sense to do the Blink side first,
unless
> we already have such an enum lying around)

OK, sure.

So the plan is:
* Add Blink side without adding the public interface that sends the crossOrigin
setting to the Chrome side.
* Add Chrome side equiv of this CL, with an enum defined on the Blink side
* Add Blink side's public interface (keeping the old one since it's still
implemented)
* Override new public interface and delete the old interface implementation on
the Chrome side
* Delete old interface on Blink's public

Does that make sense?

P.S. When are we merging the repos, again? :)

[Mike West, 2015-05-11 07:19:05]

On 2015/05/11 at 07:08:26, yoav wrote:
> So the plan is:
> * Add Blink side without adding the public interface that sends the
crossOrigin setting to the Chrome side.
> * Add Chrome side equiv of this CL, with an enum defined on the Blink side
> * Add Blink side's public interface (keeping the old one since it's still
implemented)
> * Override new public interface and delete the old interface implementation on
the Chrome side
> * Delete old interface on Blink's public

Looks about right.

> P.S. When are we merging the repos, again? :)

Not soon enough. Ask Jochen at BlinkOn4. Last I heard, we're waiting on bot
capacity. :(

[Yoav Weiss, 2015-05-11 09:42:20]

On 2015/05/11 07:19:05, Mike West (traveling. slow.) wrote:
> On 2015/05/11 at 07:08:26, yoav wrote:
> > So the plan is:
> > * Add Blink side without adding the public interface that sends the
> crossOrigin setting to the Chrome side.
> > * Add Chrome side equiv of this CL, with an enum defined on the Blink side
> > * Add Blink side's public interface (keeping the old one since it's still
> implemented)
> > * Override new public interface and delete the old interface implementation
on
> the Chrome side
> > * Delete old interface on Blink's public
> 
> Looks about right.

A couple questions:
* What's the best place to define such an enum so that it's accessible from both
loader and the Chrome side?
* Regarding the enum itself, AFAICT the possible values are: No CORS, Anonymous
CORS, and use-credentials CORS. Is that correct?

[Mike West, 2015-05-11 09:50:55]

On 2015/05/11 at 09:42:20, yoav wrote:
> A couple questions:
> * What's the best place to define such an enum so that it's accessible from
both loader and the Chrome side?

Somewhere in public/platform seems fine. If you feel like going crazy, you could
normalize the various places where we already do this parsing
(HTMLMediaElement.cpp was the first one I found).

> * Regarding the enum itself, AFAICT the possible values are: No CORS,
Anonymous CORS, and use-credentials CORS. Is that correct?

And "unset".

[Yoav Weiss, 2015-05-11 09:57:16]

On 2015/05/11 09:50:55, Mike West (traveling. slow.) wrote:

> 
> > * Regarding the enum itself, AFAICT the possible values are: No CORS,
> Anonymous CORS, and use-credentials CORS. Is that correct?
> 
> And "unset".

I knew I should ask :) Can you point me to where "unset" is defined? Couldn't
find it in
https://html.spec.whatwg.org/multipage/infrastructure.html#cors-settings-attr...

[Mike West, 2015-05-11 09:58:40]

On 2015/05/11 at 09:57:16, yoav wrote:
> On 2015/05/11 09:50:55, Mike West (traveling. slow.) wrote:
> 
> > 
> > > * Regarding the enum itself, AFAICT the possible values are: No CORS,
> > Anonymous CORS, and use-credentials CORS. Is that correct?
> > 
> > And "unset".
> 
> I knew I should ask :) Can you point me to where "unset" is defined? Couldn't
find it in
https://html.spec.whatwg.org/multipage/infrastructure.html#cors-settings-attr...

"Unset" as in "There's no `crossorigin` attribute on the element."

[Yoav Weiss, 2015-05-11 10:38:39]

On 2015/05/11 09:58:40, Mike West (traveling. slow.) wrote:
> On 2015/05/11 at 09:57:16, yoav wrote:
> > On 2015/05/11 09:50:55, Mike West (traveling. slow.) wrote:
> > 
> > > 
> > > > * Regarding the enum itself, AFAICT the possible values are: No CORS,
> > > Anonymous CORS, and use-credentials CORS. Is that correct?
> > > 
> > > And "unset".
> > 
> > I knew I should ask :) Can you point me to where "unset" is defined?
Couldn't
> find it in
>
https://html.spec.whatwg.org/multipage/infrastructure.html#cors-settings-attr...
> 
> "Unset" as in "There's no `crossorigin` attribute on the element."

Oh, OK :)

[Yoav Weiss, 2015-05-11 16:22:20]

On 2015/05/11 10:38:39, Yoav Weiss wrote:
> On 2015/05/11 09:58:40, Mike West (traveling. slow.) wrote:
> > On 2015/05/11 at 09:57:16, yoav wrote:
> > > On 2015/05/11 09:50:55, Mike West (traveling. slow.) wrote:
> > > 
> > > > 
> > > > > * Regarding the enum itself, AFAICT the possible values are: No CORS,
> > > > Anonymous CORS, and use-credentials CORS. Is that correct?
> > > > 
> > > > And "unset".
> > > 
> > > I knew I should ask :) Can you point me to where "unset" is defined?
> Couldn't
> > find it in
> >
>
https://html.spec.whatwg.org/multipage/infrastructure.html#cors-settings-attr...
> > 
> > "Unset" as in "There's no `crossorigin` attribute on the element."
> 
> Oh, OK :)

I've change the CL to plumb preconnect all the way through.
Since it's the first time I'm touching Chrome at that level, I'm sure it's all
kinds of wrong.

A couple of issues I'm aware of:
* I'm referring to blink:CrossOriginAttributeValue from chrome/browser. What
should I do here? Should I replace the enum with a bool earlier on? (since all I
need to know is if the connection is anonymous)
* It's not tested in any way. Are there examples of related unit tests? What's
the recommended testing strategy here?

[Yoav Weiss, 2015-05-12 08:18:13]

On 2015/05/11 16:22:20, Yoav Weiss wrote:
> On 2015/05/11 10:38:39, Yoav Weiss wrote:
> > On 2015/05/11 09:58:40, Mike West (traveling. slow.) wrote:
> > > On 2015/05/11 at 09:57:16, yoav wrote:
> > > > On 2015/05/11 09:50:55, Mike West (traveling. slow.) wrote:
> > > > 
> > > > > 
> > > > > > * Regarding the enum itself, AFAICT the possible values are: No
CORS,
> > > > > Anonymous CORS, and use-credentials CORS. Is that correct?
> > > > > 
> > > > > And "unset".
> > > > 
> > > > I knew I should ask :) Can you point me to where "unset" is defined?
> > Couldn't
> > > find it in
> > >
> >
>
https://html.spec.whatwg.org/multipage/infrastructure.html#cors-settings-attr...
> > > 
> > > "Unset" as in "There's no `crossorigin` attribute on the element."
> > 
> > Oh, OK :)
> 
> I've change the CL to plumb preconnect all the way through.
> Since it's the first time I'm touching Chrome at that level, I'm sure it's all
> kinds of wrong.
> 
> A couple of issues I'm aware of:
> * I'm referring to blink:CrossOriginAttributeValue from chrome/browser. What
> should I do here? Should I replace the enum with a bool earlier on? (since all
I
> need to know is if the connection is anonymous)
> * It's not tested in any way. Are there examples of related unit tests? What's
> the recommended testing strategy here?

Regarding testing, is the best way to test is in
chrome/browser/net/predictor_browsertest.cc after everything is hooked up?
Is there a way to test in the mean time?

[Mike West, 2015-05-12 08:33:07]

On 2015/05/12 at 08:18:13, yoav wrote:
> > A couple of issues I'm aware of:
> > * I'm referring to blink:CrossOriginAttributeValue from chrome/browser. What
> > should I do here? Should I replace the enum with a bool earlier on? (since
all I
> > need to know is if the connection is anonymous)

If that's all you need to know, then replacing it with an 'include_credentials'
boolean in the NetworkHintsMsg_Preconnect message seems like a reasonable way of
resolving the includes.

> > * It's not tested in any way. Are there examples of related unit tests?
What's
> > the recommended testing strategy here?
> 
> Regarding testing, is the best way to test is in
chrome/browser/net/predictor_browsertest.cc after everything is hooked up?
> Is there a way to test in the mean time?

The only behavioral change here is that `request_info.privacy_mode =
net::PRIVACY_MODE_ENABLED` is set in PreconnectOnIOThread`, right? It seems like
you should be able to put together a browsertest right now that verifies that
that flag is set. I'm not familiar enough with the preconnect unit tests, but it
would surprise me if that wasn't possible right now.

[Yoav Weiss, 2015-05-12 08:38:07]

On 2015/05/12 08:33:07, Mike West wrote:
> On 2015/05/12 at 08:18:13, yoav wrote:
> > > A couple of issues I'm aware of:
> > > * I'm referring to blink:CrossOriginAttributeValue from chrome/browser.
What
> > > should I do here? Should I replace the enum with a bool earlier on? (since
> all I
> > > need to know is if the connection is anonymous)
> 
> If that's all you need to know, then replacing it with an
'include_credentials'
> boolean in the NetworkHintsMsg_Preconnect message seems like a reasonable way
of
> resolving the includes.
> 
> > > * It's not tested in any way. Are there examples of related unit tests?
> What's
> > > the recommended testing strategy here?
> > 
> > Regarding testing, is the best way to test is in
> chrome/browser/net/predictor_browsertest.cc after everything is hooked up?
> > Is there a way to test in the mean time?
> 
> The only behavioral change here is that `request_info.privacy_mode =
> net::PRIVACY_MODE_ENABLED` is set in PreconnectOnIOThread`, right? It seems
like
> you should be able to put together a browsertest right now that verifies that
> that flag is set. I'm not familiar enough with the preconnect unit tests, but
it
> would surprise me if that wasn't possible right now.

Oh, I know what I should've done! I'll stop the Blink side and add a call to the
new API there only when crossorigin was actually used.
That would make this testable thtough browsertest

[Mike West, 2015-05-12 08:46:13]

On 2015/05/12 at 08:38:07, yoav wrote:
> On 2015/05/12 08:33:07, Mike West wrote:
> > On 2015/05/12 at 08:18:13, yoav wrote:
> > > > A couple of issues I'm aware of:
> > > > * I'm referring to blink:CrossOriginAttributeValue from chrome/browser.
What
> > > > should I do here? Should I replace the enum with a bool earlier on?
(since
> > all I
> > > > need to know is if the connection is anonymous)
> > 
> > If that's all you need to know, then replacing it with an
'include_credentials'
> > boolean in the NetworkHintsMsg_Preconnect message seems like a reasonable
way of
> > resolving the includes.
> > 
> > > > * It's not tested in any way. Are there examples of related unit tests?
> > What's
> > > > the recommended testing strategy here?
> > > 
> > > Regarding testing, is the best way to test is in
> > chrome/browser/net/predictor_browsertest.cc after everything is hooked up?
> > > Is there a way to test in the mean time?
> > 
> > The only behavioral change here is that `request_info.privacy_mode =
> > net::PRIVACY_MODE_ENABLED` is set in PreconnectOnIOThread`, right? It seems
like
> > you should be able to put together a browsertest right now that verifies
that
> > that flag is set. I'm not familiar enough with the preconnect unit tests,
but it
> > would surprise me if that wasn't possible right now.
> 
> Oh, I know what I should've done! I'll stop the Blink side and add a call to
the new API there only when crossorigin was actually used.
> That would make this testable thtough browsertest

*shrug* Sounds reasonable.

[Yoav Weiss, 2015-05-12 11:29:44]

On 2015/05/12 08:46:13, Mike West wrote:
> On 2015/05/12 at 08:38:07, yoav wrote:
> > On 2015/05/12 08:33:07, Mike West wrote:
> > > On 2015/05/12 at 08:18:13, yoav wrote:
> > > > > A couple of issues I'm aware of:
> > > > > * I'm referring to blink:CrossOriginAttributeValue from
chrome/browser.
> What
> > > > > should I do here? Should I replace the enum with a bool earlier on?
> (since
> > > all I
> > > > > need to know is if the connection is anonymous)
> > > 
> > > If that's all you need to know, then replacing it with an
> 'include_credentials'
> > > boolean in the NetworkHintsMsg_Preconnect message seems like a reasonable
> way of
> > > resolving the includes.
> > > 
> > > > > * It's not tested in any way. Are there examples of related unit
tests?
> > > What's
> > > > > the recommended testing strategy here?
> > > > 
> > > > Regarding testing, is the best way to test is in
> > > chrome/browser/net/predictor_browsertest.cc after everything is hooked up?
> > > > Is there a way to test in the mean time?
> > > 
> > > The only behavioral change here is that `request_info.privacy_mode =
> > > net::PRIVACY_MODE_ENABLED` is set in PreconnectOnIOThread`, right? It
seems
> like
> > > you should be able to put together a browsertest right now that verifies
> that
> > > that flag is set. I'm not familiar enough with the preconnect unit tests,
> but it
> > > would surprise me if that wasn't possible right now.
> > 
> > Oh, I know what I should've done! I'll stop the Blink side and add a call to
> the new API there only when crossorigin was actually used.
> > That would make this testable thtough browsertest
> 
> *shrug* Sounds reasonable.

Any advice regarding the enum includes from blink in /chrome/browser?
Should I translate that enum to a local one? Replace it with a boolean?

[Mike West, 2015-05-12 11:37:24]

On 2015/05/12 at 11:29:44, yoav wrote:
> On 2015/05/12 08:46:13, Mike West wrote:
> > On 2015/05/12 at 08:38:07, yoav wrote:
> > > On 2015/05/12 08:33:07, Mike West wrote:
> > > > On 2015/05/12 at 08:18:13, yoav wrote:
> > > > > > A couple of issues I'm aware of:
> > > > > > * I'm referring to blink:CrossOriginAttributeValue from
chrome/browser.
> > What
> > > > > > should I do here? Should I replace the enum with a bool earlier on?
> > (since
> > > > all I
> > > > > > need to know is if the connection is anonymous)
> > > > 
> > > > If that's all you need to know, then replacing it with an
> > 'include_credentials'
> > > > boolean in the NetworkHintsMsg_Preconnect message seems like a
reasonable
> > way of
> > > > resolving the includes.
> > > > 
> > > > > > * It's not tested in any way. Are there examples of related unit
tests?
> > > > What's
> > > > > > the recommended testing strategy here?
> > > > > 
> > > > > Regarding testing, is the best way to test is in
> > > > chrome/browser/net/predictor_browsertest.cc after everything is hooked
up?
> > > > > Is there a way to test in the mean time?
> > > > 
> > > > The only behavioral change here is that `request_info.privacy_mode =
> > > > net::PRIVACY_MODE_ENABLED` is set in PreconnectOnIOThread`, right? It
seems
> > like
> > > > you should be able to put together a browsertest right now that verifies
> > that
> > > > that flag is set. I'm not familiar enough with the preconnect unit
tests,
> > but it
> > > > would surprise me if that wasn't possible right now.
> > > 
> > > Oh, I know what I should've done! I'll stop the Blink side and add a call
to
> > the new API there only when crossorigin was actually used.
> > > That would make this testable thtough browsertest
> > 
> > *shrug* Sounds reasonable.
> 
> Any advice regarding the enum includes from blink in /chrome/browser?
> Should I translate that enum to a local one? Replace it with a boolean?

Either way. If this is the only place that needs to know about the enum, and you
only care about one of the states, then just pipe through a  boolean for
simplicity. If you end up using it in more than one place, then translating to a
chromium SHOUTY_STYLE_ENUM is probably the right thing to do.

[Yoav Weiss, 2015-05-12 14:58:54]

On 2015/05/12 11:37:24, Mike West wrote:
> On 2015/05/12 at 11:29:44, yoav wrote:
> > On 2015/05/12 08:46:13, Mike West wrote:
> > > On 2015/05/12 at 08:38:07, yoav wrote:
> > > > On 2015/05/12 08:33:07, Mike West wrote:
> > > > > On 2015/05/12 at 08:18:13, yoav wrote:
> > > > > > > A couple of issues I'm aware of:
> > > > > > > * I'm referring to blink:CrossOriginAttributeValue from
> chrome/browser.
> > > What
> > > > > > > should I do here? Should I replace the enum with a bool earlier
on?
> > > (since
> > > > > all I
> > > > > > > need to know is if the connection is anonymous)
> > > > > 
> > > > > If that's all you need to know, then replacing it with an
> > > 'include_credentials'
> > > > > boolean in the NetworkHintsMsg_Preconnect message seems like a
> reasonable
> > > way of
> > > > > resolving the includes.
> > > > > 
> > > > > > > * It's not tested in any way. Are there examples of related unit
> tests?
> > > > > What's
> > > > > > > the recommended testing strategy here?
> > > > > > 
> > > > > > Regarding testing, is the best way to test is in
> > > > > chrome/browser/net/predictor_browsertest.cc after everything is hooked
> up?
> > > > > > Is there a way to test in the mean time?
> > > > > 
> > > > > The only behavioral change here is that `request_info.privacy_mode =
> > > > > net::PRIVACY_MODE_ENABLED` is set in PreconnectOnIOThread`, right? It
> seems
> > > like
> > > > > you should be able to put together a browsertest right now that
verifies
> > > that
> > > > > that flag is set. I'm not familiar enough with the preconnect unit
> tests,
> > > but it
> > > > > would surprise me if that wasn't possible right now.
> > > > 
> > > > Oh, I know what I should've done! I'll stop the Blink side and add a
call
> to
> > > the new API there only when crossorigin was actually used.
> > > > That would make this testable thtough browsertest
> > > 
> > > *shrug* Sounds reasonable.
> > 
> > Any advice regarding the enum includes from blink in /chrome/browser?
> > Should I translate that enum to a local one? Replace it with a boolean?
> 
> Either way. If this is the only place that needs to know about the enum, and
you
> only care about one of the states, then just pipe through a  boolean for
> simplicity. If you end up using it in more than one place, then translating to
a
> chromium SHOUTY_STYLE_ENUM is probably the right thing to do.

Ended up with replacing it by a boolean, since it seems sufficient. Also - added
a browser test that makes sure it's actually working!

[Ryan Sleevi, 2015-05-12 15:43:20]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-05-12 15:43:20]

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
File chrome/browser/net/preconnect.h (right):

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
chrome/browser/net/preconnect.h:38: bool isAnonymous = false);
Drive-by STYLE: s/isAnonymous = false/is_anonymous/

1) No default arguments
2) Underscores, not camelCase

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/pred...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor.h:247: bool isAnonymous = false);
Ditto STYLE concerns here in this file

[Ryan Sleevi, 2015-05-12 15:49:42]

rsleevi@chromium.org changed reviewers:
+ mmenke@chromium.org

[Ryan Sleevi, 2015-05-12 15:49:43]

Mmenke will need to review //c/b/net.

I'm still deeply concerned about this being the right solution. The reason for
that is that I'm afraid if we land this, no one will be motivated to fix privacy
mode and its weird non-guarantees. That is, this CL likely works, but I'm not
sure it is best for the project in the long term. Pat was going to chase things
down, I believe.

Yes, I'm adding preemptive CC's to make sure you're going in the right direction
and so that you don't spin your wheels on a solution and have it nack'd later
on.

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/pred...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_browsertest.cc:328: net_log_observer.Attach();
As far as tests go, I would have a hard time approving this test. It is reaching
too much into the internal implementation details of //net to accomplish its
results.

The canonical way would be to ask the rest server how many sockets had
connected. However, that means _not_ using the SpawnedTestServer, as it will go
all explody when you have multiple connections. The EmbeddedTestServer should be
sufficient.

This test also hangs if your conditions aren't met, and so that's a recipe for a
bad time too. However, resolving that will be trickier.

[mmenke, 2015-05-12 15:57:20]

Was there an intent to implement and ship for this?  Is there a spec?

[Yoav Weiss, 2015-05-12 16:10:16]

Patchset #4 (id:60001) has been deleted

[Yoav Weiss, 2015-05-12 16:10:26]

Patchset #4 (id:80001) has been deleted

[Yoav Weiss, 2015-05-12 16:25:04]

Thanks for the comments! :)

mmenke: There has been an intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/CM5BaP6uVvE
Note that this is not shipping the crossorigin or preconnect parts, since they
are behind an experimental flag in Blink.

Spec change for the crossorigin part is PRed at
https://github.com/w3c/resource-hints/pull/33

Ryan: I'll change the styling. (new in these parts :D)

Otherwise, I'm not sure what you mean by fixing privacy-mode. Are you talking
about implementation issue, or spec ones?
I was skeptical about the "crossorigin" approach myself, until we found out that
Firefox are running into exactly the same issues implementing `preconnect`.
Can you detail your concerns (or point me towards an explanation)?

Regarding the testing, I more or less copy/pasted the existing Preconnect test.
(and I totally agree that having it timeout and crash when it's not working is
no fun)
Would there be a way to tell apart pm and regular connections using the rest
server method? Can we fix that as part of a separate CL? And can you point me
towards tests that do something similar?

[mmenke, 2015-05-12 18:48:01]

On 2015/05/12 16:25:04, Yoav Weiss wrote:
> Thanks for the comments! :)
> 
> mmenke: There has been an intent to implement:
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/CM5BaP6uVvE
> Note that this is not shipping the crossorigin or preconnect parts, since they
> are behind an experimental flag in Blink.
> 
> Spec change for the crossorigin part is PRed at
> https://github.com/w3c/resource-hints/pull/33

Thanks for the links!  I don't see anything about the "crossorigin" behavior in
either the intent to implement or the resource hints docs, though.

> Ryan: I'll change the styling. (new in these parts :D)
> 
> Otherwise, I'm not sure what you mean by fixing privacy-mode. Are you talking
> about implementation issue, or spec ones?
> I was skeptical about the "crossorigin" approach myself, until we found out
that
> Firefox are running into exactly the same issues implementing `preconnect`.
> Can you detail your concerns (or point me towards an explanation)?
> 
> Regarding the testing, I more or less copy/pasted the existing Preconnect
test.
> (and I totally agree that having it timeout and crash when it's not working is
> no fun)

I don't think there's a whole lot we can do about timing out on failure in a lot
of cases - you often can't wait until something doesn't happen.  In these tests,
for instance, if navigation to the test page never completes for some reason,
the test hangs.  That's the case for pretty much all browser tests.

Similarly, in terms of waiting on the test server to see the connection, we
basically have to wait on the connection.

> Would there be a way to tell apart pm and regular connections using the rest
> server method? Can we fix that as part of a separate CL? And can you point me
> towards tests that do something similar?

I'm kinda hazy on the purpose of "pm" connections, and a quick search for
"privacy mode" turns up information on FireFox's incognito-equivalent...  Any
docs on this?

Anyhow, one difference I'm aware of is they can't share connections.  So one way
to do this would be to have two test servers.  One spins up a privacy mode
preconnect to the other.  We wait until it's established (Method would have to
be added to the embedded test server to do this).  Then we issue a request from
the page that would require a privacy mode connection, and wait until the
embedded test server has seen it (Again, need to modify the test server to
support this).  Then we could make sure it used the preconnected socket.

Seems a bit involved, and agree that it would make sense to land it separately -
could land the test changes first, modified to work with the current
(non-privacy-mode) case.

[mmenke, 2015-05-12 18:50:43]

On 2015/05/12 18:48:01, mmenke wrote:
> On 2015/05/12 16:25:04, Yoav Weiss wrote:
> > Thanks for the comments! :)
> > 
> > mmenke: There has been an intent to implement:
> > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/CM5BaP6uVvE
> > Note that this is not shipping the crossorigin or preconnect parts, since
they
> > are behind an experimental flag in Blink.
> > 
> > Spec change for the crossorigin part is PRed at
> > https://github.com/w3c/resource-hints/pull/33
> 
> Thanks for the links!  I don't see anything about the "crossorigin" behavior
in
> either the intent to implement or the resource hints docs, though.

Oops - missed the relevant links danging off the second link.

[Yoav Weiss, 2015-05-12 19:36:30]

On 2015/05/12 18:48:01, mmenke wrote:
> On 2015/05/12 16:25:04, Yoav Weiss wrote:
> > Thanks for the comments! :)
> > 
> > mmenke: There has been an intent to implement:
> > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/CM5BaP6uVvE
> > Note that this is not shipping the crossorigin or preconnect parts, since
they
> > are behind an experimental flag in Blink.
> > 
> > Spec change for the crossorigin part is PRed at
> > https://github.com/w3c/resource-hints/pull/33
> 
> Thanks for the links!  I don't see anything about the "crossorigin" behavior
in
> either the intent to implement or the resource hints docs, though.
> 
> > Ryan: I'll change the styling. (new in these parts :D)
> > 
> > Otherwise, I'm not sure what you mean by fixing privacy-mode. Are you
talking
> > about implementation issue, or spec ones?
> > I was skeptical about the "crossorigin" approach myself, until we found out
> that
> > Firefox are running into exactly the same issues implementing `preconnect`.
> > Can you detail your concerns (or point me towards an explanation)?
> > 
> > Regarding the testing, I more or less copy/pasted the existing Preconnect
> test.
> > (and I totally agree that having it timeout and crash when it's not working
is
> > no fun)
> 
> I don't think there's a whole lot we can do about timing out on failure in a
lot
> of cases - you often can't wait until something doesn't happen.  In these
tests,
> for instance, if navigation to the test page never completes for some reason,
> the test hangs.  That's the case for pretty much all browser tests.
> 
> Similarly, in terms of waiting on the test server to see the connection, we
> basically have to wait on the connection.
> 
> > Would there be a way to tell apart pm and regular connections using the rest
> > server method? Can we fix that as part of a separate CL? And can you point
me
> > towards tests that do something similar?
> 
> I'm kinda hazy on the purpose of "pm" connections, and a quick search for
> "privacy mode" turns up information on FireFox's incognito-equivalent...  Any
> docs on this?

I don't have any docs. Ryan?
What I know about privacy-mode connections is that they are needed when fetching
resources in anonymous CORS mode 
(and as you say, cannot be shared with non-anonymous fetches), since otherwise
they may expose credentials.

> 
> Anyhow, one difference I'm aware of is they can't share connections.  So one
way
> to do this would be to have two test servers.  One spins up a privacy mode
> preconnect to the other. 
>  We wait until it's established (Method would have to
> be added to the embedded test server to do this).  Then we issue a request
from
> the page that would require a privacy mode connection, and wait until the
> embedded test server has seen it (Again, need to modify the test server to
> support this).  Then we could make sure it used the preconnected socket.
> 
> Seems a bit involved, and agree that it would make sense to land it separately
-
> could land the test changes first, modified to work with the current
> (non-privacy-mode) case.

[Ryan Sleevi, 2015-05-12 22:13:20]

On 2015/05/12 19:36:30, Yoav Weiss wrote:
> I don't have any docs. Ryan?

There are none. Nor is it described as part of the Web Platform. That's why I
have such an issue with Privacy Mode as a concept, let alone an implementation.

> What I know about privacy-mode connections is that they are needed when
fetching
> resources in anonymous CORS mode 
> (and as you say, cannot be shared with non-anonymous fetches), since otherwise
> they may expose credentials.

Which is somewhat vague. Let's pretend we didn't have privacy mode at all, what
would we need or rationally explain in the Web Platform?

The only point in which I can see us needing to use different connections is for
situations where we used an HTTP/1.1 violating auth method - aka NTLM &
Negotiate. In this case, re-using a connection may confer ambient authority onto
subsequent requests.

Outside of that, what is there? As discussed rather extensively in CORS,
restricting client certificates is something that doesn't really make sense (in
that the attacker can't mount a dictionary attack via URL parameters as they
presumably could via XHR + HTTP auth methods). TLS sessions aren't fundamentally
an issue. TLS Channel IDs shouldn't confer ambient authority, and if they do,
it's a broken use of Channel ID.

So how much of that matters for preconnect? I'd argue zero, since it's about
connection establishment and that should be independent of any server-initiated
HTTP auth scenarios.

[Yoav Weiss, 2015-05-13 06:29:43]

On 2015/05/12 22:13:20, Ryan Sleevi wrote:
> On 2015/05/12 19:36:30, Yoav Weiss wrote:
> > I don't have any docs. Ryan?
> 
> There are none. Nor is it described as part of the Web Platform. That's why I
> have such an issue with Privacy Mode as a concept, let alone an
implementation.
> 
> > What I know about privacy-mode connections is that they are needed when
> fetching
> > resources in anonymous CORS mode 
> > (and as you say, cannot be shared with non-anonymous fetches), since
otherwise
> > they may expose credentials.
> 
> Which is somewhat vague. Let's pretend we didn't have privacy mode at all,
what
> would we need or rationally explain in the Web Platform?
> 
> The only point in which I can see us needing to use different connections is
for
> situations where we used an HTTP/1.1 violating auth method - aka NTLM &
> Negotiate. In this case, re-using a connection may confer ambient authority
onto
> subsequent requests.
> 
> Outside of that, what is there? As discussed rather extensively in CORS,
> restricting client certificates is something that doesn't really make sense
(in
> that the attacker can't mount a dictionary attack via URL parameters as they
> presumably could via XHR + HTTP auth methods). TLS sessions aren't
fundamentally
> an issue. TLS Channel IDs shouldn't confer ambient authority, and if they do,
> it's a broken use of Channel ID.
> 
> So how much of that matters for preconnect? I'd argue zero, since it's about
> connection establishment and that should be independent of any
server-initiated
> HTTP auth scenarios.

So, you're saying that CORS anonymous fetches should not be done over
private-mode connections?
If so, can we simply change that in the implementation and solve preconnect's
problem with anonymous fetches?

Also - Gecko seem to have run into exactly the same problem. We'd probably need
to reach some agreement with them that this "crossorigin-less" path is the way
to go.

[Ryan Sleevi, 2015-05-13 06:44:01]

On 2015/05/13 06:29:43, Yoav Weiss wrote:
> On 2015/05/12 22:13:20, Ryan Sleevi wrote:
> So, you're saying that CORS anonymous fetches should not be done over
> private-mode connections?

privacy-mode as implemented has nothing to do with CORS or Auth, at least, not
intentionally. private-mode is an artifact of how TLS channel ID was implemented
(somewhat brokenly, sadly), and trying to prevent linkability of connections via
reuse.

That is, prior to channel ID, we had no notion of privacy-mode. First and
third-party connections shared the same socket pools, but the latter would not
send cookies. It also meant for CORS/Anon connections, if there was ambient HTTP
auth, it would be carried through. But we weren't trying to solve that (...
although we should have)

Instead, we wanted to prevent linkability of connections to third-parties, a
noble goal, but not entirely realistic for a vast number of reasons. Still,
because TLS Channel IDs are effectively cookies, and they happen at a connection
level, we had to distinguish the connections so that a "Don't send cookies"
request wouldn't be linked to a "sent channel ID" socket.

Now, we still need to maintain different pools, or at least, we need to not
reuse a connection that we've exchanged connection-oriented auth for. But that
applies at the Request level, whereas preconnect applies at the socket level,
and that's why I think privacy mode for socket pools (based on the idea of
linkability) is unsound at worst, and unexplained in the platform at best.

If we ripped out privacy-mode, we would likely regress CORS by reusing sockets
used to convey connection-oriented auth. But we could solve that differently.

> If so, can we simply change that in the implementation and solve preconnect's
> problem with anonymous fetches?
> 
> Also - Gecko seem to have run into exactly the same problem. We'd probably
need
> to reach some agreement with them that this "crossorigin-less" path is the way
> to go.

It is unlikely Gecko has run into this problem as I described, as it was
intrinsic to channel ID being a TLS-layer operation. However, they may have run
into it because they wanted to try for unsinkability (e.g. for Tor Browser
Bundle users, as incorrigible and incomprehensible a security model as always),
or they may have needed to solve it for auth. But what we describe for
preconnect may and probably is semi-distinct.

[mef, 2015-05-13 13:38:42]

On 2015/05/13 06:44:01, Ryan Sleevi wrote:
> On 2015/05/13 06:29:43, Yoav Weiss wrote:
> > On 2015/05/12 22:13:20, Ryan Sleevi wrote:
> > So, you're saying that CORS anonymous fetches should not be done over
> > private-mode connections?
> 
> privacy-mode as implemented has nothing to do with CORS or Auth, at least, not
> intentionally. private-mode is an artifact of how TLS channel ID was
implemented
> (somewhat brokenly, sadly), and trying to prevent linkability of connections
via
> reuse.
> 
> That is, prior to channel ID, we had no notion of privacy-mode. First and
> third-party connections shared the same socket pools, but the latter would not
> send cookies. It also meant for CORS/Anon connections, if there was ambient
HTTP
> auth, it would be carried through. But we weren't trying to solve that (...
> although we should have)
> 
> Instead, we wanted to prevent linkability of connections to third-parties, a
> noble goal, but not entirely realistic for a vast number of reasons. Still,
> because TLS Channel IDs are effectively cookies, and they happen at a
connection
> level, we had to distinguish the connections so that a "Don't send cookies"
> request wouldn't be linked to a "sent channel ID" socket.
> 
> Now, we still need to maintain different pools, or at least, we need to not
> reuse a connection that we've exchanged connection-oriented auth for. But that
> applies at the Request level, whereas preconnect applies at the socket level,
> and that's why I think privacy mode for socket pools (based on the idea of
> linkability) is unsound at worst, and unexplained in the platform at best.
> 
> If we ripped out privacy-mode, we would likely regress CORS by reusing sockets
> used to convey connection-oriented auth. But we could solve that differently.
> 
> > If so, can we simply change that in the implementation and solve
preconnect's
> > problem with anonymous fetches?
> > 
> > Also - Gecko seem to have run into exactly the same problem. We'd probably
> need
> > to reach some agreement with them that this "crossorigin-less" path is the
way
> > to go.
> 
> It is unlikely Gecko has run into this problem as I described, as it was
> intrinsic to channel ID being a TLS-layer operation. However, they may have
run
> into it because they wanted to try for unsinkability (e.g. for Tor Browser
> Bundle users, as incorrigible and incomprehensible a security model as
always),
> or they may have needed to solve it for auth. But what we describe for
> preconnect may and probably is semi-distinct.

Is replacing Channel ID with Token Binding alleviates the need for privacy mode?

[cbentzel, 2015-05-13 13:45:05]

On 2015/05/13 13:38:42, mef wrote:
> On 2015/05/13 06:44:01, Ryan Sleevi wrote:
> > On 2015/05/13 06:29:43, Yoav Weiss wrote:
> > > On 2015/05/12 22:13:20, Ryan Sleevi wrote:
> > > So, you're saying that CORS anonymous fetches should not be done over
> > > private-mode connections?
> > 
> > privacy-mode as implemented has nothing to do with CORS or Auth, at least,
not
> > intentionally. private-mode is an artifact of how TLS channel ID was
> implemented
> > (somewhat brokenly, sadly), and trying to prevent linkability of connections
> via
> > reuse.
> > 
> > That is, prior to channel ID, we had no notion of privacy-mode. First and
> > third-party connections shared the same socket pools, but the latter would
not
> > send cookies. It also meant for CORS/Anon connections, if there was ambient
> HTTP
> > auth, it would be carried through. But we weren't trying to solve that (...
> > although we should have)
> > 
> > Instead, we wanted to prevent linkability of connections to third-parties, a
> > noble goal, but not entirely realistic for a vast number of reasons. Still,
> > because TLS Channel IDs are effectively cookies, and they happen at a
> connection
> > level, we had to distinguish the connections so that a "Don't send cookies"
> > request wouldn't be linked to a "sent channel ID" socket.
> > 
> > Now, we still need to maintain different pools, or at least, we need to not
> > reuse a connection that we've exchanged connection-oriented auth for. But
that
> > applies at the Request level, whereas preconnect applies at the socket
level,
> > and that's why I think privacy mode for socket pools (based on the idea of
> > linkability) is unsound at worst, and unexplained in the platform at best.
> > 
> > If we ripped out privacy-mode, we would likely regress CORS by reusing
sockets
> > used to convey connection-oriented auth. But we could solve that
differently.
> > 
> > > If so, can we simply change that in the implementation and solve
> preconnect's
> > > problem with anonymous fetches?
> > > 
> > > Also - Gecko seem to have run into exactly the same problem. We'd probably
> > need
> > > to reach some agreement with them that this "crossorigin-less" path is the
> way
> > > to go.
> > 
> > It is unlikely Gecko has run into this problem as I described, as it was
> > intrinsic to channel ID being a TLS-layer operation. However, they may have
> run
> > into it because they wanted to try for unsinkability (e.g. for Tor Browser
> > Bundle users, as incorrigible and incomprehensible a security model as
> always),
> > or they may have needed to solve it for auth. But what we describe for
> > preconnect may and probably is semi-distinct.
> 
> Is replacing Channel ID with Token Binding alleviates the need for privacy
mode?

It may. I'd rather have that discussion outside of this CL though. It also won't
be ready anytime soon.

[chromium-reviews, 2015-05-13 16:25:40]

Sorry, I was OOTO yesterday (and today). The CORS anonymous requests are
part of the fetch spec and require that identifying information not
accompany the requests. This includes cookies, certs and auth.
Implementation wise this requires a separate connection otherwise the data
could just be mined from previous requests.

In Chrome the anonymous requests are plumbed into the privacy mode pool.
Not sure if the pm pools also have other uses (incognito?) but if they do
that could be a problem. That said, it doesn't really change the situation
wrt anonymous requests needing separate connections.

The main battle right now is if fonts should really need CORS anonymous
behavior because otherwise we could just not support preconnect for
anonymous resources.

I'll send the relevant links when I get back to a computer.

On Wednesday, May 13, 2015, <cbentzel@chromium.org> wrote:

> On 2015/05/13 13:38:42, mef wrote:
>
>> On 2015/05/13 06:44:01, Ryan Sleevi wrote:
>> > On 2015/05/13 06:29:43, Yoav Weiss wrote:
>> > > On 2015/05/12 22:13:20, Ryan Sleevi wrote:
>> > > So, you're saying that CORS anonymous fetches should not be done over
>> > > private-mode connections?
>> >
>> > privacy-mode as implemented has nothing to do with CORS or Auth, at
>> least,
>>
> not
>
>> > intentionally. private-mode is an artifact of how TLS channel ID was
>> implemented
>> > (somewhat brokenly, sadly), and trying to prevent linkability of
>> connections
>> via
>> > reuse.
>> >
>> > That is, prior to channel ID, we had no notion of privacy-mode. First
>> and
>> > third-party connections shared the same socket pools, but the latter
>> would
>>
> not
>
>> > send cookies. It also meant for CORS/Anon connections, if there was
>> ambient
>> HTTP
>> > auth, it would be carried through. But we weren't trying to solve that
>> (...
>> > although we should have)
>> >
>> > Instead, we wanted to prevent linkability of connections to
>> third-parties, a
>> > noble goal, but not entirely realistic for a vast number of reasons.
>> Still,
>> > because TLS Channel IDs are effectively cookies, and they happen at a
>> connection
>> > level, we had to distinguish the connections so that a "Don't send
>> cookies"
>> > request wouldn't be linked to a "sent channel ID" socket.
>> >
>> > Now, we still need to maintain different pools, or at least, we need to
>> not
>> > reuse a connection that we've exchanged connection-oriented auth for.
>> But
>>
> that
>
>> > applies at the Request level, whereas preconnect applies at the socket
>>
> level,
>
>> > and that's why I think privacy mode for socket pools (based on the idea
>> of
>> > linkability) is unsound at worst, and unexplained in the platform at
>> best.
>> >
>> > If we ripped out privacy-mode, we would likely regress CORS by reusing
>>
> sockets
>
>> > used to convey connection-oriented auth. But we could solve that
>>
> differently.
>
>> >
>> > > If so, can we simply change that in the implementation and solve
>> preconnect's
>> > > problem with anonymous fetches?
>> > >
>> > > Also - Gecko seem to have run into exactly the same problem. We'd
>> probably
>> > need
>> > > to reach some agreement with them that this "crossorigin-less" path
>> is the
>> way
>> > > to go.
>> >
>> > It is unlikely Gecko has run into this problem as I described, as it was
>> > intrinsic to channel ID being a TLS-layer operation. However, they may
>> have
>> run
>> > into it because they wanted to try for unsinkability (e.g. for Tor
>> Browser
>> > Bundle users, as incorrigible and incomprehensible a security model as
>> always),
>> > or they may have needed to solve it for auth. But what we describe for
>> > preconnect may and probably is semi-distinct.
>>
>
>  Is replacing Channel ID with Token Binding alleviates the need for privacy
>>
> mode?
>
> It may. I'd rather have that discussion outside of this CL though. It also
> won't
> be ready anytime soon.
>
> https://codereview.chromium.org/1131293004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[igrigorik, 2015-05-13 16:39:16]

> Now, we still need to maintain different pools, or at least, we need to not
> reuse a connection that we've exchanged connection-oriented auth for. But that
> applies at the Request level, whereas preconnect applies at the socket level,
> and that's why I think privacy mode for socket pools (based on the idea of
> linkability) is unsound at worst, and unexplained in the platform at best.

I'd love to see a rational story around all this. That said, given that nobody
in particular is owning this story, and from what I understand so far, it's a
non-trivial exercise to bring sense and order into all this (e.g. platform
explainer, update HTML spec, propagate it through all the other APIs, and so
on), I don't think we should block this discussion on solving the architectural
mess... which, even with concentrated effort would probably take several years
to bikeshed and implement.

As of today, "crossorigin" attribute is sufficient to address developers needs
across various platforms, and developers need preconnect/prefetch/preload to
build effective applications. If, in the future, we cleanup the mess and
"crossorigin" becomes a no-op, then so much the better: all the implementations
will continue to work, they'll just work better because we can improve socket
reuse.

Can we agree to unblock this, and in parallel, pursue the larger architectural
discussion?

[igrigorik, 2015-05-13 16:43:36]

On 2015/05/13 16:25:40, chromium-reviews wrote:
> The main battle right now is if fonts should really need CORS anonymous
> behavior because otherwise we could just not support preconnect for
> anonymous resources.

Note that we also have other cases that require it:
https://developer.mozilla.org/en-US/docs/Web/HTML/CORS_settings_attributes. A
such, while we should definitely pursue the font conversation, I don't think we
can say that "fixing fonts" will eliminate the need for crossorigin+preconnect.

[Ryan Sleevi, 2015-05-13 16:44:21]

On 2015/05/13 16:39:16, igrigorik wrote:
> > Now, we still need to maintain different pools, or at least, we need to not
> > reuse a connection that we've exchanged connection-oriented auth for. But
that
> > applies at the Request level, whereas preconnect applies at the socket
level,
> > and that's why I think privacy mode for socket pools (based on the idea of
> > linkability) is unsound at worst, and unexplained in the platform at best.
> 
> I'd love to see a rational story around all this. That said, given that nobody
> in particular is owning this story, and from what I understand so far, it's a
> non-trivial exercise to bring sense and order into all this (e.g. platform
> explainer, update HTML spec, propagate it through all the other APIs, and so
> on), I don't think we should block this discussion on solving the
architectural
> mess... which, even with concentrated effort would probably take several years
> to bikeshed and implement.
> 

I am...neither happy not in agreement with this summary of outcomes.
 
> Can we agree to unblock this, and in parallel, pursue the larger architectural
> discussion?

I think that's bad for Chrome and bad for the web. We have a forcing function of
a new shiny that forces this discussion. If we just sweep it under the rug by
making developers feel the pain, then we are not being responsible stewards of
the web platform.

We need to pay this cost, and need to have a track for it. I'm happy to
escalate, because so far, we've been able to hide the pain from developers, but
I do not think we can justify introducing known-broken cruft into the web
platform just because we want our shiny new toys sooner.

[Ryan Sleevi, 2015-05-13 16:49:02]

On 2015/05/13 16:25:40, chromium-reviews wrote:
> Sorry, I was OOTO yesterday (and today). The CORS anonymous requests are
> part of the fetch spec and require that identifying information not
> accompany the requests. This includes cookies, certs and auth.
> Implementation wise this requires a separate connection otherwise the data
> could just be mined from previous requests.

No, this is not true. See the Fetch spec and discussions.

This has nothing to do with linkability. The only thing that Fetch/CORS were
actually intended to do is to prevent dictionary level attacks. Since the XHR
attacker could control the creds, this is why we got the creds separation.

Cookies are per-request and don't imbue connection-oriented semantics (except
for spec violating servers). The only blessed spec violation is NTLM/Negotiate
auth, but that is a separate matter. Client certs imbue some degree of ambient
auth, but their failure (now introduced with privacy mode) is actually different
than other UAs when combined with CORS, and there is no rational story for why
CORS calls them credentials.

> In Chrome the anonymous requests are plumbed into the privacy mode pool.
> Not sure if the pm pools also have other uses (incognito?) but if they do
> that could be a problem. That said, it doesn't really change the situation
> wrt anonymous requests needing separate connections.

But it does, rather substantially.

[Yoav Weiss, 2015-05-13 16:59:22]

On 2015/05/13 16:44:21, Ryan Sleevi wrote:
> On 2015/05/13 16:39:16, igrigorik wrote:
> > > Now, we still need to maintain different pools, or at least, we need to
not
> > > reuse a connection that we've exchanged connection-oriented auth for. But
> that
> > > applies at the Request level, whereas preconnect applies at the socket
> level,
> > > and that's why I think privacy mode for socket pools (based on the idea of
> > > linkability) is unsound at worst, and unexplained in the platform at best.
> > 
> > I'd love to see a rational story around all this. That said, given that
nobody
> > in particular is owning this story, and from what I understand so far, it's
a
> > non-trivial exercise to bring sense and order into all this (e.g. platform
> > explainer, update HTML spec, propagate it through all the other APIs, and so
> > on), I don't think we should block this discussion on solving the
> architectural
> > mess... which, even with concentrated effort would probably take several
years
> > to bikeshed and implement.
> > 
> 
> I am...neither happy not in agreement with this summary of outcomes.
>  
> > Can we agree to unblock this, and in parallel, pursue the larger
architectural
> > discussion?
> 
> I think that's bad for Chrome and bad for the web. We have a forcing function
of
> a new shiny that forces this discussion. If we just sweep it under the rug by
> making developers feel the pain, then we are not being responsible stewards of
> the web platform.
> 
> We need to pay this cost, and need to have a track for it. I'm happy to
> escalate, because so far, we've been able to hide the pain from developers,
but
> I do not think we can justify introducing known-broken cruft into the web
> platform just because we want our shiny new toys sooner.

What is later? Our "shiny new toys" will enable better user experience, which
can significantly help the move to TLS, where long connection times to 3rd party
domains are still a real-life pain, which HTTP2 does not alleviate.
If resolving the architectural mess would mean keeping that pain around for a
few more years - I believe that would be an overall loss for Web developers and
for our users.

[Ryan Sleevi, 2015-05-13 18:29:51]

On 2015/05/13 16:59:22, Yoav Weiss wrote:
> What is later? Our "shiny new toys" will enable better user experience, which
> can significantly help the move to TLS, where long connection times to 3rd
party
> domains are still a real-life pain, which HTTP2 does not alleviate.
> If resolving the architectural mess would mean keeping that pain around for a
> few more years - I believe that would be an overall loss for Web developers
and
> for our users.

I can think of few Chrome features ever quantified in terms of years - the
deprecation of NPAPI and the requirement of secure origins for legacy powerful
features among them. So I think talking about "keeping pain around for a few
more years" is a bit hyperbolic, at best, and serves to damage the discussion.

The blunt answer is that it will take however long it takes someone to care
enough to fix the gradually accumulated mess. What motivates that fixing?
Altruism? I wish. Boredom? Maybe, but that's not sustainable. Wanting to get a
new feature in? Definitely.

So, I mean, it would take as long as it takes you and Ilya to take ownership of
the issues, do the due diligence, come up with a plan of action, convince
stakeholders that you'll be responsible enough to implement the plan and not
just abandon it when you get what you want (which has happened to me enough now,
even from Googlers, which is profoundly unfortunate, that I'm deeply skeptical),
and then execute. As Captain Planet liked to say, the power is yours ;)

[mmenke, 2015-06-05 16:05:49]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[Ryan Sleevi, 2015-06-10 19:30:42]

Ilya pinged me about this - I think there's a number of design and layering
issues that might be best worked through with Mike and Ilya first :) I've tried
to highlight them in my previous review.

As it stands, we'll probably end up needing this. Not for ambient authority
(even though TLS credentials do represent a form of that), but on nebulous
privacy grounds that seem unlikely to be resolved, so we'll just foist those on
to developers. That is, as Mike pushes for more of a first-party/third-party
distinction, either our socket pools will need to reflect that, or Mike will
need to stop pushing. Since Mike stopping anything is unlikely, might as well
support it.

But I mean, there's a lot here for layering to work through :)

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
chrome/browser/net/preconnect.cc:73: || isAnonymous)
So this is wrong. You should be using the |load_flags|.

It's unfortunate, because this ends up duplicating the |web_url_request_info|
logic (
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...
) but is the canonical 'correct' layering perspective.

That is, if cross-origin preconnect is requested, you should NOT set privacy
mode, but instead set DO_NOT_[SAVE/SEND]_COOKIES and DO_NOT_SEND_AUTH_DATA

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
File components/network_hints/common/network_hints_messages.h (right):

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
components/network_hints/common/network_hints_messages.h:13: #include
"third_party/WebKit/public/platform/WebCrossOriginAttribute.h"
Unused

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
components/network_hints/renderer/renderer_preconnect.cc:28: bool isAnonymous =
(crossOrigin == blink::CrossOriginAttributeAnonymous);
This isn't really correct, is it? You should be gating it on whether or not the
CORS credentials flag is set, which happens to be not set in anonymous mode.

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
File components/network_hints/renderer/renderer_preconnect.h (right):

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
components/network_hints/renderer/renderer_preconnect.h:26: // DNS prefetch
requests to the net stack.
This comment seems out of date :)

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
components/network_hints/renderer/renderer_preconnect.h:33: void
Preconnect(const GURL &url, blink::CrossOriginAttributeValue);
You shouldn't be depending on blink from components like this, AFAICT

[Yoav Weiss, 2015-06-11 12:12:42]

On 2015/06/10 19:30:42, Ryan Sleevi wrote:
> Ilya pinged me about this - I think there's a number of design and layering
> issues that might be best worked through with Mike and Ilya first :) I've
tried
> to highlight them in my previous review.
> 
> As it stands, we'll probably end up needing this. Not for ambient authority
> (even though TLS credentials do represent a form of that), but on nebulous
> privacy grounds that seem unlikely to be resolved, so we'll just foist those
on
> to developers. That is, as Mike pushes for more of a first-party/third-party
> distinction, either our socket pools will need to reflect that, or Mike will
> need to stop pushing. Since Mike stopping anything is unlikely, might as well
> support it.
> 
> But I mean, there's a lot here for layering to work through :)
> 
>
https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
> File chrome/browser/net/preconnect.cc (right):
> 
>
https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
> chrome/browser/net/preconnect.cc:73: || isAnonymous)
> So this is wrong. You should be using the |load_flags|.
> 
> It's unfortunate, because this ends up duplicating the |web_url_request_info|
> logic (
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...
> ) but is the canonical 'correct' layering perspective.
> 
> That is, if cross-origin preconnect is requested, you should NOT set privacy
> mode, but instead set DO_NOT_[SAVE/SEND]_COOKIES and DO_NOT_SEND_AUTH_DATA
> 
>
https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
> File components/network_hints/common/network_hints_messages.h (right):
> 
>
https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
> components/network_hints/common/network_hints_messages.h:13: #include
> "third_party/WebKit/public/platform/WebCrossOriginAttribute.h"
> Unused
> 
>
https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
> File components/network_hints/renderer/renderer_preconnect.cc (right):
> 
>
https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
> components/network_hints/renderer/renderer_preconnect.cc:28: bool isAnonymous
=
> (crossOrigin == blink::CrossOriginAttributeAnonymous);
> This isn't really correct, is it? You should be gating it on whether or not
the
> CORS credentials flag is set, which happens to be not set in anonymous mode.

I'm not sure I understand what you mean here. 
Did you mean that also the state where crossorigin is specified without a value
should be considered anonymous?
If so, that's taken care of at
third_party/WebKit/Source/core/html/CrossOriginAttribute.cpp

> 
>
https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
> File components/network_hints/renderer/renderer_preconnect.h (right):
> 
>
https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
> components/network_hints/renderer/renderer_preconnect.h:26: // DNS prefetch
> requests to the net stack.
> This comment seems out of date :)

yup :)

> 
>
https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
> components/network_hints/renderer/renderer_preconnect.h:33: void
> Preconnect(const GURL &url, blink::CrossOriginAttributeValue);
> You shouldn't be depending on blink from components like this, AFAICT

Hmm, components/network_hints/renderer/prescient_networking_dispatcher seems to
rely on blink, regardless of my changes...

[Yoav Weiss, 2015-06-11 12:55:14]

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
chrome/browser/net/preconnect.cc:73: || isAnonymous)
On 2015/06/10 19:30:41, Ryan Sleevi wrote:
> So this is wrong. You should be using the |load_flags|.
> 
> It's unfortunate, because this ends up duplicating the |web_url_request_info|
> logic (
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...
> ) but is the canonical 'correct' layering perspective.
> 
> That is, if cross-origin preconnect is requested, you should NOT set privacy
> mode, but instead set DO_NOT_[SAVE/SEND]_COOKIES and DO_NOT_SEND_AUTH_DATA

hmm, setting request_info.load_flags to the above flags results in preconnects
not being sent at all... :/

[Yoav Weiss, 2015-06-11 13:17:58]

On 2015/06/11 12:55:14, Yoav Weiss wrote:
>
https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
> File chrome/browser/net/preconnect.cc (right):
> 
>
https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...
> chrome/browser/net/preconnect.cc:73: || isAnonymous)
> On 2015/06/10 19:30:41, Ryan Sleevi wrote:
> > So this is wrong. You should be using the |load_flags|.
> > 
> > It's unfortunate, because this ends up duplicating the
|web_url_request_info|
> > logic (
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...
> > ) but is the canonical 'correct' layering perspective.
> > 
> > That is, if cross-origin preconnect is requested, you should NOT set privacy
> > mode, but instead set DO_NOT_[SAVE/SEND]_COOKIES and DO_NOT_SEND_AUTH_DATA
> 
> hmm, setting request_info.load_flags to the above flags results in preconnects
> not being sent at all... :/

Correction - I do see preconnects, but just as without 'crossorigin', the
connection is not being reused.

[Ryan Sleevi, 2015-06-11 18:30:09]

Your commenting style makes it very hard to both read and to respond to your
feedback. It'd be appreciated if you could try to make sure the replies go to
the right area.

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
components/network_hints/renderer/renderer_preconnect.cc:28: bool isAnonymous =
(crossOrigin == blink::CrossOriginAttributeAnonymous);
Yoav wrote:
> I'm not sure I understand what you mean here. 
> Did you mean that also the state where crossorigin is specified without a
value
> should be considered anonymous?
> If so, that's taken care of at
> third_party/WebKit/Source/core/html/CrossOriginAttribute.cpp

No, I mean this code should not at all be dealing with CrossOriginAttribute.
That's not the pivot point for the API, as reflected in Fetch.

https://fetch.spec.whatwg.org/#http-network-fetch

"HTTP requests should be grouped based on the credentials flag. HTTP requests
with the credentials flag set should not share anything about the connection or
TLS particulars (such as session identifiers) with HTTP requests that have the
credentials flag unset. In other words, ambient authority should not leak across
this grouping."

The CrossOriginAttribute is just Blink's way of tracking that Fetch attribute,
but the algorithm in 4.3 establishes

"Let credentials flag be set if either request's credentials mode is "include",
or request's credentials mode is "same-origin" and the CORS flag is unset, and
unset otherwise."

I'm saying that the distinction about "isAnonymous" is wrong. For preconnect (or
for prefetch, for that matter), the "which pool to go to" is keyed on the
credentials flag.

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
File components/network_hints/renderer/renderer_preconnect.h (right):

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
components/network_hints/renderer/renderer_preconnect.h:33: void
Preconnect(const GURL &url, blink::CrossOriginAttributeValue);
Yoav wrote:
> Hmm, components/network_hints/renderer/prescient_networking_dispatcher seems
to
> rely on blink, regardless of my changes...

It looks like this was done in
https://chromium.googlesource.com/chromium/src/+/e49c69208e7da19a0b2a3ac9f2fd...
, and that should have been flagged then.

Just because someone else is doing it doesn't make it right :/

[Ryan Sleevi, 2015-06-11 18:33:12]

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
File components/network_hints/renderer/renderer_preconnect.h (right):

https://codereview.chromium.org/1131293004/diff/40001/components/network_hint...
components/network_hints/renderer/renderer_preconnect.h:33: void
Preconnect(const GURL &url, blink::CrossOriginAttributeValue);
On 2015/06/11 18:30:09, Ryan Sleevi wrote:
> Just because someone else is doing it doesn't make it right :/

Just to be clear, this is the least of the concerns with the CL.

[Ryan Sleevi, 2015-06-11 22:20:16]

https://codereview.chromium.org/1131293004/diff/100001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/100001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:74: || !is_credentials_flag_set)
To be clear: You're going to need to dig in to why this doesn't work for your
test, what the root cause is, and what may need to change.

Using this bool to set privacy_mode_enabled, even if it causes the correct
behaviour, isn't consistent with the API contract. You shouldn't be setting this
yourself - it's //net's responsibility to do that based on the cookie settings
and load flags (fwiw, it's here -
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
)

If I were a betting man, it's because HttpStreamFactory::PreconnectStreams
likely isn't aware of the UrlRequestHttpJob (which builds atop the
HttpStreamFactory, and is thus a higher layer), and as a result, is bypassing
the code I just linked. Because of this, even though you 'should' be in privacy
mode, you're not.

The existing lines of this file (lines 71/72) of this are weird, because they're
violating the layering of the UrlRequestHttpInfo.

I'll have to think about this and talk it over with some folks to see what the
thoughts are. It's possible we may just say "whatever" and let this code
through, or it may be that we say "Push this down a layer first" as part of the
'leave the code cleaner'.

My own instinct is that if we're going to properly enshrine things, what we
probably want to do is push this logic into PreconnectStreams (really, the
HttpStreamFactory), which means that it needs to touch the NetworkDelegate (it
already can), and would need both |url| and |first_party_for_cookies| URLs
pushed into it (the same as UrlRequestHttpJob does, ish).

Just making sure to document the thinking here

https://codereview.chromium.org/1131293004/diff/100001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.h (right):

https://codereview.chromium.org/1131293004/diff/100001/chrome/browser/net/pre...
chrome/browser/net/preconnect.h:38: bool is_credentials_flag_set);
This is mostly a naming nit, but I'd like to get Mike's perspective here.

Historically, the naming has eschewed verbs like "is" in the naming of bools

bool credentials_flag_set

Alternatively, this might make sense to rename "send_credentials" or
"allow_credentials" (thinking purely in terms of 4.3/4.4 of the Fetch spec and
how //net ultimately perceives it)

https://codereview.chromium.org/1131293004/diff/100001/chrome/browser/net/pre...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/1131293004/diff/100001/chrome/browser/net/pre...
chrome/browser/net/predictor.h:244: // to the IO thread if necessary.
Should update documentation for these methods to reflect parameters.

https://codereview.chromium.org/1131293004/diff/100001/chrome/browser/net/pre...
chrome/browser/net/predictor.h:247: bool is_credentials_flag_set = true);
STYLE: Still with the defaults here; can't do this in Chrome code (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...
)

https://codereview.chromium.org/1131293004/diff/100001/chrome/browser/net/pre...
chrome/browser/net/predictor.h:247: bool is_credentials_flag_set = true);
NAMING: Same concerns re: is_

https://codereview.chromium.org/1131293004/diff/100001/components/network_hin...
File components/network_hints/renderer/prescient_networking_dispatcher.cc
(right):

https://codereview.chromium.org/1131293004/diff/100001/components/network_hin...
components/network_hints/renderer/prescient_networking_dispatcher.cc:31:
(crossOrigin != blink::CrossOriginAttributeAnonymous);
I feel like this logic should live in the //third_party/Blink layer, not in the
//components layer.

That is, for all possible implementations of preconnect(), the Web Platform
(namely, the Fetch spec) dictates what should happen. Historically, we've kept
those checks & behaviours in Blink-code.

That is, I'd expect that
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
handles examining all of the flags and converts that into an "HTTP network
fetch" per 4.5 of https://fetch.spec.whatwg.org/#http-network-fetch

('using request with an optional credentials flag') 

I also suspect that at some point, the logic for how rel=preconnect will need to
be described in terms of Fetch, which is why I filed
https://github.com/w3c/resource-hints/issues/34 to make this clearer.

https://codereview.chromium.org/1131293004/diff/100001/components/network_hin...
File components/network_hints/renderer/prescient_networking_dispatcher.h
(right):

https://codereview.chromium.org/1131293004/diff/100001/components/network_hin...
components/network_hints/renderer/prescient_networking_dispatcher.h:24: const
blink::CrossOriginAttributeValue crossOrigin) override;
STYLE: Shouldn't this follow Chrome style?

Mike would know.

[Yoav Weiss, 2015-06-12 15:53:03]

OK, I believe I now addressed everything besides the testing concern. Please let
me know if that's not the case, or if there's a better way to do the
client_socket_pool_manager parts.

[Ryan Sleevi, 2015-06-12 21:36:00]

Mostly style issues this pass, which is good!

Matt and David are closest to the test server right now and have been cleaning
up usages of it, and can suggest how to use it with the SpawnedTestServer in a
safe way. As well as how to avoid the NetLog.

https://codereview.chromium.org/1131293004/diff/180001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/180001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:79: net::LOAD_DO_NOT_SEND_AUTH_DATA;
Don't forget to "git cl format" :) This doesn't adhere to the style guide

https://codereview.chromium.org/1131293004/diff/180001/chrome/browser/net/pre...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1131293004/diff/180001/chrome/browser/net/pre...
chrome/browser/net/predictor.cc:89: const bool
Predictor::kAllowCredentialsOnPreconnect = true;
I'm not sure I understand why this is a global static?

It seems like it could just be file-local.

https://codereview.chromium.org/1131293004/diff/180001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/1131293004/diff/180001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_render_message_filter.cc:126: int count) {
STYLE: run git cl format

https://codereview.chromium.org/1131293004/diff/180001/components/network_hin...
File components/network_hints/renderer/prescient_networking_dispatcher.cc
(right):

https://codereview.chromium.org/1131293004/diff/180001/components/network_hin...
components/network_hints/renderer/prescient_networking_dispatcher.cc:33: const
bool allow_credentials) {
style

https://codereview.chromium.org/1131293004/diff/180001/components/network_hin...
File components/network_hints/renderer/prescient_networking_dispatcher.h
(right):

https://codereview.chromium.org/1131293004/diff/180001/components/network_hin...
components/network_hints/renderer/prescient_networking_dispatcher.h:26: const
bool allow_credentials) override;
style

https://codereview.chromium.org/1131293004/diff/180001/components/network_hin...
File components/network_hints/renderer/renderer_preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/180001/components/network_hin...
components/network_hints/renderer/renderer_preconnect.cc:24: bool
allow_credentials) {
style

(In case it's not clear,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl...
specifically - when having a function definition/declaration which needs
wrapping, either align to the ( or, if that's still too long, align *all* parms
- including first one - to a four space indent)

https://codereview.chromium.org/1131293004/diff/180001/net/socket/client_sock...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1131293004/diff/180001/net/socket/client_sock...
net/socket/client_socket_pool_manager.cc:221: (privacy_mode ==
PRIVACY_MODE_ENABLED);
It'd be good to add a comment here explaining why each of these conditions is
like this.

[mmenke, 2015-06-12 21:43:45]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2015-06-12 21:43:46]

Adding myself as reviewer, hope to get to it Monday.

[mmenke, 2015-06-15 17:40:10]

The main problems here are that connections are had to monitor, and that the
server can't know if a connection is in privacy mode until we actually use it.


We could make the embedded test server support connection tracking, invoking a
callback on each connect.  Then we trigger an anonymous preconnect, and wait to
see it connect.  Then we do something that requires an anonymous connection, and
make sure we don't establish a new connection to get it.

This works for non-anonymous case, too, but to do so robustly, should probably
use a bogus subdomain with a separate test server for the preconnect, so things
like favicon requests don't confuse the case.

We could also throw in a cookie in both tests that should/should not be sent to
the server depending on privacy mode, out of paranoia, to make sure that not
only are we using the socket as we should be, we're also appropriately applying
the privacy mode options.

In both tests, we could include a request that shouldn't use the preconnect
socket before the one that should, if we wanted, and make sure that results in
creating a new socket, rather than using the preconnected one, but I don't think
that gets us much.


There are other options, but I'm having trouble coming up with anything that
isn't painfully convoluted.

[Yoav Weiss, 2015-07-09 08:26:54]

I've re-written the test in terms of the new Preconnect tests. Ryan/mmenke -
PTAL?

[mmenke, 2015-07-09 15:55:56]

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:71: request_info.privacy_mode =
net::PRIVACY_MODE_ENABLED;
Wait...Privacy mode is completely separate from allow_credentials?  I thought
that was the whole point of this CL.

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:76: net::LOAD_DO_NOT_SEND_AUTH_DATA;
nit:  Use braces when the body of an if takes up more than one line.

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/predictor.cc:91: static bool kAllowCredentialsOnPreconnect =
true;
This is a confusing name.  "kAllowCredentialsOnPreconnect" sounds like it's the
constant that *means* allow credentials, not a bool representing the default
behavior.

Maybe "kAllowCredentialsOnPreconnectByDefault"?

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:388: // and that that socket is
later used when fetching an anonymous resource.
nit: Fix line wrap (Weird to wrap at a comma, and leave all that whitespace)

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:404:
"test.woff2)');font.load();</script>";
Why would this test fail without the crossorigin on the preconnect?  Why does
the previous test not need it?

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:410: }  // namespace
chrome_browser_net
nit:  Blank line before end of namespace.

[Yoav Weiss, 2015-07-09 16:19:39]

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:71: request_info.privacy_mode =
net::PRIVACY_MODE_ENABLED;
On 2015/07/09 15:55:56, mmenke wrote:
> Wait...Privacy mode is completely separate from allow_credentials?  I thought
> that was the whole point of this CL.

Ryan suggested that it's better to separate them here, even if they are doing
the same thing (connecting a socket from the pm pool) further down the stack.

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:76: net::LOAD_DO_NOT_SEND_AUTH_DATA;
On 2015/07/09 15:55:56, mmenke wrote:
> nit:  Use braces when the body of an if takes up more than one line.

added

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/predictor.cc:91: static bool kAllowCredentialsOnPreconnect =
true;
On 2015/07/09 15:55:56, mmenke wrote:
> This is a confusing name.  "kAllowCredentialsOnPreconnect" sounds like it's
the
> constant that *means* allow credentials, not a bool representing the default
> behavior.
> 
> Maybe "kAllowCredentialsOnPreconnectByDefault"?

renamed

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:388: // and that that socket is
later used when fetching an anonymous resource.
On 2015/07/09 15:55:56, mmenke wrote:
> nit: Fix line wrap (Weird to wrap at a comma, and leave all that whitespace)

fixed

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:404:
"test.woff2)');font.load();</script>";
On 2015/07/09 15:55:56, mmenke wrote:
> Why would this test fail without the crossorigin on the preconnect?  Why does
> the previous test not need it?

Since this is a request for a font resource, which is fetch as an anonymous
resource, and therefore must be fetched using a connection from the pm pool.
For the previous test, it was for an image without a crossorigin attribute, so
it was a resource allowed to use credentials, and therefore must be fetched
using a connection from the non-pm pool.

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:410: }  // namespace
chrome_browser_net
On 2015/07/09 15:55:56, mmenke wrote:
> nit:  Blank line before end of namespace.

added

[mmenke, 2015-07-09 16:31:30]

Going to defer to Ryan on this.  The more I hear about this stuff, the less I
understand it.

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:71: request_info.privacy_mode =
net::PRIVACY_MODE_ENABLED;
On 2015/07/09 16:19:39, Yoav Weiss wrote:
> On 2015/07/09 15:55:56, mmenke wrote:
> > Wait...Privacy mode is completely separate from allow_credentials?  I
thought
> > that was the whole point of this CL.
> 
> Ryan suggested that it's better to separate them here, even if they are doing
> the same thing (connecting a socket from the pm pool) further down the stack.

I'm not really following that.  So if we have preconnects that are cross domain,
but don't have the crossorigin flag, they can still have allow_credentials to be
true?

So I guess PRIVACY_MODE_ENABLED does not implicitly attach those flags already?

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:404:
"test.woff2)');font.load();</script>";
On 2015/07/09 16:19:39, Yoav Weiss wrote:
> On 2015/07/09 15:55:56, mmenke wrote:
> > Why would this test fail without the crossorigin on the preconnect?  Why
does
> > the previous test not need it?
> 
> Since this is a request for a font resource, which is fetch as an anonymous
> resource, and therefore must be fetched using a connection from the pm pool.
> For the previous test, it was for an image without a crossorigin attribute, so
> it was a resource allowed to use credentials, and therefore must be fetched
> using a connection from the non-pm pool.

So how does one request make it to the PM pool, and the other make it to the
non-PM pool?  I guess there's magic in blink that give them different
first_party_for_cookies values?

If there's a crossorigin attribute for images, may make sense to just use an
image set with it here in this test, to make it more like the first
test...Though I suppose then we could silently regress if both cross origin
attributes broke at once.  Unclear how likely that is to happen and still have
font loading correct know its an anonymous request, though.

[Yoav Weiss, 2015-07-09 16:38:33]

On 2015/07/09 16:31:30, mmenke wrote:
> Going to defer to Ryan on this.  The more I hear about this stuff, the less I
> understand it.
> 
>
https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
> File chrome/browser/net/preconnect.cc (right):
> 
>
https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
> chrome/browser/net/preconnect.cc:71: request_info.privacy_mode =
> net::PRIVACY_MODE_ENABLED;
> On 2015/07/09 16:19:39, Yoav Weiss wrote:
> > On 2015/07/09 15:55:56, mmenke wrote:
> > > Wait...Privacy mode is completely separate from allow_credentials?  I
> thought
> > > that was the whole point of this CL.
> > 
> > Ryan suggested that it's better to separate them here, even if they are
doing
> > the same thing (connecting a socket from the pm pool) further down the
stack.
> 
> I'm not really following that.  So if we have preconnects that are cross
domain,
> but don't have the crossorigin flag, they can still have allow_credentials to
be
> true?

Yes. That's how e.g. cross-domain images and scripts are fetched.

> 
> So I guess PRIVACY_MODE_ENABLED does not implicitly attach those flags
already?

See https://codereview.chromium.org/1131293004/patch/260001/270014 for how all
of them map into the same thing lower down the stack.

> 
>
https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
> File chrome/browser/net/predictor_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
> chrome/browser/net/predictor_browsertest.cc:404:
> "test.woff2)');font.load();</script>";
> On 2015/07/09 16:19:39, Yoav Weiss wrote:
> > On 2015/07/09 15:55:56, mmenke wrote:
> > > Why would this test fail without the crossorigin on the preconnect?  Why
> does
> > > the previous test not need it?
> > 
> > Since this is a request for a font resource, which is fetch as an anonymous
> > resource, and therefore must be fetched using a connection from the pm pool.
> > For the previous test, it was for an image without a crossorigin attribute,
so
> > it was a resource allowed to use credentials, and therefore must be fetched
> > using a connection from the non-pm pool.
> 
> So how does one request make it to the PM pool, and the other make it to the
> non-PM pool?  I guess there's magic in blink that give them different
> first_party_for_cookies values?

I'm not sure, tbh.

> 
> If there's a crossorigin attribute for images, may make sense to just use an
> image set with it here in this test, to make it more like the first
> test...Though I suppose then we could silently regress if both cross origin
> attributes broke at once.  Unclear how likely that is to happen and still have
> font loading correct know its an anonymous request, though.

Since fonts are the more common case, I preferred to test them, but I can change
that to a crossorigin image, if that's better.

[Yoav Weiss, 2015-07-10 09:23:08]

I added a couple more tests and fixed a related test bug.

[Ryan Sleevi, 2015-07-10 09:34:22]

(Haven't reviewed yet, responding to some of Matt's questions/concerns)

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/240001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:71: request_info.privacy_mode =
net::PRIVACY_MODE_ENABLED;
On 2015/07/09 16:19:39, Yoav Weiss wrote:
> On 2015/07/09 15:55:56, mmenke wrote:
> > Wait...Privacy mode is completely separate from allow_credentials?  I
thought
> > that was the whole point of this CL.
> 
> Ryan suggested that it's better to separate them here, even if they are doing
> the same thing (connecting a socket from the pm pool) further down the stack.

It does, but it's a layering thing. See comment below (or above, depending on
how codereview does this

Right, just to refresh everyone on the "state of things"

See this summary of the state here -
https://groups.google.com/a/chromium.org/d/msg/net-dev/t1o3rGqDQMw/4Hwb0xc8__YJ

The early comments on this CL were prior to working through this issue with the
WebPerf WG and discussing with Ilya & co the goals for this, with the end
conclusion being that we agree there needs to be separate pools for first/third
party contexts (on the basis of chrome-privacy's goals for non-linkability), and
as such, privacy mode is (unfortunately?) here to stay ( see
https://codereview.chromium.org/1131293004/#msg43 )

The result of this was the comment here -
https://codereview.chromium.org/1131293004/diff/40001/chrome/browser/net/prec...

In which we're talking about the layering of things. Because the preconnect
logic doesn't shunt through a WebURLRequest, we end up "duplicating" the logic
here that is expressed in
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...

On 2015/07/09 16:31:30, mmenke wrote:
> So I guess PRIVACY_MODE_ENABLED does not implicitly attach those flags
already?


That's what line 73-76 is representing - the same transformation done to
WebURLRequests when anonymous is set.

[Ryan Sleevi, 2015-07-10 09:46:46]

Overall direction and implementation are looking good, and I'd be totally happy
with Matt's LG at this point.

At this point, I'm doing the haggling over naming - I'm wondering whether this
should be surfaced as credentials_flag rather than allow_credentials through
many of these API calls, so that it's "clearer" (in the "clear as Mississippi
Mud" case) how this aligns to Fetch's
https://fetch.spec.whatwg.org/#http-network-or-cache-fetch "credentials flag"

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.h (right):

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
chrome/browser/net/preconnect.h:38: bool allow_credentials);
Wondering out loud (for Matt, mostly) - do we want this to be an enum?

like

chrome_browser_net {

enum CredentialsFlag {
  CREDENTIALS_FLAG_SET,
  CREDENTIALS_FLAG_UNSET,
};


Bikeshed on naming (INCLUDE_CREDENTIALS, etc) notwithstanding, my goal was to
see if it makes it clearer how this maps back to Fetch spec terminology (namely,
"credentials flag" as discussed in
https://fetch.spec.whatwg.org/#http-network-or-cache-fetch ) so that it can be
clear how this is used.

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
chrome/browser/net/predictor.cc:91: static bool
kAllowCredentialsOnPreconnectByDefault = true;
I'm still not sure why this static bool. When I read it, I keep thinking "Oh,
this is a static bool that may change in future releases" (the whole aspect of
'ByDefault'). Can you just confirm that this isn't - this is just is just
re-using the constant?

If so, does it make more sense to put these values directly in the calls (and
not use the static)? It's unclear whether or not there is a specific
relationship between the various places that use this - that is, if you change
one of these calls, do all the others need to change? Or is it a case by case
basis?

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:64: printf("Socket %d\n", socket);
*cough* :P

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:108: EXPECT_EQ(net::OK,
connection.GetPeerAddress(&address));
Wait, what? Why this change?

Connections are a tuple of
(listener ip, listener port, peer ip, peer port)

Can you explain why this change is correct compared to GetLocal, especially when
we may have situations where peer ip is different, but peer port is the same?

[Yoav Weiss, 2015-07-10 10:03:40]

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
chrome/browser/net/predictor.cc:91: static bool
kAllowCredentialsOnPreconnectByDefault = true;
On 2015/07/10 09:46:45, Ryan Sleevi (slow through 7-15 wrote:
> I'm still not sure why this static bool. When I read it, I keep thinking "Oh,
> this is a static bool that may change in future releases" (the whole aspect of
> 'ByDefault'). Can you just confirm that this isn't - this is just is just
> re-using the constant?

Yes, it is just about re-using the constant. 

> 
> If so, does it make more sense to put these values directly in the calls (and
> not use the static)? It's unclear whether or not there is a specific
> relationship between the various places that use this - that is, if you change
> one of these calls, do all the others need to change? Or is it a case by case
> basis?

They all simply call PreconnectUrl{,OnIOThread} which by default should allow
credentials.
That's not something that we want to change AFAIK, so I think a const/static is
more appropriate (since we don't like default parameter values :D)

But I can also add direct "true" values to the calls themselves, if you think
it's significantly better.

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:64: printf("Socket %d\n", socket);
On 2015/07/10 09:46:45, Ryan Sleevi (slow through 7-15 wrote:
> *cough* :P

Doh! I got accustomed to Blink's cl upload hooks yelling at me about printf
leftovers...

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:108: EXPECT_EQ(net::OK,
connection.GetPeerAddress(&address));
On 2015/07/10 09:46:45, Ryan Sleevi (slow through 7-15 wrote:
> Wait, what? Why this change?
> 
> Connections are a tuple of
> (listener ip, listener port, peer ip, peer port)
> 
> Can you explain why this change is correct compared to GetLocal, especially
when
> we may have situations where peer ip is different, but peer port is the same?

GetLocal gives me the port number of the local listening port, which doesn't
change between connections. What I wanted to keep track of is port numbers of
the peer, that do change between connections.
Since this is only used for testing, where the peer is localhost anyway, I
didn't think I need to add the IP into the mix.

[Ryan Sleevi, 2015-07-10 10:09:35]

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:108: EXPECT_EQ(net::OK,
connection.GetPeerAddress(&address));
On 2015/07/10 10:03:40, Yoav Weiss wrote:
> Since this is only used for testing, where the peer is localhost anyway, I
> didn't think I need to add the IP into the mix.

Right, that's basically what I was assuming, but wanted to make sure without
accidentally making things more confusing ;)

I think what we want to do here is just document the assumptions this function
is making and (to an extent) the known limitations. We do want to be careful
assuming it will always be localhost connections - we've had tests break those
assumptions in the past.

private;
  static uint16_t GetPort(...) {
    // Get the remote port of the peer, since the local port will always be the
port the test server
    // is listening on. This isn't strictly correct - it's possible for multiple
peers to connect with
    // the same remote port but different remote IPs - but the tests here assume
that connections
    // to the test server (running on localhost) will always come from
localhost, and thus the
    // peer port is all thats needed to distinguish two connections. This also
would be problematic
    // if the OS reused ports, but that's not something to worry about for these
tests.
    net::IPEndpoint address;
  }


Basically, just spelling out what we've discussed as a known (and intentional)
limitation, and not just an "Oops, I never thought about that", so that if it
ever breaks, it's clear _why_ it breaks to future readers ("We didn't need a
pony, so we didn't buy a pony") ;)

[Yoav Weiss, 2015-07-10 10:27:12]

On 2015/07/10 10:09:35, Ryan Sleevi (slow through 7-15 wrote:
>
https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
> File chrome/browser/net/predictor_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1131293004/diff/280001/chrome/browser/net/pre...
> chrome/browser/net/predictor_browsertest.cc:108: EXPECT_EQ(net::OK,
> connection.GetPeerAddress(&address));
> On 2015/07/10 10:03:40, Yoav Weiss wrote:
> > Since this is only used for testing, where the peer is localhost anyway, I
> > didn't think I need to add the IP into the mix.
> 
> Right, that's basically what I was assuming, but wanted to make sure without
> accidentally making things more confusing ;)
> 
> I think what we want to do here is just document the assumptions this function
> is making and (to an extent) the known limitations. We do want to be careful
> assuming it will always be localhost connections - we've had tests break those
> assumptions in the past.
> 
> private;
>   static uint16_t GetPort(...) {
>     // Get the remote port of the peer, since the local port will always be
the
> port the test server
>     // is listening on. This isn't strictly correct - it's possible for
multiple
> peers to connect with
>     // the same remote port but different remote IPs - but the tests here
assume
> that connections
>     // to the test server (running on localhost) will always come from
> localhost, and thus the
>     // peer port is all thats needed to distinguish two connections. This also
> would be problematic
>     // if the OS reused ports, but that's not something to worry about for
these
> tests.
>     net::IPEndpoint address;
>   }
> 
> 
> Basically, just spelling out what we've discussed as a known (and intentional)
> limitation, and not just an "Oops, I never thought about that", so that if it
> ever breaks, it's clear _why_ it breaks to future readers ("We didn't need a
> pony, so we didn't buy a pony") ;)

Added! :)

[mmenke, 2015-07-10 15:42:41]

Hrm...Unrelated to this CL, but is there a chance we could rename "privacy mode"
to something more intuitive at some point in the future?  If I have a "private"
connection, I would assume that was a more secure and trusted connection, not
one where I was sending less authentication information.  "Anonymous" seems much
better to me.  Easy to get that confused with incognito mode, but fits in with
the use of the term in an FTP sense.

[Yoav Weiss, 2015-07-10 15:49:21]

On 2015/07/10 15:42:41, mmenke wrote:
> Hrm...Unrelated to this CL, but is there a chance we could rename "privacy
mode"
> to something more intuitive at some point in the future?  If I have a
"private"
> connection, I would assume that was a more secure and trusted connection, not
> one where I was sending less authentication information.  "Anonymous" seems
much
> better to me.  Easy to get that confused with incognito mode, but fits in with
> the use of the term in an FTP sense.

That makes sense.

Otherwise, any opinion on Ryan's enum questions? Anything else?

[Ryan Sleevi, 2015-07-10 15:56:21]

On 2015/07/10 15:42:41, mmenke wrote:
> Hrm...Unrelated to this CL, but is there a chance we could rename "privacy
mode"
> to something more intuitive at some point in the future?  If I have a
"private"
> connection, I would assume that was a more secure and trusted connection, not
> one where I was sending less authentication information.  "Anonymous" seems
much
> better to me.  Easy to get that confused with incognito mode, but fits in with
> the use of the term in an FTP sense.

Yes :) That was the overall outcome from the aforelinked thread - that we need
to treat privacy mode as first class as part of fetch, and also reconsider the
proper layering of things. See http://crbug.com/468005 or
http://crbug.com/440185 . Filed
https://code.google.com/p/chromium/issues/detail?id=508953 to hang cleanups off
of.

(Anonymous I think is a bit overloaded with CORS anonymous, which may be weird)

[mmenke, 2015-07-10 16:12:10]

On 2015/07/10 15:49:21, Yoav Weiss wrote:
> On 2015/07/10 15:42:41, mmenke wrote:
> > Hrm...Unrelated to this CL, but is there a chance we could rename "privacy
> mode"
> > to something more intuitive at some point in the future?  If I have a
> "private"
> > connection, I would assume that was a more secure and trusted connection,
not
> > one where I was sending less authentication information.  "Anonymous" seems
> much
> > better to me.  Easy to get that confused with incognito mode, but fits in
with
> > the use of the term in an FTP sense.
> 
> That makes sense.
> 
> Otherwise, any opinion on Ryan's enum questions? Anything else?

I'd go with the bool, for now (Mostly because I'm wondering if, longer term,
we'll want to switch to whatever enum we use for it in net).

[Yoav Weiss, 2015-07-10 22:28:40]

On 2015/07/10 16:12:10, mmenke wrote:
> On 2015/07/10 15:49:21, Yoav Weiss wrote:
> > On 2015/07/10 15:42:41, mmenke wrote:
> > > Hrm...Unrelated to this CL, but is there a chance we could rename "privacy
> > mode"
> > > to something more intuitive at some point in the future?  If I have a
> > "private"
> > > connection, I would assume that was a more secure and trusted connection,
> not
> > > one where I was sending less authentication information.  "Anonymous"
seems
> > much
> > > better to me.  Easy to get that confused with incognito mode, but fits in
> with
> > > the use of the term in an FTP sense.
> > 
> > That makes sense.
> > 
> > Otherwise, any opinion on Ryan's enum questions? Anything else?
> 
> I'd go with the bool, for now (Mostly because I'm wondering if, longer term,
> we'll want to switch to whatever enum we use for it in net).

So, no more open issues with this CL, right?

[mmenke, 2015-07-13 21:02:01]

Browsertests look great. Hadn't looked at the other files, because I didn't
expect to be reviewing them.

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:36: motivation, count,
make_scoped_refptr(getter), false));
Why does this default to false, while the others are true?

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
chrome/browser/net/predictor.cc:313: kConnectionsNeeded,
kAllowCredentialsOnPreconnectByDefault);
nit (x2):  It seems weird that kConnectionsNeeded is declared just above use,
while kAllowCredentialsOnPreconnectByDefault is at the top of the file.

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:113: // something to worry about for
these tests.
optional:  Could just switch to a map of IPEndpoints to statuses, and remove
this comment.

https://codereview.chromium.org/1131293004/diff/320001/components/network_hin...
File components/network_hints/renderer/prescient_networking_dispatcher.cc
(right):

https://codereview.chromium.org/1131293004/diff/320001/components/network_hin...
components/network_hints/renderer/prescient_networking_dispatcher.cc:28: VLOG(2)
<< "Preconnect: " << url.string().utf8();
Don't think we need this VLOG, since the other method already has one.

https://codereview.chromium.org/1131293004/diff/320001/net/socket/client_sock...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/net/socket/client_sock...
net/socket/client_socket_pool_manager.cc:223: (privacy_mode ==
PRIVACY_MODE_ENABLED);
We should have tests of this changed behavior.

[Yoav Weiss, 2015-07-13 22:11:38]

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:36: motivation, count,
make_scoped_refptr(getter), false));
On 2015/07/13 21:02:01, mmenke wrote:
> Why does this default to false, while the others are true?

Good catch! I missed reversing it when the boolean changed semantics from
"anonymous" to "use-credentials".

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
chrome/browser/net/predictor.cc:313: kConnectionsNeeded,
kAllowCredentialsOnPreconnectByDefault);
On 2015/07/13 21:02:01, mmenke wrote:
> nit (x2):  It seems weird that kConnectionsNeeded is declared just above use,
> while kAllowCredentialsOnPreconnectByDefault is at the top of the file.

So should I align kConnectionsNeeded with kAllowCredentialsOnPreconnectByDefault
and move it to the top? Or the other way around?

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:113: // something to worry about for
these tests.
On 2015/07/13 21:02:01, mmenke wrote:
> optional:  Could just switch to a map of IPEndpoints to statuses, and remove
> this comment.

Maybe in a followup CL?

https://codereview.chromium.org/1131293004/diff/320001/components/network_hin...
File components/network_hints/renderer/prescient_networking_dispatcher.cc
(right):

https://codereview.chromium.org/1131293004/diff/320001/components/network_hin...
components/network_hints/renderer/prescient_networking_dispatcher.cc:28: VLOG(2)
<< "Preconnect: " << url.string().utf8();
On 2015/07/13 21:02:01, mmenke wrote:
> Don't think we need this VLOG, since the other method already has one.

removed

https://codereview.chromium.org/1131293004/diff/320001/net/socket/client_sock...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/net/socket/client_sock...
net/socket/client_socket_pool_manager.cc:223: (privacy_mode ==
PRIVACY_MODE_ENABLED);
On 2015/07/13 21:02:01, mmenke wrote:
> We should have tests of this changed behavior.

What's the best location and methodology for such tests? Are there current unit
tests for the socket pool manager?

[mmenke, 2015-07-13 22:30:16]

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
chrome/browser/net/predictor.cc:313: kConnectionsNeeded,
kAllowCredentialsOnPreconnectByDefault);
On 2015/07/13 22:11:37, Yoav Weiss wrote:
> On 2015/07/13 21:02:01, mmenke wrote:
> > nit (x2):  It seems weird that kConnectionsNeeded is declared just above
use,
> > while kAllowCredentialsOnPreconnectByDefault is at the top of the file.
> 
> So should I align kConnectionsNeeded with
kAllowCredentialsOnPreconnectByDefault
> and move it to the top? Or the other way around?

Actually, may just want to leave them alone.  I had thought the logic was more
similar than it was - this is called when actually navigating, and the other is
called when doing a prediction.  Could easily see using different values for the
two.

Sorry - didn't think as much about that comment as I should have.

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:113: // something to worry about for
these tests.
On 2015/07/13 22:11:37, Yoav Weiss wrote:
> On 2015/07/13 21:02:01, mmenke wrote:
> > optional:  Could just switch to a map of IPEndpoints to statuses, and remove
> > this comment.
> 
> Maybe in a followup CL?

Works for me (And, as I said, it's optional, so no need to commit to doing
that).

https://codereview.chromium.org/1131293004/diff/320001/net/socket/client_sock...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1131293004/diff/320001/net/socket/client_sock...
net/socket/client_socket_pool_manager.cc:223: (privacy_mode ==
PRIVACY_MODE_ENABLED);
On 2015/07/13 22:11:38, Yoav Weiss wrote:
> On 2015/07/13 21:02:01, mmenke wrote:
> > We should have tests of this changed behavior.
> 
> What's the best location and methodology for such tests? Are there current
unit
> tests for the socket pool manager?

I may have to do a little digging to figure how to reasonably test this - happy
to do it, but may not be able to get back to you until tomorrow afternoon (Which
is why I didn't give more info in the first place).  Sorry for slowness.

[mmenke, 2015-07-14 16:32:03]

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
File net/socket/client_socket_pool_manager.cc (right):

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
net/socket/client_socket_pool_manager.cc:223: (privacy_mode ==
PRIVACY_MODE_ENABLED);
I guess this is supposed to mirror the logic in URLRequestHttpJob::Start? 
There's a mismatch here, though.  URLRequestHttpJob::Start doesn't check
LOAD_DO_NOT_SEND_AUTH_DATA, but this does.

That seems problematic to me.  I'm no expert on Privacy Mode, but doesn't this
mean we may use channel ID on some privacy mode connections? 
HttpNetworkTransaction::Start has logic to disable channel ID on PM connections.
 Moreover, there's a whole bunch of other logic based on the presence or absence
of privacy mode at higher layers of the stack, which would now not apply to all
requests that go through the pm group.

Peppering the code with redundant (Or semi-redundant) checks also seems a bit
problematic, as code tends to diverge over time.  If this is something we want
to do in a lot of places, maybe we should use a shared function to perform the
logic?

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
net/socket/client_socket_pool_manager.cc:224: 
The existing test for privacy mode socket use is
HttpStreamFactoryTest.PrivacyModeUsesDifferentSocketPoolGroup.  You could just
add a test there.

That seems a bit high up the stack to be testing socket in net/socket, to me -
I'd really prefer a couple tests in net/socket for both PRIVACY_MODE_ENABLED and
the new logic - but I'm reluctant to ask you to do it, seeing as how much yak
shaving we've already done on this CL.

[Ryan Sleevi, 2015-07-14 18:41:02]

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
File net/socket/client_socket_pool_manager.cc (right):

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
net/socket/client_socket_pool_manager.cc:223: (privacy_mode ==
PRIVACY_MODE_ENABLED);
On 2015/07/14 16:32:03, mmenke wrote:
> I guess this is supposed to mirror the logic in URLRequestHttpJob::Start? 
> There's a mismatch here, though.  URLRequestHttpJob::Start doesn't check
> LOAD_DO_NOT_SEND_AUTH_DATA, but this does.

No, it's to mirror
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...

This is still correct, however. The issue is that if you're employing
NTLM/Kerberos/Negotiate, then it's connection oriented - and the entire
connection is tainted with implicit authority that shouldn't be there.

> That seems problematic to me.  I'm no expert on Privacy Mode, but doesn't this
> mean we may use channel ID on some privacy mode connections? 

I'm not sure I follow how that conclusion was reached?

> HttpNetworkTransaction::Start has logic to disable channel ID on PM
connections.
>  Moreover, there's a whole bunch of other logic based on the presence or
absence
> of privacy mode at higher layers of the stack, which would now not apply to
all
> requests that go through the pm group.

Not sure I follow this either.

> Peppering the code with redundant (Or semi-redundant) checks also seems a bit
> problematic, as code tends to diverge over time.  If this is something we want
> to do in a lot of places, maybe we should use a shared function to perform the
> logic?

There's different concepts at different layers, and it's all gotten messy since
privacy mode was introduced. This is something we can/should clean-up.

The highest layer is the fetch() abstraction, which is the WebURLRequest bits
and (in parallel) the preconnect logic.
Below that is Chrome policy for determining privacy preserving behaviours of
non-fetch()'d requests. Realistically, we want these to all be consistent, but
there's something that load flags don't express - which is whether the omission
of (cookies, auth) is done for _privacy_ reasons, or if it's done for _other_
reasons. If it's for privacy, we should have separate socket pools. If it's for
some other reason, then it's on a case-by-case basis.


That said, this CL resolves the conflict by pushing it to the lowest layer and
assuming it is always done for privacy reasons, and that's... OK. I think.
Otherwise it means duplicating the logic each time the web platform glues with
//net, and I don't think we'll end up with common interfaces for that, since the
glue is necessarily uncommon.

[mmenke, 2015-07-14 18:52:12]

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
File net/socket/client_socket_pool_manager.cc (right):

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
net/socket/client_socket_pool_manager.cc:223: (privacy_mode ==
PRIVACY_MODE_ENABLED);
On 2015/07/14 18:41:02, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/14 16:32:03, mmenke wrote:
> > I guess this is supposed to mirror the logic in URLRequestHttpJob::Start? 
> > There's a mismatch here, though.  URLRequestHttpJob::Start doesn't check
> > LOAD_DO_NOT_SEND_AUTH_DATA, but this does.
> 
> No, it's to mirror
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...
> 
> This is still correct, however. The issue is that if you're employing
> NTLM/Kerberos/Negotiate, then it's connection oriented - and the entire
> connection is tainted with implicit authority that shouldn't be there.

So then why doesn't
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
check that flag?

Also, what if only a subset of those flags are set?  Send cookies, but don't
save them...Should that really go to the privacy mode pool?  Or if auth is
allowed, but we don't want to send cookies?

> > That seems problematic to me.  I'm no expert on Privacy Mode, but doesn't
this
> > mean we may use channel ID on some privacy mode connections? 
> 
> I'm not sure I follow how that conclusion was reached?

HttpNetworkTransaction::Start disables channel ID if privacy mode was enabled. 
So suppose we just have a request issued with LOAD_DO_NOT_SEND_AUTH_DATA set,
but not privacy mode.  Then HttpNetworkTransaction::Start will not disable
channel ID, but the request will end up in the privacy mode connection pool.  We
have a similar check in URLRequestJob to enable privacy mode, but it only checks
the cookie flags, not the AUTH flag (And not sure how websockets comes into play
here, either, for that matter).

It seems to me like either we should bundle all those flags as one, or if not
all of them are set, we should not send requests to the pm pool (Which taints
the pool, no?)

> > HttpNetworkTransaction::Start has logic to disable channel ID on PM
> connections.
> >  Moreover, there's a whole bunch of other logic based on the presence or
> absence
> > of privacy mode at higher layers of the stack, which would now not apply to
> all
> > requests that go through the pm group.
> 
> Not sure I follow this either.

Looks like I was wrong about other higher layer checks - I though QUIC and SPDY
had some, but SPDY only has them in tests, and QUIC doesn't go through this
code.

> > Peppering the code with redundant (Or semi-redundant) checks also seems a
bit
> > problematic, as code tends to diverge over time.  If this is something we
want
> > to do in a lot of places, maybe we should use a shared function to perform
the
> > logic?
> 
> There's different concepts at different layers, and it's all gotten messy
since
> privacy mode was introduced. This is something we can/should clean-up.
> 
> The highest layer is the fetch() abstraction, which is the WebURLRequest bits
> and (in parallel) the preconnect logic.
> Below that is Chrome policy for determining privacy preserving behaviours of
> non-fetch()'d requests. Realistically, we want these to all be consistent, but
> there's something that load flags don't express - which is whether the
omission
> of (cookies, auth) is done for _privacy_ reasons, or if it's done for _other_
> reasons. If it's for privacy, we should have separate socket pools. If it's
for
> some other reason, then it's on a case-by-case basis.
> 
> 
> That said, this CL resolves the conflict by pushing it to the lowest layer and
> assuming it is always done for privacy reasons, and that's... OK. I think.
> Otherwise it means duplicating the logic each time the web platform glues with
> //net, and I don't think we'll end up with common interfaces for that, since
the
> glue is necessarily uncommon.

[Ryan Sleevi, 2015-07-14 18:55:32]

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
File net/socket/client_socket_pool_manager.cc (right):

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
net/socket/client_socket_pool_manager.cc:223: (privacy_mode ==
PRIVACY_MODE_ENABLED);
On 2015/07/14 18:52:12, mmenke wrote:
> HttpNetworkTransaction::Start disables channel ID if privacy mode was enabled.

> So suppose we just have a request issued with LOAD_DO_NOT_SEND_AUTH_DATA set,
> but not privacy mode.  Then HttpNetworkTransaction::Start will not disable
> channel ID, but the request will end up in the privacy mode connection pool. 
We
> have a similar check in URLRequestJob to enable privacy mode, but it only
checks
> the cookie flags, not the AUTH flag (And not sure how websockets comes into
play
> here, either, for that matter).
> 
> It seems to me like either we should bundle all those flags as one, or if not
> all of them are set, we should not send requests to the pm pool (Which taints
> the pool, no?)

Oof, yeah, good catch on the bug.

Basically, the logic needs to be if you don't want one (for privacy reasons) you
shouldn't get any. You basically need a new socket any time the
fetch-without-credentials flag is set (the high level), but the low-level bits
are... messier. Which means we probably shouldn't be touching this here, because
of that.

[mmenke, 2015-07-14 19:10:52]

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
File net/socket/client_socket_pool_manager.cc (right):

https://chromiumcodereview.appspot.com/1131293004/diff/280002/net/socket/clie...
net/socket/client_socket_pool_manager.cc:223: (privacy_mode ==
PRIVACY_MODE_ENABLED);
On 2015/07/14 18:55:32, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/14 18:52:12, mmenke wrote:
> > HttpNetworkTransaction::Start disables channel ID if privacy mode was
enabled.
> 
> > So suppose we just have a request issued with LOAD_DO_NOT_SEND_AUTH_DATA
set,
> > but not privacy mode.  Then HttpNetworkTransaction::Start will not disable
> > channel ID, but the request will end up in the privacy mode connection pool.

> We
> > have a similar check in URLRequestJob to enable privacy mode, but it only
> checks
> > the cookie flags, not the AUTH flag (And not sure how websockets comes into
> play
> > here, either, for that matter).
> > 
> > It seems to me like either we should bundle all those flags as one, or if
not
> > all of them are set, we should not send requests to the pm pool (Which
taints
> > the pool, no?)
> 
> Oof, yeah, good catch on the bug.
> 
> Basically, the logic needs to be if you don't want one (for privacy reasons)
you
> shouldn't get any. You basically need a new socket any time the
> fetch-without-credentials flag is set (the high level), but the low-level bits
> are... messier. Which means we probably shouldn't be touching this here,
because
> of that.

I'm skeptical of the idea of sharing a URLRequestContext between pm and non-pm
mode connections.  Seems like there's a huge amount of state that's potentially
shared, and we really don't want any of it.

[Ryan Sleevi, 2015-07-15 01:18:26]

On 2015/07/14 19:10:52, mmenke wrote:
> I'm skeptical of the idea of sharing a URLRequestContext between pm and non-pm
> mode connections.  Seems like there's a huge amount of state that's
potentially
> shared, and we really don't want any of it.

I'm not really sure I understand or agree with this, but I think we're just
miscommunicating again :)

[mmenke, 2015-07-15 01:47:49]

On 2015/07/15 01:18:26, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/14 19:10:52, mmenke wrote:
> > I'm skeptical of the idea of sharing a URLRequestContext between pm and
non-pm
> > mode connections.  Seems like there's a huge amount of state that's
> potentially
> > shared, and we really don't want any of it.
> 
> I'm not really sure I understand or agree with this, but I think we're just
> miscommunicating again :)

I'm saying that rather than peppering the network stack with magic privacy mode
checks and hoping we catch everything, using a separate URLRequestContext with
no cookie store, its own socket pool, etc, seems much more robust, if we're
serious about getting things right.

[Ryan Sleevi, 2015-07-15 01:51:21]

On 2015/07/15 01:47:49, mmenke wrote:
> I'm saying that rather than peppering the network stack with magic privacy
mode
> checks and hoping we catch everything, using a separate URLRequestContext with
> no cookie store, its own socket pool, etc, seems much more robust, if we're
> serious about getting things right.

Well, then we just end up shunting the problem, not solving it. Rather than
worrying about which flag you set, you have to worry about which
URLRequestContext to use, and you can still botch the flags. Anywhere we glue
with the Web Platform still has to sort out the contexts.

It means for things like proxies, you might get prompted to authenticate twice,
for example, since each context maintains a necessarily separate store.

So I guess I get where you're coming from, I just don't think it'd simplify the
problem so much as shift it, and there's still opportunity for doom. I do agree
with the overall goal though, which is to reduce the opportunity for doom -
either in the platform or by users. And yes, this whole privacy mode concept is
one of those 'big shifts' when we realized what's going on, even if it's been
slowly introduced over years.

[mmenke, 2015-07-15 01:57:05]

On 2015/07/15 01:51:21, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/15 01:47:49, mmenke wrote:
> > I'm saying that rather than peppering the network stack with magic privacy
> mode
> > checks and hoping we catch everything, using a separate URLRequestContext
with
> > no cookie store, its own socket pool, etc, seems much more robust, if we're
> > serious about getting things right.
> 
> Well, then we just end up shunting the problem, not solving it. Rather than
> worrying about which flag you set, you have to worry about which
> URLRequestContext to use, and you can still botch the flags. Anywhere we glue
> with the Web Platform still has to sort out the contexts.
> 
> It means for things like proxies, you might get prompted to authenticate
twice,
> for example, since each context maintains a necessarily separate store.

I'm confused...I thought we didn't send auth information in privacy mode?  Or is
that only some subset of privacy mode connections?

> So I guess I get where you're coming from, I just don't think it'd simplify
the
> problem so much as shift it, and there's still opportunity for doom. I do
agree
> with the overall goal though, which is to reduce the opportunity for doom -
> either in the platform or by users. And yes, this whole privacy mode concept
is
> one of those 'big shifts' when we realized what's going on, even if it's been
> slowly introduced over years.

[Ryan Sleevi, 2015-07-15 02:11:00]

On 2015/07/15 01:57:05, mmenke wrote:
> I'm confused...I thought we didn't send auth information in privacy mode?  Or
is
> that only some subset of privacy mode connections?

Refer back to e-mail thread :)

If we're not sending auth *to the server*, because we want the connection to be
unlinkable with other requests, "use privacy mode"
If we're not sending cookies *to the server*, because we want the connection to
be unlinkable with other requests, "use privacy mode"
If we're not letting the server set cookies, because we want the connection to
be unlinkable with other requests, "use privacy mode" (this is the sketchy one)

None of these affect proxy auth/connections. We're not trying to protect against
the proxy. This is merely about the endpoint being able to link requests. If a
connection uses ambient auth, then it can be linked. If a connection previously
sent cookies for a request, but now doesn't send cookies on a new request, you
can still link the new request to the old cookie.

From the Web Platform side, it's about making sure ambient authority *to the
server* doesn't get carried over. In a perfect world, we would only track
whether or not a connection used a connection-oriented auth method
(NTLM/Kerberos/Negotiate), and if so, close it and open a new 'clean' one.
That's fully sufficient for what fetch() needs (for now). In the future, where
first-party/third-party cookies are a thing that are explained, we might then
apply the same logic to cookies (if we sent a request over a conn w/ a
first-party cookie, don't send a third-party request over that same conn - but
you can send a first party request just fine)

From the //net stack side, because we've decided that for privacy reasons we'll
keep things separate, the auth data is just a linkability thing.

[mmenke, 2015-07-15 02:50:51]

On 2015/07/15 02:11:00, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/15 01:57:05, mmenke wrote:
> > I'm confused...I thought we didn't send auth information in privacy mode? 
Or
> is
> > that only some subset of privacy mode connections?
> 
> Refer back to e-mail thread :)
> 
> If we're not sending auth *to the server*, because we want the connection to
be
> unlinkable with other requests, "use privacy mode"
> If we're not sending cookies *to the server*, because we want the connection
to
> be unlinkable with other requests, "use privacy mode"
> If we're not letting the server set cookies, because we want the connection to
> be unlinkable with other requests, "use privacy mode" (this is the sketchy
one)
> 
> None of these affect proxy auth/connections. We're not trying to protect
against
> the proxy. This is merely about the endpoint being able to link requests. If a
> connection uses ambient auth, then it can be linked. If a connection
previously
> sent cookies for a request, but now doesn't send cookies on a new request, you
> can still link the new request to the old cookie.

Ahh, LOAD_DO_NOT_SEND_AUTH_DATA does not apply to proxy auth - I didn't realize
that (Makes sense, though).

> From the Web Platform side, it's about making sure ambient authority *to the
> server* doesn't get carried over. In a perfect world, we would only track
> whether or not a connection used a connection-oriented auth method
> (NTLM/Kerberos/Negotiate), and if so, close it and open a new 'clean' one.
> That's fully sufficient for what fetch() needs (for now). In the future, where
> first-party/third-party cookies are a thing that are explained, we might then
> apply the same logic to cookies (if we sent a request over a conn w/ a
> first-party cookie, don't send a third-party request over that same conn - but
> you can send a first party request just fine)
> 
> From the //net stack side, because we've decided that for privacy reasons
we'll
> keep things separate, the auth data is just a linkability thing.

[Yoav Weiss, 2015-07-15 07:55:27]

So, what's the verdict your honors? :D Can you sum up the conclusions? Do we
keep things as they are for this CL?

[Yoav Weiss, 2015-07-15 08:45:05]

On 2015/07/15 07:55:27, Yoav Weiss wrote:
> So, what's the verdict your honors? :D Can you sum up the conclusions? Do we
> keep things as they are for this CL?

Regarding the HttpNetworkTransaction::Start bug, should I fix it there? Or maybe
I should just go back to flipping the privacy_mode flag in preconnect.cc and we
would fix the layering issues in a separate CL?

[Ryan Sleevi, 2015-07-15 15:27:26]

On 2015/07/15 07:55:27, Yoav Weiss wrote:
> So, what's the verdict your honors? :D Can you sum up the conclusions? Do we
> keep things as they are for this CL?

No, we're going to need to change things. Basically, when we push something into
a pm/ socket, we need to make sure all the flags are set, rather than mix/match.

The current logic (in the socket pool) allows a mix of sockets to be entered
into the privacy mode pool, which would undermine the privacy mode.

I'll try to have more concrete suggestions tomorrow when I'm back.

[Yoav Weiss, 2015-07-19 21:45:09]

On 2015/07/15 15:27:26, Ryan Sleevi wrote:
> On 2015/07/15 07:55:27, Yoav Weiss wrote:
> > So, what's the verdict your honors? :D Can you sum up the conclusions? Do we
> > keep things as they are for this CL?
> 
> No, we're going to need to change things. Basically, when we push something
into
> a pm/ socket, we need to make sure all the flags are set, rather than
mix/match.
> 
> The current logic (in the socket pool) allows a mix of sockets to be entered
> into the privacy mode pool, which would undermine the privacy mode.
> 
> I'll try to have more concrete suggestions tomorrow when I'm back.

I've brought back the explicit setting of private_mode in preconnect.cc and
reverted the changes in the client socket pool.
I'd be happy to take on the work of tackling these layering issues as part of a
separate CL.

Matt, Ryan - WDYT?

[mmenke, 2015-07-21 17:14:29]

On 2015/07/19 21:45:09, Yoav Weiss wrote:
> On 2015/07/15 15:27:26, Ryan Sleevi wrote:
> > On 2015/07/15 07:55:27, Yoav Weiss wrote:
> > > So, what's the verdict your honors? :D Can you sum up the conclusions? Do
we
> > > keep things as they are for this CL?
> > 
> > No, we're going to need to change things. Basically, when we push something
> into
> > a pm/ socket, we need to make sure all the flags are set, rather than
> mix/match.
> > 
> > The current logic (in the socket pool) allows a mix of sockets to be entered
> > into the privacy mode pool, which would undermine the privacy mode.
> > 
> > I'll try to have more concrete suggestions tomorrow when I'm back.
> 
> I've brought back the explicit setting of private_mode in preconnect.cc and
> reverted the changes in the client socket pool.
> I'd be happy to take on the work of tackling these layering issues as part of
a
> separate CL.
> 
> Matt, Ryan - WDYT?

This LGTM, as-is.

I feel that before worrying about cleaning up the layering, we should determine
just what privacy mode is, come to some decision about what privacy mode is, and
who should know about it, and whether not sending neither cookies nor server
auth is a requirement, or if not sending either is just a hint we should use
privacy mode.

[Yoav Weiss, 2015-07-22 07:42:07]

yoav@yoav.ws changed reviewers:
+ jochen@chromium.org, ttuttle@chromium.org

[Yoav Weiss, 2015-07-22 07:42:08]

Adding Jochen as a reviewer for the chrome_render_message_filter bits and
ttuttle for the network_hints bits.

[jochen (gone - plz use gerrit), 2015-07-22 09:35:03]

you could consider using an enum class instead of the bool.

anyway, lgtm with comments addressed

https://codereview.chromium.org/1131293004/diff/350001/chrome/browser/net/pre...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/1131293004/diff/350001/chrome/browser/net/pre...
chrome/browser/net/predictor.h:249: bool allow_credentials);
the ipc message has the bool first, then the count. please keep both in sync

https://codereview.chromium.org/1131293004/diff/350001/components/network_hin...
File components/network_hints/renderer/prescient_networking_dispatcher.h
(right):

https://codereview.chromium.org/1131293004/diff/350001/components/network_hin...
components/network_hints/renderer/prescient_networking_dispatcher.h:26: const
bool allow_credentials) override;
just bool (no const)

[Yoav Weiss, 2015-07-23 08:47:20]

> 
>
https://codereview.chromium.org/1131293004/diff/350001/components/network_hin...
> File components/network_hints/renderer/prescient_networking_dispatcher.h
> (right):
> 
>
https://codereview.chromium.org/1131293004/diff/350001/components/network_hin...
> components/network_hints/renderer/prescient_networking_dispatcher.h:26: const
> bool allow_credentials) override;
> just bool (no const)

I've removed the const, but now Windows has trouble building since the interface
is defined on the Blink side, with the const.
What's the best way to tackle that? Can I overload the interface on the Blink
side with a "bool" on top of the "const bool"?

[jochen (gone - plz use gerrit), 2015-07-23 08:49:16]

meh

we should fix the blink interface then as well :-/

feel free to do that in a follow-up cl and stick with the const bool for now

[Yoav Weiss, 2015-07-23 08:55:53]

On 2015/07/23 08:49:16, jochen wrote:
> meh
> 
> we should fix the blink interface then as well :-/
> 
> feel free to do that in a follow-up cl and stick with the const bool for now

I'll do that in a followup CL, since I'd need to clean up that interface anyway
once this lands.

[Deprecated (see juliatuttle), 2015-07-23 17:42:02]

lgtm.

[Yoav Weiss, 2015-07-23 18:17:41]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-07-23 18:17:41]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1131293004/#ps430001
(title: "more build issues")

[commit-bot: I haz the power, 2015-07-23 18:17:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131293004/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1131293004/430001

[Ryan Sleevi, 2015-07-23 18:19:45]

The CQ bit was unchecked by rsleevi@chromium.org

[Ryan Sleevi, 2015-07-23 18:22:41]

LGTM % the one bug, which is why I unchecked CQ.

I won't be around to re-review, so it may help making sure someone thoroughly
checks your update JIC.

https://codereview.chromium.org/1131293004/diff/430001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/430001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:75: request_info.privacy_mode =
net::PRIVACY_MODE_ENABLED;
Sorry, I'm still out rather sick, but saw this ended up going through CQ, and
it's still wrong.

This needs to at least make sure to set the LoadFlags to the relevant 3 flags
from the WebURLRequest *in addition to* setting the privacy mode flag.

You want this file to be consistent with the behaviour of WebURLRequest in how
it maps to URLRequests.

[Yoav Weiss, 2015-07-23 21:32:40]

Thanks Ryan! I believe I addressed it now. !allow_credentials means that both
private mode flag and the three load flags are set.

Matt - Can you take a last look at preconnect.cc?

[mmenke, 2015-07-24 15:48:20]

https://codereview.chromium.org/1131293004/diff/440001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/440001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:71: request_info.privacy_mode =
net::PRIVACY_MODE_ENABLED;
Setting the load flags for only some privacy mode sockets seems like a bug to
me, though I remain hazy on the subject.

[Yoav Weiss, 2015-07-24 15:51:13]

On 2015/07/24 15:48:20, mmenke wrote:
>
https://codereview.chromium.org/1131293004/diff/440001/chrome/browser/net/pre...
> File chrome/browser/net/preconnect.cc (right):
> 
>
https://codereview.chromium.org/1131293004/diff/440001/chrome/browser/net/pre...
> chrome/browser/net/preconnect.cc:71: request_info.privacy_mode =
> net::PRIVACY_MODE_ENABLED;
> Setting the load flags for only some privacy mode sockets seems like a bug to
> me, though I remain hazy on the subject.

Do you think it's better to set them on all privacy mode sockets?

[mmenke, 2015-07-24 16:22:31]

On 2015/07/24 15:51:13, Yoav Weiss wrote:
> On 2015/07/24 15:48:20, mmenke wrote:
> >
>
https://codereview.chromium.org/1131293004/diff/440001/chrome/browser/net/pre...
> > File chrome/browser/net/preconnect.cc (right):
> > 
> >
>
https://codereview.chromium.org/1131293004/diff/440001/chrome/browser/net/pre...
> > chrome/browser/net/preconnect.cc:71: request_info.privacy_mode =
> > net::PRIVACY_MODE_ENABLED;
> > Setting the load flags for only some privacy mode sockets seems like a bug
to
> > me, though I remain hazy on the subject.
> 
> Do you think it's better to set them on all privacy mode sockets?

I'd think so, but I'm not comfortable signing off on either version.

[Ryan Sleevi, 2015-07-24 16:32:27]

https://codereview.chromium.org/1131293004/diff/440001/chrome/browser/net/pre...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1131293004/diff/440001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:71: request_info.privacy_mode =
net::PRIVACY_MODE_ENABLED;
On 2015/07/24 15:48:20, mmenke wrote:
> Setting the load flags for only some privacy mode sockets seems like a bug to
> me, though I remain hazy on the subject.

Fundamentally, yes. At the lower layer, we basically want to ensure all privacy
mode sockets have these three flags set.

But that's not something incumbent on this CL, and this CL at least ensures it
strictly matches the WebURLRequest logic for first/third party requests, which
is the minimally correct thing.

https://codereview.chromium.org/1131293004/diff/440001/chrome/browser/net/pre...
chrome/browser/net/preconnect.cc:77: request_info.privacy_mode =
net::PRIVACY_MODE_ENABLED;
I don't think you need to set this (forcing pm mode on) to get the 'right
behaviour' (since you'll have a first/third party cookie break for cross-origin
requests on line 70 anyways)

[Yoav Weiss, 2015-07-27 21:41:08]

yoav@yoav.ws changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2015-07-28 22:22:51]

Which is to say I think this is good to go and addresses the bug Matt found.
We'll still want to harmonize across instances of privacy mode, but this is at
least no longer introducing any *new* bugs.

[Yoav Weiss, 2015-07-29 05:22:41]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-07-29 05:22:41]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
ttuttle@chromium.org, mmenke@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1131293004/#ps440001
(title: "Address Ryan's concerns")

[commit-bot: I haz the power, 2015-07-29 05:22:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131293004/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1131293004/440001

[Yoav Weiss, 2015-07-29 05:38:45]

On 2015/07/28 22:22:51, Ryan Sleevi wrote:
> Which is to say I think this is good to go and addresses the bug Matt found.
> We'll still want to harmonize across instances of privacy mode, but this is at
> least no longer introducing any *new* bugs.

Thanks, Ryan! :)

[commit-bot: I haz the power, 2015-07-29 06:43:48]

Committed patchset #22 (id:440001)

[commit-bot: I haz the power, 2015-07-29 06:44:33]

Patchset 22 (id:??) landed as
https://crrev.com/9131644005a8e069c128512f5674e53fa4d651ba
Cr-Commit-Position: refs/heads/master@{#340854}