Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
414523005:
Handles proxy authentication request in ProxyResolvingClientSocket (Closed)
DescriptionHandles proxy authentication request in ProxyResolvingClientSocket.
This will make the WebRTC connection work under NTLM/Kerberos using the default credential without prompting the user.
BUG=395614
Committed: https://crrev.com/260631aa62e2096045adb0e3780ef562ef12bb4f
Cr-Commit-Position: refs/heads/master@{#292428}
Patch Set 1 : #Patch Set 2 : #
Total comments: 5
Patch Set 3 : #
Total comments: 3
Patch Set 4 : Check HaveAuth to avoid loop #
Total comments: 5
Patch Set 5 : #Messages
Total messages: 50 (0 generated)
PTAL as discussed in the bug. I see in WireShark that a new "GET" request is triggered and the proxy server returns '407' again, but Chrome does not prompt for credentials.
Where can I see where the URLRequestContext is constructed for this?
On 2014/07/22 23:19:50, willchan wrote: > Where can I see where the URLRequestContext is constructed for this? It's passed from here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
On 2014/07/22 23:23:38, jiayl wrote: > On 2014/07/22 23:19:50, willchan wrote: > > Where can I see where the URLRequestContext is constructed for this? > > It's passed from here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... Where is it constructed?
On 2014/07/22 23:36:21, willchan wrote: > On 2014/07/22 23:23:38, jiayl wrote: > > On 2014/07/22 23:19:50, willchan wrote: > > > Where can I see where the URLRequestContext is constructed for this? > > > > It's passed from here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > Where is it constructed? I don't know. jam@, could you help answer the question, i.e. where is the URLRequestContext from browser_context->GetRequestContextForRenderProcess(GetID())) constructed?
https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... jingle/glue/proxy_resolving_client_socket.cc:214: void ProxyResolvingClientSocket::HttpNetworkTransactionDone(int status) { What's the status here? Does it match ERR_PROXY_AUTH_REQUESTED (https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error...) which is -127?
https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... jingle/glue/proxy_resolving_client_socket.cc:214: void ProxyResolvingClientSocket::HttpNetworkTransactionDone(int status) { On 2014/07/22 23:46:47, willchan wrote: > What's the status here? Does it match ERR_PROXY_AUTH_REQUESTED > (https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error...) > which is -127? Yes, it's -127 again.
https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... jingle/glue/proxy_resolving_client_socket.cc:214: void ProxyResolvingClientSocket::HttpNetworkTransactionDone(int status) { On 2014/07/22 23:47:28, jiayl wrote: > On 2014/07/22 23:46:47, willchan wrote: > > What's the status here? Does it match ERR_PROXY_AUTH_REQUESTED > > > (https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error...) > > which is -127? > > Yes, it's -127 again. Well, your code needs to handle that. Here's an example of how URLRequest[Http]Job (which owns the HttpTransaction) handles that: https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur....
https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... jingle/glue/proxy_resolving_client_socket.cc:214: void ProxyResolvingClientSocket::HttpNetworkTransactionDone(int status) { On 2014/07/23 00:05:19, willchan wrote: > On 2014/07/22 23:47:28, jiayl wrote: > > On 2014/07/22 23:46:47, willchan wrote: > > > What's the status here? Does it match ERR_PROXY_AUTH_REQUESTED > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error...) > > > which is -127? > > > > Yes, it's -127 again. > > Well, your code needs to handle that. Here's an example of how > URLRequest[Http]Job (which owns the HttpTransaction) handles that: > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur.... And here's the relevant code in ResourceLoader: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo....
On 2014/07/23 00:08:07, willchan wrote: > https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... > File jingle/glue/proxy_resolving_client_socket.cc (right): > > https://codereview.chromium.org/414523005/diff/1/jingle/glue/proxy_resolving_... > jingle/glue/proxy_resolving_client_socket.cc:214: void > ProxyResolvingClientSocket::HttpNetworkTransactionDone(int status) { > On 2014/07/23 00:05:19, willchan wrote: > > On 2014/07/22 23:47:28, jiayl wrote: > > > On 2014/07/22 23:46:47, willchan wrote: > > > > What's the status here? Does it match ERR_PROXY_AUTH_REQUESTED > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error...) > > > > which is -127? > > > > > > Yes, it's -127 again. > > > > Well, your code needs to handle that. Here's an example of how > > URLRequest[Http]Job (which owns the HttpTransaction) handles that: > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur.... > > And here's the relevant code in ResourceLoader: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo.... I still have some questions: Chrome only prompts once even if I open multiple tabs for multiple sites. So there must be a way for the tabs to share the credential. I would like to get the saved credential instead of prompting the user again. Could you give me some pointers on how to achieve that?
+asanka, +rsleevi So, the HTTP auth credentials are shared in the URLRequestContext. Since you appear to be using the same URLRequestContext in the WebRTC code, I think you should have access to those credentials. Most likely your code does not handle checking the auth credentials when needed. I don't have time to re-page that logic back into memory, but I bet asanka@ still knows it. It's spread across the transaction logic and the socket pools.
On 2014/07/23 18:59:23, willchan wrote: > +asanka, +rsleevi > > So, the HTTP auth credentials are shared in the URLRequestContext. Since you > appear to be using the same URLRequestContext in the WebRTC code, I think you > should have access to those credentials. Most likely your code does not handle > checking the auth credentials when needed. I don't have time to re-page that > logic back into memory, but I bet asanka@ still knows it. It's spread across the > transaction logic and the socket pools. FYI I checked that the http_auth_cache from the URLRequestContext is empty we try to connect to the TURN server, but Chrome already got the credentials for site navigation.
Hm, if that's the case, can you double check what URLRequestContext you're getting? Does it match the "main" request context?
They have the save request context. On Wed, Jul 23, 2014 at 2:20 PM, <willchan@chromium.org> wrote: > Hm, if that's the case, can you double check what URLRequestContext you're > getting? Does it match the "main" request context? > > https://codereview.chromium.org/414523005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Please take a look at the new patch. I figured out that the http credential will be reused if I pass url_request_context->http_transaction_factory()->GetSession() to net::InitSocketHandleForRawConnect, as in the new patch. Then I see the CONNECT request for the TURN server got the right credentials, but I get a different errors from the proxy server (i.e. ERR_CONNECT_FAIL). I don't know if it's due to the reuse of HttpNetworkSession. Do you see any problem in reusing the HttpNetworkSession? I have very little knowledge about how all these "factory"/"contect"/"session" objects work together, so I might be doing something completely wrong. On Wed, Jul 23, 2014 at 2:33 PM, Jiayang Liu <jiayl@chromium.org> wrote: > They have the save request context. > > > On Wed, Jul 23, 2014 at 2:20 PM, <willchan@chromium.org> wrote: > >> Hm, if that's the case, can you double check what URLRequestContext you're >> getting? Does it match the "main" request context? >> >> https://codereview.chromium.org/414523005/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hm, I thought we shared all the http auth state when reusing URLRequestContext, but it appears that the HttpAuthCache is *not* shared. It's a member variable in HttpNetworkSession. I suspect we simply never needed to share the auth cache, but if there's a reason to share it, I don't see any problem with moving it out to URLRequestContext. Asanka?
On 2014/07/23 22:28:15, willchan wrote: > Hm, I thought we shared all the http auth state when reusing URLRequestContext, > but it appears that the HttpAuthCache is *not* shared. It's a member variable in > HttpNetworkSession. I suspect we simply never needed to share the auth cache, > but if there's a reason to share it, I don't see any problem with moving it out > to URLRequestContext. Asanka? Note that it's actually easier for this code to share the cache as it is, since net::InitSocketHandleForRawConnect does not take a URLRequestContext as input, but takes HttpNetworkSession.
Don't do that :) Unless you want to share the socket pools, which sounds like trouble. On Wed, Jul 23, 2014 at 3:31 PM, <jiayl@chromium.org> wrote: > On 2014/07/23 22:28:15, willchan wrote: > >> Hm, I thought we shared all the http auth state when reusing >> > URLRequestContext, > >> but it appears that the HttpAuthCache is *not* shared. It's a member >> variable >> > in > >> HttpNetworkSession. I suspect we simply never needed to share the auth >> cache, >> but if there's a reason to share it, I don't see any problem with moving >> it >> > out > >> to URLRequestContext. Asanka? >> > > Note that it's actually easier for this code to share the cache as it is, > since net::InitSocketHandleForRawConnect does not take a URLRequestContext > as input, but takes HttpNetworkSession. > > > > https://codereview.chromium.org/414523005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Then how can I share the HttpAuthCache based on the existing interface of net::InitSocketHandleForRawConnect? On Wed, Jul 23, 2014 at 3:45 PM, William Chan (陈智昌) <willchan@chromium.org> wrote: > Don't do that :) Unless you want to share the socket pools, which sounds > like trouble. > > > On Wed, Jul 23, 2014 at 3:31 PM, <jiayl@chromium.org> wrote: > >> On 2014/07/23 22:28:15, willchan wrote: >> >>> Hm, I thought we shared all the http auth state when reusing >>> >> URLRequestContext, >> >>> but it appears that the HttpAuthCache is *not* shared. It's a member >>> variable >>> >> in >> >>> HttpNetworkSession. I suspect we simply never needed to share the auth >>> cache, >>> but if there's a reason to share it, I don't see any problem with moving >>> it >>> >> out >> >>> to URLRequestContext. Asanka? >>> >> >> Note that it's actually easier for this code to share the cache as it is, >> since net::InitSocketHandleForRawConnect does not take a >> URLRequestContext >> as input, but takes HttpNetworkSession. >> >> >> >> https://codereview.chromium.org/414523005/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Or could I copy the HttpAuthCache from the existing HttpNetworkSession to my new HttpNetworkSession? On Wed, Jul 23, 2014 at 3:46 PM, Jiayang Liu <jiayl@chromium.org> wrote: > Then how can I share the HttpAuthCache based on the existing interface > of net::InitSocketHandleForRawConnect? > > > On Wed, Jul 23, 2014 at 3:45 PM, William Chan (陈智昌) <willchan@chromium.org > > wrote: > >> Don't do that :) Unless you want to share the socket pools, which sounds >> like trouble. >> >> >> On Wed, Jul 23, 2014 at 3:31 PM, <jiayl@chromium.org> wrote: >> >>> On 2014/07/23 22:28:15, willchan wrote: >>> >>>> Hm, I thought we shared all the http auth state when reusing >>>> >>> URLRequestContext, >>> >>>> but it appears that the HttpAuthCache is *not* shared. It's a member >>>> variable >>>> >>> in >>> >>>> HttpNetworkSession. I suspect we simply never needed to share the auth >>>> cache, >>>> but if there's a reason to share it, I don't see any problem with >>>> moving it >>>> >>> out >>> >>>> to URLRequestContext. Asanka? >>>> >>> >>> Note that it's actually easier for this code to share the cache as it is, >>> since net::InitSocketHandleForRawConnect does not take a >>> URLRequestContext >>> as input, but takes HttpNetworkSession. >>> >>> >>> >>> https://codereview.chromium.org/414523005/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That's why I asked Asanka if we could pull the HttpAuthCache out to the URLRequestContext. On Wed, Jul 23, 2014 at 3:53 PM, Jiayang Liu <jiayl@chromium.org> wrote: > Or could I copy the HttpAuthCache from the existing HttpNetworkSession to > my new HttpNetworkSession? > > > On Wed, Jul 23, 2014 at 3:46 PM, Jiayang Liu <jiayl@chromium.org> wrote: > >> Then how can I share the HttpAuthCache based on the existing interface >> of net::InitSocketHandleForRawConnect? >> >> >> On Wed, Jul 23, 2014 at 3:45 PM, William Chan (陈智昌) < >> willchan@chromium.org> wrote: >> >>> Don't do that :) Unless you want to share the socket pools, which sounds >>> like trouble. >>> >>> >>> On Wed, Jul 23, 2014 at 3:31 PM, <jiayl@chromium.org> wrote: >>> >>>> On 2014/07/23 22:28:15, willchan wrote: >>>> >>>>> Hm, I thought we shared all the http auth state when reusing >>>>> >>>> URLRequestContext, >>>> >>>>> but it appears that the HttpAuthCache is *not* shared. It's a member >>>>> variable >>>>> >>>> in >>>> >>>>> HttpNetworkSession. I suspect we simply never needed to share the auth >>>>> cache, >>>>> but if there's a reason to share it, I don't see any problem with >>>>> moving it >>>>> >>>> out >>>> >>>>> to URLRequestContext. Asanka? >>>>> >>>> >>>> Note that it's actually easier for this code to share the cache as it >>>> is, >>>> since net::InitSocketHandleForRawConnect does not take a >>>> URLRequestContext >>>> as input, but takes HttpNetworkSession. >>>> >>>> >>>> >>>> https://codereview.chromium.org/414523005/ >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That won't make much difference, as HttpAuthCache still needs to be copied from URLRequestContext to the new HttpNetworkSession. On Wed, Jul 23, 2014 at 3:55 PM, William Chan (陈智昌) <willchan@chromium.org> wrote: > That's why I asked Asanka if we could pull the HttpAuthCache out to the > URLRequestContext. > > > On Wed, Jul 23, 2014 at 3:53 PM, Jiayang Liu <jiayl@chromium.org> wrote: > >> Or could I copy the HttpAuthCache from the existing HttpNetworkSession to >> my new HttpNetworkSession? >> >> >> On Wed, Jul 23, 2014 at 3:46 PM, Jiayang Liu <jiayl@chromium.org> wrote: >> >>> Then how can I share the HttpAuthCache based on the existing interface >>> of net::InitSocketHandleForRawConnect? >>> >>> >>> On Wed, Jul 23, 2014 at 3:45 PM, William Chan (陈智昌) < >>> willchan@chromium.org> wrote: >>> >>>> Don't do that :) Unless you want to share the socket pools, which >>>> sounds like trouble. >>>> >>>> >>>> On Wed, Jul 23, 2014 at 3:31 PM, <jiayl@chromium.org> wrote: >>>> >>>>> On 2014/07/23 22:28:15, willchan wrote: >>>>> >>>>>> Hm, I thought we shared all the http auth state when reusing >>>>>> >>>>> URLRequestContext, >>>>> >>>>>> but it appears that the HttpAuthCache is *not* shared. It's a member >>>>>> variable >>>>>> >>>>> in >>>>> >>>>>> HttpNetworkSession. I suspect we simply never needed to share the >>>>>> auth cache, >>>>>> but if there's a reason to share it, I don't see any problem with >>>>>> moving it >>>>>> >>>>> out >>>>> >>>>>> to URLRequestContext. Asanka? >>>>>> >>>>> >>>>> Note that it's actually easier for this code to share the cache as it >>>>> is, >>>>> since net::InitSocketHandleForRawConnect does not take a >>>>> URLRequestContext >>>>> as input, but takes HttpNetworkSession. >>>>> >>>>> >>>>> >>>>> https://codereview.chromium.org/414523005/ >>>>> >>>> >>>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Why not share the object like we do with most members in HttpNetworkSession? On Wed, Jul 23, 2014 at 4:07 PM, Jiayang Liu <jiayl@chromium.org> wrote: > That won't make much difference, as HttpAuthCache still needs to be copied > from URLRequestContext to the new HttpNetworkSession. > > > On Wed, Jul 23, 2014 at 3:55 PM, William Chan (陈智昌) <willchan@chromium.org > > wrote: > >> That's why I asked Asanka if we could pull the HttpAuthCache out to the >> URLRequestContext. >> >> >> On Wed, Jul 23, 2014 at 3:53 PM, Jiayang Liu <jiayl@chromium.org> wrote: >> >>> Or could I copy the HttpAuthCache from the existing HttpNetworkSession >>> to my new HttpNetworkSession? >>> >>> >>> On Wed, Jul 23, 2014 at 3:46 PM, Jiayang Liu <jiayl@chromium.org> wrote: >>> >>>> Then how can I share the HttpAuthCache based on the existing interface >>>> of net::InitSocketHandleForRawConnect? >>>> >>>> >>>> On Wed, Jul 23, 2014 at 3:45 PM, William Chan (陈智昌) < >>>> willchan@chromium.org> wrote: >>>> >>>>> Don't do that :) Unless you want to share the socket pools, which >>>>> sounds like trouble. >>>>> >>>>> >>>>> On Wed, Jul 23, 2014 at 3:31 PM, <jiayl@chromium.org> wrote: >>>>> >>>>>> On 2014/07/23 22:28:15, willchan wrote: >>>>>> >>>>>>> Hm, I thought we shared all the http auth state when reusing >>>>>>> >>>>>> URLRequestContext, >>>>>> >>>>>>> but it appears that the HttpAuthCache is *not* shared. It's a member >>>>>>> variable >>>>>>> >>>>>> in >>>>>> >>>>>>> HttpNetworkSession. I suspect we simply never needed to share the >>>>>>> auth cache, >>>>>>> but if there's a reason to share it, I don't see any problem with >>>>>>> moving it >>>>>>> >>>>>> out >>>>>> >>>>>>> to URLRequestContext. Asanka? >>>>>>> >>>>>> >>>>>> Note that it's actually easier for this code to share the cache as it >>>>>> is, >>>>>> since net::InitSocketHandleForRawConnect does not take a >>>>>> URLRequestContext >>>>>> as input, but takes HttpNetworkSession. >>>>>> >>>>>> >>>>>> >>>>>> https://codereview.chromium.org/414523005/ >>>>>> >>>>> >>>>> >>>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ah, I see what you mean. Haha. Sounds good. On Wed, Jul 23, 2014 at 4:18 PM, William Chan (陈智昌) <willchan@chromium.org> wrote: > Why not share the object like we do with most members in > HttpNetworkSession? > > > On Wed, Jul 23, 2014 at 4:07 PM, Jiayang Liu <jiayl@chromium.org> wrote: > >> That won't make much difference, as HttpAuthCache still needs to be >> copied from URLRequestContext to the new HttpNetworkSession. >> >> >> On Wed, Jul 23, 2014 at 3:55 PM, William Chan (陈智昌) < >> willchan@chromium.org> wrote: >> >>> That's why I asked Asanka if we could pull the HttpAuthCache out to the >>> URLRequestContext. >>> >>> >>> On Wed, Jul 23, 2014 at 3:53 PM, Jiayang Liu <jiayl@chromium.org> wrote: >>> >>>> Or could I copy the HttpAuthCache from the existing HttpNetworkSession >>>> to my new HttpNetworkSession? >>>> >>>> >>>> On Wed, Jul 23, 2014 at 3:46 PM, Jiayang Liu <jiayl@chromium.org> >>>> wrote: >>>> >>>>> Then how can I share the HttpAuthCache based on the existing interface >>>>> of net::InitSocketHandleForRawConnect? >>>>> >>>>> >>>>> On Wed, Jul 23, 2014 at 3:45 PM, William Chan (陈智昌) < >>>>> willchan@chromium.org> wrote: >>>>> >>>>>> Don't do that :) Unless you want to share the socket pools, which >>>>>> sounds like trouble. >>>>>> >>>>>> >>>>>> On Wed, Jul 23, 2014 at 3:31 PM, <jiayl@chromium.org> wrote: >>>>>> >>>>>>> On 2014/07/23 22:28:15, willchan wrote: >>>>>>> >>>>>>>> Hm, I thought we shared all the http auth state when reusing >>>>>>>> >>>>>>> URLRequestContext, >>>>>>> >>>>>>>> but it appears that the HttpAuthCache is *not* shared. It's a >>>>>>>> member variable >>>>>>>> >>>>>>> in >>>>>>> >>>>>>>> HttpNetworkSession. I suspect we simply never needed to share the >>>>>>>> auth cache, >>>>>>>> but if there's a reason to share it, I don't see any problem with >>>>>>>> moving it >>>>>>>> >>>>>>> out >>>>>>> >>>>>>>> to URLRequestContext. Asanka? >>>>>>>> >>>>>>> >>>>>>> Note that it's actually easier for this code to share the cache as >>>>>>> it is, >>>>>>> since net::InitSocketHandleForRawConnect does not take a >>>>>>> URLRequestContext >>>>>>> as input, but takes HttpNetworkSession. >>>>>>> >>>>>>> >>>>>>> >>>>>>> https://codereview.chromium.org/414523005/ >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
asanka@, could you answer willchan's question below? "Hm, I thought we shared all the http auth state when reusing URLRequestContext, but it appears that the HttpAuthCache is *not* shared. It's a member variable in HttpNetworkSession. I suspect we simply never needed to share the auth cache, but if there's a reason to share it, I don't see any problem with moving it out to URLRequestContext. Asanka?"
On 2014/07/24 16:31:13, jiayl wrote: > asanka@, > > could you answer willchan's question below? > > "Hm, I thought we shared all the http auth state when reusing URLRequestContext, > but it appears that the HttpAuthCache is *not* shared. It's a member variable in > HttpNetworkSession. I suspect we simply never needed to share the auth cache, > but if there's a reason to share it, I don't see any problem with moving it out > to URLRequestContext. Asanka?" The HttpAuthCache is meant to be a ephemeral convenience and carries the user's identity selection state. I'm a bit apprehensive about implicitly sharing the cache in the way that I think is being proposed. This doesn't mean it's not safe all the time, and it certainly looks OK in this case. We do support one-way explicit propagation of cache entries via HttpAuthCache::UpdateAllFrom(). Have you tried using that? What about client certificate selection state? Also what about the case where the user needs to be prompted for credentials?
On 2014/07/24 19:48:41, asanka wrote: > On 2014/07/24 16:31:13, jiayl wrote: > > asanka@, > > > > could you answer willchan's question below? > > > > "Hm, I thought we shared all the http auth state when reusing > URLRequestContext, > > but it appears that the HttpAuthCache is *not* shared. It's a member variable > in > > HttpNetworkSession. I suspect we simply never needed to share the auth cache, > > but if there's a reason to share it, I don't see any problem with moving it > out > > to URLRequestContext. Asanka?" > > The HttpAuthCache is meant to be a ephemeral convenience and carries the user's > identity selection state. I'm a bit apprehensive about implicitly sharing the > cache in the way that I think is being proposed. This doesn't mean it's not safe > all the time, and it certainly looks OK in this case. > > We do support one-way explicit propagation of cache entries via > HttpAuthCache::UpdateAllFrom(). Have you tried using that? What about client > certificate selection state? Also what about the case where the user needs to be > prompted for credentials? Using UpdateAllFrom also works. Please see the new patch. When will the user need to select client certificate? I'm not sure if a user prompt should be triggered from here, but it should a rare case.
On 2014/07/24 20:31:41, jiayl wrote: > On 2014/07/24 19:48:41, asanka wrote: > > On 2014/07/24 16:31:13, jiayl wrote: > > > asanka@, > > > > > > could you answer willchan's question below? > > > > > > "Hm, I thought we shared all the http auth state when reusing > > URLRequestContext, > > > but it appears that the HttpAuthCache is *not* shared. It's a member > variable > > in > > > HttpNetworkSession. I suspect we simply never needed to share the auth > cache, > > > but if there's a reason to share it, I don't see any problem with moving it > > out > > > to URLRequestContext. Asanka?" > > > > The HttpAuthCache is meant to be a ephemeral convenience and carries the > user's > > identity selection state. I'm a bit apprehensive about implicitly sharing the > > cache in the way that I think is being proposed. This doesn't mean it's not > safe > > all the time, and it certainly looks OK in this case. > > > > We do support one-way explicit propagation of cache entries via > > HttpAuthCache::UpdateAllFrom(). Have you tried using that? What about client > > certificate selection state? Also what about the case where the user needs to > be > > prompted for credentials? > > Using UpdateAllFrom also works. Please see the new patch. > > When will the user need to select client certificate? > > I'm not sure if a user prompt should be triggered from here, but it should > a rare case. Sorry for chiming in late, but I don't think we should do this. This is one of those things I've been harping on regarding proxies and user expectations. There are a *lot* of things that are wired up to our HTTP stack that just don't exist elsewhere. Things like enterprise policies for selecting client certs, among others. These are tricky to get right, but I think they're important. They're something we kinda-work but kinda-don't with other types of requests even more "webby" (Namely, XHR, WSS, Pepper/NaCl, and <webview>). I understand the importance for WebRTC, but I think it's no less important for the others, and I think the most important thing for our users is that things work consistently. I would even go as far as argue that it's *more* important than things "working", since inconsistent behaviours always trigger at the worst times and to the worst experience. I feel like the //net stack should be the arbitrar here, and we should figure out how best to wire it up for WebRTC. The biggest concern here is that the URLRequestContext (and delegate) are the most important things for giving the higher layer (aka //content) visibility into the requests, as well as preserving the invariants - whether it be for privacy reasons or for enterprise reasons.
Adding back chromium-reviews which was removed accidentally. On Fri, Jul 25, 2014 at 5:56 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > Rather than fork this thread, can we keep it on the code review? Or > discuss on net-dev? I think this should be the extent of things I'll say on > a non-public list. > > Inconsistent handling of proxies/auth is a sort of hobby horse of mine, > because each feature-team not using URLRequest tends to implement things in > incompatible ways. > > I think we do want to try to get proxies working. However, my preferences > (in order) > 1) Do the right thing, always > 2) Do the quick thing, now, but identify where we're not doing the right > thing, and have a plan for it, and be working on it > 3) Do nothing > > I feel like the path we're doing is #4, which is do the quick thing now, > but leave the rest as a sort of "I don't know", without a concrete plan to > tackle it. That's why I want to push back, make sure we look at the > *whole* problem (and not just piecemeal it), and then figure out how we > can help WebRTC get where it needs to go. > > Do you mind explaining what's the "right thing" and what's the "whole problem"? What are the other aspects of proxy/auth we should handle for webrtc? And what is your suggestion to solve it in the right way? I can schedule a meeting to talk about this if you prefer. Thanks! > > On Fri, Jul 25, 2014 at 9:27 AM, Jiayang Liu <jiayl@chromium.org> wrote: > >> Could you explain more on what is the consistent behavior we have today, >> or we want to achieve in the future? >> >> It's inconsistent for WebRTC right now. TURN servers will work over a >> http proxy if it does not require authentication, but fails if it does. It >> would be consistent if TURN always fails over http proxy, or always work. >> >> >> >> >> >> On Thu, Jul 24, 2014 at 5:51 PM, <rsleevi@chromium.org> wrote: >> >>> On 2014/07/24 20:31:41, jiayl wrote: >>> >>>> On 2014/07/24 19:48:41, asanka wrote: >>>> > On 2014/07/24 16:31:13, jiayl wrote: >>>> > > asanka@, >>>> > > >>>> > > could you answer willchan's question below? >>>> > > >>>> > > "Hm, I thought we shared all the http auth state when reusing >>>> > URLRequestContext, >>>> > > but it appears that the HttpAuthCache is *not* shared. It's a member >>>> variable >>>> > in >>>> > > HttpNetworkSession. I suspect we simply never needed to share the >>>> auth >>>> cache, >>>> > > but if there's a reason to share it, I don't see any problem with >>>> moving >>>> >>> it >>> >>>> > out >>>> > > to URLRequestContext. Asanka?" >>>> > >>>> > The HttpAuthCache is meant to be a ephemeral convenience and carries >>>> the >>>> user's >>>> > identity selection state. I'm a bit apprehensive about implicitly >>>> sharing >>>> >>> the >>> >>>> > cache in the way that I think is being proposed. This doesn't mean >>>> it's not >>>> safe >>>> > all the time, and it certainly looks OK in this case. >>>> > >>>> > We do support one-way explicit propagation of cache entries via >>>> > HttpAuthCache::UpdateAllFrom(). Have you tried using that? What >>>> about client >>>> > certificate selection state? Also what about the case where the user >>>> needs >>>> >>> to >>> >>>> be >>>> > prompted for credentials? >>>> >>> >>> Using UpdateAllFrom also works. Please see the new patch. >>>> >>> >>> When will the user need to select client certificate? >>>> >>> >>> I'm not sure if a user prompt should be triggered from here, but it >>>> should >>>> a rare case. >>>> >>> >>> Sorry for chiming in late, but I don't think we should do this. >>> >>> This is one of those things I've been harping on regarding proxies and >>> user >>> expectations. There are a *lot* of things that are wired up to our HTTP >>> stack >>> that just don't exist elsewhere. Things like enterprise policies for >>> selecting >>> client certs, among others. >>> >>> These are tricky to get right, but I think they're important. They're >>> something >>> we kinda-work but kinda-don't with other types of requests even more >>> "webby" >>> (Namely, XHR, WSS, Pepper/NaCl, and <webview>). >>> >>> I understand the importance for WebRTC, but I think it's no less >>> important for >>> the others, and I think the most important thing for our users is that >>> things >>> work consistently. I would even go as far as argue that it's *more* >>> important >>> than things "working", since inconsistent behaviours always trigger at >>> the worst >>> times and to the worst experience. >>> >>> I feel like the //net stack should be the arbitrar here, and we should >>> figure >>> out how best to wire it up for WebRTC. The biggest concern here is that >>> the >>> URLRequestContext (and delegate) are the most important things for >>> giving the >>> higher layer (aka //content) visibility into the requests, as well as >>> preserving >>> the invariants - whether it be for privacy reasons or for enterprise >>> reasons. >>> >>> https://codereview.chromium.org/414523005/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yeah, you probably want to schedule a meeting with Ryan. But I do support what Ryan's saying. The problem with the WebRTC code is it is re-duplicating piecemeal all of the authentication flows (not just HTTP auth) that we've already labored hard to maintain in the normal URLRequest flows. So, refactoring the core code and sharing more of it is best, otherwise you're basically doomed to have a whole bunch of edge case bugs which we will fix in the main code and you'll continue to have broken. On Mon, Jul 28, 2014 at 10:40 AM, Jiayang Liu <jiayl@chromium.org> wrote: > Adding back chromium-reviews which was removed accidentally. > > > On Fri, Jul 25, 2014 at 5:56 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > >> Rather than fork this thread, can we keep it on the code review? Or >> discuss on net-dev? I think this should be the extent of things I'll say on >> a non-public list. >> >> Inconsistent handling of proxies/auth is a sort of hobby horse of mine, >> because each feature-team not using URLRequest tends to implement things in >> incompatible ways. >> >> I think we do want to try to get proxies working. However, my preferences >> (in order) >> 1) Do the right thing, always >> 2) Do the quick thing, now, but identify where we're not doing the right >> thing, and have a plan for it, and be working on it >> 3) Do nothing >> >> I feel like the path we're doing is #4, which is do the quick thing now, >> but leave the rest as a sort of "I don't know", without a concrete plan to >> tackle it. That's why I want to push back, make sure we look at the >> *whole* problem (and not just piecemeal it), and then figure out how we >> can help WebRTC get where it needs to go. >> >> Do you mind explaining what's the "right thing" and what's the "whole > problem"? > What are the other aspects of proxy/auth we should handle for webrtc? And > what is your suggestion to solve it in the right way? > > I can schedule a meeting to talk about this if you prefer. > > Thanks! > >> >> On Fri, Jul 25, 2014 at 9:27 AM, Jiayang Liu <jiayl@chromium.org> wrote: >> >>> Could you explain more on what is the consistent behavior we have today, >>> or we want to achieve in the future? >>> >>> It's inconsistent for WebRTC right now. TURN servers will work over a >>> http proxy if it does not require authentication, but fails if it does. It >>> would be consistent if TURN always fails over http proxy, or always work. >>> >>> >>> >>> >>> >>> On Thu, Jul 24, 2014 at 5:51 PM, <rsleevi@chromium.org> wrote: >>> >>>> On 2014/07/24 20:31:41, jiayl wrote: >>>> >>>>> On 2014/07/24 19:48:41, asanka wrote: >>>>> > On 2014/07/24 16:31:13, jiayl wrote: >>>>> > > asanka@, >>>>> > > >>>>> > > could you answer willchan's question below? >>>>> > > >>>>> > > "Hm, I thought we shared all the http auth state when reusing >>>>> > URLRequestContext, >>>>> > > but it appears that the HttpAuthCache is *not* shared. It's a >>>>> member >>>>> variable >>>>> > in >>>>> > > HttpNetworkSession. I suspect we simply never needed to share the >>>>> auth >>>>> cache, >>>>> > > but if there's a reason to share it, I don't see any problem with >>>>> moving >>>>> >>>> it >>>> >>>>> > out >>>>> > > to URLRequestContext. Asanka?" >>>>> > >>>>> > The HttpAuthCache is meant to be a ephemeral convenience and carries >>>>> the >>>>> user's >>>>> > identity selection state. I'm a bit apprehensive about implicitly >>>>> sharing >>>>> >>>> the >>>> >>>>> > cache in the way that I think is being proposed. This doesn't mean >>>>> it's not >>>>> safe >>>>> > all the time, and it certainly looks OK in this case. >>>>> > >>>>> > We do support one-way explicit propagation of cache entries via >>>>> > HttpAuthCache::UpdateAllFrom(). Have you tried using that? What >>>>> about client >>>>> > certificate selection state? Also what about the case where the user >>>>> needs >>>>> >>>> to >>>> >>>>> be >>>>> > prompted for credentials? >>>>> >>>> >>>> Using UpdateAllFrom also works. Please see the new patch. >>>>> >>>> >>>> When will the user need to select client certificate? >>>>> >>>> >>>> I'm not sure if a user prompt should be triggered from here, but it >>>>> should >>>>> a rare case. >>>>> >>>> >>>> Sorry for chiming in late, but I don't think we should do this. >>>> >>>> This is one of those things I've been harping on regarding proxies and >>>> user >>>> expectations. There are a *lot* of things that are wired up to our HTTP >>>> stack >>>> that just don't exist elsewhere. Things like enterprise policies for >>>> selecting >>>> client certs, among others. >>>> >>>> These are tricky to get right, but I think they're important. They're >>>> something >>>> we kinda-work but kinda-don't with other types of requests even more >>>> "webby" >>>> (Namely, XHR, WSS, Pepper/NaCl, and <webview>). >>>> >>>> I understand the importance for WebRTC, but I think it's no less >>>> important for >>>> the others, and I think the most important thing for our users is that >>>> things >>>> work consistently. I would even go as far as argue that it's *more* >>>> important >>>> than things "working", since inconsistent behaviours always trigger at >>>> the worst >>>> times and to the worst experience. >>>> >>>> I feel like the //net stack should be the arbitrar here, and we should >>>> figure >>>> out how best to wire it up for WebRTC. The biggest concern here is that >>>> the >>>> URLRequestContext (and delegate) are the most important things for >>>> giving the >>>> higher layer (aka //content) visibility into the requests, as well as >>>> preserving >>>> the invariants - whether it be for privacy reasons or for enterprise >>>> reasons. >>>> >>>> https://codereview.chromium.org/414523005/ >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ryan made a suggestion for a short-term fix for NTLM: 'HttpAuth::IDENT_SRC_DEFAULT_CREDENTIALS is the magic you set the HttpRequestIdentity to an identity with that, and we'll try a transparent auth' It's unclear to me how this could be done in the context of ProxyResolvingClientSocket. Ryan, could you explain more? I also scheduled meeting tomorrow if it's easier to explain face to face. Thanks!
https://codereview.chromium.org/414523005/diff/60001/jingle/glue/proxy_resolv... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/60001/jingle/glue/proxy_resolv... jingle/glue/proxy_resolving_client_socket.cc:63: session_params.proxy_service = request_context->proxy_service(); If you're using the main URLRequestContext, by duplicating the proxy service, you're potentially polluting the main context. That is, if a proxy fails for WebRTC, but would work if done in a main page load (e.g. if it required prompting), you've now forced it to be marked bad for the user. This is probably a BUG. https://codereview.chromium.org/414523005/diff/60001/jingle/glue/proxy_resolv... jingle/glue/proxy_resolving_client_socket.cc:67: session_params.network_delegate = request_context->network_delegate(); Just to confirm: This means that extensions can and will be able to interfere with WebRTC requests. I think this is a right answer, I just want to make sure you're aware of it. https://codereview.chromium.org/414523005/diff/60001/jingle/glue/proxy_resolv... jingle/glue/proxy_resolving_client_socket.cc:99: network_session_->http_auth_cache()->UpdateAllFrom(*existing_cache); Create your own HttpAuthCache. Confirm that you reach this code path: https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_auth... You should reach it from https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_auth... which you should reach from https://code.google.com/p/chromium/codesearch#chromium/src/net/http/proxy_cli... which you should reach from https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_prox... which you should reach from https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/client_... (asynchronously) https://codereview.chromium.org/414523005/diff/60001/jingle/glue/proxy_resolv... jingle/glue/proxy_resolving_client_socket.cc:156: // the connect always happens asynchronously. We should fix this at some point. This is seen as an anti-pattern for Chrome networking. https://codereview.chromium.org/414523005/diff/60001/jingle/glue/proxy_resolv... jingle/glue/proxy_resolving_client_socket.cc:309: weak_factory_.GetWeakPtr(), rv)); Ditto that this is a bit of an anti-pattern.
I uploaded a new patch which seems to be the minimal change to make auth with default credential work. PTAL! I'd leave the other problems in this file to a follow up change if they do not affect the NTLM regression we are trying to fix here, since I want to keep it as simple as possible for merging.
https://codereview.chromium.org/414523005/diff/80001/jingle/glue/proxy_resolv... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/80001/jingle/glue/proxy_resolv... jingle/glue/proxy_resolving_client_socket.cc:275: return static_cast<net::ProxyClientSocket*>(transport_->socket()) This strikes me as a dangerous assumption that's not guaranteed - but I don't have any better suggestions here. It at least matches what HttpSTreamFactoryImpl assumes. Our API is all sorts of weirdness: https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... passes |credentials| which calls https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre... , passing |credentials| which calls https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre... ... which just ignores |credentials| entirely, and ends up setting the state machine to hit https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre... Which is how I know it's "safe" However, none of this is a 'public' guarantee of the //net API, hence the danger.
jiayl@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/414523005/diff/80001/jingle/glue/proxy_resolv... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/80001/jingle/glue/proxy_resolv... jingle/glue/proxy_resolving_client_socket.cc:275: return static_cast<net::ProxyClientSocket*>(transport_->socket()) RestartWithAuth() only makes progress for the case where there's a valid identity selected and credentials are available. The ERR_PROXY_AUTH_REQUESTED can also be returned when there are no credentials to use, in which case you are likely to cycle around forever.
https://codereview.chromium.org/414523005/diff/80001/jingle/glue/proxy_resolv... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/80001/jingle/glue/proxy_resolv... jingle/glue/proxy_resolving_client_socket.cc:275: return static_cast<net::ProxyClientSocket*>(transport_->socket()) What do you suggest to do? Does it return error if it does not have identity/credential? On 2014/08/26 22:34:03, asanka wrote: > RestartWithAuth() only makes progress for the case where there's a valid > identity selected and credentials are available. The ERR_PROXY_AUTH_REQUESTED > can also be returned when there are no credentials to use, in which case you are > likely to cycle around forever.
On 2014/08/26 22:34:04, asanka wrote: > https://codereview.chromium.org/414523005/diff/80001/jingle/glue/proxy_resolv... > File jingle/glue/proxy_resolving_client_socket.cc (right): > > https://codereview.chromium.org/414523005/diff/80001/jingle/glue/proxy_resolv... > jingle/glue/proxy_resolving_client_socket.cc:275: return > static_cast<net::ProxyClientSocket*>(transport_->socket()) > RestartWithAuth() only makes progress for the case where there's a valid > identity selected and credentials are available. The ERR_PROXY_AUTH_REQUESTED > can also be returned when there are no credentials to use, in which case you are > likely to cycle around forever. Doh, you're right. AFAICT, after we've invalidated all identities, and we've used the default identity (which can only be used once per HttpAuthController), we should return "OK" from HttpAuthController::HaveAuthChallenge (we will have invalidated all of the controllers and cached challenges). This will then get mapped (back) into ERR_PROXY_AUTH_REQUESTED - https://code.google.com/p/chromium/codesearch#chromium/src/net/http/proxy_cli...
Patchset #4 (id:100001) has been deleted
Fixed the looping problem. PTAL.
https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... jingle/glue/proxy_resolving_client_socket.cc:279: if (proxy_socket->GetAuthController()->HaveAuth()) This feels like it's reaching a bit too much into the internal state/implementation. Can you clarify if my understanding is correct: You're checking HaveAuth() because during the HttpAuthController::GenerateAuthChallenge, it will set identity_.invalid to be true before beginning the auth method loop? Note that I have a bit of a concern as to whether this will actually "do the right thing", in as much as it's possible in some Kerberos flows (such as on Windows) for there to be an extend message exchange that goes beyond the 3-leg of (ServerChallenge, ClientResponse, ServerAck), and may become a 4/5 leg (ServerContinue, ClientContinue, ServerAck being the replacement for the three-leg ServerAck) I don't have a great idea for a solution on the problem right away - it's been a while since I've looked at this code (despite our joint debugging), but I'm hoping that Asanka may have thoughts on the right interface for surfacing this.
https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... jingle/glue/proxy_resolving_client_socket.cc:279: if (proxy_socket->GetAuthController()->HaveAuth()) This is what Asanka suggested to check. On 2014/08/27 20:44:37, Ryan Sleevi wrote: > This feels like it's reaching a bit too much into the internal > state/implementation. > > Can you clarify if my understanding is correct: > You're checking HaveAuth() because during the > HttpAuthController::GenerateAuthChallenge, it will set identity_.invalid to be > true before beginning the auth method loop? > > Note that I have a bit of a concern as to whether this will actually "do the > right thing", in as much as it's possible in some Kerberos flows (such as on > Windows) for there to be an extend message exchange that goes beyond the 3-leg > of (ServerChallenge, ClientResponse, ServerAck), and may become a 4/5 leg > (ServerContinue, ClientContinue, ServerAck being the replacement for the > three-leg ServerAck) > > I don't have a great idea for a solution on the problem right away - it's been a > while since I've looked at this code (despite our joint debugging), but I'm > hoping that Asanka may have thoughts on the right interface for surfacing this.
https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... jingle/glue/proxy_resolving_client_socket.cc:279: if (proxy_socket->GetAuthController()->HaveAuth()) On 2014/08/27 20:46:25, jiayl wrote: > > This is what Asanka suggested to check. Great, then I'll leave it to Asanka to LG if he thinks this will also handle the case I mentioned (or if Chrome doesn't handle that case in general)
Code LGTM. It would be awesome to have tests for this. https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... jingle/glue/proxy_resolving_client_socket.cc:279: if (proxy_socket->GetAuthController()->HaveAuth()) On 2014/08/27 20:57:30, Ryan Sleevi wrote: > On 2014/08/27 20:46:25, jiayl wrote: > > > > This is what Asanka suggested to check. > > Great, then I'll leave it to Asanka to LG if he thinks this will also handle the > case I mentioned (or if Chrome doesn't handle that case in general) This should handle multi-round authentication. HttpAuthController::HandleAuthChallenge sets identity_.invalid back to false if the selected AuthHandler doesn't need an identity (AuthHandler::NeedsIdentity() returns false). This is the case for multi-round authentication. Negotiate and NTLM handlers only return true for a NeedsIdentity() call if there is no incoming token (i.e. the initial ServerChallenge). So HaveAuth() should return true as long as the same AuthHandler is being used for successive authentication rounds. Elsewhere in net the socket's HttpAuthController is passed up the stack via HttpStreamFactoryImpl::Job::RunLoop() -> HttpStreamFactoryImpl::Job::OnNeedsProxyAuthCallback() -> HttpNetworkTransaction::OnNeedsProxyAuth(). Then eventually HttpNetworkTransaction consults HttpAuthController::HaveAuth() to decide whether to restart the transaction for another auth cycle. I wonder if HttpStreamFactoryImpl getting away with a static_cast<> is the reason why the API isn't more upfront about handling this case. But I feel that fixing that isn't in the scope for this CL.
lgtm when my style nit is addressed https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... File jingle/glue/proxy_resolving_client_socket.cc (right): https://codereview.chromium.org/414523005/diff/120001/jingle/glue/proxy_resol... jingle/glue/proxy_resolving_client_socket.cc:281: else nit: remove this else See http://www.chromium.org/developers/coding-style#Code_Formatting
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/414523005/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as fbefe5ba0b64fe1650035807813f8c827b49b057
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/260631aa62e2096045adb0e3780ef562ef12bb4f Cr-Commit-Position: refs/heads/master@{#292428} |
