Closed Bug 919051 Opened 11 years ago Closed 11 years ago

Media Recording - memory leak when record a media stream without any tracks

Categories

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

Product:
Core
Component:
Audio/Video: Recording
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla27
Project Flags:
blocking-b2g -

People

(Reporter: jruderman, Assigned: rlin)

Details

(Keywords: memory-leak, testcase)

Attachments

(3 files, 2 obsolete files)

testcase
293 bytes, text/html
Details
what leaked?
1.51 KB, text/plain
Details
check-in patch
3.95 KB, patch
Details | Diff | Splinter Review
Attached file testcase — 
1. Run a debug build with XPCOM_MEM_LEAK_LOG=2
2. Load the testcase
3. Quit

Result: trace-refcnt reports leaked MediaStream and other objects.
Attached file what leaked? — 
Assignee: nobody → rlin

    
    

    
  
The Media Recorder get the media stream object without tracks and enter recording state.
I will fix it.

    
    

    
  
FWIW - this is good candidate to get a mochitest with btw. You could just create a test that models the exact test case included with an additional check that the InvalidStateError fires when calling start a 2nd time.

    
    

    
  
I will let media recorder throw the InvalidStateError exception when we got this kind of source.
BTW, correct the comment 2, now it failed on principal check (no source can be checked) and throw security error without destroy the TrackUnionStream object, this unclear message would confuse UA.
Summary: MediaRecorder leak → Media Recording - memory leak when record a media stream without any tracks

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v1 (obsolete)
        — 
          — Splinter Review
      

    


  
patch for this bug, also include the test case.

        Attachment #809795 -
        Flags: review?(roc)

        Attachment #809795 -
        Flags: review?(jsmith)

    
    

    
  
Comment on attachment 809795 [details] [diff] [review]
patch v1

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

review+ with nits

::: content/media/test/test_mediarecorder_record_nosrc.html
@@ +5,5 @@
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +  <script type="text/javascript" src="manifest.js"></script>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=897776">Mozill

Nit - the href link should point to bug 919051

@@ +14,5 @@
> +function startTest() {
> +  var mediastream = document.createElement('video').mozCaptureStream();
> +
> +  var mr = new MediaRecorder(mediastream);
> +  ok(true, 'create MediaRecorder');

Nit - I would make this an info statement

        Attachment #809795 -
        Flags: review?(jsmith) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        check-in patch (obsolete)
        — 
          — Splinter Review
      

    


  
Check in patch, carry reviewer, merge with today's mc change.
try result
https://tbpl.mozilla.org/?tree=Try&rev=13fbf4296439

    
    

    
  

      
      
      
      

        Attached patch
          
          
        check-in patch
        — 
          — Splinter Review
      

    


  
this one, remove two useless header include.

        Attachment #811036 -
        Attachment is obsolete: true

        Attachment #809795 -
        Attachment is obsolete: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/021e1b5e8cab
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

    
    

    
  
Verified through successful landing of mochitest.
Status: RESOLVED → VERIFIED

    
    

    
  
bug 924724, which is koi+, depends on the change here. Need to uplift this patch
blocking-b2g: --- → koi?

    
    

    
  
(In reply to C.J. Ku[:CJKu] from comment #12)
> bug 924724, which is koi+, depends on the change here. Need to uplift this
> patch

That shouldn't influence if this bug is a blocker or not. What you need to do on bug 924724 is build a branch-specific patch to fix the issue independent of this bug.

I don't think this bug is a blocker either - it's not a likely use case a developer will do & memory leak impact isn't critical.
blocking-b2g: koi? → -

    
    

    
  
If the branch-specific patch ends up being "roll these changes into that patch", it would be preferable to just land these changes under this bug number. Also, we have a lot of historical precedent for "blocks a blocker" approvals.
Component: Video/Audio → Video/Audio: Recording
No longer blocks: MediaRecording

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: