Closed Bug 957841 Opened 12 years ago Closed 12 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: 12 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: