Refactor TestClient to use std::unique_ptr, and fix VirtualSocketServerTest leaks.

Refactor TestClient to use std::unique_ptr, and fix VirtualSocketServerTest leaks.

BUG=None

Review-Url: https://codereview.webrtc.org/2859373003
Cr-Commit-Position: refs/heads/master@{#18043}
Committed: https://chromium.googlesource.com/external/webrtc/+/32f25051853fcf483f0a6f145c278eabcc112fc7

[nisse-webrtc, 2017-05-05 08:56:23]

nisse@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, tommi@webrtc.org

[nisse-webrtc, 2017-05-05 08:56:23]

Please take a look.

I have no idea why the VirtualSocketServerTests have ever passed the memcheck
try-bot. Local valgrind run reports lots of leaks, which are fixed with this cl.

Are there some suppressions somewhere?

[kwiberg-webrtc, 2017-05-05 11:04:41]

kwiberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[kwiberg-webrtc, 2017-05-05 11:04:42]

Great initiative!

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/nat_unittest.cc
File webrtc/base/nat_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/nat_unittest.cc#n...
webrtc/base/nat_unittest.cc:43: return new TestClient(WrapUnique(new
AsyncTCPSocket(socket, false)));
Use MakeUnique instead?

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient.cc
File webrtc/base/testclient.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient.cc#new...
webrtc/base/testclient.cc:115: return res;
Combine the two statements on lines 114 and 115, now that they don't have to
have a delete in between them? (Well, strictly speaking they didn't *have* to
before either...)

Hmm. In fact, the whole function body collapses to something like

  return NextPacket(kNoPacketTimeoutMs) == nullptr;

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient.h
File webrtc/base/testclient.h (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient.h#newc...
webrtc/base/testclient.h:68: // when done with it.
The last sentence of the comment is no longer necessary.

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient_unitte...
File webrtc/base/testclient_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient_unitte...
webrtc/base/testclient_unittest.cc:42: AsyncTCPSocket::Create(socket, loopback,
server.address()));
Hmm. Maybe

  auto tcp_socket = WrapUnique(
      AsyncTCPSocket::Create(socket, loopback, server.address()));

WrapUnique has two big advantages over calling the unique_ptr constructor: you
don't have to mention the type, and the call will only work with a raw pointer
argument (so that whoever reads the code doesn't have to wonder if the argument
is a raw pointer or a unique_ptr).

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/virtualsocket_uni...
File webrtc/base/virtualsocket_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/virtualsocket_uni...
webrtc/base/virtualsocket_unittest.cc:185: new
TestClient(MakeUnique<AsyncUDPSocket>(socket)));
Why not use MakeUnique for TestClient too?

  auto client1 = MakeUnique<TestClient>(MakeUnique<AsyncUDPSocket>(socket));

(You have a bunch of these in this file.)

[nisse-webrtc, 2017-05-05 12:12:57]

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/nat_unittest.cc
File webrtc/base/nat_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/nat_unittest.cc#n...
webrtc/base/nat_unittest.cc:43: return new TestClient(WrapUnique(new
AsyncTCPSocket(socket, false)));
On 2017/05/05 11:04:42, kwiberg-webrtc wrote:
> Use MakeUnique instead?

