Adding security info to canceled requests

This CL adds a way to specify the security info when canceling a URLRequest.
This allows to tag a request on the renderer side with its security info.  It is useful for the "frame info" dialog.
When showing that dialog for blocked frames, the security info can be retrieved and users can see the cert details for the blocked frame.

TEST=Open a page containing a frame served over bad HTTPS.  The frame is blocked (replaced with a warning message). Right-click, select "Frame info". The dialog should have a "show cert" button which when pressed should show the frame's bad cert.
BUG=2853
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=11179

[wtc, 2008-10-14 00:22:40]

LGTM.

http://codereview.chromium.org/7276/diff/1/9
File chrome/browser/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7276/diff/1/9#newcode157
Line 157: const std::string& security_info) {
The style guide's examples show that in a function declaration or definition, if
all the parameters can't fit in one line, you need to put one parameter on each
line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

If you want to fix this, there are several other place in this file that you
should fix.

http://codereview.chromium.org/7276/diff/1/9#newcode230
Line 230: const std::string& security_info) {
Why is the security_info parameter not used?

http://codereview.chromium.org/7276/diff/1/9#newcode565
Line 565: queued_error_request_id_, *queued_error_.get(), std::string());
The original indentation of this line is correct.

http://codereview.chromium.org/7276/diff/1/9#newcode593
Line 593: queued_error_request_id_, *queued_error_.get(), std::string());
The original indentation of this line is correct.

http://codereview.chromium.org/7276/diff/1/9#newcode1062
Line 1062: request_->CancelWithError(net::ERR_FILE_NOT_FOUND, net::SSLInfo());
You changed the indentation of this line by mistake.

http://codereview.chromium.org/7276/diff/1/12
File chrome/browser/resource_dispatcher_host.h (right):

http://codereview.chromium.org/7276/diff/1/12#newcode80
Line 80: // reasons.
Nit: I suggest changing "for security reasons" to "because of SSL errors".  The
reason is that security reasons may also include failing the JavaScript same
origin checks, which are not what you mean here.

http://codereview.chromium.org/7276/diff/1/11
File chrome/browser/ssl_manager.h (right):

http://codereview.chromium.org/7276/diff/1/11#newcode145
Line 145: void CompleteCancelRequest(int error, const net::SSLInfo& ssl_info);
You changed the indentation of this line by mistake.

http://codereview.chromium.org/7276/diff/1/8
File chrome/browser/ssl_policy.cc (right):

http://codereview.chromium.org/7276/diff/1/8#newcode168
Line 168: error->CancelRequest(error->ssl_info());
In this file, you always call error->CancelRequest or error->DenyRequest like
this:

  error->CancelRequest(error->ssl_info());

So it seems that CancelRequest and DenyRequest don't really need the ssl_info
parameter, because they can just use the object's ssl_info_ member.

http://codereview.chromium.org/7276/diff/1/5
File net/base/ssl_info.h (right):

http://codereview.chromium.org/7276/diff/1/5#newcode19
Line 19: SSLInfo(const SSLInfo& ssl_info)
Nit: add a blank line above this line.

http://codereview.chromium.org/7276/diff/1/5#newcode22
Line 22: cert(ssl_info.cert) { }
Nit: in the initializer list, list the members in the same
order they're declared.

http://codereview.chromium.org/7276/diff/1/5#newcode28
Line 28: cert = ssl_info.cert;
Nit: list the member assignments in the same order the members are declared.

http://codereview.chromium.org/7276/diff/1/7
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/7276/diff/1/7#newcode244
Line 244: response_info_.ssl_info = ssl_info;
Should we set the ssl_info only if we set the os_error?
That is, should we move this line into the if block above?

http://codereview.chromium.org/7276/diff/1/6
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7276/diff/1/6#newcode359
Line 359: // information used to inform about any SSL specific errors.
Why is ssl_info optional?  It is a reference, so you can't
pass NULL.  You should either make it a pointer, or add a
comment that says "Pass an empty SSLInfo object if there is
no SSL information".

[darin (slow to review), 2008-10-14 07:09:13]

http://codereview.chromium.org/7276/diff/1/6
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7276/diff/1/6#newcode360
Line 360: void CancelWithError(int os_error, const net::SSLInfo& ssl_info);
This seems like a fairly strange API to me.

It almost seems like what we really want are APIs like this:

  SimulateError(int error);
  SimulateSSLError(int error, const net::SSLInfo& ssl_info);

Related question:

  Do we want to restrict these APIs to only being callable after Start has been
called but before OnResponseStarted?  I'm not sure these methods make much sense
in other contexts.

[jcampan, 2008-10-14 18:54:02]

