Remove SSLClientAuthHandler's RDH dependency.

Remove SSLClientAuthHandler's RDH dependency.

Instead, give it a callback to select the client certificate. This removes a
dependency on GlobalRequestID. Also stop reference-counting it and avoid
passing an HttpNetworkSession to the UI thread. (Use BrowserContext instead.)

BUG=376003
Committed: https://crrev.com/99aee5eb0f6733566c60dfe356a1b7169477808f
Cr-Commit-Position: refs/heads/master@{#298782}

[davidben, 2014-09-23 18:36:06]

davidben@chromium.org changed reviewers:
+ abarth@chromium.org, mmenke@chromium.org

[davidben, 2014-09-23 18:36:07]

mmenke: main review
abarth: content/browser/ssl OWNERS

[mmenke, 2014-09-23 19:24:23]

https://codereview.chromium.org/596873002/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/596873002/diff/1/content/browser/loader/resou...
content/browser/loader/resource_loader.cc:715: ssl_client_auth_handler_ = NULL;
Should we clear the auth handler first?

Hrm...I'm assuming the auth handler is refcounted for more reasons than just to
be safely cleaned up here.

https://codereview.chromium.org/596873002/diff/1/content/browser/ssl/ssl_clie...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/596873002/diff/1/content/browser/ssl/ssl_clie...
content/browser/ssl/ssl_client_auth_handler.cc:112: request_ = NULL;
Should we clear request_ first?  If the ResetAndReturn gets us anything, seems
like request_ should be reset first.  If it doesn't get us anything, I'm not
sure if we should be using ResetAndReturn, or just NULLing out the callback when
done.

[davidben, 2014-09-23 21:22:03]

Un-refcounted that class (the old logic was pretty messed up) and fixed a
questionable HttpNetworkSession bit. I'll add what OWNERS I'm now missing once
mmenke is happy.

https://codereview.chromium.org/596873002/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/596873002/diff/1/content/browser/loader/resou...
content/browser/loader/resource_loader.cc:715: ssl_client_auth_handler_ = NULL;
On 2014/09/23 19:24:23, mmenke wrote:
> Should we clear the auth handler first?

Done. Yeah, that's probably better; just in case everything else goes through
synchronously and we get another one. (It won't, but does //net promise that? I
guess we try to avoid that kind of re-entrancy in general, so if it does, that
would be a bug...)

> Hrm...I'm assuming the auth handler is refcounted for more reasons than just
to
> be safely cleaned up here.

Made it not ref-counted. The thing it was ref-counted for turns out to be broken
anyway.

https://codereview.chromium.org/596873002/diff/1/content/browser/ssl/ssl_clie...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/596873002/diff/1/content/browser/ssl/ssl_clie...
content/browser/ssl/ssl_client_auth_handler.cc:112: request_ = NULL;
On 2014/09/23 19:24:23, mmenke wrote:
> Should we clear request_ first?  If the ResetAndReturn gets us anything, seems
> like request_ should be reset first.  If it doesn't get us anything, I'm not
> sure if we should be using ResetAndReturn, or just NULLing out the callback
when
> done.

No longer relevant.

https://codereview.chromium.org/596873002/diff/20001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_client_auth_handler.cc (left):

https://codereview.chromium.org/596873002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.cc:34: DoCertificateSelected(NULL);
If dropped, the consumer should know it's been cancelled. This was in service of
the long RVH-related comment below that wasn't even true. This now only happens
right before the URLRequest is about to be destroyed or cancelled, so it's
unnecessary.

https://codereview.chromium.org/596873002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.cc:37: void
SSLClientAuthHandler::OnRequestCancelled() {
This only ever happens right before destroying the object anyway.

https://codereview.chromium.org/596873002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.cc:92: // the net::URLRequest always
gets a response.
This comment was false. The ResourceLoader always holds onto it. If
DoSelectCertificate does an early return somewhere, the request hangs. I've
fixed that in ChromeContentBrowserClient.

https://codereview.chromium.org/596873002/diff/20001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.cc:120: http_network_session_,
Parameter removed in favor of pulling the BrowserContext out of the WebContents
later on in the process. (Which was the only reason this was safe before; the
session was only accessed while we knew the WebContents was valid.)

[davidben, 2014-09-23 22:57:24]

https://codereview.chromium.org/596873002/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/596873002/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1735: NOTREACHED();
Removed this NOTREACHED because it's not NOTREACHED; a SharedWorker (probably a
ServiceWorker) too will not have a WebContents associated with it.

Unfortunately neither the old or new behavior is quite what we want... if we do
callback.Run(NULL), then we cache that no certificate was selected, and it's
tricky to get out of that state. (And even if we don't cache it, that socket
enters the socket pool...)

If we don't, then the request hangs and we hit our best friend, the cache lock.

Short of teaching the socket pool to be ultra-clever, I think our best bet is to
add a second callback to abort the request. I'll poke at that tomorrow.

[mmenke, 2014-09-24 18:03:41]

https://codereview.chromium.org/596873002/diff/100001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/596873002/diff/100001/content/browser/loader/...
content/browser/loader/resource_loader.cc:493: ssl_client_auth_handler_.reset();
Does this get us anything?  Suppose this CL makes enough changes as-is.

https://codereview.chromium.org/596873002/diff/100001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/596873002/diff/100001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:84:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Should include base/logging.h for DCHECK (Was an issue before, too)

https://codereview.chromium.org/596873002/diff/100001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:89: handler));
Can we move these two static functions into an anonymous namespace?  You'll have
to make the second one take a callback instead of
WeakPtr<SSLClientAuthHandler>...And I guess wrap it in another callback to add
cert as an argument, but we do slim down the header a bit.

[davidben, 2014-09-26 18:49:49]

https://codereview.chromium.org/596873002/diff/100001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/596873002/diff/100001/content/browser/loader/...
content/browser/loader/resource_loader.cc:493: ssl_client_auth_handler_.reset();
On 2014/09/24 18:03:41, mmenke wrote:
> Does this get us anything?  Suppose this CL makes enough changes as-is.

Not sure. Yeah, I figured I'd leave it as-is for now.

https://codereview.chromium.org/596873002/diff/100001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/596873002/diff/100001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:84:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/09/24 18:03:41, mmenke wrote:
> Should include base/logging.h for DCHECK (Was an issue before, too)

Done.

https://codereview.chromium.org/596873002/diff/100001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:89: handler));
On 2014/09/24 18:03:41, mmenke wrote:
> Can we move these two static functions into an anonymous namespace?  You'll
have
> to make the second one take a callback instead of
> WeakPtr<SSLClientAuthHandler>...And I guess wrap it in another callback to add
> cert as an argument, but we do slim down the header a bit.

Heh. I'd actually made them static to avoid the extra callback or making
CertificateSelected public. :-) Done.