Done. (This file should also have many raw pointers replaced by unique_ptr, but
that's for a different cl).

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient.cc
File webrtc/base/testclient.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient.cc#new...
webrtc/base/testclient.cc:115: return res;
On 2017/05/05 11:04:42, kwiberg-webrtc wrote:
> Combine the two statements on lines 114 and 115, now that they don't have to
> have a delete in between them? (Well, strictly speaking they didn't *have* to
> before either...)
> 
> Hmm. In fact, the whole function body collapses to something like
> 
>   return NextPacket(kNoPacketTimeoutMs) == nullptr;

Nice. Done.

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient.h
File webrtc/base/testclient.h (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient.h#newc...
webrtc/base/testclient.h:68: // when done with it.
On 2017/05/05 11:04:42, kwiberg-webrtc wrote:
> The last sentence of the comment is no longer necessary.

Done.

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient_unitte...
File webrtc/base/testclient_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient_unitte...
webrtc/base/testclient_unittest.cc:42: AsyncTCPSocket::Create(socket, loopback,
server.address()));
On 2017/05/05 11:04:42, kwiberg-webrtc wrote:
> Hmm. Maybe
> 
>   auto tcp_socket = WrapUnique(
>       AsyncTCPSocket::Create(socket, loopback, server.address()));
> 
> WrapUnique has two big advantages over calling the unique_ptr constructor: you
> don't have to mention the type,

Not mentioning the type is a mixed blessing, though... But in this case, maybe
it's fairly clear from other context.

But when AsyncTCPSocket::Create is refactored to return a unique_ptr (like it
should), we'd delete WrapUnique here, and then with auto there's no local
context left saying if we're working with a smart pointer or not. I'm feeling
we're getting close to intercal PLEASE territory.

And there's no local guarantee that a method which happens to be named
Foo::Create actually returns a Foo or a Foo pointer.

For now, I think I prefer to not use auto.

> and the call will only work with a raw pointer
> argument (so that whoever reads the code doesn't have to wonder if the
argument
> is a raw pointer or a unique_ptr).

Makes some sense. I've picked up the pattern

  std::unique_ptr<Foo> p(some raw Foo pointer);

from other places in webtrc. You're saying that we should generally stop writing
this, and instead use = WrapUnique? (I've read some posting on WrapUnique, but I
probably didn't get all the details).

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/virtualsocket_uni...
File webrtc/base/virtualsocket_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/virtualsocket_uni...
webrtc/base/virtualsocket_unittest.cc:185: new
TestClient(MakeUnique<AsyncUDPSocket>(socket)));
On 2017/05/05 11:04:42, kwiberg-webrtc wrote:
> Why not use MakeUnique for TestClient too?
> 
>   auto client1 = MakeUnique<TestClient>(MakeUnique<AsyncUDPSocket>(socket));
> 
> (You have a bunch of these in this file.)

Done. I take it you don't want to see any use of the unique_ptr<T>(T*)
constructor? And in this case, I think auto is ok, since MakeUnique<T> provides
strong local type info.

[kwiberg-webrtc, 2017-05-05 12:40:31]

lgtm (but see the comment about "0")

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/nat_unittest.cc
File webrtc/base/nat_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/nat_unittest.cc#n...
webrtc/base/nat_unittest.cc:43: return new TestClient(WrapUnique(new
AsyncTCPSocket(socket, false)));
On 2017/05/05 12:12:56, nisse-webrtc wrote:
> On 2017/05/05 11:04:42, kwiberg-webrtc wrote:
> > Use MakeUnique instead?
> 
> Done. (This file should also have many raw pointers replaced by unique_ptr,
but
> that's for a different cl).

Yes, I quickly realized that if I were to complain about every questionable
thing on the lines that you touch, we'd be here all day. :-)

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient_unitte...
File webrtc/base/testclient_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/testclient_unitte...
webrtc/base/testclient_unittest.cc:42: AsyncTCPSocket::Create(socket, loopback,
server.address()));
On 2017/05/05 12:12:56, nisse-webrtc wrote:
> On 2017/05/05 11:04:42, kwiberg-webrtc wrote:
> > Hmm. Maybe
> > 
> >   auto tcp_socket = WrapUnique(
> >       AsyncTCPSocket::Create(socket, loopback, server.address()));
> > 
> > WrapUnique has two big advantages over calling the unique_ptr constructor:
you
> > don't have to mention the type,
> 
> Not mentioning the type is a mixed blessing, though... But in this case, maybe
> it's fairly clear from other context.

Yes. I'd say it's a justifiable leap of faith to assume that Foo::Create returns
a Foo, or possibly a subclass of Foo.

> But when AsyncTCPSocket::Create is refactored to return a unique_ptr (like it
> should), we'd delete WrapUnique here, and then with auto there's no local
> context left saying if we're working with a smart pointer or not. I'm feeling
> we're getting close to intercal PLEASE territory.

Yes, ideally we'd be able to say

  unique_ptr<auto> x = Foo::Create();

in such cases, but we can't, so I agree that naming the complete type is the
least bad option in such cases.

> And there's no local guarantee that a method which happens to be named
> Foo::Create actually returns a Foo or a Foo pointer.

I would argue that if it does return something other than a Foo pointer (or a
subclass), then there's a naming bug.

> For now, I think I prefer to not use auto.

Fair enough. There are good arguments on both sides.

> > and the call will only work with a raw pointer
> > argument (so that whoever reads the code doesn't have to wonder if the
> argument
> > is a raw pointer or a unique_ptr).
> 
> Makes some sense. I've picked up the pattern
> 
>   std::unique_ptr<Foo> p(some raw Foo pointer);
> 
> from other places in webtrc. You're saying that we should generally stop
writing
> this, and instead use = WrapUnique? (I've read some posting on WrapUnique, but
I
> probably didn't get all the details).

Yes, for cases where MakeUnique isn't an option, I think

  std::unique_ptr<Foo> p = WrapUnique(x);

is strictly better than

  std::unique_ptr<Foo> p(x);

It's more characters, but it clearly highlights a point where an owning raw
pointer is converted to a unique_ptr. Every time you see a call to WrapUnique
you know that an owning raw pointer is being converted to a unique_ptr, because
that's the only thing it can do.

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/virtualsocket_uni...
File webrtc/base/virtualsocket_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/1/webrtc/base/virtualsocket_uni...
webrtc/base/virtualsocket_unittest.cc:185: new
TestClient(MakeUnique<AsyncUDPSocket>(socket)));
On 2017/05/05 12:12:56, nisse-webrtc wrote:
> On 2017/05/05 11:04:42, kwiberg-webrtc wrote:
> > Why not use MakeUnique for TestClient too?
> > 
> >   auto client1 = MakeUnique<TestClient>(MakeUnique<AsyncUDPSocket>(socket));
> > 
> > (You have a bunch of these in this file.)
> 
> Done. I take it you don't want to see any use of the unique_ptr<T>(T*)
> constructor?

No; as I said in an earlier comment, I think consistently using WrapUnique
instead makes more sense, since owning raw pointers are  easy enough to make
mistakes with that loudly calling out their use is a good choice.

> And in this case, I think auto is ok, since MakeUnique<T> provides
> strong local type info.  

Yes. With MakeUnique, there's never any reason to avoid auto.

https://codereview.webrtc.org/2859373003/diff/40001/webrtc/base/testclient.h
File webrtc/base/testclient.h (right):

https://codereview.webrtc.org/2859373003/diff/40001/webrtc/base/testclient.h#...
webrtc/base/testclient.h:66: // Returns the next packet received by the client
or 0 if none is received
Hmm. "nullptr" or "null" instead of "0" here...

[nisse-webrtc, 2017-05-05 12:56:06]

Thanks for the review.

I just remembered that Karl is root owner (I checked only base/OWNERS and
p2p/OWNERS for my initial set of reviewers). So I'm going to land this, and if
Taylor or Tommi has additional comments, I'm happy to address them in a followup
cl.

https://codereview.webrtc.org/2859373003/diff/40001/webrtc/base/testclient.h
File webrtc/base/testclient.h (right):

https://codereview.webrtc.org/2859373003/diff/40001/webrtc/base/testclient.h#...
webrtc/base/testclient.h:66: // Returns the next packet received by the client
or 0 if none is received
On 2017/05/05 12:40:31, kwiberg-webrtc wrote:
> Hmm. "nullptr" or "null" instead of "0" here...

Done.

[nisse-webrtc, 2017-05-05 12:56:39]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-05-05 12:56:40]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2859373003/#ps60001 (title:
"Comment fix, 0 -> null.")

