Closed Bug 1153690 Opened 5 years ago Closed 4 years ago

Leak with MediaSource and MediaRecorder

Categories

(Core :: Audio/Video: Recording, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jruderman, Assigned: bechen)

References

(Blocks 3 open bugs)

Details

(Keywords: memory-leak, testcase)

Attachments

(4 files, 4 obsolete files)

Attached file w.html
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.
I'll take a first look at this.
Flags: needinfo?(continuation)
There's a DETH that is holding alive the window.  rc=1, no known reference.  No other information on it.
Attached file refcount log
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.
Here's the refcount log for the MediaRecorder.  I'm not sure what is going on yet.
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)
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)
(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)
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.
Attached patch bug-1153690.v01.patch (obsolete) — Splinter Review
Attachment #8592735 - Flags: review?(roc)
Attached patch bug-1153690.v01.patch (obsolete) — Splinter Review
r=roc

tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=083e224d3457
Attachment #8592735 - Attachment is obsolete: true
Attachment #8594578 - Flags: review+
(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
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
Attached patch bug-1153690.v02.patch (obsolete) — Splinter Review
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+
Attached patch bug-1153690.v02.patch (obsolete) — Splinter Review
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+
Keywords: checkin-needed
Blocks: 1157580
https://hg.mozilla.org/mozilla-central/rev/4803c84d4976
Status: NEW → RESOLVED
Closed: 5 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 → ---
Depends on: 1157654
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/958cafbaad72
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.