Fix memory leak in video_coding::PacketBuffer::InsertPacket.

Fix memory leak in video_coding::PacketBuffer::InsertPacket.

BUG=webrtc:6788
Committed: https://crrev.com/759e0b7241edbaebf9a4506c32e82f25c8accf10
Cr-Commit-Position: refs/heads/master@{#15314}

[philipel, 2016-11-29 11:41:33]

philipel@webrtc.org changed reviewers:
+ nisse@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2016-11-29 11:59:30]

https://codereview.webrtc.org/2535203002/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2535203002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:74: delete[] packet.dataPtr;
Ownership looks a bit confused here. Argument is a const ref to a VCMPacket. And
this cl deallocates the data belonging to that packet. This isn't formally
violating the constness (if it did, the compiler would have complained), but
it's not something that I'd expect to happen to a const-ref argument.

I think there are three sane ways to handle ownership for InsertPacket, and the
choice should be clearly documented:

1. Caller always owns the data.

2. Callee (i.e., InsertFrame) always takes ownership.

3. Callee sometimes takes ownership, and the decision is reflected in the return
value. When the callee isn't retaining the data, it's the caller's
responsibility to deallocate it.

And it would be even better to convert VCMPacket to use a unique_ptr for the
data.

[philipel, 2016-11-29 12:28:45]

https://codereview.webrtc.org/2535203002/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2535203002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:74: delete[] packet.dataPtr;
On 2016/11/29 11:59:30, nisse-webrtc wrote:
> Ownership looks a bit confused here. Argument is a const ref to a VCMPacket.
And
> this cl deallocates the data belonging to that packet. This isn't formally
> violating the constness (if it did, the compiler would have complained), but
> it's not something that I'd expect to happen to a const-ref argument.
> 
> I think there are three sane ways to handle ownership for InsertPacket, and
the
> choice should be clearly documented:
> 
> 1. Caller always owns the data.
> 
> 2. Callee (i.e., InsertFrame) always takes ownership.
> 
> 3. Callee sometimes takes ownership, and the decision is reflected in the
return
> value. When the callee isn't retaining the data, it's the caller's
> responsibility to deallocate it.
> 
> And it would be even better to convert VCMPacket to use a unique_ptr for the
> data.

You are absolutely right, so I went for option 2 (PacketBuffer always take
ownership).

[nisse-webrtc, 2016-11-29 12:31:40]

https://codereview.webrtc.org/2535203002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.h (right):

https://codereview.webrtc.org/2535203002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.h:53: virtual bool
InsertPacket(VCMPacket* packet);
Please comment on the ownership convention. As far as I understand the current
code, you take ownership of the packet->dataPtr storage, but not of the storage
of the VCMPacket object itself? Which is still a bit confusing.

[philipel, 2016-11-29 12:40:23]

https://codereview.webrtc.org/2535203002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.h (right):

https://codereview.webrtc.org/2535203002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.h:53: virtual bool
InsertPacket(VCMPacket* packet);
On 2016/11/29 12:31:40, nisse-webrtc wrote:
> Please comment on the ownership convention. As far as I understand the current
> code, you take ownership of the packet->dataPtr storage, but not of the
storage
> of the VCMPacket object itself? Which is still a bit confusing.

Done.

[nisse-webrtc, 2016-11-29 13:01:49]

https://codereview.webrtc.org/2535203002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2535203002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:74: delete[] packet->dataPtr;
To guard against accidental use-after-free or double free, set dataPtr = nullptr
after delete? (At all calls to delete)

[philipel, 2016-11-29 13:23:18]

https://codereview.webrtc.org/2535203002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2535203002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:74: delete[] packet->dataPtr;
On 2016/11/29 13:01:49, nisse-webrtc wrote:
> To guard against accidental use-after-free or double free, set dataPtr =
nullptr
> after delete? (At all calls to delete)

Done.

[nisse-webrtc, 2016-11-29 14:13:45]

lgtm

I still don't quite like the InsertFrame interface, but it seems hard to make
better without a refactoring of VCMPacket, which belong in a different cl.

[philipel, 2016-11-29 14:21:06]

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

[commit-bot: I haz the power, 2016-11-29 14:21:15]

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, 2016-11-29 14:43:03]

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

[commit-bot: I haz the power, 2016-11-29 14:43:04]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-11-29 18:03:23]

lgtm

[philipel, 2016-11-30 09:30:23]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-11-30 09:30:27]

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, 2016-11-30 09:32:06]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480498223596820,
"parent_rev": "0389f8a28949647e490c8f385fbc9c4866fcb5af", "commit_rev":
"33071a225f8ffcdd7d7e2517b200bdba2e3ec731"}

[commit-bot: I haz the power, 2016-11-30 09:32:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-30 09:32:17]

Description was changed from

==========
Fix memory leak in video_coding::PacketBuffer::InsertPacket.

BUG=webrtc:6788
==========

to

==========
Fix memory leak in video_coding::PacketBuffer::InsertPacket.

BUG=webrtc:6788

Committed: https://crrev.com/759e0b7241edbaebf9a4506c32e82f25c8accf10
Cr-Commit-Position: refs/heads/master@{#15314}
==========

[commit-bot: I haz the power, 2016-11-30 09:32:18]

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