Don't select a client certs for TabContents with no TabContentsWrapper.

Don't select a client certs for TabContents with no TabContentsWrapper.

BUG=104757
TEST=manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112892

[markusheintz_, 2011-11-30 19:33:11]

@mnissler: FYI. BTW:Thanks for your support and patience with all my questions
:-)

@wtc: I'm not sure if this quick fix is free of side effects. I will need to do
more testing. I just wanted to loop  you in early on this.

[Mattias Nissler (ping if slow), 2011-11-30 19:56:16]

drive-by, so FYI.

We should also check whether this affects behavior of any TabContents that don't
have a wrapper (extensions? I don't know this stuff at all), and if so, figure
out what we can do about it.

http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:859: if (wrapper) {
no need for curlies here.

[wtc, 2011-11-30 23:59:43]

Patch Set 2 LGTM.

I'm not a good reviewer of this CL because I am not familiar
with TabContents and TabContentsWrapper.  It would be nice if
you could ask someone else who knows those two classes as a
second reviewer.

http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:862:
handler->CertificateSelected(NULL);

It seems that what we do in this case should match what we
do above on lines 853-854 if |tab| is NULL.

[Mattias Nissler (ping if slow), 2011-12-01 09:49:12]

On Thu, Dec 1, 2011 at 12:59 AM, <wtc@chromium.org> wrote:

> Patch Set 2 LGTM.
>
> I'm not a good reviewer of this CL because I am not familiar
> with TabContents and TabContentsWrapper.  It would be nice if
> you could ask someone else who knows those two classes as a
> second reviewer.


You may want to ask avi@, who knows this stuff well.


>
>
>
> http://codereview.chromium.**org/8745020/diff/2001/chrome/**
>
browser/chrome_content_**browser_client.cc<http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_content_browser_client.cc>
> File chrome/browser/chrome_content_**browser_client.cc (right):
>
> http://codereview.chromium.**org/8745020/diff/2001/chrome/**
>
browser/chrome_content_**browser_client.cc#newcode862<http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_content_browser_client.cc#newcode862>
> chrome/browser/chrome_content_**browser_client.cc:862:
> handler->CertificateSelected(**NULL);
>
> It seems that what we do in this case should match what we
> do above on lines 853-854 if |tab| is NULL.
>
>
http://codereview.chromium.**org/8745020/<http://codereview.chromium.org/8745...
>

[markusheintz_, 2011-12-02 14:54:19]

@avi: Could you check the TabContents related changes in this CL? Do you know
whether there exists a 1:1 relation ship between TabContents and
TabContentsWrapper?

@all:
I move the code for automatically selecting a client cert to the
ChromeContentBrowserClient. This way I can apply the AutoSelectCert... policy
even if I don't have a TabContentsWrapper. As far as I can tell background pages
of extensions don't have a TabContentsWrapper. Without a TabContentsWrapper I
can not show the user a dialog to select a certificate (Which I would treat the
same way as a user canceling the dialog). 

Could you please take another look at the CL? Thanks a lot

http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:859: if (wrapper) {
On 2011/11/30 19:56:16, Mattias Nissler wrote:
> no need for curlies here.

I think the style guide is ok with that if you have an else.

http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:859: if (wrapper) {
On 2011/11/30 19:56:16, Mattias Nissler wrote:
> no need for curlies here.

I guess the style quide is fine with that.

http://codereview.chromium.org/8745020/diff/2001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:862:
handler->CertificateSelected(NULL);
On 2011/11/30 23:59:43, wtc wrote:
> 
> It seems that what we do in this case should match what we
> do above on lines 853-854 if |tab| is NULL.

What happens to the request if we don't call CertificateSelected(NULL) on the
handler? Does it have to time out?

I can not add a NOTREACHED, this would crash chrome every time a background page
of an extension send a xhr to a site that requests a client cert.

[Avi (use Gerrit), 2011-12-02 15:00:27]

On 2011/12/02 14:54:19, markusheintz_ wrote:
> @avi: Could you check the TabContents related changes in this CL? Do you know
> whether there exists a 1:1 relation ship between TabContents and
> TabContentsWrapper?

For a given TC there may or may not be a TCW. If the TC is used to host web
pages in a tab, then it will have a TCW. But TCs are used raw in cases like
constrained dialogs, extension bubbles, notification bubbles, etc.

[wtc, 2011-12-02 19:01:48]

Patch Set 5 LGTM.

http://codereview.chromium.org/8745020/diff/9002/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8745020/diff/9002/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:915: // show the user a dialog
to select a client certificate. So we simple

Nit: simple => simply

NOTE: "cancel the request" is not accurate.  handler->CertificateSelected(NULL)
instructs the SSL library to proceed with no client certificate.
In most cases, the server will abort the SSL handshake.
But some servers may allow the SSL handshake to succeed
but return an HTTP error page.

[markusheintz_, 2011-12-03 13:25:41]

Thanks for the reviews and comments.

http://codereview.chromium.org/8745020/diff/9002/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8745020/diff/9002/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:915: // show the user a dialog
to select a client certificate. So we simple
On 2011/12/02 19:01:48, wtc wrote:
> 
> Nit: simple => simply
> 
> NOTE: "cancel the request" is not accurate. 
handler->CertificateSelected(NULL)
> instructs the SSL library to proceed with no client certificate.
> In most cases, the server will abort the SSL handshake.
> But some servers may allow the SSL handshake to succeed
> but return an HTTP error page.

Done.

[commit-bot: I haz the power, 2011-12-03 13:31:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/8745020/15003

[commit-bot: I haz the power, 2011-12-03 15:19:46]

Change committed as 112892

[wtc, 2011-12-06 00:22:47]

http://codereview.chromium.org/8745020/diff/15003/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8745020/diff/15003/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:917: // cancel the request.

Nit: you should also change "cancel the request" to
"proceed with no client certificate".