The Media Recorder API crash when we do a lot of stop/start and we destroy the Session. It also leaks a Listener

RESOLVED FIXED in Firefox 52

Status

()

Core
Audio/Video: Recording
P1
normal
Rank:
15
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: Pierre Brochard, Assigned: Pierre Brochard)

Tracking

Trunk
mozilla53
All
Unspecified
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox-esr45 ?)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

4 months ago
Created attachment 8822543 [details] [diff] [review]
MediaRecorder.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160823121617

Steps to reproduce:

When we do a lot of stop/start in MediaRecorder associated with a Webcam via getUserMedia, the memory and number of thread grows too much (can crash FF very quickly).
The ShutdownObserver kill the Sessions only when the navigator exit (too late).

So I modify dom/media/MediaRecorder.cpp to purge the Sessions sooner, just by calling :
    nsContentUtils::UnregisterShutdownObserver(this);
in Session::Stop() method.

And with that FF crashed.

With valgrind I saw that a videoTracks[0]->AddDirectListener was not removed in CleanupStreams causing an access by the stream to memory which was released by MediaRecorder::~Session, so ...

I just added in  CleanupStreams :
    if(mEncoder) {
        nsTArray<RefPtr<mozilla::dom::VideoStreamTrack>> videoTracks;
        DOMMediaStream* domStream = mRecorder->Stream();
        if (domStream) {
          domStream->GetVideoTracks(videoTracks);
          if (!videoTracks.IsEmpty()) {
            videoTracks[0]->RemoveDirectListener(mEncoder->GetVideoSink());
          }
        }
      }

And it works. And the threads and memory are closed sooner and all the problems disappear. The memory and the threads don't grow any more and no crash appears.



Actual results:

Too much memory and thread. Or crash if free sooner.


Expected results:

No crash and no excessive consumption of memory and threads.
With that a site could crash FF very quickly with too much memory consumption.
(Assignee)

Updated

4 months ago
Hardware: Unspecified → All
Version: 48 Branch → 51 Branch

Updated

4 months ago
Component: Untriaged → Audio/Video: Recording
Product: Firefox → Core

Updated

4 months ago
Attachment #8822543 - Flags: review?(jolin)
Comment on attachment 8822543 [details] [diff] [review]
MediaRecorder.patch

Review of attachment 8822543 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits - patch needs to be corrected and re-uploaded before landing.

This patch must be against dom/media/MediaRecorder.cpp, not MediaRecorder.cpp.save (in the root directory of the repo).

The comment for the patch should not be the content of the bug report.  It should include the bug #, and describe the patch and what it fixes:

Bug 1326311: Cleanup MediaRecorder shutdown r?jesup
Clean up handling of ShutdownObservers in MediaRecorder, and ensure DirectTrackListeners are removed.

::: MediaRecorder.cpp.save
@@ +519,4 @@
>        // End the Session directly if there is no ExtractRunnable.
>        DoSessionEndTask(NS_OK);
>      }
> +    nsContentUtils::UnregisterShutdownObserver(this);

The Shutdown observer is set up in InitEncoder(), which is called from Start() (for WebAudio), or indirectly in a callback via OnTracksAvailable().  If we call Stop() before the OnTracksAvailable has fired, then it sets mStopIssued, and when TracksAvailable is called, it will exit without setting up a ShutdownObserver.

So this works... but it's quite non-obvious that it works.  A comment as to why this works (similar to above) is needed, and probably a comment in TracksAvailable where it checks mStopIssued.

@@ +598,4 @@
>        mReadThread = nullptr;
>        // Inside the if() so that if we delete after xpcom-shutdown's Observe(), we
>        // won't try to remove it after the observer service is shut down.
> +    //  nsContentUtils::UnregisterShutdownObserver(this);

We need to still do this if Stop() was never called before the Session is destroyed.  Since Observers do *not* hold a reference, we must be certain they don't refer to anything that has been deleted, and after this, the Session object is gone.  Also, RemoveObserver(thing-not-in-observer-array) is safe.

You could add a comment // Unregistering for safety in case Stop() was never called.

