Leverage profile's http network session's HttpAuthCache to support proxy auth.

Leverage profile's http network session HttpAuthCache to support proxy auth.

This allows the connect job to leverage any auth cache credentials the profile
has, including those the user gets prompted for as part of loading a page
while behind an authenticated proxy. GCM continues to use its own network
session for creating the connections, but at connection time the HttpAuthCache
is coped over from the profile's http network session (if available).

BUG=385748
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283973

[Nicolas Zea, 2014-07-08 00:33:46]

+Filip, PTAL. Apparently it's as easy as this!

[Nicolas Zea, 2014-07-08 00:36:26]

Also +Chris FYI. I tested this locally and GCM connections succeeded once I
entered the user/pass into the browser (via loading a page).

[cbentzel, 2014-07-08 13:58:05]

Adding mmenke who can probably do a better review of this and understand the
ramifications. My primary concern is around object lifetime issues.

[mmenke, 2014-07-08 14:30:56]

On 2014/07/08 13:58:05, cbentzel wrote:
> Adding mmenke who can probably do a better review of this and understand the
> ramifications. My primary concern is around object lifetime issues.

May not get to this today - I have a lot of codereviews, and I'll need to
unwrangle the ownership and threading models here.

What is the GCMClient?  The description seems to indicates you want profile auth
credentials, but it looks like there's one created globally (And one per profile
as well?)

It looks like you're using direct connections, rather than HTTP/HTTPS/WS/WSS,
and sharing the network session means you're sharing the socket pool.  As a
result, you'll be subject to connection pool limits shared with wherever you're
getting the NetworkSession from, which seems like a problem.

You also have your own proxy fallback logic, which you're now applying to the
proxy service managed by the network stack, which also seems problematic.

[fgorski, 2014-07-08 16:51:33]

lgtm

https://codereview.chromium.org/375663002/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/375663002/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:281: ->http_transaction_factory()
nit: you are moving the arrows from end of line to the next. If git cl format
does not complain I am ok with that, but please check.

[cbentzel, 2014-07-08 16:54:03]

Please do not land until mmenke gives the go-ahead. Thanks.

[fgorski, 2014-07-08 16:54:42]

On 2014/07/08 16:54:03, cbentzel wrote:
> Please do not land until mmenke gives the go-ahead. Thanks.

I agree. Just read through all of the comments.

[Nicolas Zea, 2014-07-08 17:32:46]

On 2014/07/08 14:30:56, mmenke wrote:
> On 2014/07/08 13:58:05, cbentzel wrote:
> > Adding mmenke who can probably do a better review of this and understand the
> > ramifications. My primary concern is around object lifetime issues.
> 
> May not get to this today - I have a lot of codereviews, and I'll need to
> unwrangle the ownership and threading models here.
> 
> What is the GCMClient?  The description seems to indicates you want profile
auth
> credentials, but it looks like there's one created globally (And one per
profile
> as well?)
> 
> It looks like you're using direct connections, rather than HTTP/HTTPS/WS/WSS,
> and sharing the network session means you're sharing the socket pool.  As a
> result, you'll be subject to connection pool limits shared with wherever
you're
> getting the NetworkSession from, which seems like a problem.
> 
> You also have your own proxy fallback logic, which you're now applying to the
> proxy service managed by the network stack, which also seems problematic.

Right, GCMClient is currently per-profile, but we're also adding support for a
device-specific connection for those devices that need it (and for when users
aren't signed in).

And yes, the connection is a TLS connection with a custom protocol built for
Android. The network session is, in the primary case (signed in users), coming
from the profile's request context. Else (non signed in users and device
specific connections) it comes from the system's request context.

The socket pool limits would just have the effect of delaying the connection,
right? Conversely, it means that we're occupying one socket, creating more
contention for other connections. How large are the limits? Is this likely to
have a big impact?

As far as the proxy fallback goes, by using the profile's network session can we
bypass the GCM proxy fallback logic and just rely on the one handled by the
network stack? We're not doing anything special in our proxy resolution logic
that needs to be handled separately, it's just there because we had previously
assumed it would be better to have our own network session, much like the jingle
connection (which much of our code was based on).

[mmenke, 2014-07-08 17:46:06]

