Propagate the remote socket address to URLRequest and to ViewHostMsg_FrameNavigate.

Propagate the remote socket address to URLRequest and to ViewHostMsg_FrameNavigate.

This will be used to run pre-classification checks for client-side phishing
detection, and will also enable the socket address to be exposed via the
webRequest extension API.  This is adapted from the original patch by pmarks
on http://codereview.chromium.org/6369003/ .

BUG=51663
TEST=added socket address checks to various unittests

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

[pmarks, 2011-02-11 00:36:38]

Note that in my original change, the JavaScript part served as an end-to-end
test for the whole data path.  It would be nice to have a replacement test, but
I wouldn't know how to achieve that without the API.

http://codereview.chromium.org/6488010/diff/1/chrome/common/render_messages_p...
File chrome/common/render_messages_params.h (right):

http://codereview.chromium.org/6488010/diff/1/chrome/common/render_messages_p...
chrome/common/render_messages_params.h:308: // to inquisitive users.  Can be
"ipv4:port", "[ipv6]:port", or empty.
In my original change, I used this wording to clarify that the value was for the
UI, and didn't actually affect the operation of the browser.  It sounds like
your use case makes this no longer true.

http://codereview.chromium.org/6488010/diff/1/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/6488010/diff/1/net/http/http_response_info.cc#...
net/http/http_response_info.cc:164: // so we don't fail if it can't be read.
I would add "If additional fields are added in a future version, then they must
only be read if this operation succeeds."

[Paweł Hajdan Jr., 2011-02-11 19:34:54]

Drive-by with FTP-related comments. Please let me do another review before
committing.

http://codereview.chromium.org/6488010/diff/1/net/ftp/ftp_network_transaction.cc
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/6488010/diff/1/net/ftp/ftp_network_transaction...
net/ftp/ftp_network_transaction.cc:631: response_.socket_address.clear();
Do we really want it to be a silent failure? That seems worrying.

http://codereview.chromium.org/6488010/diff/1/net/spdy/spdy_network_transacti...
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6488010/diff/1/net/spdy/spdy_network_transacti...
net/spdy/spdy_network_transaction_unittest.cc:184: return;
nit: While you're here, could you remove this weird return statement at the end
of a method?

http://codereview.chromium.org/6488010/diff/1/net/url_request/url_request_ftp...
File net/url_request/url_request_ftp_job.cc (right):

http://codereview.chromium.org/6488010/diff/1/net/url_request/url_request_ftp...
net/url_request/url_request_ftp_job.cc:56: if (!transaction_.get()) {
It seems that other methods here assume transaction_ is non-NULL. Is this
NULL-check really needed?

http://codereview.chromium.org/6488010/diff/1/net/url_request/url_request_ftp...
net/url_request/url_request_ftp_job.cc:61: return response_info ?
response_info->socket_address : "";
Similarly here, do we need to NULL-check?

[Brian Ryner, 2011-02-11 23:01:07]

I added an end-to-end test as pmarks suggested -- currently it's in
render_view_host_browsertest, but I'm happy to move it elsewhere if there's a
better spot.

Please take another look.

http://codereview.chromium.org/6488010/diff/1/chrome/common/render_messages_p...
File chrome/common/render_messages_params.h (right):

http://codereview.chromium.org/6488010/diff/1/chrome/common/render_messages_p...
chrome/common/render_messages_params.h:308: // to inquisitive users.  Can be
"ipv4:port", "[ipv6]:port", or empty.
On 2011/02/11 00:36:39, pmarks wrote:
> In my original change, I used this wording to clarify that the value was for
the
> UI, and didn't actually affect the operation of the browser.  It sounds like
> your use case makes this no longer true.

True.  I removed the "for presenting to inquisitive users" phrase.

http://codereview.chromium.org/6488010/diff/1/net/ftp/ftp_network_transaction.cc
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/6488010/diff/1/net/ftp/ftp_network_transaction...
net/ftp/ftp_network_transaction.cc:631: response_.socket_address.clear();
On 2011/02/11 19:34:54, Paweł Hajdan Jr. wrote:
> Do we really want it to be a silent failure? That seems worrying.

GetPeerAddress does seem to occasionally return errors in the unittests, at
least for HTTP.  I didn't dig into it too closely since I figured it was better
to always check the status code.  Is it possible that the socket has already
disconnected by the time this runs?

http://codereview.chromium.org/6488010/diff/1/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/6488010/diff/1/net/http/http_response_info.cc#...
net/http/http_response_info.cc:164: // so we don't fail if it can't be read.
On 2011/02/11 00:36:39, pmarks wrote:
> I would add "If additional fields are added in a future version, then they
must
> only be read if this operation succeeds."

Done.

http://codereview.chromium.org/6488010/diff/1/net/spdy/spdy_network_transacti...
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6488010/diff/1/net/spdy/spdy_network_transacti...
net/spdy/spdy_network_transaction_unittest.cc:184: return;
On 2011/02/11 19:34:54, Paweł Hajdan Jr. wrote:
> nit: While you're here, could you remove this weird return statement at the
end
> of a method?

Done.

http://codereview.chromium.org/6488010/diff/1/net/url_request/url_request_ftp...
File net/url_request/url_request_ftp_job.cc (right):

http://codereview.chromium.org/6488010/diff/1/net/url_request/url_request_ftp...
net/url_request/url_request_ftp_job.cc:56: if (!transaction_.get()) {
On 2011/02/11 19:34:54, Paweł Hajdan Jr. wrote:
> It seems that other methods here assume transaction_ is non-NULL. Is this
> NULL-check really needed?

I was trying to make sure it was safe to call GetSocketAddress() at any time,
such as before Start() is called, or after Kill() is called.

http://codereview.chromium.org/6488010/diff/1/net/url_request/url_request_ftp...
net/url_request/url_request_ftp_job.cc:61: return response_info ?
response_info->socket_address : "";
On 2011/02/11 19:34:54, Paweł Hajdan Jr. wrote:
> Similarly here, do we need to NULL-check?

Here, probably not, it looks like GetResponseInfo() can never return NULL. 
Fixed.

[Paweł Hajdan Jr., 2011-02-14 09:10:02]

http://codereview.chromium.org/6488010/diff/1/net/ftp/ftp_network_transaction.cc
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/6488010/diff/1/net/ftp/ftp_network_transaction...
net/ftp/ftp_network_transaction.cc:631: response_.socket_address.clear();
On 2011/02/11 23:01:07, Brian Ryner wrote:
> On 2011/02/11 19:34:54, Paweł Hajdan Jr. wrote:
> > Do we really want it to be a silent failure? That seems worrying.
> 
> GetPeerAddress does seem to occasionally return errors in the unittests, at
> least for HTTP.  I didn't dig into it too closely since I figured it was
better
> to always check the status code.  Is it possible that the socket has already
> disconnected by the time this runs?

The response seems to be missing the point. Maybe we need this check. However,
I'd rather want to _propagate_ the failure rather than clear some random member
variable.

I'm not sure whether the socket can be disconnected right after connecting. I
guess it might be, for example when the network suddenly breaks, but I'm not
sure.

[Brian Ryner, 2011-02-14 20:29:20]

So just to clarify, you're thinking that we would want to take the error code
returned by GetPeerAddress (if it's not OK), and return it from the calling
function rather than proceeding?

I'd be fine with that -- it would ensure that the socket address is populated if
the request succeeds, and would give us a better idea whether GetPeerAddress is
failing in any unusual ways.  It does increase the risk from this change
slightly however.

[Paweł Hajdan Jr., 2011-02-15 08:37:38]

On 2011/02/14 20:29:20, Brian Ryner wrote:
> So just to clarify, you're thinking that we would want to take the error code
> returned by GetPeerAddress (if it's not OK), and return it from the calling
> function rather than proceeding?

Yeah.

> I'd be fine with that -- it would ensure that the socket address is populated
if
> the request succeeds, and would give us a better idea whether GetPeerAddress
is
> failing in any unusual ways.  It does increase the risk from this change
> slightly however.

Indeed, but I think the risk of silent failures is greater.

[Brian Ryner, 2011-02-15 21:32:45]

Ok, updated the CL to propagate GetPeerAddress failures.  Please take another
look.

[eroman, 2011-02-16 21:41:13]

http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_network_transac...
File net/ftp/ftp_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_network_transac...
net/ftp/ftp_network_transaction_unittest.cc:834: EXPECT_EQ("192.0.2.33:0",
transaction_.GetResponseInfo()->socket_address);
Is that port number of zero an artifact of how the mock socket dependencies
work? (should be 21 otherwise).

http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_response_info.h
File net/ftp/ftp_response_info.h (right):

http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_response_info.h...
net/ftp/ftp_response_info.h:44: std::string socket_address;
Can you use a HostPortPair here instead. We prefer passing that around as
opposed to formatted strings, since it better abstracts the separate components
and formatting issues with IPv6.

http://codereview.chromium.org/6488010/diff/19001/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/6488010/diff/19001/net/http/http_response_info...
net/http/http_response_info.cc:77: socket_address(rhs.socket_address),
Out of curiosity, how did you know to update these? Did a test fail, or were you
awesome enough to anticipate the need?

(Want to understand what practices can make this pattern less fragile. I have
always wanted to do some form of compile-time assert that checked
copyconstructor/assignment operators got updated when fields were added).

http://codereview.chromium.org/6488010/diff/19001/net/http/http_response_info.h
File net/http/http_response_info.h (right):

http://codereview.chromium.org/6488010/diff/19001/net/http/http_response_info...
net/http/http_response_info.h:73: std::string socket_address;
Please use a HostPortPair instead, and avoid describing the formatting rules
here. (Leave the formatting decisions to caller).

http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request...
net/url_request/url_request.cc:263: return job_->GetSocketAddress();
Isn't the information in response_info_ too?

http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request...
net/url_request/url_request.h:422: std::string GetSocketAddress() const;
Can you use HostPortPair instead?

[Brian Ryner, 2011-02-16 22:52:29]

One question about switching to HostPortPair -- do you think we should do this
for the ResourceResponse and ViewHostMsg_FrameNavigate as well, or just for the
structures under net/ ?   Propagating the HostPortPair all the way through is
slightly trickier because the WebKit portion of this uses a string to store the
socket address in WebURLResponse, so we would either have to parse it back out
of there, or break it up there into separate host and port fields.  Thoughts?


On 2011/02/16 21:41:13, eroman wrote:
>
http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_network_transac...
> File net/ftp/ftp_network_transaction_unittest.cc (right):
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_network_transac...
> net/ftp/ftp_network_transaction_unittest.cc:834: EXPECT_EQ("192.0.2.33:0",
> transaction_.GetResponseInfo()->socket_address);
> Is that port number of zero an artifact of how the mock socket dependencies
> work? (should be 21 otherwise).
> 
> http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_response_info.h
> File net/ftp/ftp_response_info.h (right):
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_response_info.h...
> net/ftp/ftp_response_info.h:44: std::string socket_address;
> Can you use a HostPortPair here instead. We prefer passing that around as
> opposed to formatted strings, since it better abstracts the separate
components
> and formatting issues with IPv6.
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/http/http_response_info.cc
> File net/http/http_response_info.cc (right):
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/http/http_response_info...
> net/http/http_response_info.cc:77: socket_address(rhs.socket_address),
> Out of curiosity, how did you know to update these? Did a test fail, or were
you
> awesome enough to anticipate the need?
> 
> (Want to understand what practices can make this pattern less fragile. I have
> always wanted to do some form of compile-time assert that checked
> copyconstructor/assignment operators got updated when fields were added).
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/http/http_response_info.h
> File net/http/http_response_info.h (right):
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/http/http_response_info...
> net/http/http_response_info.h:73: std::string socket_address;
> Please use a HostPortPair instead, and avoid describing the formatting rules
> here. (Leave the formatting decisions to caller).
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request.cc
> File net/url_request/url_request.cc (right):
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request...
> net/url_request/url_request.cc:263: return job_->GetSocketAddress();
> Isn't the information in response_info_ too?
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request.h
> File net/url_request/url_request.h (right):
> 
>
http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request...
> net/url_request/url_request.h:422: std::string GetSocketAddress() const;
> Can you use HostPortPair instead?

[eroman, 2011-02-17 00:39:10]

> One question about switching to HostPortPair -- do you think we should do this
> for the ResourceResponse and ViewHostMsg_FrameNavigate as well, or just for
the
> structures under net/ ? 

Primarily I would like this for the structures under net/*. I'll be happy if you
can do it just for that set of files.

That said, I do think it would be a good thing to keep structured fields on the
renderer side too. Even if that just means having 2 separate fields: "host" and
"port". This will avoid having to worry about bracketing issues for consumers
that just want to operate on the IP address.

[Brian Ryner, 2011-02-17 00:51:23]

Splitting up the host and port makes sense to me, and we may as well propagate
this all the way through.  I'll submit a WebKit patch to split up the fields,
then post a revised patch here.

On 2011/02/17 00:39:10, eroman wrote:
> > One question about switching to HostPortPair -- do you think we should do
this
> > for the ResourceResponse and ViewHostMsg_FrameNavigate as well, or just for
> the
> > structures under net/ ? 
> 
> Primarily I would like this for the structures under net/*. I'll be happy if
you
> can do it just for that set of files.
> 
> That said, I do think it would be a good thing to keep structured fields on
the
> renderer side too. Even if that just means having 2 separate fields: "host"
and
> "port". This will avoid having to worry about bracketing issues for consumers
> that just want to operate on the IP address.

[pmarks, 2011-02-17 01:01:45]

On 2011/02/17 00:51:23, Brian Ryner wrote:
> Splitting up the host and port makes sense to me, and we may as well propagate
> this all the way through.  I'll submit a WebKit patch to split up the fields,
> then post a revised patch here.

I think the IP address field should be named something like "ip" or "ip_address"
instead of "host", to clarify that it's just an IP, and never a hostname. 
Especially since Chromium "host"s have a tendency to include the square brackets
around IPv6 literals.

[Brian Ryner, 2011-02-17 02:28:09]

I'm currently thinking "remoteIPAddress" and "remotePort" for the WebKit parts,
sound reasonable?

On 2011/02/17 01:01:45, pmarks wrote:
> On 2011/02/17 00:51:23, Brian Ryner wrote:
> > Splitting up the host and port makes sense to me, and we may as well
propagate
> > this all the way through.  I'll submit a WebKit patch to split up the
fields,
> > then post a revised patch here.
> 
> I think the IP address field should be named something like "ip" or
"ip_address"
> instead of "host", to clarify that it's just an IP, and never a hostname. 
> Especially since Chromium "host"s have a tendency to include the square
brackets
> around IPv6 literals.

[pmarks, 2011-02-17 02:33:07]

On 2011/02/17 02:28:09, Brian Ryner wrote:
> I'm currently thinking "remoteIPAddress" and "remotePort" for the WebKit
parts,
> sound reasonable?

SGTM.

My suggestion is to make remotePort a uint16, and use ("", 0) to indicate
"unknown".

[Brian Ryner, 2011-02-17 04:48:23]

https://bugs.webkit.org/show_bug.cgi?id=54607

On 2011/02/17 02:33:07, pmarks wrote:
> On 2011/02/17 02:28:09, Brian Ryner wrote:
> > I'm currently thinking "remoteIPAddress" and "remotePort" for the WebKit
> parts,
> > sound reasonable?
> 
> SGTM.
> 
> My suggestion is to make remotePort a uint16, and use ("", 0) to indicate
> "unknown".

[Brian Ryner, 2011-02-18 02:19:30]

Updated to use HostPortPair (everywhere) to hold the remote IP address and port.
 Please take another look.

http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_network_transac...
File net/ftp/ftp_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6488010/diff/19001/net/ftp/ftp_network_transac...
net/ftp/ftp_network_transaction_unittest.cc:834: EXPECT_EQ("192.0.2.33:0",
transaction_.GetResponseInfo()->socket_address);
On 2011/02/16 21:41:13, eroman wrote:
> Is that port number of zero an artifact of how the mock socket dependencies
> work? (should be 21 otherwise).

Correct.

http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6488010/diff/19001/net/url_request/url_request...
net/url_request/url_request.cc:263: return job_->GetSocketAddress();
On 2011/02/16 21:41:13, eroman wrote:
> Isn't the information in response_info_ too?

For http requests, yes -- see the earlier discussion with Pawel.  To get this
working for ftp, it seemed cleaner to not copy non-http state into that
structure.

[eroman, 2011-02-18 02:48:29]

LGTM, nice test coverage!

http://codereview.chromium.org/6488010/diff/25003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/test/render_view_host_browsertest.cc (right):

http://codereview.chromium.org/6488010/diff/25003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/test/render_view_host_browsertest.cc:232:
RenderViewHostTestTabContentsObserver(TabContents* tab_contents)
nit: mark single-argument ctors as "explicit".

http://codereview.chromium.org/6488010/diff/25003/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6488010/diff/25003/chrome/renderer/render_view...
chrome/renderer/render_view.cc:1635:
params.socket_address.set_host(response.remoteIPAddress().utf8());
I am unfamiliar with this code; i'll try to take a better look later to confirm
it.

http://codereview.chromium.org/6488010/diff/25003/net/base/host_port_pair.h
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/6488010/diff/25003/net/base/host_port_pair.h#n...
net/base/host_port_pair.h:27: static HostPortPair FromAddrInfo(const struct
addrinfo* ai);
You might consider having this operate on the head of a |const AddressList&|
instead, since all the consumers are giving those. (Either approach seems
reasonable be to me, so whatever you prefer).

http://codereview.chromium.org/6488010/diff/25003/net/ftp/ftp_network_transac...
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/6488010/diff/25003/net/ftp/ftp_network_transac...
net/ftp/ftp_network_transaction.cc:625: // Put the peer's ip and port into the
response.
nit:  "ip" --> "IP address"

[eroman, 2011-02-18 02:49:24]

http://codereview.chromium.org/6488010/diff/25003/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6488010/diff/25003/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:235: // Put the peer's ip and port into the
response.
nit: "ip" --> "IP address" (my main nit is regarding capitalization).

[Brian Ryner, 2011-02-18 04:41:58]

http://codereview.chromium.org/6488010/diff/25003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/test/render_view_host_browsertest.cc (right):

http://codereview.chromium.org/6488010/diff/25003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/test/render_view_host_browsertest.cc:232:
RenderViewHostTestTabContentsObserver(TabContents* tab_contents)
On 2011/02/18 02:48:30, eroman wrote:
> nit: mark single-argument ctors as "explicit".

Done.

http://codereview.chromium.org/6488010/diff/25003/net/base/host_port_pair.h
File net/base/host_port_pair.h (right):

http://codereview.chromium.org/6488010/diff/25003/net/base/host_port_pair.h#n...
net/base/host_port_pair.h:27: static HostPortPair FromAddrInfo(const struct
addrinfo* ai);
I think I'll stick with addrinfo, since it's a little more flexible.

On 2011/02/18 02:48:30, eroman wrote:
> You might consider having this operate on the head of a |const AddressList&|
> instead, since all the consumers are giving those. (Either approach seems
> reasonable be to me, so whatever you prefer).

http://codereview.chromium.org/6488010/diff/25003/net/ftp/ftp_network_transac...
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/6488010/diff/25003/net/ftp/ftp_network_transac...
net/ftp/ftp_network_transaction.cc:625: // Put the peer's ip and port into the
response.
On 2011/02/18 02:48:30, eroman wrote:
> nit:  "ip" --> "IP address"

Done.

http://codereview.chromium.org/6488010/diff/25003/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6488010/diff/25003/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:235: // Put the peer's ip and port into the
response.
On 2011/02/18 02:49:24, eroman wrote:
> nit: "ip" --> "IP address" (my main nit is regarding capitalization).

Done.

[Paweł Hajdan Jr., 2011-02-18 12:09:38]

FTP code LGTM, thank you.

[pmarks, 2011-02-23 02:06:53]

Looks like Brian checked this in today:
http://src.chromium.org/viewvc/chrome?view=rev&revision=75620

[wtc, 2011-04-21 22:44:25]

http://codereview.chromium.org/6488010/diff/35001/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/6488010/diff/35001/net/http/http_response_info...
net/http/http_response_info.cc:173: }
We should have bumped the version number when we started to
serialize socket_address.

[Brian Ryner, 2011-04-22 00:44:27]

http://codereview.chromium.org/6488010/diff/35001/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/6488010/diff/35001/net/http/http_response_info...
net/http/http_response_info.cc:173: }
On 2011/04/21 22:44:25, wtc wrote:
> We should have bumped the version number when we started to
> serialize socket_address.

Hm, wouldn't that have caused all existing responses to be discarded (at the
check on line 118)?  The intent was to add this in a backwards-compatible way,
so that the socket address would just not be present for older cache entries.

Is this causing a problem somewhere?

[wtc, 2011-04-22 19:07:13]

No, this is not causing problems.

http://codereview.chromium.org/6488010/diff/35001/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/6488010/diff/35001/net/http/http_response_info...
net/http/http_response_info.cc:173: }
We could change the check on line 118 to allow version 1, and
read socket_address only if version is > 1 here.

Now that I studied your solution again, I think it is a fine
way to avoid bumping the version number because the new field
is serialized at the end.  The only drawbacks I see are:
1) version 1 records may or may not have the socket_address
field.
2) The code pattern for decoding fields changes starting
on line 167 from
  if (!pickle.Foo())
    return false;
to
  if (pickle.Foo()) {
    // decode next field.
  }