New extension API: "tab.socketAddress"

This change captures the low-level socket address whenever a page is loaded,
and pushes it all the way up to the 'tabs' API, where extensions can read it
and do nifty things, such as displaying an IPv4/IPv6 indicator.

Here's a description of what's happening:

1.  Extract the address in http_stream_parser.cc (for HTTP) or spdy_http_stream
    (for SPDY).  Note that if the connection is via a proxy, then we get the
    address of the proxy server... not terribly useful, but not wrong either.
2.  Put it into an HttpResponseInfo instance.
3.  HttpResponseInfo (de)serializes the address.
4.  Add a socket_address accessor in url_request.h, which pulls the address
    from its HttpResponseInfo instance.
5.  In resource_dispatcher_host.cc, move the address from URLRequest to
    ResourceResponseHead (which is a subclass of ResourceResponseInfo).
6.  In render_messages.cc (de)serialize the ResourceResponseInfo.
7.  In weburlloader_impl.cc, copy from ResourceResponseInfo to WebURLResponse.

At this point, a patch to third_party/WebKit/Source is required:
http://www.pmarks.net/posted_links/socket_address_webkit.patch

8.  In render_view.cc, copy from WebURLResponse to
    ViewHostMsg_FrameNavigate_Params.
9.  In render_messages_params.cc, *_FrameNavigate_Params is (de)serialized.
10. In navigation_controller.cc, copy from *_FrameNavigate_Params to a
    NavigationEntry.

Note: there's a warning about SessionService in navigation_entry.h, but I don't
      think it's relevant here.  In the worst case, we just don't report the
      address after restoring a tab, but I haven't seen this occur.

11. In extension_tabs_module.cc, check if the current NavigationEntry has a
    socket address.  If so, add a "socketAddress" string to the JSON object.
12. Finally, when someone registers a callback using chrome.tabs.onUpdated,
    the 'tab' parameter includes the new socketAddress parameter.

Extensions can use the new paramter to display an IPv4/IPv6 icon, or do
whatever else with the IP and port.  I've written a simple demo:
http://www.pmarks.net/posted_links/seetheips-newapi.crx

BUG=51663
TEST=unit tests, demo extension.

[pmarks, 2011-01-19 10:08:35]

Hi Eric, let me know if you can do this code review, or if you know of someone
else who'd be better-suited for it.

(You may recall that I worked on some Chrome stuff back in 2009, related to
googleurl and IP/hostname parsing and canonicalization.)

[agl, 2011-01-19 13:59:25]

Have you confirmed this change with the extensions folks? They are very careful
about their API.

[Erik does not do reviews, 2011-01-19 18:38:13]

As Adam says, there are some rules and restrictions about adding new
extension APIs.  Please do not commit this CL.

See this document for info on how to proceed:
https://sites.google.com/a/chromium.org/dev/developers/design-documents/exten...


Erik

<https://sites.google.com/a/chromium.org/dev/developers/design-documents/exten...>

On Wed, Jan 19, 2011 at 5:59 AM, <agl@chromium.org> wrote:

> Have you confirmed this change with the extensions folks? They are very
> careful
> about their API.
>
>
> http://codereview.chromium.org/6369003/
>

[eroman, 2011-01-19 19:21:30]

Some high level comments:

 - Need similar plumbing for FTP responses
 - What is the meaning of the "socket address" field when the response was
served from cache? It looks to me like the current code will return the socket
address of the initial network response. Not sure it makes sense to fill the
socket address field at all in this case. Similarly, what if the cache entry's
headers are updated (say by an if-modified-since) -- the socket address is going
to remain as the original server.

 - What happens on redirects? Specifically, it seems like we need to reset this
field on redirects. Lets say for example we do a URLRequest to an http:// URL,
which then redirects to a file:// URL... On sending the HTTP response we will
write the socket address, however ultimately we end up in a file:// URL for
which socket address field has no meaning, and should therefore be empty string.



Anyhow, I will review this more thoroughly once the extension folks have
reviewed your API proposal.

http://codereview.chromium.org/6369003/diff/45001/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/6369003/diff/45001/net/http/http_response_info...
net/http/http_response_info.cc:210: flags |= RESPONSE_INFO_HAS_SOCKET_ADDRESS;
Is the flag necessary? I would expect that pickle->WriteString() /
pickle->ReadString() should work with zero-length strings just the same.

[eroman, 2011-01-19 19:28:15]

Seems like this use-case might fit into the webNavigation/webRequest API.

Be sure to include Jochen in the extension API discussion.

[pmarks, 2011-01-20 08:05:53]

Awesome, so I'm looking at the documentation for
chrome.webRequest.onRequestSent, and it says:

"ip (string) - The server IP address that is actually connected to.  Note that
it may be a literal IPv6 address."

Jochen, has that been implemented already?

[jochen (gone - plz use gerrit), 2011-01-20 09:16:23]

It's on our okr for this quarter, however, nothing is implemented yet

Jochen
On Jan 20, 2011 9:06 AM, <pmarks@google.com> wrote:
> Awesome, so I'm looking at the documentation for
> chrome.webRequest.onRequestSent, and it says:
>
> "ip (string) - The server IP address that is actually connected to. Note
> that
> it may be a literal IPv6 address."
>
> Jochen, has that been implemented already?
>
>
> http://codereview.chromium.org/6369003/

[pmarks, 2011-01-21 08:19:54]