On 2014/07/08 17:32:46, Nicolas Zea wrote:
> On 2014/07/08 14:30:56, mmenke wrote:
> > On 2014/07/08 13:58:05, cbentzel wrote:
> > > Adding mmenke who can probably do a better review of this and understand
the
> > > ramifications. My primary concern is around object lifetime issues.
> > 
> > May not get to this today - I have a lot of codereviews, and I'll need to
> > unwrangle the ownership and threading models here.
> > 
> > What is the GCMClient?  The description seems to indicates you want profile
> auth
> > credentials, but it looks like there's one created globally (And one per
> profile
> > as well?)
> > 
> > It looks like you're using direct connections, rather than
HTTP/HTTPS/WS/WSS,
> > and sharing the network session means you're sharing the socket pool.  As a
> > result, you'll be subject to connection pool limits shared with wherever
> you're
> > getting the NetworkSession from, which seems like a problem.
> > 
> > You also have your own proxy fallback logic, which you're now applying to
the
> > proxy service managed by the network stack, which also seems problematic.
> 
> Right, GCMClient is currently per-profile, but we're also adding support for a
> device-specific connection for those devices that need it (and for when users
> aren't signed in).
> 
> And yes, the connection is a TLS connection with a custom protocol built for
> Android. The network session is, in the primary case (signed in users), coming
> from the profile's request context. Else (non signed in users and device
> specific connections) it comes from the system's request context.
> 
> The socket pool limits would just have the effect of delaying the connection,
> right? Conversely, it means that we're occupying one socket, creating more
> contention for other connections. How large are the limits? Is this likely to
> have a big impact?

The limits are 6 sockets per host/port/protocol/proxy pair, 256 sockets for the
session in total, 64 sockets per proxy.  Also, if you're using the same
host/port/proxy as there was an HTTP/HTTPS connection to, I believe there's
nothing stopping you from getting a connection previously used for that purpose.

> As far as the proxy fallback goes, by using the profile's network session can
we
> bypass the GCM proxy fallback logic and just rely on the one handled by the
> network stack? We're not doing anything special in our proxy resolution logic
> that needs to be handled separately, it's just there because we had previously
> assumed it would be better to have our own network session, much like the
jingle
> connection (which much of our code was based on).

Proxy fallback logic is done at a higher level than the socket pools,
unfortunately.  If we've marked a proxy as bad because of HTTP failures, you get
the benefit of that, but we won't mark a proxy as bad and then retry at the
level you're connecting at.

In general, I think using the same HttpNetworkSession for HTTP[S] requests and
non-HTTP requests is a bad idea, and not something we're really set up to
handle, unfortunately.  I think it's going to be much safer to just make a new
HttpNetworkSession, and just share a couple specific HttpNetworkSession::Params.

[Nicolas Zea, 2014-07-08 19:28:55]

On 2014/07/08 17:46:06, mmenke wrote:
> On 2014/07/08 17:32:46, Nicolas Zea wrote:
> > On 2014/07/08 14:30:56, mmenke wrote:
> > > On 2014/07/08 13:58:05, cbentzel wrote:
> > > > Adding mmenke who can probably do a better review of this and understand
> the
> > > > ramifications. My primary concern is around object lifetime issues.
> > > 
> > > May not get to this today - I have a lot of codereviews, and I'll need to
> > > unwrangle the ownership and threading models here.
> > > 
> > > What is the GCMClient?  The description seems to indicates you want
profile
> > auth
> > > credentials, but it looks like there's one created globally (And one per
> > profile
> > > as well?)
> > > 
> > > It looks like you're using direct connections, rather than
> HTTP/HTTPS/WS/WSS,
> > > and sharing the network session means you're sharing the socket pool.  As
a
> > > result, you'll be subject to connection pool limits shared with wherever
> > you're
> > > getting the NetworkSession from, which seems like a problem.
> > > 
> > > You also have your own proxy fallback logic, which you're now applying to
> the
> > > proxy service managed by the network stack, which also seems problematic.
> > 
> > Right, GCMClient is currently per-profile, but we're also adding support for
a
> > device-specific connection for those devices that need it (and for when
users
> > aren't signed in).
> > 
> > And yes, the connection is a TLS connection with a custom protocol built for
> > Android. The network session is, in the primary case (signed in users),
coming
> > from the profile's request context. Else (non signed in users and device
> > specific connections) it comes from the system's request context.
> > 
> > The socket pool limits would just have the effect of delaying the
connection,
> > right? Conversely, it means that we're occupying one socket, creating more
> > contention for other connections. How large are the limits? Is this likely
to
> > have a big impact?
> 
> The limits are 6 sockets per host/port/protocol/proxy pair, 256 sockets for
the
> session in total, 64 sockets per proxy.  Also, if you're using the same
> host/port/proxy as there was an HTTP/HTTPS connection to, I believe there's
> nothing stopping you from getting a connection previously used for that
purpose.