[commit-bot: I haz the power, 2017-05-05 12:56:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-05 13:25:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 13:25:02]

Try jobs failed on following builders:
  linux_memcheck on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/7751)

[nisse-webrtc, 2017-05-05 14:47:07]

On 2017/05/05 13:25:02, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_memcheck on master.tryserver.webrtc (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/7751)

There are more leaks in virtualsocket_unittest.cc which I'll look into.

[Taylor Brandstetter, 2017-05-07 21:37:00]

So far lgtm

https://codereview.webrtc.org/2859373003/diff/60001/webrtc/base/virtualsocket...
File webrtc/base/virtualsocket_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/60001/webrtc/base/virtualsocket...
webrtc/base/virtualsocket_unittest.cc:179: AsyncSocket* socket =
Would be better to use MakeUnique here rather than when it's passed into
client1, so there's never a raw pointer to deal with.

[nisse-webrtc, 2017-05-08 07:51:15]

This has now passed a local valgrind run. We'll see if the bots are happy.

https://codereview.webrtc.org/2859373003/diff/60001/webrtc/base/virtualsocket...
File webrtc/base/virtualsocket_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/60001/webrtc/base/virtualsocket...
webrtc/base/virtualsocket_unittest.cc:179: AsyncSocket* socket =
On 2017/05/07 21:37:00, Taylor Brandstetter wrote:
> Would be better to use MakeUnique here rather than when it's passed into
> client1, so there's never a raw pointer to deal with.