I think I'm just going to drop this CL and leave it here as a reference, since
this feature is already on the plate of the webRequest implementers, and they
almost certainly have more coherent answers to the caching and redirect
questions than I do.

[pmarks, 2011-01-21 08:21:56]

http://codereview.chromium.org/6369003/diff/45001/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/6369003/diff/45001/net/http/http_response_info...
net/http/http_response_info.cc:210: flags |= RESPONSE_INFO_HAS_SOCKET_ADDRESS;
On 2011/01/19 19:21:30, eroman wrote:
> Is the flag necessary? I would expect that pickle->WriteString() /
> pickle->ReadString() should work with zero-length strings just the same.

I added this flag so that it'd be possible to unpickle an older version of the
object.  I think that allowing ReadString() to return false would also work.

[Brian Ryner, 2011-02-03 19:21:01]

FYI, we'd like to use this functionality for client-side phishing detection as
well.  After some discussion with the WebRequest folks, I'm going to proceed
with adding the backend support for this (largely based on pmarks' patch, though
I'll open a new issue), and then the WebRequest folks can handle adding an
extension API once they're ready.

[Brian Ryner, 2011-02-05 19:50:21]

On 2011/01/21 08:21:56, pmarks wrote:
>
http://codereview.chromium.org/6369003/diff/45001/net/http/http_response_info.cc
> File net/http/http_response_info.cc (right):
> 
>
http://codereview.chromium.org/6369003/diff/45001/net/http/http_response_info...
> net/http/http_response_info.cc:210: flags |= RESPONSE_INFO_HAS_SOCKET_ADDRESS;
> On 2011/01/19 19:21:30, eroman wrote:
> > Is the flag necessary? I would expect that pickle->WriteString() /
> > pickle->ReadString() should work with zero-length strings just the same.
> 
> I added this flag so that it'd be possible to unpickle an older version of the
> object.  I think that allowing ReadString() to return false would also work.

It seems like if we don't have a flag, and instead just ignore the return value
from ReadString(), we won't be able to distinguish between the case where the
socket address is missing (which is ok) and the case where it's malformed (for
example, if the data was truncated somehow).  In the latter case, I'd think we
would want to give up on deserializing the ResponseInfo.  So the flag seems
preferable to me -- am I missing something?

[Brian Ryner, 2011-02-05 21:27:05]

On 2011/01/19 19:21:30, eroman wrote:
>  - What happens on redirects? Specifically, it seems like we need to reset
this
> field on redirects. Lets say for example we do a URLRequest to an http:// URL,
> which then redirects to a file:// URL... On sending the HTTP response we will
> write the socket address, however ultimately we end up in a file:// URL for
> which socket address field has no meaning, and should therefore be empty
string.

This particular case doesn't end up working anyway, because file: isn't
considered a "safe" redirect for HTTP (see URLRequestHttpJob::IsSafeRedirect) so
the redirect is not followed.  But in general it seems like it's working as you
described, because URLRequest::Redirect calls PrepareToRestart, which clears out
the HTTPResponseInfo before beginning the redirect.  I verified this by testing
a redirect to FTP.

re: making the socket address work for FTP, I'm not sure what the best way is to
proceed.  URLRequest only has an HTTPResponseInfo field (i.e. there's no
FTPResponseInfo).  I think it would be possible to copy the value across to the
HTTPResponseInfo in URLRequestFTPJob; would that make sense or is there a better
way to do this (without making major changes to URLRequest).

Thanks,

[pmarks, 2011-02-06 03:26:59]

On 2011/02/05 19:50:21, Brian Ryner wrote:
> > I added this flag so that it'd be possible to unpickle an older version of
the
> > object.  I think that allowing ReadString() to return false would also work.
> 
> It seems like if we don't have a flag, and instead just ignore the return
value
> from ReadString(), we won't be able to distinguish between the case where the
> socket address is missing (which is ok) and the case where it's malformed (for
> example, if the data was truncated somehow).  In the latter case, I'd think we
> would want to give up on deserializing the ResponseInfo.  So the flag seems
> preferable to me -- am I missing something?

Well, look at BaseSessionService::RestoreUpdateTabNavigationCommand(), for
example.  The type_mask and referrer fields were added later, so they're
permitted to fail.  As soon as one fails, it stops reading and assumes it's got
an older version.

In order to make the versioning work without a flag, you'd have to *always*
write the socket address, even if it's empty.

On 2011/02/05 21:27:05, Brian Ryner wrote:
> re: making the socket address work for FTP, I'm not sure what the best way is
to
> proceed.  URLRequest only has an HTTPResponseInfo field (i.e. there's no
> FTPResponseInfo).  I think it would be possible to copy the value across to
the
> HTTPResponseInfo in URLRequestFTPJob; would that make sense or is there a
better
> way to do this (without making major changes to URLRequest).

For FTP, the socket address should be extracted from
FtpNetworkTransaction::ctrl_socket_ at some point.

To export the address from URLRequest, I see two approaches:
1. Implement URLRequestFtpJob::GetResponseInfo(), and make it set only the
socket_address field.
2. Add a URLRequest::GetSocketAddress() method, which works analogously to
GetLoadState().  This requires adding a virtual GetSocketAddress() to
URLRequestJob as well.

Option 1 is less code, but it involves putting non-HTTP state into
"HttpResponseInfo", which could be considered dishonest.