Note that the host/port pairs we use do not support http, only the custom
protocol mentioned. So that shouldn't be an issue in this case (but certainly
something to keep in mind).

> 
> > As far as the proxy fallback goes, by using the profile's network session
can
> we
> > bypass the GCM proxy fallback logic and just rely on the one handled by the
> > network stack? We're not doing anything special in our proxy resolution
logic
> > that needs to be handled separately, it's just there because we had
previously
> > assumed it would be better to have our own network session, much like the
> jingle
> > connection (which much of our code was based on).
> 
> Proxy fallback logic is done at a higher level than the socket pools,
> unfortunately.  If we've marked a proxy as bad because of HTTP failures, you
get
> the benefit of that, but we won't mark a proxy as bad and then retry at the
> level you're connecting at.
> 
> In general, I think using the same HttpNetworkSession for HTTP[S] requests and
> non-HTTP requests is a bad idea, and not something we're really set up to
> handle, unfortunately.  I think it's going to be much safer to just make a new
> HttpNetworkSession, and just share a couple specific
HttpNetworkSession::Params.

My concern with this is that, from the digging around I've done, it doesn't seem
there's a good way to access/share the HttpAuthCache objects used by the
profile's NetworkSession, and we don't want to duplicate the logic used to
create them (we don't want to pop up new UI, or have to add special logic to
read system/device policy controlled credentials).

Perhaps I wasn't following the logic properly though. Which
HttpNetworkSession::Params were you thinking of?

[mmenke, 2014-07-08 20:19:25]

On 2014/07/08 19:28:55, Nicolas Zea wrote:
> On 2014/07/08 17:46:06, mmenke wrote:
> > On 2014/07/08 17:32:46, Nicolas Zea wrote:
> > > On 2014/07/08 14:30:56, mmenke wrote:
> > > > On 2014/07/08 13:58:05, cbentzel wrote:
> > > > > Adding mmenke who can probably do a better review of this and
understand
> > the
> > > > > ramifications. My primary concern is around object lifetime issues.
> > > > 
> > > > May not get to this today - I have a lot of codereviews, and I'll need
to
> > > > unwrangle the ownership and threading models here.
> > > > 
> > > > What is the GCMClient?  The description seems to indicates you want
> profile
> > > auth
> > > > credentials, but it looks like there's one created globally (And one per
> > > profile
> > > > as well?)
> > > > 
> > > > It looks like you're using direct connections, rather than
> > HTTP/HTTPS/WS/WSS,
> > > > and sharing the network session means you're sharing the socket pool. 
As
> a
> > > > result, you'll be subject to connection pool limits shared with wherever
> > > you're
> > > > getting the NetworkSession from, which seems like a problem.
> > > > 
> > > > You also have your own proxy fallback logic, which you're now applying
to
> > the
> > > > proxy service managed by the network stack, which also seems
problematic.
> > > 
> > > Right, GCMClient is currently per-profile, but we're also adding support
for
> a
> > > device-specific connection for those devices that need it (and for when
> users
> > > aren't signed in).
> > > 
> > > And yes, the connection is a TLS connection with a custom protocol built
for
> > > Android. The network session is, in the primary case (signed in users),
> coming
> > > from the profile's request context. Else (non signed in users and device
> > > specific connections) it comes from the system's request context.
> > > 
> > > The socket pool limits would just have the effect of delaying the
> connection,
> > > right? Conversely, it means that we're occupying one socket, creating more
> > > contention for other connections. How large are the limits? Is this likely
> to
> > > have a big impact?
> > 
> > The limits are 6 sockets per host/port/protocol/proxy pair, 256 sockets for
> the
> > session in total, 64 sockets per proxy.  Also, if you're using the same
> > host/port/proxy as there was an HTTP/HTTPS connection to, I believe there's
> > nothing stopping you from getting a connection previously used for that
> purpose.
> 
> Note that the host/port pairs we use do not support http, only the custom
> protocol mentioned. So that shouldn't be an issue in this case (but certainly
> something to keep in mind).
> 
> > 
> > > As far as the proxy fallback goes, by using the profile's network session
> can
> > we
> > > bypass the GCM proxy fallback logic and just rely on the one handled by
the
> > > network stack? We're not doing anything special in our proxy resolution
> logic
> > > that needs to be handled separately, it's just there because we had
> previously
> > > assumed it would be better to have our own network session, much like the
> > jingle
> > > connection (which much of our code was based on).
> > 
> > Proxy fallback logic is done at a higher level than the socket pools,
> > unfortunately.  If we've marked a proxy as bad because of HTTP failures, you
> get
> > the benefit of that, but we won't mark a proxy as bad and then retry at the
> > level you're connecting at.
> > 
> > In general, I think using the same HttpNetworkSession for HTTP[S] requests
and
> > non-HTTP requests is a bad idea, and not something we're really set up to
> > handle, unfortunately.  I think it's going to be much safer to just make a
new
> > HttpNetworkSession, and just share a couple specific
> HttpNetworkSession::Params.
> 
> My concern with this is that, from the digging around I've done, it doesn't
seem
> there's a good way to access/share the HttpAuthCache objects used by the
> profile's NetworkSession, and we don't want to duplicate the logic used to
> create them (we don't want to pop up new UI, or have to add special logic to
> read system/device policy controlled credentials).
> 
> Perhaps I wasn't following the logic properly though. Which
> HttpNetworkSession::Params were you thinking of?

