Closed
Bug 1153690
Opened 9 years ago
Closed 9 years ago
Leak with MediaSource and MediaRecorder
Categories
(Core :: Audio/Video: Recording, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jruderman, Assigned: bechen)
References
(Blocks 1 open bug)
Details
(Keywords: memory-leak, testcase)
Attachments
(4 files, 4 obsolete files)
1. Run a (debug) build with XPCOM_MEM_LEAK_LOG=2 2. Load the testcase 3. Wait at least 500ms 4. Quit Firefox Leaks nsGlobalWindow & nsDocument.
Comment 2•9 years ago
|
||
There's a DETH that is holding alive the window. rc=1, no known reference. No other information on it.
Comment 3•9 years ago
|
||
According to refcount logging the DETH is a DOMMediaStream. Attached is the log for this object. I don't know this code very well, but it looks like the AddRef from the MediaRecorder() ctor is unaccounted for. This looks like it must be the mDOMStream field, but that field is probably reported to the cycle collector. Oh, looking at the CC logs some more, there is what looks like a MediaRecorder holding onto our DOMMediaStream in the first CC log, which zero known references, but then it isn't in the second log, so that's maybe what is holding things alive. I'll investigate that.
Comment 4•9 years ago
|
||
Here's the refcount log for the MediaRecorder. I'm not sure what is going on yet.
Comment 5•9 years ago
|
||
Doing some simple cancellation, it looks like the unaccounted for reference to the MediaRecorder was created in the MediaRecorder::Session ctor: <DOMEventTargetHelper> 0x11981e880 12 AddRef 2 #01: mozilla::dom::MediaRecorder::Session::Session(mozilla::dom::MediaRecorder*, int) (MediaRecorder.cpp:119, in XUL) #02: mozilla::dom::MediaRecorder::Start(mozilla::dom::Optional<int> const&, mozilla::ErrorResult&) (nsRefPtr.h:28, in XUL) #03: mozilla::dom::MediaRecorderBinding::start(JSContext*, JS::Handle<JSObject*>, mozilla::dom::MediaRecorder*, JSJitMethodCallArgs const&) (ErrorResult.h:134, in XUL)
Flags: needinfo?(continuation)
Comment 6•9 years ago
|
||
Benjamin, it looks like you looked into the lifetime of these objects fairly recently in bug 1047022. Is this leak something you have time to investigate? Thanks.
Flags: needinfo?(bechen)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #6) > Benjamin, it looks like you looked into the lifetime of these objects fairly > recently in bug 1047022. Is this leak something you have time to > investigate? Thanks. Thanks for your investigation, I'll look it soon. I'll try to reproduce it on my ubuntu desktop build. And according to the STR in comment 0, the session object should receive the notification from ShutdownObserver then release the MediaRecorder.
Assignee: nobody → bechen
Flags: needinfo?(bechen)
Assignee | ||
Comment 8•9 years ago
|
||
The MediaRecorder and Session's destroy flow rely on the ExtractRunnable -> DestroyRunnable. At this case, the SetupStreams() function send the TracksAvailableCallback to DOMMediaStream, but the callback function is never called. (https://dxr.mozilla.org/mozilla-central/source/dom/media/DOMMediaStream.cpp#504) So there is no ExtractRunnable running cause the leak.
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8592735 -
Flags: review?(roc)
Attachment #8592735 -
Flags: review?(roc) → review+
Assignee | ||
Comment 10•9 years ago
|
||
r=roc tryserver: https://treeherder.mozilla.org/#/jobs?repo=try&revision=083e224d3457
Attachment #8592735 -
Attachment is obsolete: true
Attachment #8594578 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Benjamin Chen [:bechen] from comment #10) > Created attachment 8594578 [details] [diff] [review] > bug-1153690.v01.patch > > r=roc > > tryserver: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=083e224d3457 Now I'm dixing the build break and other testscase failed like TEST-UNEXPECTED-FAIL | dom/media/test/test_mediarecorder_record_immediate_stop.html | application terminated with exit code 11
Assignee | ||
Comment 12•9 years ago
|
||
There are 2 situations we need to handle in this patch. 1. MediaRecorder.Start then MediaRecorder.Stop immediately (test_mediarecorder_record_immediate_stop.html) , then TracksAvailableCallback comes. 2. MediaRecorder.Start then close tab(comment 0) then no TracksAvailableCallback comes. My v01 patch breaks the test_mediarecorder_record_immediate_stop.html
Assignee | ||
Comment 13•9 years ago
|
||
Fix build break and testcase fail. Carry r+ from v01 because the only change is to call DoSessionEndTask() instead of BreakCycle() and put some prlog. try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0352305daca6
Attachment #8594578 -
Attachment is obsolete: true
Attachment #8595298 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
Fix other MediaRecorder testcase failed. tryserver: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fab282e6dd3
Attachment #8595298 -
Attachment is obsolete: true
Attachment #8595776 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4803c84d4976
Keywords: checkin-needed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4803c84d4976
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Backed out this bug and bug 1140995 for causing bug 1157654.
Status: RESOLVED → REOPENED
Flags: needinfo?(bechen)
Resolution: FIXED → ---
Assignee | ||
Comment 18•9 years ago
|
||
Rebase. try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53da71ef6339
Attachment #8595776 -
Attachment is obsolete: true
Flags: needinfo?(bechen)
Attachment #8602671 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/958cafbaad72
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/958cafbaad72
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•