862302 - DataChannel leak on shutdown if peerconnection instances are not released

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: patch to reproduce the leak

I experience a leak in the DataChannel code on shutdown. It happens when createDataChannel() is called before the createOffer() call, and a shutdown of the browser without releasing the created peerconnection instances.

I will attach a simple patch which let you demonstrate the leak with the current testing code. Just run any of our basic peer connection tests.

Leak output:

TEST-INFO | leakcheck | leaked 1 DataChannel (144 bytes)
TEST-INFO | leakcheck | leaked 1 Mutex (24 bytes)
TEST-INFO | leakcheck | leaked 2 nsTArray_base (16 bytes)

TEST-UNEXPECTED-FAIL| leakcheck | 184 bytes leaked (DataChannel, Mutex, nsTArray_base)

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Don't lose reference to queued DataChannels when they open, handle null labels properly

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 738030 [details] [diff] [review]
Don't lose reference to queued DataChannels when they open, handle null labels properly

Assigning to mcmanus as this is just a reference-counting issue (and a minor foobar with "" labels where we weren't accounting for the base structure having a not-always-used char[1] trailing).  

ProcessQueuedOpens() was calling OpenFinish(), but ignoring that it returned an already_AddRefed DataChannel and so dropping the ref on the floor (too bad the compiler can't whine about that)

The rest is just comments/LOG changes

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 738030 [details] [diff] [review]
Don't lose reference to queued DataChannels when they open, handle null labels properly

Review of attachment 738030 [details] [diff] [review]:
-----------------------------------------------------------------

go ahead and plug the leak with this patch. Add NS_WARN_UNUSED_RESULT ?

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +1295,5 @@
>  
>    switch (ppid) {
>      case DATA_CHANNEL_PPID_CONTROL:
> +      // structure includes a possibly-unused char label[1] (in a packed structure)
> +      NS_ENSURE_TRUE_VOID(length >= sizeof(*req) - 1);

I don't understand how is this germane

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Patrick McManus [:mcmanus] from comment #3)
> Comment on attachment 738030 [details] [diff] [review]
> Don't lose reference to queued DataChannels when they open, handle null
> labels properly
> 
> Review of attachment 738030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> go ahead and plug the leak with this patch. Add NS_WARN_UNUSED_RESULT ?
> 
> ::: netwerk/sctp/datachannel/DataChannel.cpp
> @@ +1295,5 @@
> >  
> >    switch (ppid) {
> >      case DATA_CHANNEL_PPID_CONTROL:
> > +      // structure includes a possibly-unused char label[1] (in a packed structure)
> > +      NS_ENSURE_TRUE_VOID(length >= sizeof(*req) - 1);
> 
> I don't understand how is this germane

Because the testcase here uses a label of "" which was failing due to the test (no label bytes were included in the Open packet, so it had 12 bytes, but sizeof(*req) is 13 due to the char label[1].

This is why the patch also had "handle null labels properly" in the title; sorry I didn't explain in the bug (and perhaps the comment in the code wasn't sufficiently clear).

I.e. this testcase invokes two bugs, both important to fix (and simple).

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Patrick McManus [:mcmanus] from comment #3)
> Comment on attachment 738030 [details] [diff] [review]
> Don't lose reference to queued DataChannels when they open, handle null
> labels properly
> 
> Review of attachment 738030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> go ahead and plug the leak with this patch. Add NS_WARN_UNUSED_RESULT ?

Perhaps we should add that to all already_AddRefed<>'d-returning functions

Comment 6

[Nicholas Nethercote [inactive]]

> TEST-UNEXPECTED-FAIL| leakcheck | 184 bytes leaked (DataChannel, Mutex,
> nsTArray_base)

MemShrink:P3 because the leak is so small.  khuey will file a bug on the general issue.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23ed57f33db0

This is just the leak portion; I'll break off the null label as a separate issue.

Comment 8

[Michael Tüxen]

Comment on attachment 738030 [details] [diff] [review]
Don't lose reference to queued DataChannels when they open, handle null labels properly

Review of attachment 738030 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +1300,1 @@
>  

I think it is correct. The -1 compensates for the declaration
of the final field as label[1] instead of label[0].

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23ed57f33db0

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 738030 [details] [diff] [review]
Don't lose reference to queued DataChannels when they open, handle null labels properly

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 855623

User impact if declined: Channels with empty ("") labels won't open.  Leak that blocks landing any DataChannel tests in Aurora.

Testing completed (on m-c, etc.): on m-c, tested with testcase in bug.

Risk to taking this patch (and alternatives if risky): nil - avoids dropping a returned reference on the floor.

String or IDL/UUID changes made by this patch: none

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bb3c481d2f70