Sorry, I hadn't realized the auth cache was actually owned by HttpNetworkSession
(I'm not terribly familiar with the auth code).

I believe I may have been wrong about the risk of sockets being reused in this
particular case, I need to refresh my memory of just how that codepath works.

I also want to see if anyone else is doing this currently, which would make me a
bit less concerned about potential side effects.

I do still feel that we should not be using proxy fallback logic outside of net/
to muck with net's proxy service.  Are GCM requests often made when there's no
network activity?  If not, we could just rely on the network logic to do all the
proxy fallback stuff.

[mmenke, 2014-07-09 17:28:19]

On 2014/07/08 20:19:25, mmenke wrote:
> On 2014/07/08 19:28:55, Nicolas Zea wrote:
> > On 2014/07/08 17:46:06, mmenke wrote:
> > > On 2014/07/08 17:32:46, Nicolas Zea wrote:
> > > > On 2014/07/08 14:30:56, mmenke wrote:
> > > > > On 2014/07/08 13:58:05, cbentzel wrote:
> > > > > > Adding mmenke who can probably do a better review of this and
> understand
> > > the
> > > > > > ramifications. My primary concern is around object lifetime issues.
> > > > > 
> > > > > May not get to this today - I have a lot of codereviews, and I'll need
> to
> > > > > unwrangle the ownership and threading models here.
> > > > > 
> > > > > What is the GCMClient?  The description seems to indicates you want
> > profile
> > > > auth
> > > > > credentials, but it looks like there's one created globally (And one
per
> > > > profile
> > > > > as well?)
> > > > > 
> > > > > It looks like you're using direct connections, rather than
> > > HTTP/HTTPS/WS/WSS,
> > > > > and sharing the network session means you're sharing the socket pool. 
> As
> > a
> > > > > result, you'll be subject to connection pool limits shared with
wherever
> > > > you're
> > > > > getting the NetworkSession from, which seems like a problem.
> > > > > 
> > > > > You also have your own proxy fallback logic, which you're now applying
> to
> > > the
> > > > > proxy service managed by the network stack, which also seems
> problematic.
> > > > 
> > > > Right, GCMClient is currently per-profile, but we're also adding support
> for
> > a
> > > > device-specific connection for those devices that need it (and for when
> > users
> > > > aren't signed in).
> > > > 
> > > > And yes, the connection is a TLS connection with a custom protocol built
> for
> > > > Android. The network session is, in the primary case (signed in users),
> > coming
> > > > from the profile's request context. Else (non signed in users and device
> > > > specific connections) it comes from the system's request context.
> > > > 
> > > > The socket pool limits would just have the effect of delaying the
> > connection,
> > > > right? Conversely, it means that we're occupying one socket, creating
more
> > > > contention for other connections. How large are the limits? Is this
likely
> > to
> > > > have a big impact?
> > > 
> > > The limits are 6 sockets per host/port/protocol/proxy pair, 256 sockets
for
> > the
> > > session in total, 64 sockets per proxy.  Also, if you're using the same
> > > host/port/proxy as there was an HTTP/HTTPS connection to, I believe
there's
> > > nothing stopping you from getting a connection previously used for that
> > purpose.
> > 
> > Note that the host/port pairs we use do not support http, only the custom
> > protocol mentioned. So that shouldn't be an issue in this case (but
certainly
> > something to keep in mind).
> > 
> > > 
> > > > As far as the proxy fallback goes, by using the profile's network
session
> > can
> > > we
> > > > bypass the GCM proxy fallback logic and just rely on the one handled by
> the
> > > > network stack? We're not doing anything special in our proxy resolution
> > logic
> > > > that needs to be handled separately, it's just there because we had
> > previously
> > > > assumed it would be better to have our own network session, much like
the
> > > jingle
> > > > connection (which much of our code was based on).
> > > 
> > > Proxy fallback logic is done at a higher level than the socket pools,
> > > unfortunately.  If we've marked a proxy as bad because of HTTP failures,
you
> > get
> > > the benefit of that, but we won't mark a proxy as bad and then retry at
the
> > > level you're connecting at.
> > > 
> > > In general, I think using the same HttpNetworkSession for HTTP[S] requests
> and
> > > non-HTTP requests is a bad idea, and not something we're really set up to
> > > handle, unfortunately.  I think it's going to be much safer to just make a
> new
> > > HttpNetworkSession, and just share a couple specific
> > HttpNetworkSession::Params.
> > 
> > My concern with this is that, from the digging around I've done, it doesn't
> seem
> > there's a good way to access/share the HttpAuthCache objects used by the
> > profile's NetworkSession, and we don't want to duplicate the logic used to
> > create them (we don't want to pop up new UI, or have to add special logic to
> > read system/device policy controlled credentials).
> > 
> > Perhaps I wasn't following the logic properly though. Which
> > HttpNetworkSession::Params were you thinking of?
> 
> Sorry, I hadn't realized the auth cache was actually owned by
HttpNetworkSession
> (I'm not terribly familiar with the auth code).
> 
> I believe I may have been wrong about the risk of sockets being reused in this
> particular case, I need to refresh my memory of just how that codepath works.
> 
> I also want to see if anyone else is doing this currently, which would make me
a
> bit less concerned about potential side effects.
> 
> I do still feel that we should not be using proxy fallback logic outside of
net/
> to muck with net's proxy service.  Are GCM requests often made when there's no
> network activity?  If not, we could just rely on the network logic to do all
the
> proxy fallback stuff.

I'm not sure about your object lifetimes, but in
google_apis/gcm/engine/connection_factory_impl.cc, you should probably be
calling CloseSocket() in ConnectionFactoryImpl::~ConnectionFactoryImpl() to make
absolutely sure it's never returned to the socket pool, regardless of how we
decide to do anything else.

I can't find any places where we use InitSocketHandleForTlsConnect or
InitSocketHandleForRawConnect with a HttpNetworkSession that's shared with a
URLRequestContext, so this would be a first.  The very idea of potentially
sharing sockets through the socket pool mechanism this way just seems like a bad
idea.  Unfortunately, socket pools and socket creation are pretty strongly
coupled right now, so bypassing the pools would be a pretty significant change.

I'm also still concerned about marking proxies as bad, particularly as the logic
in net/ potentially diverges.  I think it may make more sense to make it
possible to share the HttpAuthCache between sessions, though, unfortunately,
that means adding more stuff to HttpNetworkSession::Params.

[Nicolas Zea, 2014-07-10 19:59:41]

On 2014/07/09 17:28:19, mmenke wrote:
> On 2014/07/08 20:19:25, mmenke wrote:
> > On 2014/07/08 19:28:55, Nicolas Zea wrote:
> > > On 2014/07/08 17:46:06, mmenke wrote:
> > > > On 2014/07/08 17:32:46, Nicolas Zea wrote:
> > > > > On 2014/07/08 14:30:56, mmenke wrote:
> > > > > > On 2014/07/08 13:58:05, cbentzel wrote:
> > > > > > > Adding mmenke who can probably do a better review of this and
> > understand
> > > > the
> > > > > > > ramifications. My primary concern is around object lifetime
issues.
> > > > > > 
> > > > > > May not get to this today - I have a lot of codereviews, and I'll
need
> > to
> > > > > > unwrangle the ownership and threading models here.
> > > > > > 
> > > > > > What is the GCMClient?  The description seems to indicates you want
> > > profile
> > > > > auth
> > > > > > credentials, but it looks like there's one created globally (And one
> per
> > > > > profile
> > > > > > as well?)
> > > > > > 
> > > > > > It looks like you're using direct connections, rather than
> > > > HTTP/HTTPS/WS/WSS,
> > > > > > and sharing the network session means you're sharing the socket
pool. 
> > As
> > > a
> > > > > > result, you'll be subject to connection pool limits shared with
> wherever
> > > > > you're
> > > > > > getting the NetworkSession from, which seems like a problem.
> > > > > > 
> > > > > > You also have your own proxy fallback logic, which you're now
applying
> > to
> > > > the
> > > > > > proxy service managed by the network stack, which also seems
> > problematic.
> > > > > 
> > > > > Right, GCMClient is currently per-profile, but we're also adding
support
> > for
> > > a
> > > > > device-specific connection for those devices that need it (and for
when
> > > users
> > > > > aren't signed in).
> > > > > 
> > > > > And yes, the connection is a TLS connection with a custom protocol
built
> > for
> > > > > Android. The network session is, in the primary case (signed in
users),
> > > coming
> > > > > from the profile's request context. Else (non signed in users and
device
> > > > > specific connections) it comes from the system's request context.
> > > > > 
> > > > > The socket pool limits would just have the effect of delaying the
> > > connection,
> > > > > right? Conversely, it means that we're occupying one socket, creating
> more
> > > > > contention for other connections. How large are the limits? Is this
> likely
> > > to
> > > > > have a big impact?
> > > > 
> > > > The limits are 6 sockets per host/port/protocol/proxy pair, 256 sockets
> for
> > > the
> > > > session in total, 64 sockets per proxy.  Also, if you're using the same
> > > > host/port/proxy as there was an HTTP/HTTPS connection to, I believe
> there's
> > > > nothing stopping you from getting a connection previously used for that
> > > purpose.
> > > 
> > > Note that the host/port pairs we use do not support http, only the custom
> > > protocol mentioned. So that shouldn't be an issue in this case (but
> certainly
> > > something to keep in mind).
> > > 
> > > > 
> > > > > As far as the proxy fallback goes, by using the profile's network
> session
> > > can
> > > > we
> > > > > bypass the GCM proxy fallback logic and just rely on the one handled
by
> > the
> > > > > network stack? We're not doing anything special in our proxy
resolution
> > > logic
> > > > > that needs to be handled separately, it's just there because we had
> > > previously
> > > > > assumed it would be better to have our own network session, much like
> the
> > > > jingle
> > > > > connection (which much of our code was based on).
> > > > 
> > > > Proxy fallback logic is done at a higher level than the socket pools,
> > > > unfortunately.  If we've marked a proxy as bad because of HTTP failures,
> you
> > > get
> > > > the benefit of that, but we won't mark a proxy as bad and then retry at
> the
> > > > level you're connecting at.
> > > > 
> > > > In general, I think using the same HttpNetworkSession for HTTP[S]
requests
> > and
> > > > non-HTTP requests is a bad idea, and not something we're really set up
to
> > > > handle, unfortunately.  I think it's going to be much safer to just make
a
> > new
> > > > HttpNetworkSession, and just share a couple specific
> > > HttpNetworkSession::Params.
> > > 
> > > My concern with this is that, from the digging around I've done, it
doesn't
> > seem
> > > there's a good way to access/share the HttpAuthCache objects used by the
> > > profile's NetworkSession, and we don't want to duplicate the logic used to
> > > create them (we don't want to pop up new UI, or have to add special logic
to
> > > read system/device policy controlled credentials).
> > > 
> > > Perhaps I wasn't following the logic properly though. Which
> > > HttpNetworkSession::Params were you thinking of?
> > 
> > Sorry, I hadn't realized the auth cache was actually owned by
> HttpNetworkSession
> > (I'm not terribly familiar with the auth code).
> > 
> > I believe I may have been wrong about the risk of sockets being reused in
this
> > particular case, I need to refresh my memory of just how that codepath
works.
> > 
> > I also want to see if anyone else is doing this currently, which would make
me
> a
> > bit less concerned about potential side effects.
> > 
> > I do still feel that we should not be using proxy fallback logic outside of
> net/
> > to muck with net's proxy service.  Are GCM requests often made when there's
no
> > network activity?  If not, we could just rely on the network logic to do all
> the
> > proxy fallback stuff.
> 
> I'm not sure about your object lifetimes, but in
> google_apis/gcm/engine/connection_factory_impl.cc, you should probably be
> calling CloseSocket() in ConnectionFactoryImpl::~ConnectionFactoryImpl() to
make
> absolutely sure it's never returned to the socket pool, regardless of how we
> decide to do anything else.

Good point, I've filed crbug.com/392546 for that.
> 
> I can't find any places where we use InitSocketHandleForTlsConnect or
> InitSocketHandleForRawConnect with a HttpNetworkSession that's shared with a
> URLRequestContext, so this would be a first.  The very idea of potentially
> sharing sockets through the socket pool mechanism this way just seems like a
bad
> idea.  Unfortunately, socket pools and socket creation are pretty strongly
> coupled right now, so bypassing the pools would be a pretty significant
change.
> 
> I'm also still concerned about marking proxies as bad, particularly as the
logic
> in net/ potentially diverges.  I think it may make more sense to make it
> possible to share the HttpAuthCache between sessions, though, unfortunately,
> that means adding more stuff to HttpNetworkSession::Params.

Okay, I'll leave this patch on hold for now and take a look at how we might go
about sharing the HttpAuthCache. It sounds like there's enough possible
conflicts/subtleties here to pursue that approach.

[mmenke, 2014-07-10 20:08:58]

On 2014/07/10 19:59:41, Nicolas Zea wrote:
> On 2014/07/09 17:28:19, mmenke wrote:
> > I can't find any places where we use InitSocketHandleForTlsConnect or
> > InitSocketHandleForRawConnect with a HttpNetworkSession that's shared with a
> > URLRequestContext, so this would be a first.  The very idea of potentially
> > sharing sockets through the socket pool mechanism this way just seems like a
> bad
> > idea.  Unfortunately, socket pools and socket creation are pretty strongly
> > coupled right now, so bypassing the pools would be a pretty significant
> change.
> > 
> > I'm also still concerned about marking proxies as bad, particularly as the
> logic
> > in net/ potentially diverges.  I think it may make more sense to make it
> > possible to share the HttpAuthCache between sessions, though, unfortunately,
> > that means adding more stuff to HttpNetworkSession::Params.
> 
> Okay, I'll leave this patch on hold for now and take a look at how we might go
> about sharing the HttpAuthCache. It sounds like there's enough possible
> conflicts/subtleties here to pursue that approach.

Yea.  Despite my bellyaching about potential socket reuse issues, I suspect we
could work around them, though I'm not convince that's a good idea.  It's the
bad proxy list that I'm most concerned about.

[cbentzel, 2014-07-14 17:47:41]

On 2014/07/10 20:08:58, mmenke wrote:
> On 2014/07/10 19:59:41, Nicolas Zea wrote:
> > On 2014/07/09 17:28:19, mmenke wrote:
> > > I can't find any places where we use InitSocketHandleForTlsConnect or
> > > InitSocketHandleForRawConnect with a HttpNetworkSession that's shared with
a
> > > URLRequestContext, so this would be a first.  The very idea of potentially
> > > sharing sockets through the socket pool mechanism this way just seems like
a
> > bad
> > > idea.  Unfortunately, socket pools and socket creation are pretty strongly
> > > coupled right now, so bypassing the pools would be a pretty significant
> > change.
> > > 
> > > I'm also still concerned about marking proxies as bad, particularly as the
> > logic
> > > in net/ potentially diverges.  I think it may make more sense to make it
> > > possible to share the HttpAuthCache between sessions, though,
unfortunately,
> > > that means adding more stuff to HttpNetworkSession::Params.
> > 
> > Okay, I'll leave this patch on hold for now and take a look at how we might
go
> > about sharing the HttpAuthCache. It sounds like there's enough possible
> > conflicts/subtleties here to pursue that approach.
> 
> Yea.  Despite my bellyaching about potential socket reuse issues, I suspect we
> could work around them, though I'm not convince that's a good idea.  It's the
> bad proxy list that I'm most concerned about.

Quick question: How frequently is this context created? We have code in
HttpAuthCache to clone all credentials (the UpdateAllFrom) which could
potentially be used in this case.

[Nicolas Zea, 2014-07-14 20:57:59]

> Quick question: How frequently is this context created? We have code in
> HttpAuthCache to clone all credentials (the UpdateAllFrom) which could
> potentially be used in this case.

The lifetime of our network session is basically tied to the lifetime of the
profile, so it's fairly long-lived. That said, we'd still need some sort of
observer/change notification interface to know when to actually copy over the
data, right?

[mmenke, 2014-07-14 21:06:19]

On 2014/07/14 20:57:59, Nicolas Zea wrote:
> > Quick question: How frequently is this context created? We have code in
> > HttpAuthCache to clone all credentials (the UpdateAllFrom) which could
> > potentially be used in this case.
> 
> The lifetime of our network session is basically tied to the lifetime of the
> profile, so it's fairly long-lived. That said, we'd still need some sort of
> observer/change notification interface to know when to actually copy over the
> data, right?

If it was just created as needed, and then destroyed immediately after use, you
wouldn't, which I think is what Chris was getting at.

[cbentzel, 2014-07-16 16:28:17]

On 2014/07/14 21:06:19, mmenke wrote:
> On 2014/07/14 20:57:59, Nicolas Zea wrote:
> > > Quick question: How frequently is this context created? We have code in
> > > HttpAuthCache to clone all credentials (the UpdateAllFrom) which could
> > > potentially be used in this case.
> > 
> > The lifetime of our network session is basically tied to the lifetime of the
> > profile, so it's fairly long-lived. That said, we'd still need some sort of
> > observer/change notification interface to know when to actually copy over
the
> > data, right?
> 
> If it was just created as needed, and then destroyed immediately after use,
you
> wouldn't, which I think is what Chris was getting at.

Yes, that was what I was getting at.

[Nicolas Zea, 2014-07-16 23:01:09]

PTAL. Patch now passes in the profile's HttpNetworkSession as an optional param,
from which GCM's HttpNetworkSession will consume the HttpAuthCache (via
UpdateAllFrom).