[mmenke, 2014-10-01 14:02:03]

LGTM!  Sorry about latency.

[davidben, 2014-10-01 21:36:19]

davidben@chromium.org changed reviewers:
+ benm@chromium.org, jam@chromium.org

[davidben, 2014-10-01 21:36:20]

Okay, the owners coverage needed expanded dramatically. +jam for content/ and
chrome/ and +benm for android_webview/

[jam, 2014-10-06 17:03:50]

lgtm

[davidben, 2014-10-08 04:17:01]

benm: friendly ping

[benm (inactive), 2014-10-08 17:19:27]

On 2014/10/08 04:17:01, David Benjamin wrote:
> benm: friendly ping

lgtm % rebase.

cheers!

[davidben, 2014-10-08 17:57:04]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-10-08 17:58:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/596873002/140001

[commit-bot: I haz the power, 2014-10-09 04:46:50]

Committed patchset #8 (id:140001) as 04fa9dd1e145d59b995ebd7016f2f2cac2d273f7

[commit-bot: I haz the power, 2014-10-09 04:47:34]

Patchset 8 (id:??) landed as
https://crrev.com/99aee5eb0f6733566c60dfe356a1b7169477808f
Cr-Commit-Position: refs/heads/master@{#298782}

[robliao, 2014-11-26 17:51:38]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/766463002/ by robliao@chromium.org.

The reason for reverting is: Leak in SSLUITestWithClientCert.TestWSSClientCert

http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...

First reported leak:
Indirect leak of 104 byte(s) in 1 object(s) allocated from:
    #0 0x57659b in operator new(unsigned long)
(/tmp/run_tha_test0QXqvt/out/Release/browser_tests+0x57659b)
    #1 0x2c7762d in ProfileIOData::ResourceContext::CreateClientCertStore()
chrome/browser/profiles/profile_io_data.cc:920:7
    #2 0x6c908bd in
content::ResourceLoader::OnCertificateRequested(net::URLRequest*,
net::SSLCertRequestInfo*) content/browser/loader/resource_loader.cc:298:7
    #3 0x3e3e15f in
net::URLRequest::NotifyCertificateRequested(net::SSLCertRequestInfo*)
net/url_request/url_request.cc:1109:5
    #4 0x405ea0d in net::URLRequestHttpJob::OnStartCompleted(int)
net/url_request/url_request_http_job.cc:925:5
    #5 0x3ccd86c in Run base/callback.h:396:12
    #6 0x3ccd86c in net::HttpCache::Transaction::DoCallback(int)
net/http/http_cache_transaction.cc:710
    #7 0x3cca160 in HandleResult net/http/http_cache_transaction.cc:716:5
    #8 0x3cca160 in net::HttpCache::Transaction::DoLoop(int)
net/http/http_cache_transaction.cc:977
    #9 0x3cc7a68 in net::HttpCache::Transaction::OnIOComplete(int)
net/http/http_cache_transaction.cc:2934:3
    #10 0xc78765a in Run base/callback.h:396:12
    #11 0xc78765a in DevToolsNetworkTransaction::OnCallback(int)
chrome/browser/devtools/devtools_network_transaction.cc:73
    #12 0x3ceab0c in Run base/callback.h:396:12
    #13 0x3ceab0c in DoCallback net/http/http_network_transaction.cc:577
    #14 0x3ceab0c in net::HttpNetworkTransaction::OnIOComplete(int)
net/http/http_network_transaction.cc:594
    #15 0x394b58f in Run base/callback.h:396:12
    #16 0x394b58f in base::debug::TaskAnnotator::RunTask(char const*, char
const*, base::PendingTask const&) base/debug/task_annotator.cc:63
    #17 0x388dd3c in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:448:3
    #18 0x388e49b in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
const&) base/message_loop/message_loop.cc:458:5
    #19 0x388ef57 in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:567:13
    #20 0x3932db7 in
base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_libevent.cc:232:21
    #21 0x38c0c1c in base::RunLoop::Run() base/run_loop.cc:55:3
    #22 0x388c843 in base::MessageLoop::Run()
base/message_loop/message_loop.cc:310:3
    #23 0x6af33c1 in content::BrowserThreadImpl::IOThreadRun(base::MessageLoop*)
content/browser/browser_thread_impl.cc:218:3
    #24 0x6af375e in content::BrowserThreadImpl::Run(base::MessageLoop*)
content/browser/browser_thread_impl.cc:253:14
    #25 0x390cec1 in base::Thread::ThreadMain() base/threading/thread.cc:228:5
    #26 0x3901e17 in base::(anonymous namespace)::ThreadFunc(void*)
base/threading/platform_thread_posix.cc:80:3
    #27 0x7f99b0016e99 in start_thread
/build/buildd/eglibc-2.15/nptl/pthread_create.c:308.