Closed Bug 957841 Opened 10 years ago Closed 10 years ago

MediaRecorder crash [@ mozilla::dom::MediaRecorder::Session::AfterTracksAdded]

Categories

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

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla29
Project Flags:
blocking-b2g 1.4+
Tracking Flags:
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jruderman, Assigned: rlin)

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(4 files, 7 obsolete files)

testcase
355 bytes, text/html
Details
stack
4.53 KB, text/plain
Details
check-in patch p1
5.84 KB, patch
Details | Diff | Splinter Review
check-in patch p2
3.90 KB, patch
Details | Diff | Splinter Review
Attached file testcase — 
      No description provided.
Attached file stack — 
Nightly: bp-792b58f2-d6bd-4101-aa99-b8bbf2140108
Crash Signature: [@ mozilla::dom::MediaRecorder::Session::AfterTracksAdded]
Assignee: nobody → rlin
blocking-b2g: --- → 1.4?
Keywords: regression
AfterTracksAdded notify to an instance that already stopped.
Attached patch patch v1 (obsolete) — — Splinter Review 
Add more check for this problem, would add test on another patch.
Attachment #8359129 - Flags: review?(roc)
Attached patch patch v1.1 (obsolete) — — Splinter Review 
Found a extra space line.. remove it.
Attachment #8359129 - Attachment is obsolete: true
Attachment #8359129 - Flags: review?(roc)
Attachment #8359132 - Flags: review?(roc)
Comment on attachment 8359132 [details] [diff] [review]
patch v1.1

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

::: content/media/MediaRecorder.cpp
@@ +323,5 @@
>        return;
>      }
> +    // 1. media stream is ready but has been issued stop command
> +    // 2. mediaRecorder call start stop start in sequence.
> +    //    This notify would invoke to a shutdown instance.

This is difficult to understand.

Can you explain specifically how mTrackUnionStream would have become null?
Attachment #8359132 - Flags: review?(roc) → review-
MediaRecorder creates a session object and register trackChange notify. 
If MediaRecorder call the stop method follow by start method, it would invoke the Session::CleanupStreams try to destroy mTrackUnionStream object.
But media stream still fired TrackChange notify to session through AfterTracksAdded method.
Originally check would be success
    if (mRecorder->mState == RecordingState::Inactive) {
      DoSessionEndTask(NS_OK);
      return;
    }
But this test case just change the recorder status to be recording. So this check is fail and cause 
crash on the follow statement
  mTrackUnionStream->AddListener(mEncoder);
Note - we should get a mochitest written for this bug.
Comment on attachment 8359132 [details] [diff] [review]
patch v1.1

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

::: content/media/MediaRecorder.cpp
@@ +323,5 @@
>        return;
>      }
> +    // 1. media stream is ready but has been issued stop command
> +    // 2. mediaRecorder call start stop start in sequence.
> +    //    This notify would invoke to a shutdown instance.

OK, instead of checking mRecorder->mState, which could be wrong for us since it might be for another session, I think we should *only* check !mTrackUnionStream.
Attached patch patch v2 (obsolete) — — Splinter Review 
Test case would create in another one.
Attachment #8359132 - Attachment is obsolete: true
Attachment #8360435 - Flags: review?(roc)
Attached patch test case v1 (obsolete) — — Splinter Review 

        Attachment #8360883 -
        Flags: review?(jsmith)

    
    

    
  
Comment on attachment 8360883 [details] [diff] [review]
test case v1

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

review+ with comments addressed

::: content/media/test/test_mediarecorder_record_startstopstart.html
@@ +7,5 @@
> +</head>
> +<body>
> +<pre id="test">
> +<div id="content" style="display: none">
> +  <audio id="testAudio"></audio>

Nit - this isn't needed

@@ +27,5 @@
> +    is(recorder.stream, dest.stream,
> +       'Media recorder stream = element stream post recording');
> +    stopCount++;
> +    if (stopCount == 2) {
> +      is(2, dataavailable, 'Should has two dataavailable event');

Given the time dependency seen in the patch with setTimeout, I'd caution doing this is statement check, as the setTimeout call makes this test run a bit more unpredictably. We could get intermittent failure from this.

@@ +55,5 @@
> +  recorder.onwarning = function() {
> +    ok(false, 'onwarning unexpectedly fired');
> +  };
> +
> +  recorder.start(2000);

We should also check that the recording state is recording here.

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test case v2 (obsolete)
        — 
          — Splinter Review
      

    


  
test case v2, avoid to use setTimeout.

        Attachment #8360883 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Found problem when 2nd start calls.
The session object would trigger media recorder stop method when 2nd start calls.

        Attachment #8360435 -
        Attachment is obsolete: true

        Attachment #8362743 -
        Flags: review?(roc)

        Attachment #8362739 -
        Flags: review?(jsmith)

    
    

    
  
Comment on attachment 8362739 [details] [diff] [review]
test case v2

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

r- for the additional unnecessary audio file playback

::: content/media/test/test_mediarecorder_record_startstopstart.html
@@ +24,5 @@
> +  recorder.onstop = function (e) {
> +    info('onstop fired');
> +    if (startIssued == 0) {
> +      // the 2nd start may call before this onstop event.
> +      is(recorder.state, 'inactive', 'check recording status is inactive');

This is already checked elsewhere, so this isn't necessary.

@@ +46,5 @@
> +     info('blob size = ' + evt.data.size);
> +     is(evt.data.type, expectedMimeType,
> +       'Blob data received should have type = ' + expectedMimeType);
> +    } else {
> +    is(evt.data.type, '',

Nit - indentation

@@ +66,5 @@
> +  recorder.start(10000); // This bug would crash on this line without this fix.
> +  startIssued = 1;
> +  is(recorder.state, 'recording', 'check recording status is recording');
> +  // Simulate delay stop, only delay stop no no stop can trigger crash.
> +  var element = document.createElement('audio');

I don't understand what this is trying to do. It looks like it's playing a different file to simulate time passing & then calling stop after the file finishes being played back. Why can't you call stop immediately here?

        Attachment #8362739 -
        Flags: review?(jsmith) → review-

    
    

    
  
Hi Jason, If we prefer to avoid use the setTimeOut, Is any better way to delay the execution?

    
    

    
  
(In reply to Randy Lin [:rlin] from comment #16)
> Hi Jason, If we prefer to avoid use the setTimeOut, Is any better way to
> delay the execution?

Probably not. Sounds like we're stuck using setTimeout then.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test case v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Remove audio element, remove useless status check.

        Attachment #8362805 -
        Flags: review?(jsmith)

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

        Attachment #8362739 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8362743 [details] [diff] [review]
patch v3

Hi Roc, still waiting for reviewing this patch  :)
blocking-b2g: 1.4? → 1.4+

        Attachment #8362743 -
        Attachment is obsolete: true

        Attachment #8362805 -
        Attachment is obsolete: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/6440ae81d003
https://hg.mozilla.org/mozilla-central/rev/82ad4f431104
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

    
    

    
  
Verified per mochitest landing cleanly.
Status: RESOLVED → VERIFIED
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: