Add the ability to use AllowAddressReuse for UDP sockets.

Add the ability to use AllowAddressReuse for UDP sockets.

Currently, UDP sockets can only bind to unused ports. However, sockets that
are joined to multicast groups may want to bind to ports that are already in use
(for example, a v2 app that uses the same multicast protocol as a system
daemon). For this, I added the AllowAddressReuse API, similar to the one in
the underlying UDPSocket object. This must be called before binding/connecting,
and will return an error otherwise. It returns an error for TCP sockets
explaining that the call is not necessary.

BUG=238819

[Noam Samuel, 2013-05-07 21:50:23]

Hey Mike, Matt,

Could you please review this CL?

Thanks,
Noam.

[miket_OOO, 2013-05-24 20:53:48]

LGTM and thanks!

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:48: " address reuse by
default.";
I think you accidentally a word.

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.h (right):

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.h:456: };
add vertical whitespace (this was an error before your change)

https://codereview.chromium.org/15039012/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/socket.idl (right):

https://codereview.chromium.org/15039012/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/socket.idl:333: // Allow a UDP socket to bind to an
already-bound address. Note that binding
I know you were just following convention, but we are inconsistent with the
voice we use in the documentation in this file. I tend to prefer Javadoc voice
("allows a UDP socket" vs. "allow a UDP socket"). If you have a moment would you
mind making it consistent throughout the file? Either way is fine. If you're in
a hurry, OK to ignore.

[mmenke, 2013-05-24 21:11:59]

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:895: : params_(NULL) {}
nit:  Should be 4-space indent.

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:898:
~SocketAllowAddressReuseFunction() {}
nit:  This should be 4-space indent or 0-space (Both are used in Chrome, I think
4 may be more common, but both are used)

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:911:
SetResult(Value::CreateIntegerValue(result));
I know this is done elsewhere, but this is really weird.  We set the result to
be a network error code in "SetResult(Value::CreateIntegerValue(result))", but
here we set it to -1.  -1 actually the network error code for ERR_IO_PENDING,
which doesn't seem quite right.

This probably isn't the CL to fix this, but...Ick.

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/udp_socket.cc (right):

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/udp_socket.cc:251: return net::OK;
I think "return 0;" is actually more consistent with the rest of these two
files, particularly since the caller compares the result directly with 0, not
net::OK.

Ideally, I think a lot of the 0 comparisons should be checking against net::OK,
but changing that is beyond the scoope of this CL.

[Noam Samuel, 2013-05-24 21:43:53]

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:48: " address reuse by
default.";
On 2013/05/24 20:53:48, miket wrote:
> I think you accidentally a word.

Fixed.

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:895: : params_(NULL) {}
On 2013/05/24 21:12:00, mmenke wrote:
> nit:  Should be 4-space indent.

Done.

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:898:
~SocketAllowAddressReuseFunction() {}
Looks like it fits on one line. Not sure why I formatted it like that.

On 2013/05/24 21:12:00, mmenke wrote:
> nit:  This should be 4-space indent or 0-space (Both are used in Chrome, I
think
> 4 may be more common, but both are used)

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.h (right):

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.h:456: };
On 2013/05/24 20:53:48, miket wrote:
> add vertical whitespace (this was an error before your change)

Done.

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/udp_socket.cc (right):

https://codereview.chromium.org/15039012/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/udp_socket.cc:251: return net::OK;
On 2013/05/24 21:12:00, mmenke wrote:
> I think "return 0;" is actually more consistent with the rest of these two
> files, particularly since the caller compares the result directly with 0, not
> net::OK.
> 
> Ideally, I think a lot of the 0 comparisons should be checking against
net::OK,
> but changing that is beyond the scoope of this CL.

Done.

https://codereview.chromium.org/15039012/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/socket.idl (right):

https://codereview.chromium.org/15039012/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/socket.idl:333: // Allow a UDP socket to bind to an
already-bound address. Note that binding
On 2013/05/24 20:53:48, miket wrote:
> I know you were just following convention, but we are inconsistent with the
> voice we use in the documentation in this file. I tend to prefer Javadoc voice
> ("allows a UDP socket" vs. "allow a UDP socket"). If you have a moment would
you
> mind making it consistent throughout the file? Either way is fine. If you're
in
> a hurry, OK to ignore.

Done.

[mmenke, 2013-05-28 15:47:25]

LGTM

[commit-bot: I haz the power, 2013-05-28 17:43:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/15039012/24001

[commit-bot: I haz the power, 2013-05-28 17:43:08]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 530.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
44b709f0766fa633a1ec5c4c1a8675a6d7b9d982..02bd8a3c95a575276ce7626a5c87c16da19782b4
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -530,6 +530,7 @@ enum HistogramValue {
   WEBVIEW_INSERTCSS,
   METRICSPRIVATE_GETISCRASHRECORDINGENABLED,
   IDENTITYPRIVATE_GETRESOURCES,
+  SOCKET_ALLOW_ADDRESS_REUSE,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-05-29 21:44:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/15039012/31001

[commit-bot: I haz the power, 2013-05-30 00:40:40]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Noam Samuel, 2013-07-23 16:48:12]

On 2013/05/30 00:40:40, I haz the power (commit-bot) wrote:
> Retried try job too often on mac_rel for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

For posterity and/or later return: The reason this patch didn't come through is
that SO_REUSEADDR doesn't work in quite the same way on Mac OSX, where you're
supposed to use SO_REUSEPORT which has subtly different semantics. Given the
subtlety of it, I'm putting it off until I'm a bit less busy with Privet stuff.
When I do it, I'll submit it as a separate patch for a new review, so I'm
closing this issue.

[zbrusso, 2015-06-10 21:32:13]

On 2013/07/23 16:48:12, Noam Samuel wrote:
> On 2013/05/30 00:40:40, I haz the power (commit-bot) wrote:
> > Retried try job too often on mac_rel for step(s) browser_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
> 
> For posterity and/or later return: The reason this patch didn't come through
is
> that SO_REUSEADDR doesn't work in quite the same way on Mac OSX, where you're
> supposed to use SO_REUSEPORT which has subtly different semantics. Given the
> subtlety of it, I'm putting it off until I'm a bit less busy with Privet
stuff.
> When I do it, I'll submit it as a separate patch for a new review, so I'm
> closing this issue.

have you ever finished?