I think AsyncUDPSocket takes over ownership, via a raw pointer (changing that to
unique_ptr is desirable, but not in this cl).

So it could be changed to unique_ptr + .release, but I'm not sure that makes
things clearer.

[nisse-webrtc, 2017-05-08 07:51:28]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 07:51:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-08 08:52:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 08:52:36]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-05-08 08:55:03]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-05-08 08:55:03]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2859373003/#ps80001 (title:
"Fix more leaks in VirtualSocketServerTest.")

[commit-bot: I haz the power, 2017-05-08 08:55:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-08 08:57:19]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494233703432360,
"parent_rev": "e8f12406b91741cbcffd40cdf031667c21bad9b7", "commit_rev":
"32f25051853fcf483f0a6f145c278eabcc112fc7"}

[commit-bot: I haz the power, 2017-05-08 08:57:22]

Description was changed from

==========
Refactor TestClient to use std::unique_ptr, and fix VirtualSocketServerTest
leaks.

BUG=None
==========

to

==========
Refactor TestClient to use std::unique_ptr, and fix VirtualSocketServerTest
leaks.

BUG=None

Review-Url: https://codereview.webrtc.org/2859373003
Cr-Commit-Position: refs/heads/master@{#18043}
Committed:
https://chromium.googlesource.com/external/webrtc/+/32f25051853fcf483f0a6f145...
==========

[commit-bot: I haz the power, 2017-05-08 08:57:23]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/32f25051853fcf483f0a6f145...

[kwiberg-webrtc, 2017-05-08 10:21:26]

https://codereview.webrtc.org/2859373003/diff/60001/webrtc/base/virtualsocket...
File webrtc/base/virtualsocket_unittest.cc (right):

https://codereview.webrtc.org/2859373003/diff/60001/webrtc/base/virtualsocket...
webrtc/base/virtualsocket_unittest.cc:179: AsyncSocket* socket =
On 2017/05/08 07:51:15, nisse-webrtc wrote:
> On 2017/05/07 21:37:00, Taylor Brandstetter wrote:
> > Would be better to use MakeUnique here rather than when it's passed into
> > client1, so there's never a raw pointer to deal with.
> 
> I think AsyncUDPSocket takes over ownership, via a raw pointer (changing that
to
> unique_ptr is desirable, but not in this cl).
> 
> So it could be changed to unique_ptr + .release, but I'm not sure that makes
> things clearer.

I would definitely say that using unique_ptr here would make things clearer. As
it is, you have to look at the docs (or the implementation) of
CreateAsyncSocket() and the AsyncUDPSocket constructor to see what's going on
with the ownership; but if you had stored the AsyncSocket in a unique_ptr and
called release() on it at the end, it would be immediately obvious that
CreateAsyncSocket() returns an owning pointer, and that we pass the ownership to
AsyncUDPSocket().

No need to go back and amend this CL, though; I just wanted to provide some
unrequested good advice for the future. :-)