Explicitly whitelist the test server port.

Explicitly whitelist the test server port.

BUG=65859
TEST=yes please

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

[Bernhard Bauer, 2010-12-08 12:00:47]

Please review.

[akalin, 2010-12-08 15:02:13]

On 2010/12/08 12:00:47, Bernhard Bauer wrote:
> Please review.

If the test server is started with --xmpp, there's an extra port that is opened.
 Can you add similar code to live_sync_test.cc around like 337 etc.?

[akalin, 2010-12-08 15:02:34]

http://codereview.chromium.org/5519015/diff/1/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/5519015/diff/1/net/test/test_server.cc#newcode161
net/test/test_server.cc:161: 
Add a TODO(akalin) to add code to store all ports opened by the testserver and
to add them, so that we don't have to have code in other places (e.g.,
live_sync_test.cc) to do that.

[cbentzel, 2010-12-08 15:05:27]

driveby:

Should you remove in the TestServer destructor as well as Stop?

Also - this probably isn't an issue given that each testserver instance
should be listening on a unique port, but using a set rather than a map of
port->count could cause invalidation of a port that should still be
whitelisted.

On Wed, Dec 8, 2010 at 10:02 AM, <akalin@chromium.org> wrote:

> On 2010/12/08 12:00:47, Bernhard Bauer wrote:
>
>> Please review.
>>
>
> If the test server is started with --xmpp, there's an extra port that is
> opened.
>  Can you add similar code to live_sync_test.cc around like 337 etc.?
>
>
> http://codereview.chromium.org/5519015/
>

[Bernhard Bauer, 2010-12-09 10:48:08]

On 2010/12/08 15:05:27, cbentzel wrote:
> driveby:
> 
> Should you remove in the TestServer destructor as well as Stop?

I added a helper class with a scoped_ptr, so it should get cleaned up.

> Also - this probably isn't an issue given that each testserver instance
> should be listening on a unique port, but using a set rather than a map of
> port->count could cause invalidation of a port that should still be
> whitelisted.

std::multiset FTW, as the kids say these days.

> On Wed, Dec 8, 2010 at 10:02 AM, <mailto:akalin@chromium.org> wrote:
> 
> > On 2010/12/08 12:00:47, Bernhard Bauer wrote:
> >
> >> Please review.
> >>
> >
> > If the test server is started with --xmpp, there's an extra port that is
> > opened.
> >  Can you add similar code to live_sync_test.cc around like 337 etc.?

done.

> >
> >
> > http://codereview.chromium.org/5519015/
> >

http://codereview.chromium.org/5519015/diff/1/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/5519015/diff/1/net/test/test_server.cc#newcode161
net/test/test_server.cc:161: 
On 2010/12/08 15:02:34, akalin wrote:
> Add a TODO(akalin) to add code to store all ports opened by the testserver and
> to add them, so that we don't have to have code in other places (e.g.,
> live_sync_test.cc) to do that.

Done.

[cbentzel, 2010-12-09 14:25:12]

LGTM

Thanks for fixing this!

http://codereview.chromium.org/5519015/diff/7001/net/base/net_util.h
File net/base/net_util.h (right):

http://codereview.chromium.org/5519015/diff/7001/net/base/net_util.h#newcode341
net/base/net_util.h:341: class ScopedPortException {
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/5519015/diff/7001/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/5519015/diff/7001/net/test/test_server.cc#newc...
net/test/test_server.cc:162: // TODO(akalin): Automaticall whitelist all ports
openened by the test server.
This TODO isn't needed anymore.

[cbentzel, 2010-12-09 14:26:39]

Oh - and please fix the live_sync_test.cc compile issues before landing.

[Bernhard Bauer, 2010-12-09 14:38:41]

All done. Running tryjobs now.

On Thu, Dec 9, 2010 at 15:26,  <cbentzel@chromium.org> wrote:
> Oh - and please fix the live_sync_test.cc compile issues before landing.
>
> http://codereview.chromium.org/5519015/
>

[akalin, 2010-12-09 19:10:31]

LGTM

http://codereview.chromium.org/5519015/diff/18001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sync_test.h (right):

http://codereview.chromium.org/5519015/diff/18001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_test.h:177: // Helper class to whitelist the
notificition port.
notificition -> notification

http://codereview.chromium.org/5519015/diff/18001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/5519015/diff/18001/net/base/net_util.cc#newcod...
net/base/net_util.cc:1757: explicitly_allowed_ports.erase(it);
suggest LOG(DFATAL) if it == end()

[Bernhard Bauer, 2010-12-10 14:47:04]

http://codereview.chromium.org/5519015/diff/18001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sync_test.h (right):

http://codereview.chromium.org/5519015/diff/18001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_test.h:177: // Helper class to whitelist the
notificition port.
On 2010/12/09 19:10:31, akalin wrote:
> notificition -> notification

Done.

http://codereview.chromium.org/5519015/diff/18001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/5519015/diff/18001/net/base/net_util.cc#newcod...
net/base/net_util.cc:1757: explicitly_allowed_ports.erase(it);
On 2010/12/09 19:10:31, akalin wrote:
> suggest LOG(DFATAL) if it == end()

I used NOTREACHED() if that's okay. We don't really get anything from logging in
release mode.

[akalin, 2010-12-10 19:19:17]

LGTM

On 2010/12/10 14:47:04, Bernhard Bauer wrote:
>
http://codereview.chromium.org/5519015/diff/18001/chrome/test/live_sync/live_...
> File chrome/test/live_sync/live_sync_test.h (right):
> 
>
http://codereview.chromium.org/5519015/diff/18001/chrome/test/live_sync/live_...
> chrome/test/live_sync/live_sync_test.h:177: // Helper class to whitelist the
> notificition port.
> On 2010/12/09 19:10:31, akalin wrote:
> > notificition -> notification
> 
> Done.
> 
> http://codereview.chromium.org/5519015/diff/18001/net/base/net_util.cc
> File net/base/net_util.cc (right):
> 
>
http://codereview.chromium.org/5519015/diff/18001/net/base/net_util.cc#newcod...
> net/base/net_util.cc:1757: explicitly_allowed_ports.erase(it);
> On 2010/12/09 19:10:31, akalin wrote:
> > suggest LOG(DFATAL) if it == end()
> 
> I used NOTREACHED() if that's okay. We don't really get anything from logging
in
> release mode.