@@ +880,4 @@
>      mInputPorts.Clear();
>  
>      if (mTrackUnionStream) {
> +      if(mEncoder) {

if (mEncoder) {
(space after 'if')
Attachment #8822543 - Flags: review?(jolin) → review+

Updated

4 months ago
Assignee: nobody → epinal99-bugzilla2
Status: UNCONFIRMED → NEW
Rank: 15
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → ?
Ever confirmed: true
Priority: -- → P1
Version: 51 Branch → Trunk

Comment 2

4 months ago
I'm not the assignee. :)
Assignee: epinal99-bugzilla2 → pierre.brochard.1982
(Assignee)

Comment 3

4 months ago
Created attachment 8822623 [details] [diff] [review]
MediaRecorder.patch

I need to retest it with my use case.
(Assignee)

Comment 4

4 months ago
(In reply to Randell Jesup [:jesup] from comment #1)
> Comment on attachment 8822543 [details] [diff] [review]
> MediaRecorder.patch
> 
> Review of attachment 8822543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits - patch needs to be corrected and re-uploaded before landing.
> 
> This patch must be against dom/media/MediaRecorder.cpp, not
> MediaRecorder.cpp.save (in the root directory of the repo).
> 
OK. I do it quickly and "by hand"

> The comment for the patch should not be the content of the bug report.  It
> should include the bug #, and describe the patch and what it fixes:
> 
> Bug 1326311: Cleanup MediaRecorder shutdown r?jesup
> Clean up handling of ShutdownObservers in MediaRecorder, and ensure
> DirectTrackListeners are removed.
> 
> ::: MediaRecorder.cpp.save
> @@ +519,4 @@
> >        // End the Session directly if there is no ExtractRunnable.
> >        DoSessionEndTask(NS_OK);
> >      }
> > +    nsContentUtils::UnregisterShutdownObserver(this);
> 
> The Shutdown observer is set up in InitEncoder(), which is called from
> Start() (for WebAudio), or indirectly in a callback via OnTracksAvailable().
> If we call Stop() before the OnTracksAvailable has fired, then it sets
> mStopIssued, and when TracksAvailable is called, it will exit without
> setting up a ShutdownObserver.
> 

Yes. So I can add a nsContentUtils::RegisterShutdownObserver(mSession);
in the case mSession->mStopIssued in NotifyTracksAvailable

A better way can be to set a flag or a incremental value ?
There is also the case of Errors in NotifyTracksAvailable.

> So this works... but it's quite non-obvious that it works.  A comment as to
> why this works (similar to above) is needed, and probably a comment in
> TracksAvailable where it checks mStopIssued.

OK. This case should be handle.

> 
> @@ +598,4 @@
> >        mReadThread = nullptr;
> >        // Inside the if() so that if we delete after xpcom-shutdown's Observe(), we
> >        // won't try to remove it after the observer service is shut down.
> > +    //  nsContentUtils::UnregisterShutdownObserver(this);
> 
> We need to still do this if Stop() was never called before the Session is
> destroyed.  Since Observers do *not* hold a reference, we must be certain
> they don't refer to anything that has been deleted, and after this, the
> Session object is gone.  Also, RemoveObserver(thing-not-in-observer-array)
> is safe.
> 

OK I added the comment.

> You could add a comment // Unregistering for safety in case Stop() was never
> called.
> 
> @@ +880,4 @@
> >      mInputPorts.Clear();
> >  
> >      if (mTrackUnionStream) {
> > +      if(mEncoder) {
> 

OK

> if (mEncoder) {
> (space after 'if')
(Assignee)

Comment 5

4 months ago
I have tested my last patch with my use case and it works on the version 51.0b9.

I do a lot of Javascript stop/start (one couple every 100 ms) with the MediaRecorder on a getUserMedia and a Webcam in HD (video + audio).
I record the blobs on a serveur. I can replay then with a little work in Javascript.

I also directly read the video in a video tag in parallel.

I don't test the case where we do a stop without a start. But i shouldn't work less good than before.

Without this patch, the above don't work as explained. Before version 49, it works with increasing threads not deleted. After version 49, it works with increasing threads AND memory not deleted in time. It quickly crash.
(Assignee)

Comment 6

4 months ago
FF esr45 is affected as FF 47 or 48 :
It works with increasing threads not deleted. 1 thread with each stop/start couple.
With 100 ms, you have 36000 threads in 1 hour. With a good machine, it works but...

After and with FF 49, the memory also grows so the machine quickly become unusable.

I can do more test in other conditions : stop/stop or without stop or ...

I can test with other format than webm (ogg or mp4).

My webcam is HD.
(Assignee)

Comment 7

4 months ago
(In reply to Randell Jesup [:jesup] from comment #1)

Can you see if my new patch meets your requirements ? It works with my use case (i have the 51.0b9 compiled in debug mode and tested with valgrind for my use case). The result is stable in memory and number of threads and don't crash.

For the MediaRecorder, I have a simple test case I should extract from my bigger application if somebody is interested with it. But may be there is also somewhere this simple case.

Best regards.
(Assignee)

Updated

4 months ago
Attachment #8822623 - Flags: review?(rjesup)
Comment on attachment 8822623 [details] [diff] [review]
MediaRecorder.patch

Review of attachment 8822623 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaRecorder.cpp
@@ +308,4 @@
>      virtual void NotifyTracksAvailable(DOMMediaStream* aStream)
>      {
>        if (mSession->mStopIssued) {
> +        // Because the Stop method Unregistered it. 

nit: delete trailing spaces before checkin, please.

@@ +308,5 @@
>      virtual void NotifyTracksAvailable(DOMMediaStream* aStream)
>      {
>        if (mSession->mStopIssued) {
> +        // Because the Stop method Unregistered it. 
> +        nsContentUtils::RegisterShutdownObserver(mSession);

No, this isn't needed -- see my comment.  "So this works - but it's non-obvious why it works".

See my original comments - you need to add comments, and maybe a MOZ_ASSERT() somewhereto ensure no one breaks the assumptions here (or even MOZ_RELEASE_ASSERT(), if you're worried about the dangers of something unexpected in the wild, or think it wouldn't get good testing in debug builds.
Attachment #8822623 - Flags: review?(rjesup) → review-
(Assignee)

Comment 9

4 months ago
Created attachment 8823584 [details] [diff] [review]
MediaRecorder.patch
Attachment #8823584 - Flags: review?(rjesup)
Comment on attachment 8823584 [details] [diff] [review]
MediaRecorder.patch

Review of attachment 8823584 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8823584 - Flags: review?(rjesup) → review+
(Assignee)

Comment 11

4 months ago
(In reply to Randell Jesup [:jesup] from comment #10)
> Comment on attachment 8823584 [details] [diff] [review]
> MediaRecorder.patch
> 
> Review of attachment 8823584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!

What should I do next ?
The instruction I received from bugzilla are :
...
"Once the patch is correctly formatted and all the nits are addressed,
you should ask the person who reviewed your patch to push it to the
try server for testing.

Once it comes back passing all our tests, your patch will be ready
to check in!  To get your patch checked in you can add "checkin-needed"
to the Keywords field on the bug summary page, and one of our helpful
Code Sheriffs will come by to merge it into the Mozilla codebase."...

Do you have time to test with the try server ?

Do I have to do more test ?

I have tested it with the 51.0b11 on one of my machines with a webcam. It works with lots of stop/start or requestData. I monitor the memory and the threads and it is ok (plus debug mode and MOZ_LOG debug for mediarecorder). I have also tested with intervals of 100ms or 5000ms.

I can test with the 52 and 53 if you want it.

Updated

4 months ago
Attachment #8822543 - Attachment is obsolete: true

Updated

4 months ago
Attachment #8822623 - Attachment is obsolete: true
Try build:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc1cad9c48f49b74cd01c556eb61cab69401853d
(Assignee)

Comment 13

4 months ago
(In reply to Randell Jesup [:jesup] from comment #12)
> Try build:
>  https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=cc1cad9c48f49b74cd01c556eb61cab69401853d

In the try test you launch, we have :
-------------------

TEST-START | dom/media/test/test_mediarecorder_record_audionode.html
[Parent 1616] WARNING: pipe error (68): Connection reset by peer: file /home/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 346
[Parent 1616] WARNING: pipe error (64): Connection reset by peer: file /home/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 346
###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0084,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

So I think no audio source ?
It leads to a segfault in :
--------------------------

Thread 34 (crashed)
 0  libxul.so!nsObserverService::RemoveObserver [nsObserverService.cpp:cc1cad9c48f4 : 233 + 0x0]
    rax = 0x000000000061ea18   rdx = 0x00007f1d15628cf7
    rcx = 0x00007f1d1562f207   rbx = 0x00007f1cf4a185e0
    rsi = 0x00007f1ced015550   rdi = 0x00007f1d16c8e310
    rbp = 0x00007f1ce7e54c20   rsp = 0x00007f1ce7e54be0
     r8 = 0x0000000000000008    r9 = 0x0000000000000008
    r10 = 0x00007f1d21100299   r11 = 0x0000000000000000
    r12 = 0x00007f1cf4a185f8   r13 = 0x00007f1d0ec6df10
    r14 = 0x00007f1d15628cf7   r15 = 0x00007f1ce7e54daf
    rip = 0x00007f1d12fa4e25
    Found by: given as instruction pointer in context
 1  libxul.so!nsContentUtils::UnregisterShutdownObserver [nsContentUtils.cpp:cc1cad9c48f4 : 4228 + 0x10]
    rbx = 0x00007f1cf4a185e0   rbp = 0x00007f1ce7e54c50
    rsp = 0x00007f1ce7e54c30   r12 = 0x00007f1cf4a185f8
    r13 = 0x00007f1cf4a18630   r14 = 0x00007f1ce7e54d28
    r15 = 0x00007f1ce7e54daf   rip = 0x00007f1d137c7158
    Found by: call frame info
 2  libxul.so!mozilla::dom::MediaRecorder::Session::~Session [MediaRecorder.cpp:cc1cad9c48f4 : 603 + 0x8]
    rbx = 0x00007f1cf4a185e0   rbp = 0x00007f1ce7e54c80
    rsp = 0x00007f1ce7e54c60   r12 = 0x00007f1cf4a185f8
    r13 = 0x00007f1cf4a18630   r14 = 0x00007f1ce7e54d28
    r15 = 0x00007f1ce7e54daf   rip = 0x00007f1d13f570ce
    Found by: call frame info
 3  libxul.so!mozilla::dom::MediaRecorder::Session::~Session [MediaRecorder.cpp:cc1cad9c48f4 : 605 + 0x5]
    rbx = 0x00007f1cf4a185e0   rbp = 0x00007f1ce7e54ca0
    rsp = 0x00007f1ce7e54c90   r12 = 0x00007f1cf4a185f8
    r13 = 0x00007f1ce7e54d20   r14 = 0x00007f1ce7e54d28
    r15 = 0x00007f1ce7e54daf   rip = 0x00007f1d13f57182
    Found by: call frame info
 4  libxul.so!mozilla::dom::MediaRecorder::Session::Release [MediaRecorder.cpp:cc1cad9c48f4 : 950 + 0x9]
    rbx = 0x00007f1cf4a185e0   rbp = 0x00007f1ce7e54cc0
    rsp = 0x00007f1ce7e54cb0   r12 = 0x00007f1cf4a185f8
    r13 = 0x00007f1ce7e54d20   r14 = 0x00007f1ce7e54d28
    r15 = 0x00007f1ce7e54daf   rip = 0x00007f1d13f45d32
    Found by: call frame info
 5  libxul.so!mozilla::dom::MediaRecorder::Session::ExtractRunnable::~ExtractRunnable [MediaRecorder.cpp:cc1cad9c48f4 : 270 + 0x5]
    rbx = 0x00007f1cf3fb1400   rbp = 0x00007f1ce7e54ce0
    rsp = 0x00007f1ce7e54cd0   r12 = 0x0000000000000000
    r13 = 0x00007f1ce7e54d20   r14 = 0x00007f1ce7e54d28
    r15 = 0x00007f1ce7e54daf   rip = 0x00007f1d13f532d2
    Found by: call frame info

So may be my first patch was right in not doing a UnregisterShutdownObserver in ~Session.
It seems not good in this pathological case.
Is it really harmless to unregister a no more existent reference ?

In my testcase, it does nothing bad.
(In reply to Pierre Brochard from comment #13) 
> In the try test you launch, we have :
> -------------------

> So I think no audio source ?
> It leads to a segfault in :
> --------------------------
> 
> Thread 34 (crashed)
>  0  libxul.so!nsObserverService::RemoveObserver
> [nsObserverService.cpp:cc1cad9c48f4 : 233 + 0x0]


> So may be my first patch was right in not doing a UnregisterShutdownObserver
> in ~Session.
> It seems not good in this pathological case.
> Is it really harmless to unregister a no more existent reference ?
> 
> In my testcase, it does nothing bad.

It is harmless... but that line points to NS_ENSURE_VALIDCALL, which checks if we're on MainThread (thread 1), and this was called from thread 54.  Note the comment:
  // Only DestroyRunnable is allowed to delete Session object.
  virtual ~Session()

Looking at it, there are bad race conditions in ExtractRunnable and DestroyRunnable where they could spawn a DestroyRunnable to release the Session on MainThread, but then it might run before the code that sent it exits.  In this case, the ExtractRunnable (or DestroyRunnable) running on non-MainThread holds the last reference still, and so that's where the crash happens. 

The right solution to that is to use mSession.forget() when spawning the DestroyRunnables, handing over out existing reference to the code that will drop it on mainthread, causing it to be deleted.  

I'll give that a try
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=57d4972efaa1ca0553d7c43bcd4f0f06123813f4
(Assignee)

Comment 16

4 months ago
(In reply to Randell Jesup [:jesup] from comment #15)
>  https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=57d4972efaa1ca0553d7c43bcd4f0f06123813f4

OK, your patch and test seems good. I will test it tomorrow on my use case just to be sure but it should work.
(In reply to Randell Jesup [:jesup] from comment #15)
>  https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=57d4972efaa1ca0553d7c43bcd4f0f06123813f4

Try is green now.  I'll get my addition reviewed and land both.  Thanks Pierre!  Good work.
Summary: The Media Recorder API crash when we do a lot of stop/start and we destroy the Session. There is one Listener of the video tracks which is not closed when we kill the Session read thread. I have corrected it and it works. → The Media Recorder API crash when we do a lot of stop/start and we destroy the Session. It also leaks a Listener
Created attachment 8824620 [details] [diff] [review]
Avoid races in destroying MediaRecorder sessions - destroy on MainThread

MozReview-Commit-ID: 3DA4HxUuzje
Attachment #8824620 - Flags: review?(jib)

Updated

4 months ago
Assignee: pierre.brochard.1982 → rjesup
Status: NEW → ASSIGNED
Comment on attachment 8824620 [details] [diff] [review]
Avoid races in destroying MediaRecorder sessions - destroy on MainThread

Review of attachment 8824620 [details] [diff] [review]:
-----------------------------------------------------------------

mSession.forget() clears mSession, but none of the code-paths dereference mSession after this, so lgtm. Might be worth a comment though to not trip up future modifications.
Attachment #8824620 - Flags: review?(jib) → review+

Updated

4 months ago
Assignee: rjesup → pierre.brochard.1982
Pierre - let me know when you've tried it locally, and I'll land the patches, and probably request uplift to Aurora a bit after that. Thanks!
Flags: needinfo?(pierre.brochard.1982)
(Assignee)

Comment 21

4 months ago
(In reply to Randell Jesup [:jesup] from comment #20)
> Pierre - let me know when you've tried it locally, and I'll land the
> patches, and probably request uplift to Aurora a bit after that. Thanks!

I have tried it and it works very well with the latest 51 version. The log messages with MOZ_LOG=MediaRecorder:4,timestamp seems also OK. The Sessions are cleaned by DestroyRunnable every 100ms and during long times.

After uplift to Aurora, will you request also for 51 and 52 version ?

I think it is useful patches because without, it can leads to crash all versions of FF.

Thanks for your help. It is useful for my usages and I think it could also help others projects.

Comment 22

4 months ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8600fe754cc1
Fix thread and Track listener leaks starting and stopping MediaRecorder r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/901b049ef121
Avoid races in destroying MediaRecorder sessions - destroy on MainThread r=jib

Comment 23

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8600fe754cc1
https://hg.mozilla.org/mozilla-central/rev/901b049ef121
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8823584 [details] [diff] [review]
MediaRecorder.patch

Review of attachment 8823584 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaRecorder.cpp
@@ +889,5 @@
> +        DOMMediaStream* domStream = mRecorder->Stream();
> +        if (domStream) {
> +          domStream->GetVideoTracks(videoTracks);
> +          if (!videoTracks.IsEmpty()) {
> +            videoTracks[0]->RemoveDirectListener(mEncoder->GetVideoSink());

If this cleans something up...

@@ +898,3 @@
>        // Sometimes the MediaEncoder might be initialized fail and go to
>        // |CleanupStreams|. So the mEncoder might be a nullptr in this case.
>        if (mEncoder && mSelectedVideoTrackID != TRACK_NONE) {

...it means that mSelectedVideoTrackID == TRACK_NONE here. How could that be? Figuring that out would have been the appropriate fix here.

I'll try to take care of this in bug 1296531. Pierre, please do follow that bug so you can retest your case when it lands. Or if you have a reproducible testcase..
(Assignee)

Comment 25

4 months ago
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #24)
> Comment on attachment 8823584 [details] [diff] [review]
> MediaRecorder.patch
> 
> Review of attachment 8823584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaRecorder.cpp
> @@ +889,5 @@
> > +        DOMMediaStream* domStream = mRecorder->Stream();
> > +        if (domStream) {
> > +          domStream->GetVideoTracks(videoTracks);
> > +          if (!videoTracks.IsEmpty()) {
> > +            videoTracks[0]->RemoveDirectListener(mEncoder->GetVideoSink());
> 
> If this cleans something up...

Yes. Without it segfault very easily with my generic usecase and we can see it with valgrind as I said in the user story.
There was one bug behind an other (and a third with the Jesup patch). The fix is a simple one. It may be cleaned.

> 
> @@ +898,3 @@
> >        // Sometimes the MediaEncoder might be initialized fail and go to
> >        // |CleanupStreams|. So the mEncoder might be a nullptr in this case.
> >        if (mEncoder && mSelectedVideoTrackID != TRACK_NONE) {
> 
> ...it means that mSelectedVideoTrackID == TRACK_NONE here. How could that
> be? Figuring that out would have been the appropriate fix here.
> 

You are probably right. I didn't write this code. The code is not very big but not as simple as it seems to be.
May be the track is half initialized in this case ? ie the track exist in a bad state. I didn't track if this is possible.

> I'll try to take care of this in bug 1296531. Pierre, please do follow that
> bug so you can retest your case when it lands. Or if you have a reproducible
> testcase..

OK, I will test and track your bug.
I have a reproductible test case but it is part of a bigger application and I have begun to extract parts of it.
I will give it as soon as I have time for that. It is a simple but generic usage of the MediaRecorder API.
(Assignee)

Comment 26

4 months ago
Created attachment 8825360 [details]
Test case for MediaRecorder on getUserMedia

Test case for the bug. Can be used by hand or included in tests cases.
Work locally or on a web server.
Some parameters can be changed by hand (at the end of the file).
Attachment #8825360 - Flags: review?(rjesup)
Attachment #8825360 - Flags: review?(pehrson)
(Assignee)

Updated

4 months ago
Flags: needinfo?(pierre.brochard.1982)
Comment on attachment 8825360 [details]
Test case for MediaRecorder on getUserMedia

Great to have a test that we can run manually. To land we'd need to convert it to a crashtest or a mochitest however. I'll try to use this in my work on bug 1296531.
Attachment #8825360 - Flags: review?(pehrson)
(Assignee)

Comment 28

4 months ago
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #27)
> Comment on attachment 8825360 [details]
> Test case for MediaRecorder on getUserMedia
> 
> Great to have a test that we can run manually. To land we'd need to convert
> it to a crashtest or a mochitest however. I'll try to use this in my work on
> bug 1296531.

OK. We should look at memory and thread usage when the html page is loaded.
A good test is to parse the calls to ~Session with setting env 'MOZ_LOG=MediaRecorder:4,timestamp'. They should appear each MediaRecorder.stop ie 100ms in the default params of the test.
You can also do a 'ps -aefL|grep firefox' on Linux machines and see if it increases too much (it should be stable). We can also test directly the RSS memory usage.
The page can be read as a file or http uri.

Updated

4 months ago
Attachment #8825360 - Attachment is patch: true
Attachment #8825360 - Attachment mime type: text/html → text/plain

Updated

4 months ago
Attachment #8825360 - Attachment is patch: false
Attachment #8825360 - Attachment mime type: text/plain → text/html
Too late for 51. Mark 51 won't fix.
status-firefox51: affected → wontfix
Should we nominate for 52/aurora?
Flags: needinfo?(pehrson)

Updated

3 months ago
Attachment #8825360 - Flags: review?(rjesup) → feedback+
Sounds like a good idea. Is this related to the leak after the export finished per the original description in bug 1329079 comment 0?
Flags: needinfo?(pehrson) → needinfo?(rjesup)
(Assignee)

Comment 32

3 months ago
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #31)
> Sounds like a good idea. Is this related to the leak after the export
> finished per the original description in bug 1329079 comment 0?

I try to do the test of the "original description in bug 1329079 comment 0" with Firefox 52 patched with the above patches and I do not see leaks (about:memory and continuous top on my linux 64 bits redhat machine).

So it may be connected as we have here a memory and thread leak with the mediarecorder connected with a video source.

You can do another test with the last Firefox 53 which have the patches.

Hope that can help.
(Assignee)

Comment 33

3 months ago
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #31)
> Sounds like a good idea. Is this related to the leak after the export
> finished per the original description in bug 1329079 comment 0?

More tests with test 1329079 comment 0 :
52.0a2 (2017-01-21) (64 bits) WITHOUT the above patches
about:memory
...
    0.00 MB ?? shmem-allocated
5,274.09 MB ?? shmem-mapped
=> The bug is visible (quickly after download panel).

53.0a1 (2017-01-10) (64 bits) which HAS the above patches
about:memory
...
0.00 MB ?? shmem-allocated
3.53 MB ?? shmem-mapped
=> No more leaks.

With the last 52.0a2 patched by me, there is also NO bug as I said in the comment 32.

So I think bug 1329079 comment 0 is corrected by the above patches.
Thanks Pierre! Excellent news.

Randell, can you request uplift? You've been more involved than me here.
Comment on attachment 8823584 [details] [diff] [review]
MediaRecorder.patch

Note: for 52, which will be beta after today

Approval Request Comment
[Feature/Bug causing the regression]: N/A

[User impact if declined]: significant leaks using MediaRecorder

[Is this code covered by automated tests?]: No

[Has the fix been verified in Nightly?]: yes

[Needs manual test from QE? If yes, steps to reproduce]: No.  Steps are in bug if needed.

[List of other uplifts needed for the feature/fix]:  The other patch on this bug.

[Is the change risky?]: No.  Should have requested uplift before the merge.

[Why is the change risky/not risky?]: Generally just adds code to release things when done.  Risk is low.

[String changes made/needed]: none
Flags: needinfo?(rjesup)
Attachment #8823584 - Flags: approval-mozilla-beta?
Comment on attachment 8823584 [details] [diff] [review]
MediaRecorder.patch

leak fix, beta52+, should be in b2
Attachment #8823584 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8824620 - Flags: approval-mozilla-beta+

Comment 37

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/cc222ceb5dec
https://hg.mozilla.org/releases/mozilla-beta/rev/3bc9db15d99a
status-firefox52: affected → fixed
Setting qe-verify- per Comment 35.
Flags: qe-verify-

Updated

2 months ago
Duplicate of this bug: 1344124
You need to log in before you can comment on or make changes to this bug.