Bind open firewall ports to visible application windows.

Bind open firewall ports to visible application windows.

To prevent an application from continuing to listen for TCP connections
or incoming UDP packets when the user is not aware that it is still
running this patch observes the state of its associated AppWindows and
opens and closes ports as necessary. The socket itself is left open so
that state is not lost.

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

[Reilly Grant (use Gerrit), 2015-03-18 23:24:09]

reillyg@chromium.org changed reviewers:
+ jorgelo@chromium.org, scheib@chromium.org

[Reilly Grant (use Gerrit), 2015-03-18 23:24:55]

reillyg@chromium.org changed reviewers:
+ rpaquay@chromium.org
- scheib@chromium.org

[Reilly Grant (use Gerrit), 2015-03-18 23:26:45]

rpaquay@, please take a look.
jorgelo@, please give security sign-off.

[Jorge Lucangeli Obes, 2015-03-20 17:34:14]

On 2015/03/18 23:26:45, reillyg wrote:
> rpaquay@, please take a look.
> jorgelo@, please give security sign-off.

Security lgtm.

[Reilly Grant (use Gerrit), 2015-03-23 16:12:44]

On 2015/03/18 23:26:45, reillyg wrote:
> rpaquay@, please take a look.

Ping?

[rpaquay, 2015-03-23 18:29:07]

Sorry about the delay, the change is good i think, I just had a couple of
question/nit about raw pointer usages.

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
File extensions/browser/api/socket/app_firewall_hole_manager.cc (right):

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
extensions/browser/api/socket/app_firewall_hole_manager.cc:71:
AppFirewallHole::~AppFirewallHole() {
nit: Move destructor after constructor?

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
File extensions/browser/api/socket/app_firewall_hole_manager.h (right):

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
extensions/browser/api/socket/app_firewall_hole_manager.h:51:
AppFirewallHoleManager* manager_;
nit: comment why it is ok to have raw ptr?

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
extensions/browser/api/socket/app_firewall_hole_manager.h:73: AppFirewallHole*
Open(AppFirewallHole::PortType type,
Why don't we return a scoped_ptr<AppFirewallHole,
BrowserThread::DeleteOnUIThread> here instead of a raw pointer?

From what I understand of the code, the ownership is an implementation detail,
as AppFirewallHole unregister itselfs when the desctructor is called. So, from a
consumer perspective, AppFirewallHoleManager is more a factory than a "manager":
you call into a method to get an instance (as scoped_ptr) that you are
responsible to release when you are done.

(Unless I am missing something of course :))

[Reilly Grant (use Gerrit), 2015-03-23 20:09:31]

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
File extensions/browser/api/socket/app_firewall_hole_manager.cc (right):

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
extensions/browser/api/socket/app_firewall_hole_manager.cc:71:
AppFirewallHole::~AppFirewallHole() {
On 2015/03/23 18:29:07, rpaquay wrote:
> nit: Move destructor after constructor?

This matches the order in the header file.

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
File extensions/browser/api/socket/app_firewall_hole_manager.h (right):

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
extensions/browser/api/socket/app_firewall_hole_manager.h:51:
AppFirewallHoleManager* manager_;
On 2015/03/23 18:29:07, rpaquay wrote:
> nit: comment why it is ok to have raw ptr?

Done.

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
extensions/browser/api/socket/app_firewall_hole_manager.h:51:
AppFirewallHoleManager* manager_;
On 2015/03/23 18:29:07, rpaquay wrote:
> nit: comment why it is ok to have raw ptr?

Done.

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
extensions/browser/api/socket/app_firewall_hole_manager.h:73: AppFirewallHole*
Open(AppFirewallHole::PortType type,
On 2015/03/23 18:29:07, rpaquay wrote:
> Why don't we return a scoped_ptr<AppFirewallHole,
> BrowserThread::DeleteOnUIThread> here instead of a raw pointer?

I've switched it to a regular scoped_ptr. I want to keep the BrowserThread
dependency out of this class. It's easy to do the conversion in socket_api.cc.

> From what I understand of the code, the ownership is an implementation detail,
> as AppFirewallHole unregister itselfs when the desctructor is called. So, from
a
> consumer perspective, AppFirewallHoleManager is more a factory than a
"manager":
> you call into a method to get an instance (as scoped_ptr) that you are
> responsible to release when you are done.
> 
> (Unless I am missing something of course :))

While in general I agree with this comment. The class does straddle the boundary
between manager and factory there are two reasons why I think manager is more
appropriate:

1) It is the owner of all AppFirewallHole objects, while factories typically
just construct them.
2) I refuse the name a class AppFirewallHoleFactoryFactory.

[rpaquay, 2015-03-23 21:04:57]

lgtm

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
File extensions/browser/api/socket/app_firewall_hole_manager.h (right):

https://codereview.chromium.org/1022663003/diff/20001/extensions/browser/api/...
extensions/browser/api/socket/app_firewall_hole_manager.h:73: AppFirewallHole*
Open(AppFirewallHole::PortType type,
> While in general I agree with this comment. The class does straddle the
boundary
> between manager and factory there are two reasons why I think manager is more
> appropriate:
> 
> 1) It is the owner of all AppFirewallHole objects, while factories typically
> just construct them.
> 2) I refuse the name a class AppFirewallHoleFactoryFactory.

Yes, yes, agreed, I didn't mean to imply you should change the name, I was just
continuing my train of thought wrt to using scoped_ptr for return value.

[Reilly Grant (use Gerrit), 2015-03-23 23:10:38]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2015-03-23 23:10:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
jorgelo@chromium.org, rpaquay@chromium.org
Link to the patchset: https://codereview.chromium.org/1022663003/#ps60001
(title: "Actually fix the GN build.")

[commit-bot: I haz the power, 2015-03-23 23:11:03]

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

[commit-bot: I haz the power, 2015-03-24 00:32:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-24 00:32:35]

Patchset 4 (id:??) landed as
https://crrev.com/8d0ba1dfa407f78253c4c676b6e9f81baea2d69f
Cr-Commit-Position: refs/heads/master@{#321911}