Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(357)

Issue 3058013: Fix late binding induced mismatch of Socket and AuthController (Closed)

Created:
10 years, 5 months ago by vandebo (ex-Chrome)
Modified:
9 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, ben+cc_chromium.org, Paweł Hajdan Jr.
Visibility:
Public.

Description

Fix late binding induced mismatch of Socket and AuthController ClientSocketPool treats all pending SocketParams as interchangeable. Therefore they can not contain any connection specific data. This only affects the Http Proxy tunnel case. The lowest risk change to fix this problem is to create the HttpAuthController in the HttpProxyClientSocket. If we get a 407 and need to restart the Tunnel, the pending HttpProxyClientSocket is returned to the HttpNetworkTransaction in the additional error state of the connection and then complete the auth in a pair of states in the HttpNetworkTransaction. This reintroduces a dependency between tunnel setup and the HttpNetworkTransaction, but that will need to be fixed at a later date. BUG=49493 TEST=existing unit tests + manually visiting many SSL sites through a kerberized http proxy. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54405

Patch Set 1 #

Patch Set 2 : Completely pull HttpAuthController out of SocketParams #

Patch Set 3 : Fix most unit tests, update 'connected' notion #

Patch Set 4 : Finish cleanup #

Patch Set 5 : Rebase #

Total comments: 2

Patch Set 6 : Fix memory leaks and address Paweł's comments #

Total comments: 14

Patch Set 7 : Address comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+359 lines, -389 lines) Patch
M net/base/net_error_list.h View 1 chunk +0 lines, -5 lines 0 comments Download
M net/http/http_network_transaction.h View 1 2 3 4 5 6 3 chunks +7 lines, -0 lines 1 comment Download
M net/http/http_network_transaction.cc View 1 2 3 4 5 6 8 chunks +77 lines, -29 lines 1 comment Download
M net/http/http_network_transaction_unittest.cc View 4 5 1 chunk +105 lines, -0 lines 0 comments Download
M net/http/http_proxy_client_socket.h View 2 3 4 chunks +11 lines, -6 lines 0 comments Download
M net/http/http_proxy_client_socket.cc View 2 3 4 7 chunks +36 lines, -16 lines 0 comments Download
M net/http/http_proxy_client_socket_pool.h View 3 chunks +5 lines, -4 lines 0 comments Download
M net/http/http_proxy_client_socket_pool.cc View 3 chunks +9 lines, -5 lines 0 comments Download
M net/http/http_proxy_client_socket_pool_unittest.cc View 3 4 5 16 chunks +39 lines, -31 lines 0 comments Download
M net/socket/client_socket_handle.h View 1 2 3 3 chunks +7 lines, -0 lines 0 comments Download
M net/socket/client_socket_handle.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M net/socket/socket_test_util.h View 3 4 1 chunk +0 lines, -40 lines 0 comments Download
M net/socket/socket_test_util.cc View 3 4 1 chunk +0 lines, -47 lines 0 comments Download
M net/socket/ssl_client_socket_pool.h View 1 2 3 2 chunks +1 line, -3 lines 0 comments Download
M net/socket/ssl_client_socket_pool.cc View 1 2 3 5 chunks +8 lines, -20 lines 0 comments Download
M net/socket/ssl_client_socket_pool_unittest.cc View 1 2 3 4 5 27 chunks +53 lines, -183 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
vandebo (ex-Chrome)
10 years, 4 months ago (2010-07-29 22:30:01 UTC) #1
Paweł Hajdan Jr.
Drive-by with some test crashiness comments. No need to wait for another review by me. ...
10 years, 4 months ago (2010-07-29 22:49:02 UTC) #2
cbentzel
I took a once-over and this approach seems reasonable for now, if not ideal long ...
10 years, 4 months ago (2010-07-30 02:40:25 UTC) #3
vandebo (ex-Chrome)
On 2010/07/30 02:40:25, cbentzel wrote: > I took a once-over and this approach seems reasonable ...
10 years, 4 months ago (2010-07-30 02:46:24 UTC) #4
cbentzel
On Thu, Jul 29, 2010 at 10:46 PM, <vandebo@chromium.org> wrote: > On 2010/07/30 02:40:25, cbentzel ...
10 years, 4 months ago (2010-07-30 02:48:56 UTC) #5
cbentzel
Thanks for debugging and working on this fix Steve. Most of the suggestions I have ...
10 years, 4 months ago (2010-07-30 14:26:37 UTC) #6
cbentzel
LGTM as well.
10 years, 4 months ago (2010-07-30 14:27:21 UTC) #7
vandebo (ex-Chrome)
http://codereview.chromium.org/3058013/diff/5002/17001 File net/base/net_error_list.h (left): http://codereview.chromium.org/3058013/diff/5002/17001#oldcode169 net/base/net_error_list.h:169: On 2010/07/30 14:26:37, cbentzel wrote: > Do the net ...
10 years, 4 months ago (2010-07-30 18:42:31 UTC) #8
eroman
10 years, 4 months ago (2010-07-30 22:11:11 UTC) #9
I didn't fully understand this change... will defer the review to Chris.

Just a couple of nits:

http://codereview.chromium.org/3058013/diff/3013/25002
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3058013/diff/3013/25002#newcode886
net/http/http_network_transaction.cc:886: // Other state (i.e. |using_ssl_|
suggests that |connection_| will have an
This seems like an unmatched parenthesis.

http://codereview.chromium.org/3058013/diff/3013/25003
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/3058013/diff/3013/25003#newcode177
net/http/http_network_transaction.h:177: // Called to handle an http proxy
tunnel request for auth
nit: end with period. also "http" --> "HTTP".

Powered by Google App Engine
This is Rietveld 408576698