http://codereview.chromium.org/7276/diff/1/9
File chrome/browser/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7276/diff/1/9#newcode157
Line 157: const std::string& security_info) {
On 2008/10/14 00:22:40, wtc wrote:
> The style guide's examples show that in a function declaration or definition,
if
> all the parameters can't fit in one line, you need to put one parameter on
each
> line:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...
> 
> If you want to fix this, there are several other place in this file that you
> should fix.

OK, I fixed all the places where this happened in the file.

http://codereview.chromium.org/7276/diff/1/9#newcode230
Line 230: const std::string& security_info) {
On 2008/10/14 00:22:40, wtc wrote:
> Why is the security_info parameter not used?

The information is not used today for synchronous requests.
Filed a bug and added a TODO.

http://codereview.chromium.org/7276/diff/1/9#newcode565
Line 565: queued_error_request_id_, *queued_error_.get(), std::string());
On 2008/10/14 00:22:40, wtc wrote:
> The original indentation of this line is correct.

Done.

http://codereview.chromium.org/7276/diff/1/9#newcode593
Line 593: queued_error_request_id_, *queued_error_.get(), std::string());
On 2008/10/14 00:22:40, wtc wrote:
> The original indentation of this line is correct.

Done.

http://codereview.chromium.org/7276/diff/1/9#newcode1062
Line 1062: request_->CancelWithError(net::ERR_FILE_NOT_FOUND, net::SSLInfo());
On 2008/10/14 00:22:40, wtc wrote:
> You changed the indentation of this line by mistake.

Done.

http://codereview.chromium.org/7276/diff/1/12
File chrome/browser/resource_dispatcher_host.h (right):

http://codereview.chromium.org/7276/diff/1/12#newcode80
Line 80: // reasons.
On 2008/10/14 00:22:40, wtc wrote:
> Nit: I suggest changing "for security reasons" to "because of SSL errors". 
The
> reason is that security reasons may also include failing the JavaScript same
> origin checks, which are not what you mean here.
> 

Done.

http://codereview.chromium.org/7276/diff/1/11
File chrome/browser/ssl_manager.h (right):

http://codereview.chromium.org/7276/diff/1/11#newcode145
Line 145: void CompleteCancelRequest(int error, const net::SSLInfo& ssl_info);
On 2008/10/14 00:22:40, wtc wrote:
> You changed the indentation of this line by mistake.

Done.

http://codereview.chromium.org/7276/diff/1/8
File chrome/browser/ssl_policy.cc (right):

http://codereview.chromium.org/7276/diff/1/8#newcode168
Line 168: error->CancelRequest(error->ssl_info());
On 2008/10/14 00:22:40, wtc wrote:
> In this file, you always call error->CancelRequest or error->DenyRequest like
> this:
> 
>   error->CancelRequest(error->ssl_info());
> 
> So it seems that CancelRequest and DenyRequest don't really need the ssl_info
> parameter, because they can just use the object's ssl_info_ member.
Per our discussion: that is because CancelRequest is implemented in the base
class ErrorHandler which does not have access to the ssl_info_ member (defined
in the sub-class ErrorHandler).

http://codereview.chromium.org/7276/diff/1/5
File net/base/ssl_info.h (right):

http://codereview.chromium.org/7276/diff/1/5#newcode19
Line 19: SSLInfo(const SSLInfo& ssl_info)
On 2008/10/14 00:22:40, wtc wrote:
> Nit: add a blank line above this line.

Done.

http://codereview.chromium.org/7276/diff/1/5#newcode22
Line 22: cert(ssl_info.cert) { }
On 2008/10/14 00:22:40, wtc wrote:
> Nit: in the initializer list, list the members in the same
> order they're declared.

Done.

http://codereview.chromium.org/7276/diff/1/5#newcode28
Line 28: cert = ssl_info.cert;
On 2008/10/14 00:22:40, wtc wrote:
> Nit: list the member assignments in the same order the members are declared.

Done.

http://codereview.chromium.org/7276/diff/1/7
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/7276/diff/1/7#newcode244
Line 244: response_info_.ssl_info = ssl_info;
On 2008/10/14 00:22:40, wtc wrote:
> Should we set the ssl_info only if we set the os_error?
> That is, should we move this line into the if block above?

Done.

http://codereview.chromium.org/7276/diff/1/6
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7276/diff/1/6#newcode359
Line 359: // information used to inform about any SSL specific errors.
On 2008/10/14 00:22:40, wtc wrote:
> Why is ssl_info optional?  It is a reference, so you can't
> pass NULL.  You should either make it a pointer, or add a
> comment that says "Pass an empty SSLInfo object if there is
> no SSL information".
After renaming the API as Darin suggested, this no longer applies.

http://codereview.chromium.org/7276/diff/1/6#newcode360
Line 360: void CancelWithError(int os_error, const net::SSLInfo& ssl_info);
On 2008/10/14 07:09:13, darin wrote:
> This seems like a fairly strange API to me.
> 
> It almost seems like what we really want are APIs like this:
> 
>   SimulateError(int error);
>   SimulateSSLError(int error, const net::SSLInfo& ssl_info);
OK, I renamed the API as you suggested.

