Set the internal listen socket only when it opens to socket successfully.

Set the internal listen socket only when it opens to socket successfully.

Android devtools tries open socket twice on same UnixDomainServerSocket
instance. Instead of destroying UnixDomainServerSocket instance, not assigning
internal listen socket on socket open error is cheaper and makes more sense.

BUG=408224
Committed: https://crrev.com/14ac2486c0eba1266d2da1c52e8759d9c784fe80
Cr-Commit-Position: refs/heads/master@{#292260}

[byungchul, 2014-08-27 21:12:38]

byungchul@chromium.org changed reviewers:
+ mmenke@chromium.org

[byungchul, 2014-08-27 21:13:17]

Please review. This bug crashes debug build clank.

[mmenke, 2014-08-27 21:24:26]

On 2014/08/27 21:13:17, byungchul wrote:
> Please review. This bug crashes debug build clank.

Trying to make sense of that crash dump.  So the initial listen attempt is
failing, and we're trying again, with the same socket?

[byungchul, 2014-08-27 21:34:12]

On 2014/08/27 21:24:26, mmenke wrote:
> On 2014/08/27 21:13:17, byungchul wrote:
> > Please review. This bug crashes debug build clank.
> 
> Trying to make sense of that crash dump.  So the initial listen attempt is
> failing, and we're trying again, with the same socket?

same = UnixDomainServerSocket instance.
diff = internal listen_socket.

So, this fix is not to set listen_socket if UnixDomainServerSocket failed to
open internal socket.

[byungchul, 2014-08-27 21:38:27]

On 2014/08/27 21:34:12, byungchul wrote:
> On 2014/08/27 21:24:26, mmenke wrote:
> > On 2014/08/27 21:13:17, byungchul wrote:
> > > Please review. This bug crashes debug build clank.
> > 
> > Trying to make sense of that crash dump.  So the initial listen attempt is
> > failing, and we're trying again, with the same socket?
> 
> same = UnixDomainServerSocket instance.
> diff = internal listen_socket.
> 
> So, this fix is not to set listen_socket if UnixDomainServerSocket failed to
> open internal socket.

Please see chrome/browser/android/dev_tools_server.cc:454 (code search has not
updated yet).

[mmenke, 2014-08-27 21:43:59]

On 2014/08/27 21:38:27, byungchul wrote:
> On 2014/08/27 21:34:12, byungchul wrote:
> > On 2014/08/27 21:24:26, mmenke wrote:
> > > On 2014/08/27 21:13:17, byungchul wrote:
> > > > Please review. This bug crashes debug build clank.
> > > 
> > > Trying to make sense of that crash dump.  So the initial listen attempt is
> > > failing, and we're trying again, with the same socket?
> > 
> > same = UnixDomainServerSocket instance.
> > diff = internal listen_socket.
> > 
> > So, this fix is not to set listen_socket if UnixDomainServerSocket failed to
> > open internal socket.
> 
> Please see chrome/browser/android/dev_tools_server.cc:454 (code search has not
> updated yet).

Ok, looks like TcpServerSocket supports this too, so seems reasonable.

Can we have a unit test for this?  Use an address that can't be bound, and then
use an address than can be?

[byungchul, 2014-08-27 21:54:01]

On 2014/08/27 21:43:59, mmenke wrote:
> On 2014/08/27 21:38:27, byungchul wrote:
> > On 2014/08/27 21:34:12, byungchul wrote:
> > > On 2014/08/27 21:24:26, mmenke wrote:
> > > > On 2014/08/27 21:13:17, byungchul wrote:
> > > > > Please review. This bug crashes debug build clank.
> > > > 
> > > > Trying to make sense of that crash dump.  So the initial listen attempt
is
> > > > failing, and we're trying again, with the same socket?
> > > 
> > > same = UnixDomainServerSocket instance.
> > > diff = internal listen_socket.
> > > 
> > > So, this fix is not to set listen_socket if UnixDomainServerSocket failed
to
> > > open internal socket.
> > 
> > Please see chrome/browser/android/dev_tools_server.cc:454 (code search has
not
> > updated yet).
> 
> Ok, looks like TcpServerSocket supports this too, so seems reasonable.
> 
> Can we have a unit test for this?  Use an address that can't be bound, and
then
> use an address than can be?

Done.

[mmenke, 2014-08-27 22:14:15]

On 2014/08/27 21:54:01, byungchul wrote:
> On 2014/08/27 21:43:59, mmenke wrote:
> > On 2014/08/27 21:38:27, byungchul wrote:
> > > On 2014/08/27 21:34:12, byungchul wrote:
> > > > On 2014/08/27 21:24:26, mmenke wrote:
> > > > > On 2014/08/27 21:13:17, byungchul wrote:
> > > > > > Please review. This bug crashes debug build clank.
> > > > > 
> > > > > Trying to make sense of that crash dump.  So the initial listen
attempt
> is
> > > > > failing, and we're trying again, with the same socket?
> > > > 
> > > > same = UnixDomainServerSocket instance.
> > > > diff = internal listen_socket.
> > > > 
> > > > So, this fix is not to set listen_socket if UnixDomainServerSocket
failed
> to
> > > > open internal socket.
> > > 
> > > Please see chrome/browser/android/dev_tools_server.cc:454 (code search has
> not
> > > updated yet).
> > 
> > Ok, looks like TcpServerSocket supports this too, so seems reasonable.
> > 
> > Can we have a unit test for this?  Use an address that can't be bound, and
> then
> > use an address than can be?
> 
> Done.

Thanks!  LGTM!

[byungchul, 2014-08-27 22:16:41]

The CQ bit was checked by byungchul@chromium.org

[commit-bot: I haz the power, 2014-08-27 22:17:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/byungchul@chromium.org/515653003/40001

[commit-bot: I haz the power, 2014-08-27 23:04:43]

Committed patchset #3 (id:40001) as 86d3361179a17461088dd823a3479b821569cce6

[commit-bot: I haz the power, 2014-09-10 02:55:22]

Patchset 3 (id:??) landed as
https://crrev.com/14ac2486c0eba1266d2da1c52e8759d9c784fe80
Cr-Commit-Position: refs/heads/master@{#292260}