nsIDOMDataChannel is leaking on shutdown

VERIFIED FIXED in Firefox 22

Status

()

Core
WebRTC: Networking
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: whimboo, Assigned: jesup)

Tracking

({mlk, reproducible})

22 Branch
mozilla22
mlk, reproducible
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox20 unaffected, firefox21+ disabled, firefox22+ fixed)

Details

(Whiteboard: [MemShrink:P3][WebRTC][blocking-webrtc+][qa-], URL)

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc?][automation-blocked] → [WebRTC][blocking-webrtc+][automation-blocked]
Ethan - Any ideas why we are leaking?
(Reporter)

Comment 2

5 years ago
It might be that this leak is caused by bug 837035 because I haven't seen it before. I can check that locally.
(Reporter)

Comment 3

5 years ago
I backed out the patch from bug 837035 locally and it doesn't seem to be the reason. The leak is still there.
(Reporter)

Comment 4

5 years ago
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?
Flags: needinfo?(rjesup)
(Reporter)

Comment 5

5 years ago
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",{})
Summary: Datachannel is leaking → nsIDOMDataChannel is leaking on shutdown
Whiteboard: [WebRTC][blocking-webrtc+][automation-blocked] → [MemShrink][WebRTC][blocking-webrtc+][automation-blocked]
(Assignee)

Comment 6

5 years ago
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
Flags: needinfo?(rjesup)
(Reporter)

Comment 7

5 years ago
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.
(Reporter)

Comment 8

5 years ago
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)
(Reporter)

Comment 9

5 years ago
Same happens with a x86-64 tinderbox debug build on Ubuntu 12.10 64bit.
OS: Mac OS X → All
Hardware: x86 → All
Is FF21 affected?
Assignee: nobody → rjesup
status-firefox20: --- → unaffected
tracking-firefox21: --- → ?
tracking-firefox22: ? → +
Whiteboard: [MemShrink][WebRTC][blocking-webrtc+][automation-blocked] → [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked]

Updated

5 years ago
Flags: needinfo?(rjesup)
(Assignee)

Comment 11

5 years ago
Created attachment 728940 [details] [diff] [review]
In shutdown, don't wait for DataChannel closes to go to peer and back
Flags: needinfo?(rjesup)
(Assignee)

Comment 12

5 years ago
Created attachment 728942 [details] [diff] [review]
fixes/updates to test_peerconnection_basicDataChannel.html
(Assignee)

Comment 13

5 years ago
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.
Attachment #728940 - Flags: review?(mcmanus)
(Assignee)

Comment 14

5 years ago
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.
Attachment #728940 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a38caee2b5
Target Milestone: --- → mozilla22

Updated

5 years ago
status-firefox21: --- → affected
tracking-firefox21: ? → +
(Reporter)

Comment 16

5 years ago
(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.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/b7a38caee2b5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox22: affected → fixed
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked] → [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked][qa-]
(Reporter)

Comment 18

5 years ago
I can verify that the datachannel mochitest does no longer causes leaks in a recent debug build. Thanks Randell.
Status: RESOLVED → VERIFIED

Updated

4 years ago
Flags: in-testsuite-
(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?

Updated

4 years ago
Flags: needinfo?(rjesup)
(Assignee)

Comment 20

4 years ago
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
Flags: needinfo?(rjesup)
(Reporter)

Updated

4 years ago
Whiteboard: [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked][qa-] → [MemShrink:P3][WebRTC][blocking-webrtc+][qa-]

Updated

4 years ago
status-firefox21: affected → disabled
You need to log in before you can comment on or make changes to this bug.