Closed Bug 812641 Opened 7 years ago Closed 7 years ago

Merely creating a PeerConnection leaks a Mutex

Categories

(Core :: WebRTC, defect)

x86_64
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla19

People

(Reporter: jruderman, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, testcase, Whiteboard: [MemShrink][qa-])

Attachments

(2 files)

Attached file testcase
With:
  user_pref("media.peerconnection.enabled", true);

The testcase makes Firefox leak a Mutex (as shown by XPCOM_MEM_LEAK_LOG=2).

This is probably less serious than bug 798323, but my fuzzer is actually able to hit this one ;)
Whiteboard: [MemShrink]
The 4 mutexes leaked by the testcase are the 4 used by different parts of media/webrtc/signaling (SipCC).  This shuts down the SipCC Instance when the number of PeerConnections is 0, but a better solution is likely (such as having the Instance use refcounting, and/or use a delayed release to avoid churn of a starting/stopping a large module.
Attachment #682836 - Flags: review?(ethanhugg)
Comment on attachment 682836 [details] [diff] [review]
Shut down SipCC instance when number of PeerConnections == 0

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +280,5 @@
>  }
>  
>  PeerConnectionImpl::~PeerConnectionImpl()
>  {
> +  Close();

Let's put ASSERTS here and in creation for main thread in case the threading gets changed later.

@@ +289,4 @@
>    peerconnections.erase(mHandle);
> +  if (peerconnections.empty())
> +    Shutdown();
> + 

Trailing whitespace here
Attachment #682836 - Flags: review?(ethanhugg) → review+
https://hg.mozilla.org/mozilla-central/rev/b3b972e271a7
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
For future reference - how would I detect that an automated test I wrote ends up having a memory leak?

I'd imagine with this leak example that if we had a basic peer connection test in mochitest, we would hit this memory leak.
Whiteboard: [MemShrink] → [MemShrink][qa-]
(In reply to Jason Smith [:jsmith] from comment #5)
> For future reference - how would I detect that an automated test I wrote
> ends up having a memory leak?

At the end of a mochitest run, you should see something like:

"BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process"

If there are any leaks, they will be listed in the table below this line.

This is the same leak detection infrastructure I used to find this bug (by setting XPCOM_MEM_LEAK_LOG=2).
(In reply to Jason Smith [:jsmith] from comment #5)
> I'd imagine with this leak example that if we had a basic peer connection
> test in mochitest, we would hit this memory leak.

We have a couple of memory leaks in our existent crashtests which I'm going to investigate the next couple of days. So it's not only dependent on Mochitests!
(In reply to Randell Jesup [:jesup] from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b972e271a7

The patch pushed here doesn't seem to match the one in the bug at all. Moreover, it's only assertions, so it seems very unlikely that this bug is fixed.

What's the deal?
Reopening just to make sure this gets attention. Sorry if I'm just bumbling over some subtleties somewhere.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Thanks greatly for catching that.  Some foobar with queues while addressing the nits caused me to commit just the requested added assert and not the actual patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/49edf1af4140
https://hg.mozilla.org/mozilla-central/rev/49edf1af4140
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Leak is gone with:

BuildID=20121123161546
SourceRepository=https://hg.mozilla.org/mozilla-central
SourceStamp=d8e4f06198dc
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.