Closed Bug 1008619 Opened 10 years ago Closed 10 years ago

RTCPReceiver::UpdateRTCPReceiveInformationTimers() keeps firing after all WebRTC activity ends...


(Core :: WebRTC, defect)

Not set



Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- fixed
firefox32 --- fixed
firefox-esr24 --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- affected
b2g-v2.0 --- fixed


(Reporter: jst, Assigned: jesup)





(1 file)

In a m-c build from maybe a week ago or so, RTCPReceiver::UpdateRTCPReceiveInformationTimers() keeps being called many times a second after I do:

1 - open new tab
2 - load
3 - join a random room (happens w/o sharing any devices or anything)
4 - close tab

I noticed this due to Firefox chewing up CPU time here, and I applied this in my tree to verify that perf wasn't lying:

diff --git a/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/ 
index 5b45919..2e86f30 100644
--- a/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/
+++ b/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/
@@ -730,6 +730,10 @@ bool RTCPReceiver::UpdateRTCPReceiveInformationTimers() {
   std::map<uint32_t, RTCPReceiveInformation*>::iterator receiveInfoIt =
+  static uint32_t n;
+  printf("In RTCPReceiver::UpdateRTCPReceiveInformationTimers() %d\n", n++);
   while (receiveInfoIt != _receivedInfoMap.end()) {
     RTCPReceiveInformation* receiveInfo = receiveInfoIt->second;
     if (receiveInfo == NULL) {
JST, can you supply a backtrace?
Flags: needinfo?(jst)
#0  webrtc::RTCPReceiver::UpdateRTCPReceiveInformationTimers (
    at /home/jst/fast/work/tip/mozilla/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/
#1  0x00007fe9f628a524 in UpdateRTCPReceiveInformationTimers (
    at /home/jst/fast/work/tip/mozilla/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/
#2  webrtc::ModuleRtpRtcpImpl::Process (this=0x7fe9cf0df000)
    at /home/jst/fast/work/tip/mozilla/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/
#3  0x00007fe9f62484dd in webrtc::ProcessThreadImpl::Process (
    at /home/jst/fast/work/tip/mozilla/media/webrtc/trunk/webrtc/modules/utility/source/
#4  0x00007fe9f623190c in webrtc::ThreadPosix::Run (this=0x7fe97af1a300)
    at /home/jst/fast/work/tip/mozilla/media/webrtc/trunk/webrtc/system_wrappers/source/
#5  0x00007fe9f62319bb in webrtc::StartThread (lp_parameter=<optimized out>)
    at /home/jst/fast/work/tip/mozilla/media/webrtc/trunk/webrtc/system_wrappers/source/
#6  0x00007fe9fae85124 in start_thread () from /usr/lib/
#7  0x00007fe9fa1904bd in clone () from /usr/lib/
Flags: needinfo?(jst)
This likely is related to some other issues we've seen at times in particular shutdown-time Process crashes we've seen recently, such as bug 957837
Assignee: nobody → rjesup
This ensures the remaining refs to the VoiceEngine and VideoEngine are released earlier; the ones that were leaking past xpcom-shutdown were the references from MediaEngineWebRTC (the backend in MediaManager in mBackend).  We cache this which makes subsequent getUserMedia calls far faster, but unfortunately the engines start the ProcessThreads whenever they're open.  More refactoring to the code will be needed to disable them (at minimum stop processing, preferably end the threads) when they're not needed.  This patch ensure they don't leak all the way past MediaManager shutdown (they'll die earlier now - at xpcom-shutdown, instead of when all services are shut down (MediaManager is a service)).  I'm fairly certain from logging/testing they were being shut down, but later.
Note that ProcessThread wakes periodically to do what's typically in-call processing.  We had talked in the past about releasing the MediaThread (and backend) if they're idle for XXXX after the last user closes the last MediaStream.  We don't want to release them XXX seconds after the last gUM call since we may need to call gUM in a call to change inputs and don't want a delay there.
Comment on attachment 8420932 [details] [diff] [review]
Release MediaEngineWebRTC at MediaManager shutdown

We'll want to file a bug on upstream to drop the persistent overhead for having an engine open that's not in use, and another on us for dropping mBackends when idle long enough.
Attachment #8420932 - Flags: review?(anant)
Comment on attachment 8420932 [details] [diff] [review]
Release MediaEngineWebRTC at MediaManager shutdown

Attachment #8420932 - Flags: review?(anant) → review+
FWIW this patch alone does not apply to m-c, I'm guessing there's other related bugs that this patch depends on?
Depends on: 988877
Blocks: 957837

This might be more interesting to take on B2G builds since this can leave a thread waking up regularly (if so, the bug it depends on should come along)
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [webrtc-uplift]
Target Milestone: --- → mozilla32
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 8420932 [details] [diff] [review]
Release MediaEngineWebRTC at MediaManager shutdown

Approval Request Comment
[Feature/regressing bug #]: From the beginning

[User impact if declined]: Leaks threads past xpcom-shutdown

[Describe test coverage new/current, TBPL]: Manual testing; landed a month ago, also tested with a patch that logs threads-that-leaked-past-shutdown

[Risks and why]: virtually no risk

[String/UUID change made/needed]: none
Attachment #8420932 - Flags: approval-mozilla-aurora?
Comment on attachment 8420932 [details] [diff] [review]
Release MediaEngineWebRTC at MediaManager shutdown

Randell, I think you want to see that fixed in 31 (ie beta). 32 seems to be fixed for me.
Flags: needinfo?(rjesup)
Comment on attachment 8420932 [details] [diff] [review]
Release MediaEngineWebRTC at MediaManager shutdown

Right - see previous request.
Attachment #8420932 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Flags: needinfo?(rjesup)
Comment on attachment 8420932 [details] [diff] [review]
Release MediaEngineWebRTC at MediaManager shutdown

Leaks are bad! We have it for a while in 33 & 32. Taking it for 31.
Attachment #8420932 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: verifyme
Per my conversation with :jesup via IRC, the patch for this issue cannot be manually verified by QA. Marking it [qa-].
QA Whiteboard: [qa-]
Keywords: verifyme
Whiteboard: [webrtc-uplift]
You need to log in before you can comment on or make changes to this bug.