> Related question:
>   Do we want to restrict these APIs to only being callable after Start has
been
> called but before OnResponseStarted?  I'm not sure these methods make much
sense
> in other contexts.
Yep, that makes sense. I test for that now. I had to expose the URLRequestJob
has_handled_response_ field (through a getter) to test whether the response was
started.
Let me know if there is a better way to do this.

[wtc, 2008-10-14 23:31:37]

http://codereview.chromium.org/7276/diff/1/6
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7276/diff/1/6#newcode360
Line 360: void CancelWithError(int os_error, const net::SSLInfo& ssl_info);
On 2008/10/14 07:09:13, darin wrote:
> This seems like a fairly strange API to me.
> 
> It almost seems like what we really want are APIs like this:
> 
>   SimulateError(int error);
>   SimulateSSLError(int error, const net::SSLInfo& ssl_info);

Darin, "SimulateError" also sounds strange because the 'error' argument is a
real error; it's just not the default net::ERR_ABORTED.

http://codereview.chromium.org/7276/diff/39/43
File net/base/ssl_info.h (right):

http://codereview.chromium.org/7276/diff/39/43#newcode32
Line 32: }
One more thing: if the copy constructor and assignment operator are the same as
what the compiler would generate for us (memberwise copy), I remember Darin's
preference is that we should not bother to define them.

[jcampan, 2008-10-16 20:48:01]

New snapshot pushed.
Darin, are you OK with the change?

Jay

http://codereview.chromium.org/7276/diff/39/43
File net/base/ssl_info.h (right):

http://codereview.chromium.org/7276/diff/39/43#newcode32
Line 32: }
On 2008/10/14 23:31:37, wtc wrote:
> One more thing: if the copy constructor and assignment operator are the same
as
> what the compiler would generate for us (memberwise copy), I remember Darin's
> preference is that we should not bother to define them.

Done.

[darin (slow to review), 2009-02-18 17:27:26]

It looks like this CL didn't land.  Perhaps because you were still waiting for
my feedback?  If so, I'm sorry for the huge delay.  Let me know if I should
still review the patch since it has been a while.

[jcampan, 2009-02-19 18:58:02]

On 2009/02/18 17:27:26, darin wrote:
> It looks like this CL didn't land.  Perhaps because you were still waiting for
> my feedback?  If so, I'm sorry for the huge delay.  Let me know if I should
> still review the patch since it has been a while.

OK, I resurrected the CL, ready for review again.

[jcampan, 2009-03-04 19:15:56]

Do you think you'll have time to look at this?

Jay

[darin (slow to review), 2009-03-04 19:29:01]

just some initial comments..

http://codereview.chromium.org/7276/diff/2639/1653
File webkit/glue/resource_handle_impl.cc (right):

http://codereview.chromium.org/7276/diff/2639/1653#newcode608
Line 608: const std::string& security_info) {
i don't see us doing anything with this security_info parameter here, which
leads me to wonder why we need this parameter at all.

http://codereview.chromium.org/7276/diff/2639/1640
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7276/diff/2639/1640#newcode95
Line 95: const std::string& security_info) = 0;
so, iirc, we echo the security_info here in case OnReceivedResponse was not
called?  and that only happens in the case of an error, right?

[jcampan, 2009-03-04 22:33:43]

http://codereview.chromium.org/7276/diff/2639/1653
File webkit/glue/resource_handle_impl.cc (right):

http://codereview.chromium.org/7276/diff/2639/1653#newcode608
Line 608: const std::string& security_info) {
On 2009/03/04 19:29:01, darin wrote:
> i don't see us doing anything with this security_info parameter here, which
> leads me to wonder why we need this parameter at all.

It's now part of the peer interface.
the security filter peers (which for example display the blocked frame message)
use it.

http://codereview.chromium.org/7276/diff/2639/1640
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7276/diff/2639/1640#newcode95
Line 95: const std::string& security_info) = 0;
On 2009/03/04 19:29:01, darin wrote:
> so, iirc, we echo the security_info here in case OnReceivedResponse was not
> called?
That's right, if the load of a resource failed we only get that call. The
provided security info can be used by the security filter peer and attached to
content they create. This is the case with sub-frames, if a sub-frame fails to
load because of a SSL error we replace it with some HTML showing an error. Since
now we attach the ssl info, the bad cert will be viewable for that frame.

>and that only happens in the case of an error, right?
We still provide the info for successful cases, even though it was in that case
already provided in the ResponseInfo of OnReceiveResponse.

[darin (slow to review), 2009-03-05 00:01:15]

LGTM except for:

http://codereview.chromium.org/7276/diff/2639/1654
File net/base/ssl_info.h (right):

http://codereview.chromium.org/7276/diff/2639/1654#newcode20
Line 20: SSLInfo(const SSLInfo& ssl_info)
what's wrong with the default copy constructor and assignment operator?  it
should be the same thing.