Open a firewall hole when a TCP server or UDP socket is bound.

Open a firewall hole when a TCP server or UDP socket is bound.

This patch implements firewall hole punching when a Chrome App uses
the chrome.socket, chrome.sockets.tcpServer or chrome.sockets.udp APIs
to open a socket to receive packets from the network. Sockets that only
listen on the local loopback device do not open a port.

A primitive FirewallHole class is added that could be reused by the
Pepper sockets API as well.

BUG=435404
Committed: https://crrev.com/bed8fdc7a0995427e0af24e63693ad790920b612
Cr-Commit-Position: refs/heads/master@{#320552}

[Reilly Grant (use Gerrit), 2015-02-26 23:32:13]

reillyg@chromium.org changed reviewers:
+ finnur@chromium.org, gauravsh@chromium.org, rpaquay@chromium.org

[Reilly Grant (use Gerrit), 2015-02-27 00:10:44]

gauravsh@, please take a look at the new class in //chromeos/network.

finnur@, please take a look at the updates to the chrome.socket API
implementation.

rpaquay@, please take a look at the updates to the chrome.sockets.tcpServer/udp
API implementations.

[Finnur, 2015-02-27 11:23:52]

https://codereview.chromium.org/965613002/diff/20001/extensions/browser/api/s...
File extensions/browser/api/socket/socket_api.h (right):

https://codereview.chromium.org/965613002/diff/20001/extensions/browser/api/s...
extensions/browser/api/socket/socket_api.h:125: // Only implemented on Chrome
OS.
Is this intended to remain a ChromeOS-only feature?

https://codereview.chromium.org/965613002/diff/20001/extensions/browser/api/s...
extensions/browser/api/socket/socket_api.h:126: void OpenFirewallHole(const
std::string& address,
Also, did this change go through API review?

[Reilly Grant (use Gerrit), 2015-02-27 15:57:24]

https://codereview.chromium.org/965613002/diff/20001/extensions/browser/api/s...
File extensions/browser/api/socket/socket_api.h (right):

https://codereview.chromium.org/965613002/diff/20001/extensions/browser/api/s...
extensions/browser/api/socket/socket_api.h:125: // Only implemented on Chrome
OS.
On 2015/02/27 11:23:52, Finnur wrote:
> Is this intended to remain a ChromeOS-only feature?

Yes, it is unnecessary on other platforms where there are existing methods to
control the system firewall.

https://codereview.chromium.org/965613002/diff/20001/extensions/browser/api/s...
extensions/browser/api/socket/socket_api.h:126: void OpenFirewallHole(const
std::string& address,
On 2015/02/27 11:23:52, Finnur wrote:
> Also, did this change go through API review?

It's been signed off on by security (who did the permission_broker work). See
the document linked from the associated bug.

[Finnur, 2015-02-27 15:58:49]

In that case, LGTM

[Reilly Grant (use Gerrit), 2015-02-27 18:17:29]

CCing jorgelo@ FYI.

[Jorge Lucangeli Obes, 2015-02-27 19:35:50]

On 2015/02/27 15:57:24, reillyg wrote:
>
https://codereview.chromium.org/965613002/diff/20001/extensions/browser/api/s...
> File extensions/browser/api/socket/socket_api.h (right):
> 
>
https://codereview.chromium.org/965613002/diff/20001/extensions/browser/api/s...
> extensions/browser/api/socket/socket_api.h:125: // Only implemented on Chrome
> OS.
> On 2015/02/27 11:23:52, Finnur wrote:
> > Is this intended to remain a ChromeOS-only feature?
> 
> Yes, it is unnecessary on other platforms where there are existing methods to
> control the system firewall.
> 
>
https://codereview.chromium.org/965613002/diff/20001/extensions/browser/api/s...
> extensions/browser/api/socket/socket_api.h:126: void OpenFirewallHole(const
> std::string& address,
> On 2015/02/27 11:23:52, Finnur wrote:
> > Also, did this change go through API review?
> 
> It's been signed off on by security (who did the permission_broker work). See
> the document linked from the associated bug.

It's quite convenient when the security team develops the feature ;-)

[Reilly Grant (use Gerrit), 2015-03-02 18:58:03]

reillyg@chromium.org changed reviewers:
+ pneubeck@chromium.org
- gauravsh@chromium.org

[Reilly Grant (use Gerrit), 2015-03-02 18:58:24]

pneubeck@, please take a look at the new class in //chromeos/network.

[rpaquay, 2015-03-02 19:23:51]

lgtm

[Reilly Grant (use Gerrit), 2015-03-03 20:25:02]

reillyg@chromium.org changed reviewers:
+ armansito@chromium.org
- pneubeck@chromium.org

[Reilly Grant (use Gerrit), 2015-03-03 20:26:04]

armansito@, please take a look at the new class in //chromeos/network.

[pneubeck (no reviews), 2015-03-03 22:24:48]

pneubeck@chromium.org changed reviewers:
+ pneubeck@chromium.org

[pneubeck (no reviews), 2015-03-03 22:24:52]

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
File chromeos/network/firewall_hole.cc (right):

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:41: const char* port_type;
please explicitly initialize to null, or probably better move this to a helper
PortTypeToString that you can call directly in the LOG(...) <<.

Currently this code is executed even if success==true

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:84: if (client) {
the 'return' below is a bit hidden.
would be clearer with
  DCHECK(client) << ...;
  ...
  return;

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:116: opened_(false),
optional:
consider using the new c++11 "Non-Static Class Member Initializers" instead in
the header, see http://chromium-cpp.appspot.com/.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:124:
DBusThreadManager::Get()->GetPermissionBrokerClient();
please don't rely on the singleton and pass in the client explicitly.
That way you can mock the client in a unit test and you don't have to check
!client in here.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:136:
base::Bind(&FirewallHole::OnPortAccessGranted, base::Unretained(this),
are you assuming here that |this| is outliving |client| ?
that seems not to be the case looking at the NOTREACHEDs in line 126 and 99.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:157: delete this;
delete this is... dangerous.

E.g. if client->RequestUdpPortAccess calls its callback synchronously (which
isn't unrealistic in certain error cases), you have an invalid this pointer on
the call stack after this pointer.
You would have to document that at several places.

Better not rely on a |delete this| at all.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
File chromeos/network/firewall_hole.h (right):

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:10: #include "base/basictypes.h"
what for? please see the comment in that header regarding uint16* .

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:11: #include "base/callback.h"
is callback_forward.h sufficient?

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:25: enum PortType {
I think 'enum class' is now the preferred kind as it provides correct
scoping/name qualification.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:32: static void Open(PortType type,
This interface implies that the newly created object is self-owned (or say
unonwed) until it is passed to |callback|, which forces the caller to use a
WeakPtr.

Can you instead synchronously pass ownership and instead hide a weakptr
internally?

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:34: const std::string& interface,
needs a comment what |interface| is / what format it expects / what values are
valid.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:43: dbus::FileDescriptor* lifeline_fd);
this takes ownership of lifeline_fd, which should be documented. ideally using
scoped_ptr and not a comment.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:45: void
OnLifelineCreated(dbus::FileDescriptor* lifeline_remote,
what's lifeline? that's unclear by looking only at this declaration.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:58: dbus::FileDescriptor* lifeline_fd_;
should document that this is owning the filedescriptor.

[Reilly Grant (use Gerrit), 2015-03-04 01:35:50]

I've rewritten FirewallHole to avoid the use of |delete this|.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
File chromeos/network/firewall_hole.cc (right):

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:41: const char* port_type;
On 2015/03/03 22:24:51, pneubeck wrote:
> please explicitly initialize to null, or probably better move this to a helper
> PortTypeToString that you can call directly in the LOG(...) <<.
> 
> Currently this code is executed even if success==true

Done.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:116: opened_(false),
On 2015/03/03 22:24:51, pneubeck wrote:
> optional:
> consider using the new c++11 "Non-Static Class Member Initializers" instead in
> the header, see http://chromium-cpp.appspot.com/.

Done.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:124:
DBusThreadManager::Get()->GetPermissionBrokerClient();
On 2015/03/03 22:24:51, pneubeck wrote:
> please don't rely on the singleton and pass in the client explicitly.
> That way you can mock the client in a unit test and you don't have to check
> !client in here.

Done.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:157: delete this;
On 2015/03/03 22:24:50, pneubeck wrote:
> delete this is... dangerous.
> 
> E.g. if client->RequestUdpPortAccess calls its callback synchronously (which
> isn't unrealistic in certain error cases), you have an invalid this pointer on
> the call stack after this pointer.
> You would have to document that at several places.
> 
> Better not rely on a |delete this| at all.

Done.

[Reilly Grant (use Gerrit), 2015-03-06 18:33:58]

Ping?

[armansito, 2015-03-06 22:44:20]

On 2015/03/06 18:33:58, reillyg wrote:
> Ping?

I leave it up to pneubeck@ since he has already been reviewing.

[pneubeck (no reviews), 2015-03-07 02:02:37]

Please reply to my other comments whether you addressed them or why you think
otherwise.

[Reilly Grant (use Gerrit), 2015-03-07 02:36:03]

Sorry, I lost track of your comments on firewall_hole.h. This should resolve
them.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
File chromeos/network/firewall_hole.cc (right):

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:84: if (client) {
On 2015/03/03 22:24:51, pneubeck wrote:
> the 'return' below is a bit hidden.
> would be clearer with
>   DCHECK(client) << ...;
>   ...
>   return;

Done.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:136:
base::Bind(&FirewallHole::OnPortAccessGranted, base::Unretained(this),
On 2015/03/03 22:24:51, pneubeck wrote:
> are you assuming here that |this| is outliving |client| ?
> that seems not to be the case looking at the NOTREACHEDs in line 126 and 99.

I believe this is addressed in the latest patchset.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
File chromeos/network/firewall_hole.h (right):

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:10: #include "base/basictypes.h"
On 2015/03/03 22:24:51, pneubeck wrote:
> what for? please see the comment in that header regarding uint16* .

Done.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:11: #include "base/callback.h"
On 2015/03/03 22:24:51, pneubeck wrote:
> is callback_forward.h sufficient?

Done.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:25: enum PortType {
On 2015/03/03 22:24:52, pneubeck wrote:
> I think 'enum class' is now the preferred kind as it provides correct
> scoping/name qualification.

Done.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:32: static void Open(PortType type,
On 2015/03/03 22:24:51, pneubeck wrote:
> This interface implies that the newly created object is self-owned (or say
> unonwed) until it is passed to |callback|, which forces the caller to use a
> WeakPtr.
> 
> Can you instead synchronously pass ownership and instead hide a weakptr
> internally?

I don't understand your objection. There are no weak pointers necessary in this
implementation.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:34: const std::string& interface,
On 2015/03/03 22:24:51, pneubeck wrote:
> needs a comment what |interface| is / what format it expects / what values are
> valid.

Done.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:43: dbus::FileDescriptor* lifeline_fd);
On 2015/03/03 22:24:51, pneubeck wrote:
> this takes ownership of lifeline_fd, which should be documented. ideally using
> scoped_ptr and not a comment.

I believe this is addressed in the latest patchset.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:45: void
OnLifelineCreated(dbus::FileDescriptor* lifeline_remote,
On 2015/03/03 22:24:51, pneubeck wrote:
> what's lifeline? that's unclear by looking only at this declaration.

I believe this is addressed in the latest patchset.

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:58: dbus::FileDescriptor* lifeline_fd_;
On 2015/03/03 22:24:51, pneubeck wrote:
> should document that this is owning the filedescriptor.

I believe this is addressed in the latest patchset.

[pneubeck (no reviews), 2015-03-08 03:35:01]

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
File chromeos/network/firewall_hole.cc (right):

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:124:
DBusThreadManager::Get()->GetPermissionBrokerClient();
On 2015/03/04 01:35:50, reillyg wrote:
> On 2015/03/03 22:24:51, pneubeck wrote:
> > please don't rely on the singleton and pass in the client explicitly.
> > That way you can mock the client in a unit test and you don't have to check
> > !client in here.
> 
> Done.

Hm. Not really done but I also see the issue with the lifetime if you pass the
client explicitly and that you need some global instance that tells you whether
the client is still existent or not.

Please add at least a unit test that exercises the positive/successful flow of
creating the firewall hole.
You'll probably see then what the issues with DBusThreadManager are. (You'll
end-up initializing all Stub DBusClients, e.g. Bluetooth)

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
File chromeos/network/firewall_hole.h (right):

https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
chromeos/network/firewall_hole.h:32: static void Open(PortType type,
On 2015/03/07 02:36:03, reillyg wrote:
> On 2015/03/03 22:24:51, pneubeck wrote:
> > This interface implies that the newly created object is self-owned (or say
> > unonwed) until it is passed to |callback|, which forces the caller to use a
> > WeakPtr.
> > 
> > Can you instead synchronously pass ownership and instead hide a weakptr
> > internally?
> 
> I don't understand your objection.
Don't get me wrong. I just want to make sure we're on the same line
understanding what the interface of this class is and that we agree that it is
what is reasonable.

> There are no weak pointers necessary in this implementation.
With the current Open(...,callback) function, |callback| is called at anytime in
the future and the caller can't prevent it.
So the caller has to guarantee that |callback| is valid until it's called, but
doesn't know when. The only way to do that is using a WeakPtr or binding
ownership to |callback|, e.g. with a ref counted ptr.

In your case it happens that ExtensionFunction is already refcounted.

An alternative interface that doesn't force the caller to an indefinite lifetime
and instead uses explicit ownership could have been:

  // |callback| is not called after the returned object is destructed.
  static scoped_ptr<FirewallHole> Open(..., callback);
  bool IsOpen();

Please think about it. I know that you changed your code already because of
another comment. I should have stated my point about the WeakPtr a bit clearer
to prevent this.

https://codereview.chromium.org/965613002/diff/80001/chromeos/network/firewal...
File chromeos/network/firewall_hole.cc (right):

https://codereview.chromium.org/965613002/diff/80001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:46: default:
could you just add a 'return' after the switch instead of using 'default'?
Without default you get a nice compile time error if a case is missing. With
default+NOTREACHED you only get a runtime error if you ever execute the missing
case.

https://codereview.chromium.org/965613002/diff/80001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:99: PermissionBrokerClient* client =
will you be even able to test this code path in browser/unit_tests executed on
linux?
Looking at the implementation of SysInfo::IsRunningOnChromeOS, it is uncommon to
fake the OS version in tests:
https://code.google.com/p/chromium/codesearch#search/&q=SysInfo::SetChromeOSV...

Is there a reason why you have to make this conditional depending on whether it
runs on Linux or ChromeOS ?
I'd leave that conditional IsRunningOnChromeOS() out.

https://codereview.chromium.org/965613002/diff/80001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:135: if (base::SysInfo::IsRunningOnChromeOS())
{
same comment as above.

[Reilly Grant (use Gerrit), 2015-03-09 19:56:37]

On 2015/03/08 03:35:01, pneubeck wrote:
>
https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
> File chromeos/network/firewall_hole.h (right):
> 
>
https://codereview.chromium.org/965613002/diff/20001/chromeos/network/firewal...
> chromeos/network/firewall_hole.h:32: static void Open(PortType type,
> On 2015/03/07 02:36:03, reillyg wrote:
> > On 2015/03/03 22:24:51, pneubeck wrote:
> > > This interface implies that the newly created object is self-owned (or say
> > > unonwed) until it is passed to |callback|, which forces the caller to use
a
> > > WeakPtr.
> > > 
> > > Can you instead synchronously pass ownership and instead hide a weakptr
> > > internally?
> > 
> > I don't understand your objection.
> Don't get me wrong. I just want to make sure we're on the same line
> understanding what the interface of this class is and that we agree that it is
> what is reasonable.
> 
> > There are no weak pointers necessary in this implementation.
> With the current Open(...,callback) function, |callback| is called at anytime
in
> the future and the caller can't prevent it.
> So the caller has to guarantee that |callback| is valid until it's called, but
> doesn't know when. The only way to do that is using a WeakPtr or binding
> ownership to |callback|, e.g. with a ref counted ptr.
> 
> In your case it happens that ExtensionFunction is already refcounted.
> 
> An alternative interface that doesn't force the caller to an indefinite
lifetime
> and instead uses explicit ownership could have been:
> 
>   // |callback| is not called after the returned object is destructed.
>   static scoped_ptr<FirewallHole> Open(..., callback);
>   bool IsOpen();
> 
> Please think about it. I know that you changed your code already because of
> another comment. I should have stated my point about the WeakPtr a bit clearer
> to prevent this.

I haven't done a callback API like this and I'm not sure how I feel about it.
I've generally considered it the caller's responsibility to make sure that their
callback is safe to call anytime, either with weak references or reference
counting. I can see how a "free this and the callback won't be called" interface
is attractive. I am a bit concerned that making this safe will complicate the
implementation of FirewallHole more than forcing callers to use a WeakPtr will.
I'll work on the unit tests you asked for and think about it.

[Reilly Grant (use Gerrit), 2015-03-09 19:57:06]

https://codereview.chromium.org/965613002/diff/80001/chromeos/network/firewal...
File chromeos/network/firewall_hole.cc (right):

https://codereview.chromium.org/965613002/diff/80001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:46: default:
On 2015/03/08 03:35:01, pneubeck wrote:
> could you just add a 'return' after the switch instead of using 'default'?
> Without default you get a nice compile time error if a case is missing. With
> default+NOTREACHED you only get a runtime error if you ever execute the
missing
> case.

Done.

https://codereview.chromium.org/965613002/diff/80001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:99: PermissionBrokerClient* client =
On 2015/03/08 03:35:01, pneubeck wrote:
> will you be even able to test this code path in browser/unit_tests executed on
> linux?
> Looking at the implementation of SysInfo::IsRunningOnChromeOS, it is uncommon
to
> fake the OS version in tests:
>
https://code.google.com/p/chromium/codesearch#search/&q=SysInfo::SetChromeOSV...
> 
> Is there a reason why you have to make this conditional depending on whether
it
> runs on Linux or ChromeOS ?
> I'd leave that conditional IsRunningOnChromeOS() out.

The goal is to make it so that Chromium built for Chromium OS, running on Linux
(which is my development environment), will execute as much of this code as
possible, without actually requiring the permission broker to be available over
DBus.

[pneubeck (no reviews), 2015-03-09 20:03:47]

https://codereview.chromium.org/965613002/diff/80001/chromeos/network/firewal...
File chromeos/network/firewall_hole.cc (right):

https://codereview.chromium.org/965613002/diff/80001/chromeos/network/firewal...
chromeos/network/firewall_hole.cc:99: PermissionBrokerClient* client =
On 2015/03/09 19:57:06, reillyg wrote:
> On 2015/03/08 03:35:01, pneubeck wrote:
> > will you be even able to test this code path in browser/unit_tests executed
on
> > linux?
> > Looking at the implementation of SysInfo::IsRunningOnChromeOS, it is
uncommon
> to
> > fake the OS version in tests:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=SysInfo::SetChromeOSV...
> > 
> > Is there a reason why you have to make this conditional depending on whether
> it
> > runs on Linux or ChromeOS ?
> > I'd leave that conditional IsRunningOnChromeOS() out.
> 
> The goal is to make it so that Chromium built for Chromium OS, running on
Linux
> (which is my development environment), will execute as much of this code as
> possible, without actually requiring the permission broker to be available
over
> DBus.

But you're also disabling this code paths from test where you have to test it.

See here, how other clients are replaced by stub implementations if you run
Chrome build for ChromeOS on Linux:

https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/dbus/dbus...

[Reilly Grant (use Gerrit), 2015-03-09 23:32:47]

I've added unit tests that use a MockPermissionBrokerClient to exercise the
success and failure cases. I also removed the sysinfo checks by simply making
FakePermssionBrokerClient always succeed, so that we don't have to bypass it
when not on Chromium OS.

I decided not to change the open interface because doing it as you suggest ends
up requiring some very awkward cleanup code for the case where the class is
destroyed while waiting for the permission broker as there is then no object
around to keep track of the fact that we have to turn around and tell the
permission broker to release the port again.

[pneubeck (no reviews), 2015-03-10 08:49:22]

chromeos/ lgtm with a few nits

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
File chromeos/network/firewall_hole.cc (right):

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole.cc:110: NOTREACHED();
this is equivalent to the DCHECK above.
and DCHECKs shouldn't be handled (according to the chromium styleguide), so just
remove lines 99, 110 and 111.

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole.cc:132: if (!client) {
same as above. DCHECKs should not be handled.
Please remove lines 132-135.

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole.cc:149: callback.Run(nullptr);
NOTREACHED = DCHECK should not be handled.
Remove line 149

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
File chromeos/network/firewall_hole_unittest.cc (right):

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole_unittest.cc:24: class MockPermissionsBrokerClient
: public chromeos::PermissionBrokerClient {
this and the template above should be wrapped in an anonymous namespace

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole_unittest.cc:82: MockPermissionsBrokerClient*
mock_permissions_broker_client_;
= null;

https://codereview.chromium.org/965613002/diff/100001/extensions/browser/api/...
File extensions/browser/api/socket/socket_api.cc (right):

https://codereview.chromium.org/965613002/diff/100001/extensions/browser/api/...
extensions/browser/api/socket/socket_api.cc:126: &chromeos::FirewallHole::Open,
port_type, local_address.port(), "",
please add a comment /* all interfaces */ after ""

[Reilly Grant (use Gerrit), 2015-03-10 20:14:16]

reillyg@chromium.org changed reviewers:
+ stevenjb@chromium.org
- armansito@chromium.org

[Reilly Grant (use Gerrit), 2015-03-10 20:15:17]

Adding stevenjb@ as I have modified
chromeos/dbus/fake_permission_broker_client.cc so that requests succeed,
allowing me to avoid the sysinfo checks in the original implementation.

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
File chromeos/network/firewall_hole.cc (right):

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole.cc:110: NOTREACHED();
On 2015/03/10 08:49:21, pneubeck wrote:
> this is equivalent to the DCHECK above.
> and DCHECKs shouldn't be handled (according to the chromium styleguide), so
just
> remove lines 99, 110 and 111.

Done.

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole.cc:132: if (!client) {
On 2015/03/10 08:49:21, pneubeck wrote:
> same as above. DCHECKs should not be handled.
> Please remove lines 132-135.

Done.

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole.cc:149: callback.Run(nullptr);
On 2015/03/10 08:49:21, pneubeck wrote:
> NOTREACHED = DCHECK should not be handled.
> Remove line 149

Done.

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
File chromeos/network/firewall_hole_unittest.cc (right):

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole_unittest.cc:24: class MockPermissionsBrokerClient
: public chromeos::PermissionBrokerClient {
On 2015/03/10 08:49:21, pneubeck wrote:
> this and the template above should be wrapped in an anonymous namespace

Done.

https://codereview.chromium.org/965613002/diff/100001/chromeos/network/firewa...
chromeos/network/firewall_hole_unittest.cc:82: MockPermissionsBrokerClient*
mock_permissions_broker_client_;
On 2015/03/10 08:49:21, pneubeck wrote:
> = null;

Done.

https://codereview.chromium.org/965613002/diff/100001/extensions/browser/api/...
File extensions/browser/api/socket/socket_api.cc (right):

https://codereview.chromium.org/965613002/diff/100001/extensions/browser/api/...
extensions/browser/api/socket/socket_api.cc:126: &chromeos::FirewallHole::Open,
port_type, local_address.port(), "",
On 2015/03/10 08:49:22, pneubeck wrote:
> please add a comment /* all interfaces */ after ""

Done.

[Reilly Grant (use Gerrit), 2015-03-11 20:52:42]

reillyg@chromium.org changed reviewers:
+ jwd@chromium.org

[Reilly Grant (use Gerrit), 2015-03-11 20:52:42]

Add jwd@ because I've added the flag to histograms.xml.

[jwd, 2015-03-12 17:12:20]

lgtm

[Reilly Grant (use Gerrit), 2015-03-13 18:13:25]

stevenjb@, ping?

[stevenjb, 2015-03-13 19:02:31]

chromeos/ owner rubber stamp lgtm

[Reilly Grant (use Gerrit), 2015-03-13 19:31:28]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2015-03-13 19:31:28]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
rpaquay@chromium.org, pneubeck@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/965613002/#ps200001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-03-13 19:31:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/965613002/200001

[commit-bot: I haz the power, 2015-03-13 19:42:04]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-13 19:43:49]

Patchset 11 (id:??) landed as
https://crrev.com/bed8fdc7a0995427e0af24e63693ad790920b612
Cr-Commit-Position: refs/heads/master@{#320552}