Last Comment Bug 862302 - DataChannel leak on shutdown if peerconnection instances are not released
: DataChannel leak on shutdown if peerconnection instances are not released
Status: RESOLVED FIXED
[WebRTC][blocking-webrtc+][MemShrink:...
: mlk
Product: Core
Classification: Components
Component: WebRTC: Networking (show other bugs)
: 23 Branch
: All All
P1 normal (vote)
: mozilla23
Assigned To: Randell Jesup [:jesup]
: Jason Smith [:jsmith]
: Maire Reavy [:mreavy] Please needinfo me
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-16 04:57 PDT by Henrik Skupin (:whimboo)
Modified: 2013-05-13 17:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed


Attachments
patch to reproduce the leak (1.40 KB, patch)
2013-04-16 04:57 PDT, Henrik Skupin (:whimboo)
no flags Details | Diff | Splinter Review
Don't lose reference to queued DataChannels when they open, handle null labels properly (5.75 KB, patch)
2013-04-16 08:55 PDT, Randell Jesup [:jesup]
mcmanus: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Henrik Skupin (:whimboo) 2013-04-16 04:57:46 PDT
Created attachment 737919 [details] [diff] [review]
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 User image Randell Jesup [:jesup] 2013-04-16 08:55:39 PDT
Created attachment 738030 [details] [diff] [review]
Don't lose reference to queued DataChannels when they open, handle null labels properly
Comment 2 User image Randell Jesup [:jesup] 2013-04-16 09:01:20 PDT
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 User image Patrick McManus [:mcmanus] 2013-04-16 10:33:45 PDT
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 User image Randell Jesup [:jesup] 2013-04-16 14:14:07 PDT
(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 User image Randell Jesup [:jesup] 2013-04-16 14:17:36 PDT
(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 User image Nicholas Nethercote [:njn] 2013-04-16 16:41:39 PDT
> 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 User image Randell Jesup [:jesup] 2013-04-16 21:02:14 PDT
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 User image Michael Tüxen 2013-04-17 05:23:16 PDT
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 User image Ryan VanderMeulen [:RyanVM] 2013-04-17 10:00:00 PDT
https://hg.mozilla.org/mozilla-central/rev/23ed57f33db0
Comment 10 User image Randell Jesup [:jesup] 2013-04-17 15:30:34 PDT
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

Note You need to log in before you can comment on or make changes to this bug.