Last Comment Bug 848946 - nsIDOMDataChannel is leaking on shutdown
: nsIDOMDataChannel is leaking on shutdown
Status: VERIFIED FIXED
[MemShrink:P3][WebRTC][blocking-webrt...
: mlk, reproducible
Product: Core
Classification: Components
Component: WebRTC: Networking (show other bugs)
: 22 Branch
: All All
: -- normal (vote)
: mozilla22
Assigned To: [:jesup] on pto until 2016/7/5 Randell Jesup
: Jason Smith [:jsmith]
Mentors:
http://mozilla.github.com/webrtc-land...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-07 13:15 PST by Henrik Skupin (:whimboo)
Modified: 2013-04-05 10:20 PDT (History)
7 users (show)
jsmith: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
disabled
+
fixed


Attachments
example datachannel test (5.52 KB, text/plain)
2013-03-07 13:15 PST, Henrik Skupin (:whimboo)
no flags Details
NSPR log (45.46 KB, text/plain)
2013-03-12 10:59 PDT, Henrik Skupin (:whimboo)
no flags Details
In shutdown, don't wait for DataChannel closes to go to peer and back (3.64 KB, patch)
2013-03-25 06:26 PDT, [:jesup] on pto until 2016/7/5 Randell Jesup
mcmanus: review+
Details | Diff | Review
fixes/updates to test_peerconnection_basicDataChannel.html (5.64 KB, patch)
2013-03-25 06:26 PDT, [:jesup] on pto until 2016/7/5 Randell Jesup
no flags Details | Diff | Review

Description Henrik Skupin (:whimboo) 2013-03-07 13:15:34 PST
Created attachment 722452 [details]
example datachannel test

Seen with the attached not-done-yet datachannel mochitest. Even we don't clean-up everything correctly we should not leak.

Once applied run the test via:

./mach mochitest-plain dom/media/tests/mochitest/test_peerConnection_basicDataChannel.html

Then quit the browser with Cmd+Q.

Leaks as seen:

7:12.26 nsTraceRefcntImpl::DumpStatistics: 967 entries
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 5040 bytes during test execution
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 3 instances of DataChannel with size 128 bytes each (384 bytes total)
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1 instance of DataChannelConnection with size 592 bytes
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1 instance of DtlsIdentity with size 32 bytes
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 5 instances of Mutex with size 24 bytes each (120 bytes total)
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1 instance of NrIceCtx with size 160 bytes
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 3 instances of NrIceMediaStream with size 168 bytes each (504 bytes total)
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 12 instances of NrSocket with size 200 bytes each (2400 bytes total)
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of ReentrantMonitor with size 32 bytes
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of TransportFlow with size 224 bytes
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of VerificationDigest with size 88 bytes
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDeque with size 96 bytes
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsSocketTransportService with size 216 bytes
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 3 instances of nsStringBuffer with size 8 bytes each (24 bytes total)
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 9 instances of nsTArray_base with size 8 bytes each (72 bytes total)
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsTimerImpl with size 96 bytes

I haven't tested on 21.0a2 so not sure if that's also happening there.
Comment 1 Jason Smith [:jsmith] 2013-03-07 13:19:31 PST
Ethan - Any ideas why we are leaking?
Comment 2 Henrik Skupin (:whimboo) 2013-03-07 23:23:56 PST
It might be that this leak is caused by bug 837035 because I haven't seen it before. I can check that locally.
Comment 3 Henrik Skupin (:whimboo) 2013-03-08 00:28:48 PST
I backed out the patch from bug 837035 locally and it doesn't seem to be the reason. The leak is still there.
Comment 4 Henrik Skupin (:whimboo) 2013-03-12 00:32:19 PDT
Randell, sadly I missed the line on IRC where you mentioned which NSPR logs you want to have. Would you mind to tell me that again? Was it datachannel:5 only?
Comment 5 Henrik Skupin (:whimboo) 2013-03-12 04:15:45 PDT
So this leak is caused by the following line which creates a datachannel. There is no need to listen for events. Once the code has been run close the browser.

var pc1 = mozRTCPeerConnection();
[..] (setup peer connection and data channel)
pc1.createDataChannel("This is channel request from pcLocal",{})
Comment 6 [:jesup] on pto until 2016/7/5 Randell Jesup 2013-03-12 08:16:06 PDT
If you could do a run with datachannel:5,signaling:5 and add the results it would help a bit, as I'm working from a conference currently.  Thanks
Comment 7 Henrik Skupin (:whimboo) 2013-03-12 10:59:46 PDT
Created attachment 724030 [details]
NSPR log

Log via NSPR_MODULES as given by Randell in the last comment. I hope it's helpful for you.
Comment 8 Henrik Skupin (:whimboo) 2013-03-14 04:11:29 PDT
It might not only be on shutdown. So it needs further investigation. And as I have seen right now it also leaks the same stuff when running our demo page for data channels.

http://mozilla.github.com/webrtc-landing/data_test.html

Steps:
1. Click on Start
2. Quit the browser (closing the tab might also be the action here)
Comment 9 Henrik Skupin (:whimboo) 2013-03-14 05:53:14 PDT
Same happens with a x86-64 tinderbox debug build on Ubuntu 12.10 64bit.
Comment 10 Alex Keybl [:akeybl] 2013-03-15 15:46:08 PDT
Is FF21 affected?
Comment 11 [:jesup] on pto until 2016/7/5 Randell Jesup 2013-03-25 06:26:01 PDT
Created attachment 728940 [details] [diff] [review]
In shutdown, don't wait for DataChannel closes to go to peer and back
Comment 12 [:jesup] on pto until 2016/7/5 Randell Jesup 2013-03-25 06:26:58 PDT
Created attachment 728942 [details] [diff] [review]
fixes/updates to test_peerconnection_basicDataChannel.html
Comment 13 [:jesup] on pto until 2016/7/5 Randell Jesup 2013-03-25 06:28:22 PDT
Comment on attachment 728940 [details] [diff] [review]
In shutdown, don't wait for DataChannel closes to go to peer and back

In shutdown, we can't wait for the other side to close their end of the DataChannel before releasing ours (since we're going to close the socket as soon as we send all our resets.
Comment 14 [:jesup] on pto until 2016/7/5 Randell Jesup 2013-03-25 06:29:42 PDT
Comment on attachment 728942 [details] [diff] [review]
fixes/updates to test_peerconnection_basicDataChannel.html

Henrik - you should look at some of the fixes here.  There's more to do (in particular factoring out the onmessage/onopen/onclose's) and also this test has no exit condition.
Comment 15 [:jesup] on pto until 2016/7/5 Randell Jesup 2013-03-25 15:35:15 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a38caee2b5
Comment 16 Henrik Skupin (:whimboo) 2013-03-25 16:59:02 PDT
(In reply to Randell Jesup [:jesup] from comment #14)
> Henrik - you should look at some of the fixes here.  There's more to do (in
> particular factoring out the onmessage/onopen/onclose's) and also this test
> has no exit condition.

I know. It was a pretty hacky work in progress patch which showed this behavior. However we should never leak. :) Thanks for landing the fix. I will check it tomorrow.
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-03-26 07:23:12 PDT
https://hg.mozilla.org/mozilla-central/rev/b7a38caee2b5
Comment 18 Henrik Skupin (:whimboo) 2013-03-28 06:17:26 PDT
I can verify that the datachannel mochitest does no longer causes leaks in a recent debug build. Thanks Randell.
Comment 19 bhavana bajaj [:bajaj] 2013-04-02 13:52:05 PDT
(In reply to Randell Jesup [:jesup] from comment #15)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a38caee2b5

Ideally,we should uplift this to beta asap as Fx21 is affected here and this ix fixed on Fx22.
I am also ok to let the patch ride the trains and mark status-firefox21:disabled since we have webRTC preffed of by default.Jesup,based on the risk of the patch, what do you think would be best here?
Comment 20 [:jesup] on pto until 2016/7/5 Randell Jesup 2013-04-02 14:04:24 PDT
This is a fairly low-risk change - however, webrtc is disabled by default in Fx21, and this merely closes a small leak if it's enabled and used, so I see no advantage in uplifitng to beta

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