Tested manually and this handles proxy auth swimmingly (once we retry the
connection).

[fgorski, 2014-07-17 04:23:25]

On 2014/07/16 23:01:09, Nicolas Zea wrote:
> PTAL. Patch now passes in the profile's HttpNetworkSession as an optional
param,
> from which GCM's HttpNetworkSession will consume the HttpAuthCache (via
> UpdateAllFrom).
> 
> Tested manually and this handles proxy auth swimmingly (once we retry the
> connection).

A couple of nits. I'd put gcm_network_session first everywhere as it is
mandatory.
still looks ok, but I know you are waiting for lg from networking people.

[mmenke, 2014-07-17 18:34:07]

This approach LGTM.

https://codereview.chromium.org/375663002/diff/80001/components/gcm_driver/gc...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/375663002/diff/80001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:284: GetNetworkSessionParams();
Oh...You're already using the main proxy service, and modifying its bad proxy
list.  I hadn't realized that.  Hrm...still think this is the better approach,
though.

[Nicolas Zea, 2014-07-17 19:52:29]

Updated ordering of params and committing via Commitbot

[Nicolas Zea, 2014-07-17 19:52:39]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-07-17 19:54:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/375663002/100001

[Nicolas Zea, 2014-07-17 20:00:09]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-07-17 20:00:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/375663002/120001

[commit-bot: I haz the power, 2014-07-17 23:58:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-07-18 00:25:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:25:02]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Nicolas Zea, 2014-07-18 00:27:02]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:28:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/375663002/120001

[commit-bot: I haz the power, 2014-07-18 02:19:00]

Change committed as 283973