Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10854139:
Use HttpAuthController in SocketStream (Closed)
|
Created:
8 years, 4 months ago by bashi Modified:
8 years, 3 months ago Reviewers:
cbentzel, asanka, jar (doing other things), Takashi Toyoshima, wtc, Ryan Hamilton, eroman, kuan, Ben Goodger (Google) CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Yuta Kitamura, Takashi Toyoshima Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUse HttpAuthController in SocketStream
We need to share http auth cache with http stack because WebSocket is a
subresource and chromium won't show the login dialog for subresource
loading. Use HttpAuthController and pass the common http auth cache to
it to solve the issue.
BUG=47069
TEST=net_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155582
Patch Set 1 #
Total comments: 15
Patch Set 2 : #
Total comments: 16
Patch Set 3 : #
Total comments: 10
Patch Set 4 : Add browser test. #
Total comments: 29
Patch Set 5 : #Patch Set 6 : Fix browser_test #
Total comments: 4
Patch Set 7 : #Patch Set 8 : Rebased and Skipped added browser test on chromeos #
Messages
Total messages: 57 (0 generated)
Hi cbentzel, wtc, Could you take a look?
Patch set 1 LGTM. I am no longer that familiar with the current HTTP auth code, so please wait for the review by cbentzel. It'd be nice if you could go over your own code carefully. Thanks. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... File net/socket_stream/socket_stream.cc (right): https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:713: return !is_secure() && (proxy_info_.is_http() || proxy_info_.is_https()); IMPORTANT: can you explain why you test !is_secure() here? https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:748: request_info.url = url(); Nit: this request_info.url assignment should be unnecessary. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:801: tunnel_request_headers_->headers_ += authorization_headers.ToString(); IMPORTANT: this seems wrong. This implies that authorization_headers.ToString() ends with "\r\n\r\n". Update: I see why this code is correct now. Perhaps it would be clearer to build tunnel_request_headers_ from a HttpRequestHeaders. I am not suggesting that you make that change. Just making an observation. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:910: return ERR_UNEXPECTED_PROXY_AUTH; Nit: add curly braces because the conditional expression is more than one line. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:921: } IMPORTANT: you are falling through the the default case here. If this is intentional, please add a comment: + // Fall through. default: break; or add a break statement: + break; default: break; https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... File net/socket_stream/socket_stream_unittest.cc (right): https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream_unittest.cc:442: MockWrite("CONNECT example.com:80 HTTP/1.1\r\n" Nit: the CONNECT method is usually used to set up an SSL tunnel. So the port number 80 is not realistic. I suggest using port 443.
Added asanka as well - my knowledge is getting increasingly rusty too. A general concern: I thought the WebSocket code was migrating to share the connection logic with HttpNetworkTransaction. Is this being done as a short-term fix while the refactoring effort is going on?
Thank you for review! https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... File net/socket_stream/socket_stream.cc (right): https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:713: return !is_secure() && (proxy_info_.is_http() || proxy_info_.is_https()); On 2012/08/14 18:34:37, wtc wrote: > > IMPORTANT: can you explain why you test !is_secure() here? Done. The logic is the same as HttpNetworkTransaction::ShouldApplyProxyAuth(). https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:748: request_info.url = url(); On 2012/08/14 18:34:37, wtc wrote: > > Nit: this request_info.url assignment should be unnecessary. Removed. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:801: tunnel_request_headers_->headers_ += authorization_headers.ToString(); On 2012/08/14 18:34:37, wtc wrote: > > IMPORTANT: this seems wrong. This implies that > authorization_headers.ToString() ends with "\r\n\r\n". > > Update: I see why this code is correct now. Perhaps > it would be clearer to build tunnel_request_headers_ > from a HttpRequestHeaders. I am not suggesting that > you make that change. Just making an observation. I agree. Done. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:910: return ERR_UNEXPECTED_PROXY_AUTH; On 2012/08/14 18:34:37, wtc wrote: > > Nit: add curly braces because the conditional expression is > more than one line. Done. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:921: } On 2012/08/14 18:34:37, wtc wrote: > > IMPORTANT: you are falling through the the default case here. > If this is intentional, please add a comment: > + // Fall through. > default: > break; > > or add a break statement: > + break; > default: > break; Thank you for the catch. Added break. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... File net/socket_stream/socket_stream_unittest.cc (right): https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream_unittest.cc:442: MockWrite("CONNECT example.com:80 HTTP/1.1\r\n" On 2012/08/14 18:34:37, wtc wrote: > > Nit: the CONNECT method is usually used to set up an SSL > tunnel. So the port number 80 is not realistic. I suggest > using port 443. WebSocket uses CONNECT even if it doesn't use ssl when it uses proxy. http://tools.ietf.org/html/rfc6455#section-4.1 I'd like to keep it. Added a comment.
On 2012/08/14 19:32:38, cbentzel wrote: > A general concern: I thought the WebSocket code was migrating to share the > connection logic with HttpNetworkTransaction. Is this being done as a short-term > fix while the refactoring effort is going on? Yes. This is a short-term fix. The authentication logic will be shared between http stack and websocket stack eventually, but the refactoring would take time so I'd like to make the fix.
On 2012/08/14 21:49:14, bashik wrote: > On 2012/08/14 19:32:38, cbentzel wrote: > > A general concern: I thought the WebSocket code was migrating to share the > > connection logic with HttpNetworkTransaction. Is this being done as a > short-term > > fix while the refactoring effort is going on? > > Yes. This is a short-term fix. The authentication logic will be shared between > http stack and websocket stack eventually, but the refactoring would take time > so I'd like to make the fix. Thanks for the explanation. I will take a pass over this tomorrow. Sorry for the delay.
Patch set 2 LGTM. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... File net/socket_stream/socket_stream.cc (right): https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:713: return !is_secure() && (proxy_info_.is_http() || proxy_info_.is_https()); On 2012/08/14 21:45:45, bashik wrote: > The logic is the same as HttpNetworkTransaction::ShouldApplyProxyAuth(). Thank you for the explanation. I think this test may be incorrect for this class. Based on the code on lines 763-774 in SocketStream::DoGenerateProxyAuthTokenComplete, it seems that we need the proxy auth header if proxy_mode_ == kTunnelProxy, so it seems that we should test that in this function. HttpNetworkTransaction is different because it seems to do tunnel connection in some other class. https://chromiumcodereview.appspot.com/10854139/diff/1/net/socket_stream/sock... net/socket_stream/socket_stream.cc:801: tunnel_request_headers_->headers_ += authorization_headers.ToString(); On 2012/08/14 21:45:45, bashik wrote: > > I agree. Done. Great! This change exceeds my expectations :-)
https://chromiumcodereview.appspot.com/10854139/diff/6001/net/socket_stream/s... File net/socket_stream/socket_stream_unittest.cc (right): https://chromiumcodereview.appspot.com/10854139/diff/6001/net/socket_stream/s... net/socket_stream/socket_stream_unittest.cc:443: // proxy. Nit: please add "so the port may not be 443" or something like that. Otherwise it's not clear what the comment is referring to.
Do you have something like a browser_test for this? It doesn't seem like the existing test framework would catch future regressions like this.
My high level concern is that I do not believe this will work for WSS connections tunneling through an HTTP/HTTPS proxy. Also adding rch, since he had to deal with this ugly connection establishment logic. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:29: #include "net/http/http_auth_handler_factory.h" http_auth_handler_factory include no longer needed. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:612: if (!proxy_url_.SchemeIs("https")) { Not part of this CL - but do you only want to do this if the scheme is wss? If the scheme is ws, should you fall back to http when doing proxy evaluation? http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:714: return !is_secure() && (proxy_info_.is_http() || proxy_info_.is_https()); This isn't going to work when tunneling WSS connections over an authenticating HTTP or HTTPS proxy. The reason we have the |is_https_request| logic in HttpNetworkTransaction::ShouldApplyProxyAuth is that the tunneling authentication logic takes place earlier, during the STATE_CREATE_STREAM. See net/http/http_stream_factory_impl_job.cc if you are curious. It looks like there is no equivalent of this for SocketStream - you are establishing the tunnel directly. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:739: const char* scheme = proxy_info_.is_https() ? "https://" : "http://"; Perhaps share with HttpNetworkTransaction::AuthURL? http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... File net/socket_stream/socket_stream.h (right): http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.h:22: #include "net/http/http_auth.h" The net/http/http_auth*.h includes look like they are no longer needed. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.h:37: class HttpAuthHandlerFactory; You no longer need to forward declare HttpAuthController.
Hi wtc, cbentzel, Thank you for review! I'm ooo till Aug 19 so I'll address comments next week. Sorry for the delay.
Thank you for review. We need a proxy server implementation (perhaps in testserver.py) to add browser tests for this issue. Do you think it's worth to implement it? http://codereview.chromium.org/10854139/diff/1/net/socket_stream/socket_strea... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/1/net/socket_stream/socket_strea... net/socket_stream/socket_stream.cc:713: return !is_secure() && (proxy_info_.is_http() || proxy_info_.is_https()); On 2012/08/14 23:20:35, wtc wrote: > Thank you for the explanation. I think this test may > be incorrect for this class. > > Based on the code on lines 763-774 in > SocketStream::DoGenerateProxyAuthTokenComplete, it seems > that we need the proxy auth header if proxy_mode_ == kTunnelProxy, > so it seems that we should test that in this function. You are right. Removed !is_secure() and added the check (proxy_mode_ == kTunnelProxy). http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:29: #include "net/http/http_auth_handler_factory.h" On 2012/08/15 18:08:01, cbentzel wrote: > http_auth_handler_factory include no longer needed. Done. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:612: if (!proxy_url_.SchemeIs("https")) { On 2012/08/15 18:08:01, cbentzel wrote: > Not part of this CL - but do you only want to do this if the scheme is wss? > > If the scheme is ws, should you fall back to http when doing proxy evaluation? I'm not familiar with the logic, but according to the git log and above comment, we would want to use https proxy (if exists) even if the scheme is ws. WebSocket always uses CONNECT method. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:714: return !is_secure() && (proxy_info_.is_http() || proxy_info_.is_https()); On 2012/08/15 18:08:01, cbentzel wrote: > This isn't going to work when tunneling WSS connections over an authenticating > HTTP or HTTPS proxy. > > The reason we have the |is_https_request| logic in > HttpNetworkTransaction::ShouldApplyProxyAuth is that the tunneling > authentication logic takes place earlier, during the STATE_CREATE_STREAM. See > net/http/http_stream_factory_impl_job.cc if you are curious. > > It looks like there is no equivalent of this for SocketStream - you are > establishing the tunnel directly Yes. the logic was wrong. Thank you for pointing this out. Updated the logic. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:739: const char* scheme = proxy_info_.is_https() ? "https://" : "http://"; On 2012/08/15 18:08:01, cbentzel wrote: > Perhaps share with HttpNetworkTransaction::AuthURL? I'd like to keep this because: - SocketStream only needs proxy auth at this time - SocketStream will eventually be removed http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... File net/socket_stream/socket_stream.h (right): http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.h:22: #include "net/http/http_auth.h" On 2012/08/15 18:08:01, cbentzel wrote: > The net/http/http_auth*.h includes look like they are no longer needed. Done. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.h:37: class HttpAuthHandlerFactory; On 2012/08/15 18:08:01, cbentzel wrote: > You no longer need to forward declare HttpAuthController. HttpAuthHandlerFactory? Removed. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... File net/socket_stream/socket_stream_unittest.cc (right): http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream_unittest.cc:443: // proxy. On 2012/08/14 23:22:26, wtc wrote: > > Nit: please add "so the port may not be 443" or something > like that. Otherwise it's not clear what the comment is > referring to. Done.
Have you tested if this works through an NTLM or Negotiate authenticating proxy - they are a bit tricky because they require multiple rounds of authentication, and have exposed issues with other areas of code. http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... File net/socket_stream/socket_stream_unittest.cc (right): http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream_unittest.cc:466: new SocketStream(GURL("ws://example.com/demo"), delegate.get())); You should add tests for wss.
Also - I think the browser_tests would be worth it, particularly if there are even larger infrastructural changes afoot. Don't want to regress in the future. If this is particularly difficult, let me know. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:612: if (!proxy_url_.SchemeIs("https")) { On 2012/08/20 01:35:10, bashik wrote: > On 2012/08/15 18:08:01, cbentzel wrote: > > Not part of this CL - but do you only want to do this if the scheme is wss? > > > > If the scheme is ws, should you fall back to http when doing proxy evaluation? > > I'm not familiar with the logic, but according to the git log and above comment, > we would want to use https proxy (if exists) even if the scheme is ws. WebSocket > always uses CONNECT method. What is going on here is if you fail to find a proxy using the ws or wss schemes, try to find one using the https scheme. The resulting proxy may be DIRECT, or may be an http, https, or even SOCKS proxy. It's a bit of a hack in my opinion, but probably needed to deal with PAC scripts which haven't added support for ws and wss URLs.
Patch set 3 LGTM. Please do not check this in until cbentzel is happy with it. I reviewed the CL from scratch today, so I have some new questions and suggestions. Please see my comments below. http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:256: base::Bind(&SocketStream::DoRestartWithAuth, this)); Do you know why we can't just call DoRestartWithAuth directly? http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:713: (proxy_info_.is_http() || proxy_info_.is_https()); Based on my code inspection, it is enough to just test proxy_mode_ == kTunnelProxy here. This matches the test in DoGenerateProxyAuthTokenComplete on line 762. The reason is that whether proxy_info_ is http or https, we eventually go to the STATE_WRITE_TUNNEL_HEADERS state. Note: we can also get rid of the ShouldApplyProxyAuth() method, and simply insert the STATE_GENERATE_PROXY_AUTH_TOKEN and STATE_GENERATE_PROXY_AUTH_TOKEN_COMPLETE states before the STATE_WRITE_TUNNEL_HEADERS state. http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:748: request_info.method = "GET"; Since we are generating the auth token for proxy tunnel connection, it seems that the method should be "CONNECT" rather than "GET". http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:904: if (!proxy_auth_controller_.get() || proxy_mode_ != kTunnelProxy || It seems that we just need to test proxy_mode_ != kTunnelProxy here. I don't know the purpose of testing proxy_info_.is_direct(). If proxy_mode_ == kTunnelProxy, then proxy_auth_controller_.get() cannot be NULL because we must have constructed proxy_auth_controller_ earlier.
Adding jar@ for chrome/ review. Added a browser test and a simple proxy server for the test. As for NTLM/Negotiate, how can I test with them? It seems that I need an ActiveDirectory domain that can be used for testing, but I don't have such domain. http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:612: if (!proxy_url_.SchemeIs("https")) { On 2012/08/20 11:23:01, cbentzel wrote: > What is going on here is if you fail to find a proxy using the ws or wss > schemes, try to find one using the https scheme. The resulting proxy may be > DIRECT, or may be an http, https, or even SOCKS proxy. Sorry for lack of explanation. I should have said that CONNECT is used when we use a proxy. If the proxy rejects CONNECT request, we drop the connection and I think it's reasonable. > > It's a bit of a hack in my opinion, but probably needed to deal with PAC scripts > which haven't added support for ws and wss URLs. http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:256: base::Bind(&SocketStream::DoRestartWithAuth, this)); On 2012/08/21 21:30:45, wtc wrote: > > Do you know why we can't just call DoRestartWithAuth directly? Hmm, I have no idea. yutak@, toshoshim@, do you have any ideas? http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:713: (proxy_info_.is_http() || proxy_info_.is_https()); On 2012/08/21 21:30:45, wtc wrote: > > Based on my code inspection, it is enough to just test > proxy_mode_ == kTunnelProxy here. This matches the test > in DoGenerateProxyAuthTokenComplete on line 762. > > The reason is that whether proxy_info_ is http or https, > we eventually go to the STATE_WRITE_TUNNEL_HEADERS state. > > Note: we can also get rid of the ShouldApplyProxyAuth() > method, and simply insert the > STATE_GENERATE_PROXY_AUTH_TOKEN and > STATE_GENERATE_PROXY_AUTH_TOKEN_COMPLETE states before the > STATE_WRITE_TUNNEL_HEADERS state. Thank you for the suggestion. Removed ShouldApplyProxyAuth(). http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:748: request_info.method = "GET"; On 2012/08/21 21:30:45, wtc wrote: > > Since we are generating the auth token for proxy tunnel > connection, it seems that the method should be "CONNECT" > rather than "GET". Done. http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream.cc:904: if (!proxy_auth_controller_.get() || proxy_mode_ != kTunnelProxy || On 2012/08/21 21:30:45, wtc wrote: > > It seems that we just need to test proxy_mode_ != kTunnelProxy > here. > > I don't know the purpose of testing proxy_info_.is_direct(). > > If proxy_mode_ == kTunnelProxy, then proxy_auth_controller_.get() > cannot be NULL because we must have constructed proxy_auth_controller_ earlier. Done. HttpNetworkTransaction has similar code and I wasn't sure the logic when I initially wrote the CL. http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... File net/socket_stream/socket_stream_unittest.cc (right): http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... net/socket_stream/socket_stream_unittest.cc:466: new SocketStream(GURL("ws://example.com/demo"), delegate.get())); On 2012/08/20 11:13:40, cbentzel wrote: > You should add tests for wss. Done.
On 2012/08/22 08:36:43, bashik wrote: > Adding jar@ for chrome/ review. > > Added a browser test and a simple proxy server for the test. > > As for NTLM/Negotiate, how can I test with them? It seems that I need an > ActiveDirectory domain that can be used for testing, but I don't have such > domain. We have proxies set up in our office like this - I'll send you directions separately from this CL. I'll review this CL later today. > > http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... > File net/socket_stream/socket_stream.cc (right): > > http://codereview.chromium.org/10854139/diff/6001/net/socket_stream/socket_st... > net/socket_stream/socket_stream.cc:612: if (!proxy_url_.SchemeIs("https")) { > On 2012/08/20 11:23:01, cbentzel wrote: > > What is going on here is if you fail to find a proxy using the ws or wss > > schemes, try to find one using the https scheme. The resulting proxy may be > > DIRECT, or may be an http, https, or even SOCKS proxy. > > Sorry for lack of explanation. I should have said that CONNECT is used when we > use a proxy. If the proxy rejects CONNECT request, we drop the connection and I > think it's reasonable. > > > > > It's a bit of a hack in my opinion, but probably needed to deal with PAC > scripts > > which haven't added support for ws and wss URLs. > > http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... > File net/socket_stream/socket_stream.cc (right): > > http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... > net/socket_stream/socket_stream.cc:256: > base::Bind(&SocketStream::DoRestartWithAuth, this)); > On 2012/08/21 21:30:45, wtc wrote: > > > > Do you know why we can't just call DoRestartWithAuth directly? > > Hmm, I have no idea. yutak@, toshoshim@, do you have any ideas? > > http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... > net/socket_stream/socket_stream.cc:713: (proxy_info_.is_http() || > proxy_info_.is_https()); > On 2012/08/21 21:30:45, wtc wrote: > > > > Based on my code inspection, it is enough to just test > > proxy_mode_ == kTunnelProxy here. This matches the test > > in DoGenerateProxyAuthTokenComplete on line 762. > > > > The reason is that whether proxy_info_ is http or https, > > we eventually go to the STATE_WRITE_TUNNEL_HEADERS state. > > > > Note: we can also get rid of the ShouldApplyProxyAuth() > > method, and simply insert the > > STATE_GENERATE_PROXY_AUTH_TOKEN and > > STATE_GENERATE_PROXY_AUTH_TOKEN_COMPLETE states before the > > STATE_WRITE_TUNNEL_HEADERS state. > > Thank you for the suggestion. Removed ShouldApplyProxyAuth(). > > http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... > net/socket_stream/socket_stream.cc:748: request_info.method = "GET"; > On 2012/08/21 21:30:45, wtc wrote: > > > > Since we are generating the auth token for proxy tunnel > > connection, it seems that the method should be "CONNECT" > > rather than "GET". > > Done. > > http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... > net/socket_stream/socket_stream.cc:904: if (!proxy_auth_controller_.get() || > proxy_mode_ != kTunnelProxy || > On 2012/08/21 21:30:45, wtc wrote: > > > > It seems that we just need to test proxy_mode_ != kTunnelProxy > > here. > > > > I don't know the purpose of testing proxy_info_.is_direct(). > > > > If proxy_mode_ == kTunnelProxy, then proxy_auth_controller_.get() > > cannot be NULL because we must have constructed proxy_auth_controller_ > earlier. > > Done. HttpNetworkTransaction has similar code and I wasn't sure the logic when I > initially wrote the CL. > > http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... > File net/socket_stream/socket_stream_unittest.cc (right): > > http://codereview.chromium.org/10854139/diff/9002/net/socket_stream/socket_st... > net/socket_stream/socket_stream_unittest.cc:466: new > SocketStream(GURL("ws://example.com/demo"), delegate.get())); > On 2012/08/20 11:13:40, cbentzel wrote: > > You should add tests for wss. > > Done.
Patch set 4 LGTM. I like the new tests! http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... File chrome/browser/net/proxy_browsertest.cc (right): http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:29: class LoginPromptObserver : public content::NotificationObserver { Nit: it may be a good idea to add a comment to describe what this class does. For example, it's not clear to me what login_details->handler() is. If that's obvious to people who work on login, you can omit the comment. http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:32: : auth_handled_(false) {} Nit: you can define this constructor on a single line. http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:41: ASCIIToUTF16("bar")); The fact that "foo:bar" is the required username:password for our test server is not obvious. This should be documented. http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:46: bool AuthHandled() const { return auth_handled_; } Nit: our Style Guide recommends naming this getter auth_handled(). http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:103: EXPECT_TRUE(LowerCaseEqualsASCII(result, "pass")); Nit: it seems that we already know the page title on success is 'PASS' (see line 93), so we should not need to convert the title to lower case for comparison. http://codereview.chromium.org/10854139/diff/15001/net/socket_stream/socket_s... File net/socket_stream/socket_stream.cc (left): http://codereview.chromium.org/10854139/diff/15001/net/socket_stream/socket_s... net/socket_stream/socket_stream.cc:722: next_state_ = STATE_WRITE_TUNNEL_HEADERS; I think we can simplify state transition by keeping the structure of the original DoTcpConnectComplete method, but simply replacing next_state_ = STATE_WRITE_TUNNEL_HEADERS; with next_state_ = STATE_GENERATE_PROXY_AUTH_TOKEN; here. Similarly, any place in the original code where we do next_state_ = STATE_WRITE_TUNNEL_HEADERS; should be replaced with next_state_ = STATE_GENERATE_PROXY_AUTH_TOKEN; Then, DoGenerateProxyAuthTokenComplete() just needs to transition to next_state_ = STATE_WRITE_TUNNEL_HEADERS; on success. You don't need to make this change if I didn't describe it clearly. I will try it if I have time. http://codereview.chromium.org/10854139/diff/15001/net/test/base_test_server.h File net/test/base_test_server.h (right): http://codereview.chromium.org/10854139/diff/15001/net/test/base_test_server.... net/test/base_test_server.h:41: TYPE_BASIC_AUTH_PROXY, The enumerators are listed in alphabetical order, so please add TYPE_BASIC_AUTH_PROXY to the beginning. http://codereview.chromium.org/10854139/diff/15001/net/tools/testserver/tests... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/10854139/diff/15001/net/tools/testserver/tests... net/tools/testserver/testserver.py:67: SERVER_BASIC_AUTH_PROXY = 5 Hmm... here if we add SERVER_BASIC_AUTH_PROXY to the beginning, we will need to change all the values. I'm not sure what's the right thing to do. http://codereview.chromium.org/10854139/diff/15001/net/tools/testserver/tests... net/tools/testserver/testserver.py:120: a dedicated thread for each request.""" Should we also mention "adds client verification", as we do on lines 111-112 above?
Why is the Threading mixin required for testserver.py? http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... File chrome/browser/net/proxy_browsertest.cc (right): http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:99: StringPrintf("%s%d%s", "http://localhost:", port, "/ws.html"); Perhaps not in this CL - but it seems like it would be easier if TestWebSocketServer had something similar to TestServer::GetURL(). Also, I noticed that TestWebSocketServer::UseRandomPort just does a random number. There is an unlikely, but real, chance that this will lead to collisions with other ports that have been claimed. The TestServer approach is much more resilient to this, if more complex - it creates an ephemeral listening port in the testserver.py process, and sends the port up to the browser process over a pipe or other IPC mechanism. http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... File chrome/test/data/http/tests/ws.html (right): http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... chrome/test/data/http/tests/ws.html:4: <title>test ws connection</title> Are there other tests which use ws.html? http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... chrome/test/data/http/tests/ws.html:35: setTimeout(timeOutCallback, 3000); Why is this needed? Let's say the ws access is very slow - won't the title change from "test ws connection" to "FAIL" to "PASS". Given that there is a TitleWatcher in the browser test, this wouldn't terminate as soon as the timeout happens. http://codereview.chromium.org/10854139/diff/15001/net/socket_stream/socket_s... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/15001/net/socket_stream/socket_s... net/socket_stream/socket_stream.cc:743: request_info.method = "CONNECT"; BUG: This isn't sufficient for a digest-authenticating proxy, which includes information about the path in the auth challenge. They are fairly rare, but they do exist. Essentially, part of the text that gets hashed is the HTTP method and the host-port-pair, which matches the first line in an HTTP request. These are both determined from request_info.url, which you do not set here. The details of how the extraction takes place is in HttpAuthHandlerDigest::GetRequestMethodAndPath. That does look at if the URL scheme is https. It may need to change to look at the other scheme. I don't believe we can use request_info.method because the method would typically be "GET", but we happen to be trying to establish a tunnel at the time so the verb needs to be "CONNECT". In summary: you definitely need to fill in request_info.url here, and may need to change HttpAuthHandlerDigest::GetRequestMethodAndPath.
Also - other than the Digest-related BUG, this generally looks good. Thanks for taking the time to write all the tests!
bashi: I prototyped my suggested change to state transition in https://chromiumcodereview.appspot.com/10875022/ because I am worried my description of the change wasn't clear. Please see the diffs between patch sets 1 and 2 in that CL and see if you like it. Thanks.
> We have proxies set up in our office like this - I'll send you directions > separately from this CL. Thanks. Patch set 4 doesn't work for NTLM actually. Fixed the bug and confirmed that patch set 5 works for NTML. I also checked manually that the CL works fine for digest authentication. > Why is the Threading mixin required for testserver.py? Without threading, some requests were timed out when I tested the proxy for actual webpages. But for browser tests, threading isn't needed so I removed ThreadingHTTPServer. http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... File chrome/browser/net/proxy_browsertest.cc (right): http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:29: class LoginPromptObserver : public content::NotificationObserver { On 2012/08/22 19:42:27, wtc wrote: > > Nit: it may be a good idea to add a comment to describe what > this class does. For example, it's not clear to me what > login_details->handler() is. If that's obvious to people > who work on login, you can omit the comment. Done. http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:32: : auth_handled_(false) {} On 2012/08/22 19:42:27, wtc wrote: > > Nit: you can define this constructor on a single line. Done. http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:41: ASCIIToUTF16("bar")); On 2012/08/22 19:42:27, wtc wrote: > > The fact that "foo:bar" is the required username:password > for our test server is not obvious. This should be documented. Done. http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:46: bool AuthHandled() const { return auth_handled_; } On 2012/08/22 19:42:27, wtc wrote: > > Nit: our Style Guide recommends naming this getter > auth_handled(). Done. http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:99: StringPrintf("%s%d%s", "http://localhost:", port, "/ws.html"); On 2012/08/22 20:19:43, cbentzel wrote: > Perhaps not in this CL - but it seems like it would be easier if > TestWebSocketServer had something similar to TestServer::GetURL(). > > Also, I noticed that TestWebSocketServer::UseRandomPort just does a random > number. There is an unlikely, but real, chance that this will lead to collisions > with other ports that have been claimed. The TestServer approach is much more > resilient to this, if more complex - it creates an ephemeral listening port in > the testserver.py process, and sends the port up to the browser process over a > pipe or other IPC mechanism. I agree. I'll make another CL for it after this CL finished. Added TODO comment for now. http://codereview.chromium.org/10854139/diff/15001/chrome/browser/net/proxy_b... chrome/browser/net/proxy_browsertest.cc:103: EXPECT_TRUE(LowerCaseEqualsASCII(result, "pass")); On 2012/08/22 19:42:27, wtc wrote: > > Nit: it seems that we already know the page title on success > is 'PASS' (see line 93), so we should not need to convert the > title to lower case for comparison. Done. http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... File chrome/test/data/http/tests/ws.html (right): http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... chrome/test/data/http/tests/ws.html:4: <title>test ws connection</title> On 2012/08/22 20:19:43, cbentzel wrote: > Are there other tests which use ws.html? No tests use ws.html at this time. http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... chrome/test/data/http/tests/ws.html:35: setTimeout(timeOutCallback, 3000); On 2012/08/22 20:19:43, cbentzel wrote: > Why is this needed? Let's say the ws access is very slow - won't the title > change from "test ws connection" to "FAIL" to "PASS". Given that there is a > TitleWatcher in the browser test, this wouldn't terminate as soon as the timeout > happens. I copied this test from wss.html and wss.html has it. I think the original author wanted to treat the slow connection establishment as fail. toyoshim@, could you explain the reason? The difference is very small between wss.html ans ws.html. I merged them. http://codereview.chromium.org/10854139/diff/15001/net/socket_stream/socket_s... File net/socket_stream/socket_stream.cc (left): http://codereview.chromium.org/10854139/diff/15001/net/socket_stream/socket_s... net/socket_stream/socket_stream.cc:722: next_state_ = STATE_WRITE_TUNNEL_HEADERS; On 2012/08/22 19:42:27, wtc wrote: > > I think we can simplify state transition by keeping the > structure of the original DoTcpConnectComplete method, > but simply replacing > next_state_ = STATE_WRITE_TUNNEL_HEADERS; > with > next_state_ = STATE_GENERATE_PROXY_AUTH_TOKEN; > here. Similarly, any place in the original code where > we do > next_state_ = STATE_WRITE_TUNNEL_HEADERS; > should be replaced with > next_state_ = STATE_GENERATE_PROXY_AUTH_TOKEN; > > Then, DoGenerateProxyAuthTokenComplete() just needs > to transition to > next_state_ = STATE_WRITE_TUNNEL_HEADERS; > on success. > > You don't need to make this change if I didn't describe it > clearly. I will try it if I have time. Thank you so much for detailed suggestion. I followed your way:) http://codereview.chromium.org/10854139/diff/15001/net/socket_stream/socket_s... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/15001/net/socket_stream/socket_s... net/socket_stream/socket_stream.cc:743: request_info.method = "CONNECT"; On 2012/08/22 20:19:43, cbentzel wrote: > BUG: This isn't sufficient for a digest-authenticating proxy, which includes > information about the path in the auth challenge. They are fairly rare, but they > do exist. > > Essentially, part of the text that gets hashed is the HTTP method and the > host-port-pair, which matches the first line in an HTTP request. These are both > determined from request_info.url, which you do not set here. The details of how > the extraction takes place is in HttpAuthHandlerDigest::GetRequestMethodAndPath. > > That does look at if the URL scheme is https. It may need to change to look at > the other scheme. I don't believe we can use request_info.method because the > method would typically be "GET", but we happen to be trying to establish a > tunnel at the time so the verb needs to be "CONNECT". > > In summary: you definitely need to fill in request_info.url here, and may need > to change HttpAuthHandlerDigest::GetRequestMethodAndPath. Thanks. I didn't realized the issue. Changed to set request_info.url and modified HttpAuthHandlerDigest::GetRequestMethodAndPath so that it use CONNECT when the scheme is ws or wss. http://codereview.chromium.org/10854139/diff/15001/net/test/base_test_server.h File net/test/base_test_server.h (right): http://codereview.chromium.org/10854139/diff/15001/net/test/base_test_server.... net/test/base_test_server.h:41: TYPE_BASIC_AUTH_PROXY, On 2012/08/22 19:42:27, wtc wrote: > > The enumerators are listed in alphabetical order, so please > add TYPE_BASIC_AUTH_PROXY to the beginning. Done. http://codereview.chromium.org/10854139/diff/15001/net/tools/testserver/tests... File net/tools/testserver/testserver.py (right): http://codereview.chromium.org/10854139/diff/15001/net/tools/testserver/tests... net/tools/testserver/testserver.py:67: SERVER_BASIC_AUTH_PROXY = 5 On 2012/08/22 19:42:27, wtc wrote: > > Hmm... here if we add SERVER_BASIC_AUTH_PROXY to the beginning, > we will need to change all the values. I'm not sure what's > the right thing to do. I realized that the values aren't the same of TYPE_XXX. So I think we don't need to change values. http://codereview.chromium.org/10854139/diff/15001/net/tools/testserver/tests... net/tools/testserver/testserver.py:120: a dedicated thread for each request.""" On 2012/08/22 19:42:27, wtc wrote: > > Should we also mention "adds client verification", as we do > on lines 111-112 above? Removed ThreadingHTTPServer.
On 2012/08/22 22:19:08, wtc wrote: > bashi: I prototyped my suggested change to state transition > in https://chromiumcodereview.appspot.com/10875022/ because > I am worried my description of the change wasn't clear. > > Please see the diffs between patch sets 1 and 2 in that CL > and see if you like it. Thanks. Thank you! I now understand your suggestion. Followed the suggestion.
http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... File chrome/test/data/http/tests/ws.html (right): http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... chrome/test/data/http/tests/ws.html:35: setTimeout(timeOutCallback, 3000); Yes. Originally, I intended to realize timeout for slow connection. TitleWatcher waits for the title changing to "PASS" or "FAIL" in my tests. If browser_tests has another timeout mechanism, this must be redundant. Please remove it.
http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... File chrome/test/data/http/tests/ws.html (right): http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... chrome/test/data/http/tests/ws.html:35: setTimeout(timeOutCallback, 3000); On 2012/08/23 11:27:26, toyoshim wrote: > Yes. Originally, I intended to realize timeout for slow connection. > TitleWatcher waits for the title changing to "PASS" or "FAIL" in my tests. > > If browser_tests has another timeout mechanism, this must be redundant. > Please remove it. Thank you. Removed.
http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... File chrome/test/data/http/tests/ws.html (right): http://codereview.chromium.org/10854139/diff/15001/chrome/test/data/http/test... chrome/test/data/http/tests/ws.html:35: setTimeout(timeOutCallback, 3000); On 2012/08/23 11:53:24, bashik wrote: > On 2012/08/23 11:27:26, toyoshim wrote: > > Yes. Originally, I intended to realize timeout for slow connection. > > TitleWatcher waits for the title changing to "PASS" or "FAIL" in my tests. > > > > If browser_tests has another timeout mechanism, this must be redundant. > > Please remove it. > > Thank you. Removed. I realized that your approach would fail faster than the default browser_tests timeout - but one downside is it doesn't vary the timeout based on whether it is running under valgrind or other dynamic analysis tools which tend to slow it down.
LGTM Thank you for cleaning this up and adding all of the tests. The browser test in particular should help when WebSockets migrate away from SocketStream. http://codereview.chromium.org/10854139/diff/29003/net/socket_stream/socket_s... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/29003/net/socket_stream/socket_s... net/socket_stream/socket_stream.cc:752: request_info.method = "CONNECT"; Do you need to fill in any of the fields other than url?
Patch set 6 LGTM. http://codereview.chromium.org/10854139/diff/29003/net/socket_stream/socket_s... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/29003/net/socket_stream/socket_s... net/socket_stream/socket_stream.cc:911: } else if (delegate_) { Nit: omit "else" after a return statement.
Adding ben@ for chrome/ review. Thank you for review. I noticed that |sock| in testserver.py could be undefined when connection fails so I fixed it. http://codereview.chromium.org/10854139/diff/29003/net/socket_stream/socket_s... File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/10854139/diff/29003/net/socket_stream/socket_s... net/socket_stream/socket_stream.cc:752: request_info.method = "CONNECT"; On 2012/08/23 15:10:26, cbentzel wrote: > Do you need to fill in any of the fields other than url? It seems that we need to fill in request.method for basic auth. http://codereview.chromium.org/10854139/diff/29003/net/socket_stream/socket_s... net/socket_stream/socket_stream.cc:911: } else if (delegate_) { On 2012/08/23 21:04:24, wtc wrote: > > Nit: omit "else" after a return statement. Done.
Hi jar, ben, Could you take a look for chrome/ ?
jar can review the majority of the chrome stuff. gyp change lgtm ;-)
On 2012/08/27 15:48:30, Ben Goodger (Google) wrote: > jar can review the majority of the chrome stuff. gyp change lgtm ;-) wtc and cbentezl's LGTM should suffice for the net and ssl stuff. ben gave his LGTM for the gyp stuff. There is a chance you should get agreement from jam, as he has a watchful eye on some of the chrome/* stuff (although this really seems to relate to chrome/browser/net, (where wtc or cbentzel are OWNERS) and chrome/browser/ssl (which has no special OWNERS... but wtc *should* suffice). IMO... you are good to go with the LGTMs you now have..
On 2012/08/27 17:52:59, jar wrote: > On 2012/08/27 15:48:30, Ben Goodger (Google) wrote: > > jar can review the majority of the chrome stuff. gyp change lgtm ;-) > > wtc and cbentezl's LGTM should suffice for the net and ssl stuff. > > ben gave his LGTM for the gyp stuff. > > There is a chance you should get agreement from jam, as he has a watchful eye on > some of the chrome/* stuff (although this really seems to relate to > chrome/browser/net, (where wtc or cbentzel are OWNERS) and chrome/browser/ssl > (which has no special OWNERS... but wtc *should* suffice). I see. Thank you! > > IMO... you are good to go with the LGTMs you now have..
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bashi@chromium.org/10854139/22006
Try job failure for 10854139-22006 (retry) on win for step "runhooks" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bashi@chromium.org/10854139/22006
Try job failure for 10854139-22006 (retry) on linux_chromeos for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
+kuan As for failing the new browser test on chromeos, it seems that chromeos bypasses proxy configuration for localhost. (I referred comments on ProxyConfigServiceImpl::DetermineEffectiveConfig). Kuan, could you confirm that? I think I can simply skip the browser test on chromeos. What do you think?
Let's try to keep on ChromeOS. What if you do something like a MappedHostResolver which will map (in Chrome) a name like websocket-server to 127.0.0.1. Would that work? On Tue, Aug 28, 2012 at 10:05 PM, <bashi@chromium.org> wrote: > +kuan > > As for failing the new browser test on chromeos, it seems that chromeos > bypasses > proxy configuration for localhost. > (I referred comments on ProxyConfigServiceImpl::**DetermineEffectiveConfig). > Kuan, > could you confirm that? > > I think I can simply skip the browser test on chromeos. What do you think? > > > http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >
On 2012/08/29 05:05:57, bashik wrote: > +kuan > > As for failing the new browser test on chromeos, it seems that chromeos bypasses > proxy configuration for localhost. > (I referred comments on ProxyConfigServiceImpl::DetermineEffectiveConfig). Kuan, > could you confirm that? i'm no longer with chromeos project. but, back then when i wrote the chromeos code, i did bypass proxy config for local host. > I think I can simply skip the browser test on chromeos. What do you think?
On 2012/08/29 13:52:30, cbentzel wrote: > Let's try to keep on ChromeOS. What if you do something like a > MappedHostResolver which will map (in Chrome) a name like websocket-server > to 127.0.0.1. Would that work? Looks like it doesn't work. I passed --host-resolver-rules="MAP websocket-server 127.0.0.1", but chromeos bypassed the proxy for "websocket-server". > > On Tue, Aug 28, 2012 at 10:05 PM, <mailto:bashi@chromium.org> wrote: > > > +kuan > > > > As for failing the new browser test on chromeos, it seems that chromeos > > bypasses > > proxy configuration for localhost. > > (I referred comments on ProxyConfigServiceImpl::**DetermineEffectiveConfig). > > Kuan, > > could you confirm that? > > > > I think I can simply skip the browser test on chromeos. What do you think? > > > > > > > http://codereview.chromium.**org/10854139/%3Chttp://codereview.chromium.org/1...> > >
+eroman Eric - any idea if proxy being bypassed for localhost on ChromeOS makes sense?
Not sure exactly the context of this question, but it is pretty common to have proxy configurations that bypass the proxy for localhost. Failure to do so can result in problems. On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: > +eroman > > Eric - any idea if proxy being bypassed for localhost on ChromeOS makes > sense? > > http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >
Not sure exactly the context of this question, but it is pretty common to have proxy configurations that bypass the proxy for localhost. Failure to do so can result in problems. On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: > +eroman > > Eric - any idea if proxy being bypassed for localhost on ChromeOS makes > sense? > > http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >
Sorry - this is a browser_test failure on ChromeOS only where --proxy-server=localhost:4488 or similar is provided. It looks like ChromeOS has a different version of ProxyConfigServiceImpl which handles the "read from pref and convert to ProxyConfig" On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <eroman@chromium.org> wrote: > Not sure exactly the context of this question, but it is pretty common to > have proxy configurations that bypass the proxy for localhost. Failure to > do so can result in problems. > > > On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: > >> +eroman >> >> Eric - any idea if proxy being bypassed for localhost on ChromeOS makes >> sense? >> >> http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >> > >
Sorry - this is a browser_test failure on ChromeOS only where --proxy-server=localhost:4488 or similar is provided. It looks like ChromeOS has a different version of ProxyConfigServiceImpl which handles the "read from pref and convert to ProxyConfig" On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <eroman@chromium.org> wrote: > Not sure exactly the context of this question, but it is pretty common to > have proxy configurations that bypass the proxy for localhost. Failure to > do so can result in problems. > > > On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: > >> +eroman >> >> Eric - any idea if proxy being bypassed for localhost on ChromeOS makes >> sense? >> >> http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >> > >
Looking at ChromeOS's ProxyConfigServiceImpl, it looks like the issue may be that --proxy-server flag isn't supported on that platform. On Wed, Aug 29, 2012 at 8:17 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > Sorry - this is a browser_test failure on ChromeOS only where > --proxy-server=localhost:4488 or similar is provided. > > It looks like ChromeOS has a different version of ProxyConfigServiceImpl > which handles the "read from pref and convert to ProxyConfig" > > > On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <eroman@chromium.org> wrote: > >> Not sure exactly the context of this question, but it is pretty common to >> have proxy configurations that bypass the proxy for localhost. Failure to >> do so can result in problems. >> >> >> On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: >> >>> +eroman >>> >>> Eric - any idea if proxy being bypassed for localhost on ChromeOS makes >>> sense? >>> >>> http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >>> >> >> >
Looking at ChromeOS's ProxyConfigServiceImpl, it looks like the issue may be that --proxy-server flag isn't supported on that platform. On Wed, Aug 29, 2012 at 8:17 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > Sorry - this is a browser_test failure on ChromeOS only where > --proxy-server=localhost:4488 or similar is provided. > > It looks like ChromeOS has a different version of ProxyConfigServiceImpl > which handles the "read from pref and convert to ProxyConfig" > > > On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <eroman@chromium.org> wrote: > >> Not sure exactly the context of this question, but it is pretty common to >> have proxy configurations that bypass the proxy for localhost. Failure to >> do so can result in problems. >> >> >> On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: >> >>> +eroman >>> >>> Eric - any idea if proxy being bypassed for localhost on ChromeOS makes >>> sense? >>> >>> http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >>> >> >> >
I think --proxy-server works on chromeos. I can see a login dialog for proxy server if I specify --proxy-server and try to connect www.google.comor other external sites. 2012/08/30 12:22 "Chris Bentzel" <cbentzel@chromium.org>: > Looking at ChromeOS's ProxyConfigServiceImpl, it looks like the issue may > be that --proxy-server flag isn't supported on that platform. > > On Wed, Aug 29, 2012 at 8:17 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > >> Sorry - this is a browser_test failure on ChromeOS only where >> --proxy-server=localhost:4488 or similar is provided. >> >> It looks like ChromeOS has a different version of ProxyConfigServiceImpl >> which handles the "read from pref and convert to ProxyConfig" >> >> >> On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <eroman@chromium.org> wrote: >> >>> Not sure exactly the context of this question, but it is pretty common >>> to have proxy configurations that bypass the proxy for localhost. Failure >>> to do so can result in problems. >>> >>> >>> On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: >>> >>>> +eroman >>>> >>>> Eric - any idea if proxy being bypassed for localhost on ChromeOS makes >>>> sense? >>>> >>>> http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >>>> >>> >>> >> >
I think --proxy-server works on chromeos. I can see a login dialog for proxy server if I specify --proxy-server and try to connect www.google.comor other external sites. 2012/08/30 12:22 "Chris Bentzel" <cbentzel@chromium.org>: > Looking at ChromeOS's ProxyConfigServiceImpl, it looks like the issue may > be that --proxy-server flag isn't supported on that platform. > > On Wed, Aug 29, 2012 at 8:17 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > >> Sorry - this is a browser_test failure on ChromeOS only where >> --proxy-server=localhost:4488 or similar is provided. >> >> It looks like ChromeOS has a different version of ProxyConfigServiceImpl >> which handles the "read from pref and convert to ProxyConfig" >> >> >> On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <eroman@chromium.org> wrote: >> >>> Not sure exactly the context of this question, but it is pretty common >>> to have proxy configurations that bypass the proxy for localhost. Failure >>> to do so can result in problems. >>> >>> >>> On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: >>> >>>> +eroman >>>> >>>> Eric - any idea if proxy being bypassed for localhost on ChromeOS makes >>>> sense? >>>> >>>> http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >>>> >>> >>> >> >
OK, sounds like I read the code incorrectly. Thank you for checking. I can dive into the code to help understand this, but am at about the same position as you. I also am not near a development machine for a while. On Wed, Aug 29, 2012 at 8:39 PM, Kenichi Ishibashi <bashi@chromium.org>wrote: > I think --proxy-server works on chromeos. I can see a login dialog for > proxy server if I specify --proxy-server and try to connect www.google.comor other external sites. > 2012/08/30 12:22 "Chris Bentzel" <cbentzel@chromium.org>: > > Looking at ChromeOS's ProxyConfigServiceImpl, it looks like the issue may >> be that --proxy-server flag isn't supported on that platform. >> >> On Wed, Aug 29, 2012 at 8:17 PM, Chris Bentzel <cbentzel@chromium.org>wrote: >> >>> Sorry - this is a browser_test failure on ChromeOS only where >>> --proxy-server=localhost:4488 or similar is provided. >>> >>> It looks like ChromeOS has a different version of ProxyConfigServiceImpl >>> which handles the "read from pref and convert to ProxyConfig" >>> >>> >>> On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <eroman@chromium.org> wrote: >>> >>>> Not sure exactly the context of this question, but it is pretty common >>>> to have proxy configurations that bypass the proxy for localhost. Failure >>>> to do so can result in problems. >>>> >>>> >>>> On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: >>>> >>>>> +eroman >>>>> >>>>> Eric - any idea if proxy being bypassed for localhost on ChromeOS >>>>> makes sense? >>>>> >>>>> http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >>>>> >>>> >>>> >>> >>
OK, sounds like I read the code incorrectly. Thank you for checking. I can dive into the code to help understand this, but am at about the same position as you. I also am not near a development machine for a while. On Wed, Aug 29, 2012 at 8:39 PM, Kenichi Ishibashi <bashi@chromium.org>wrote: > I think --proxy-server works on chromeos. I can see a login dialog for > proxy server if I specify --proxy-server and try to connect www.google.comor other external sites. > 2012/08/30 12:22 "Chris Bentzel" <cbentzel@chromium.org>: > > Looking at ChromeOS's ProxyConfigServiceImpl, it looks like the issue may >> be that --proxy-server flag isn't supported on that platform. >> >> On Wed, Aug 29, 2012 at 8:17 PM, Chris Bentzel <cbentzel@chromium.org>wrote: >> >>> Sorry - this is a browser_test failure on ChromeOS only where >>> --proxy-server=localhost:4488 or similar is provided. >>> >>> It looks like ChromeOS has a different version of ProxyConfigServiceImpl >>> which handles the "read from pref and convert to ProxyConfig" >>> >>> >>> On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <eroman@chromium.org> wrote: >>> >>>> Not sure exactly the context of this question, but it is pretty common >>>> to have proxy configurations that bypass the proxy for localhost. Failure >>>> to do so can result in problems. >>>> >>>> >>>> On Wed, Aug 29, 2012 at 7:49 PM, <cbentzel@chromium.org> wrote: >>>> >>>>> +eroman >>>>> >>>>> Eric - any idea if proxy being bypassed for localhost on ChromeOS >>>>> makes sense? >>>>> >>>>> http://codereview.chromium.**org/10854139/<http://codereview.chromium.org/108... >>>>> >>>> >>>> >>> >>
Hi cbentzel, As described separately, it's somewhat difficult to make this browser test work on chromeos so I'd like to skip the test on chromeos. What do you think? On 2012/08/30 03:42:17, cbentzel wrote: > OK, sounds like I read the code incorrectly. Thank you for checking. > > I can dive into the code to help understand this, but am at about the same > position as you. I also am not near a development machine for a while. > > On Wed, Aug 29, 2012 at 8:39 PM, Kenichi Ishibashi <bashi@chromium.org>wrote: > > > I think --proxy-server works on chromeos. I can see a login dialog for > > proxy server if I specify --proxy-server and try to connect http://www.google.comor > other external sites. > > 2012/08/30 12:22 "Chris Bentzel" <cbentzel@chromium.org>: > > > > Looking at ChromeOS's ProxyConfigServiceImpl, it looks like the issue may > >> be that --proxy-server flag isn't supported on that platform. > >> > >> On Wed, Aug 29, 2012 at 8:17 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > >> > >>> Sorry - this is a browser_test failure on ChromeOS only where > >>> --proxy-server=localhost:4488 or similar is provided. > >>> > >>> It looks like ChromeOS has a different version of ProxyConfigServiceImpl > >>> which handles the "read from pref and convert to ProxyConfig" > >>> > >>> > >>> On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <mailto:eroman@chromium.org> wrote: > >>> > >>>> Not sure exactly the context of this question, but it is pretty common > >>>> to have proxy configurations that bypass the proxy for localhost. Failure > >>>> to do so can result in problems. > >>>> > >>>> > >>>> On Wed, Aug 29, 2012 at 7:49 PM, <mailto:cbentzel@chromium.org> wrote: > >>>> > >>>>> +eroman > >>>>> > >>>>> Eric - any idea if proxy being bypassed for localhost on ChromeOS > >>>>> makes sense? > >>>>> > >>>>> > http://codereview.chromium.**org/10854139/%3Chttp://codereview.chromium.org/1...> > >>>>> > >>>> > >>>> > >>> > >>
On 2012/09/02 23:54:24, bashik wrote: > Hi cbentzel, > > As described separately, it's somewhat difficult to make this browser test work > on chromeos so I'd like to skip the test on chromeos. What do you think? LGTM Sorry for delayed response, I'm on vacation this week. > > On 2012/08/30 03:42:17, cbentzel wrote: > > OK, sounds like I read the code incorrectly. Thank you for checking. > > > > I can dive into the code to help understand this, but am at about the same > > position as you. I also am not near a development machine for a while. > > > > On Wed, Aug 29, 2012 at 8:39 PM, Kenichi Ishibashi <bashi@chromium.org>wrote: > > > > > I think --proxy-server works on chromeos. I can see a login dialog for > > > proxy server if I specify --proxy-server and try to connect > http://www.google.comor > > other external sites. > > > 2012/08/30 12:22 "Chris Bentzel" <cbentzel@chromium.org>: > > > > > > Looking at ChromeOS's ProxyConfigServiceImpl, it looks like the issue may > > >> be that --proxy-server flag isn't supported on that platform. > > >> > > >> On Wed, Aug 29, 2012 at 8:17 PM, Chris Bentzel > <cbentzel@chromium.org>wrote: > > >> > > >>> Sorry - this is a browser_test failure on ChromeOS only where > > >>> --proxy-server=localhost:4488 or similar is provided. > > >>> > > >>> It looks like ChromeOS has a different version of ProxyConfigServiceImpl > > >>> which handles the "read from pref and convert to ProxyConfig" > > >>> > > >>> > > >>> On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <mailto:eroman@chromium.org> > wrote: > > >>> > > >>>> Not sure exactly the context of this question, but it is pretty common > > >>>> to have proxy configurations that bypass the proxy for localhost. Failure > > >>>> to do so can result in problems. > > >>>> > > >>>> > > >>>> On Wed, Aug 29, 2012 at 7:49 PM, <mailto:cbentzel@chromium.org> wrote: > > >>>> > > >>>>> +eroman > > >>>>> > > >>>>> Eric - any idea if proxy being bypassed for localhost on ChromeOS > > >>>>> makes sense? > > >>>>> > > >>>>> > > > http://codereview.chromium.**org/10854139/%253Chttp://codereview.chromium.org...> > > >>>>> > > >>>> > > >>>> > > >>> > > >>
Thanks, cbentzel! On 2012/09/07 12:54:16, cbentzel wrote: > On 2012/09/02 23:54:24, bashik wrote: > > Hi cbentzel, > > > > As described separately, it's somewhat difficult to make this browser test > work > > on chromeos so I'd like to skip the test on chromeos. What do you think? > > LGTM > > Sorry for delayed response, I'm on vacation this week. > > > > On 2012/08/30 03:42:17, cbentzel wrote: > > > OK, sounds like I read the code incorrectly. Thank you for checking. > > > > > > I can dive into the code to help understand this, but am at about the same > > > position as you. I also am not near a development machine for a while. > > > > > > On Wed, Aug 29, 2012 at 8:39 PM, Kenichi Ishibashi > <bashi@chromium.org>wrote: > > > > > > > I think --proxy-server works on chromeos. I can see a login dialog for > > > > proxy server if I specify --proxy-server and try to connect > > http://www.google.comor > > > other external sites. > > > > 2012/08/30 12:22 "Chris Bentzel" <cbentzel@chromium.org>: > > > > > > > > Looking at ChromeOS's ProxyConfigServiceImpl, it looks like the issue may > > > >> be that --proxy-server flag isn't supported on that platform. > > > >> > > > >> On Wed, Aug 29, 2012 at 8:17 PM, Chris Bentzel > > <cbentzel@chromium.org>wrote: > > > >> > > > >>> Sorry - this is a browser_test failure on ChromeOS only where > > > >>> --proxy-server=localhost:4488 or similar is provided. > > > >>> > > > >>> It looks like ChromeOS has a different version of ProxyConfigServiceImpl > > > >>> which handles the "read from pref and convert to ProxyConfig" > > > >>> > > > >>> > > > >>> On Wed, Aug 29, 2012 at 8:12 PM, Eric Roman <mailto:eroman@chromium.org> > > wrote: > > > >>> > > > >>>> Not sure exactly the context of this question, but it is pretty common > > > >>>> to have proxy configurations that bypass the proxy for localhost. > Failure > > > >>>> to do so can result in problems. > > > >>>> > > > >>>> > > > >>>> On Wed, Aug 29, 2012 at 7:49 PM, <mailto:cbentzel@chromium.org> wrote: > > > >>>> > > > >>>>> +eroman > > > >>>>> > > > >>>>> Eric - any idea if proxy being bypassed for localhost on ChromeOS > > > >>>>> makes sense? > > > >>>>> > > > >>>>> > > > > > > http://codereview.chromium.**org/10854139/%25253Chttp://codereview.chromium.o...> > > > >>>>> > > > >>>> > > > >>>> > > > >>> > > > >>
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bashi@chromium.org/10854139/40001
Change committed as 155582 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
