I found can't get the principal object from mediaStream. Need to fix it.

test case.

can't get principal object from mediaStream bool MediaRecorder::CheckPrincipal() { nsCOMPtr<nsIPrincipal> principal = mStream->GetPrincipal();

Add principal according to :roc's suggestion.

Comment on attachment 779566 [details] [diff] [review] addPrincipal.patch Review of attachment 779566 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp @@ +74,5 @@ > MediaStreamDestinationEngine* engine = new MediaStreamDestinationEngine(this, tus); > mStream = aContext->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM); > mPort = tus->AllocateInputPort(mStream, 0); > + > + mDOMStream->CombineWithPrincipal(aContext->GetParentObject()->GetExtantDoc()->NodePrincipal()); You need to null-check the result of GetExtantDoc. If it's null I guess you just skip this call.

Let jwwang to take this bug. :)

Add null-check the result of GetExtantDoc.

Comment on attachment 779577 [details] [diff] [review] addPrincipal-v2.patch Review of attachment 779577 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp @@ +74,5 @@ > MediaStreamDestinationEngine* engine = new MediaStreamDestinationEngine(this, tus); > mStream = aContext->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM); > mPort = tus->AllocateInputPort(mStream, 0); > + > + nsCOMPtr<nsIDocument> doc = aContext->GetParentObject()->GetExtantDoc(); You can just use nsIDocument* here.

oh, got possible memory leak 1:05.46 Per-Inst Leaked Total Rem Mean StdDev Total Rem Mean StdDev 1:05.46 0 TOTAL 22 200 1012738 11 ( 8667.89 +/- 7578.15) 1265373 1 ( 2747.69 +/- 5558.76) 1:05.46 52 CondVar 32 32 33 1 ( 14.48 +/- 7.43) 0 0 ( 0.00 +/- 0.00) 1:05.46 168 MediaInputPort 40 80 6 2 ( 3.50 +/- 1.58) 6 1 ( 3.27 +/- 1.68) 1:05.46 175 MediaStreamGraph 16 16 1 1 ( 1.00 +/- 0.00) 0 0 ( 0.00 +/- 0.00) 1:05.46 203 Mutex 24 24 497 1 ( 151.97 +/- 72.15) 0 0 ( 0.00 +/- 0.00) 1:05.46 853 nsTArray_base 8 48 505570 6 (15645.28 +/- 3011.14) 0 0 ( 0.00 +/- 0.00)

You probably need to call MediaInputPort::Destroy on the MediaInputPort you create.

Comment on attachment 779663 [details] [diff] [review] test case Review of attachment 779663 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by review. review- mainly because of some cleanup needed here. Also - I'm noticing some common logic were building here. We might want to refactor common setup logic into a separate JS file. However, the refactoring I do not think is a blocker to landing this patch. We could get that in a followup if needed. ::: content/media/test/test_mediarecorder_record_audiocontext.html @@ +25,5 @@ > + source.connect(dest); > + var elem = document.createElement('audio'); > + elem.token = token; > + manager.started(token); > + elem.mozSrcObject = dest.stream; The one thing I'm confused about in this test is how you are making us of the providing test files (in this case, the opus file). Is it ever used here? If not, we should reconsider the test framework/approach being used here. @@ +29,5 @@ > + elem.mozSrcObject = dest.stream; > + mMediaStream = dest.stream; > + source.start(0); > + elem.play(); > + mMediaRecorder = new MediaRecorder(dest.stream); I'd also add a check here to ensure that onwarning, onerror, and onstop does not fire here. You can do that by doing the following: mMediaRecorder.onwarning = function() { ok(false, 'onwarning unexpectedly fired'); }; mMediaRecorder.onerror = function() { ok(false, 'onerror unexpectedly fired'); }; mMediaRecorder.onstop = function() { ok(false, 'onstop unexpectedly fired'); }; @@ +32,5 @@ > + elem.play(); > + mMediaRecorder = new MediaRecorder(dest.stream); > + mMediaRecorder.ondataavailable = function (e) { > + if (mMediaRecorder.state == 'recording') { > + mMediaRecorder.stop(); Since you are already planning to call stop, why not also add a test for onstop here? So do the following in the if statement: ok(e.data.size > 0, 'check blob has data'); mMediaRecorder.onstop = function() { info('onstop fired successfully'); // add checks for the recording state & mimetype here manager.finished(token); }; mMediaRecorder.stop(); @@ +37,5 @@ > + is(e.data.size >0, true, 'check blob has data'); > + manager.finished(token); > + } > + }; > + try { Nit - trailing whitespace. @@ +41,5 @@ > + try { > + mMediaRecorder.start(1000); > + is('recording', mMediaRecorder.state, "check record state recording"); > + } catch (e) { > + is('false', '', "can't record audio context"); Note - you probably instead want this instead: ok(false, 'Can't record audio context'); I would include a call to manager.finished(token) as well.

Thanks Jason's help review. I will modify my test case later.

(In reply to comment #10) > You probably need to call MediaInputPort::Destroy on the MediaInputPort you > create. We do that in MediaStreamAudioDestinationNode::DestroyMediaStream. Am I missing something?

Hi Ehsan, I try to call destroy inputport before destory TUM. It can solve the memory leak problem. Do I miss something? MediaRecorder::~MediaRecorder() { if (mTrackUnionStream) { mInputPort->Destroy(); <---- mTrackUnionStream->Destroy(); } }

(In reply to Randy Lin [:rlin] from comment #14) > Hi Ehsan, > I try to call destroy inputport before destory TUM. > It can solve the memory leak problem. > Do I miss something? > > MediaRecorder::~MediaRecorder() > { > if (mTrackUnionStream) { > mInputPort->Destroy(); <---- > mTrackUnionStream->Destroy(); > } > } That's what I meant --- the port created by MediaRecorder. Please submit this as a proper patch for review :-)

Avoid memory leak after mediaRecorder destroy.

check-in patch, carry reviewer

Hi Ryan, Could you help to check-in the "addPrincipal-v3.patch" and "check-in patch"? Hi Jason, Could you cover this item also? :)

(In reply to Randy Lin [:rlin] from comment #18) > Hi Ryan, > Could you help to check-in the "addPrincipal-v3.patch" and "check-in > patch"? > > Hi Jason, > Could you cover this item also? :) Yup, sure.

Comment on attachment 779663 [details] [diff] [review] test case :jsmith will help this test also.

https://hg.mozilla.org/projects/birch/rev/7111c04c1158 https://hg.mozilla.org/projects/birch/rev/73188e11f7b1

Backed out for mochitest-1 crashes. https://hg.mozilla.org/projects/birch/rev/b64e00a36321 https://tbpl.mozilla.org/php/getParsedLog.php?id=25716334&tree=Birch

Hi Jason, The mediaStream source can come from the webAudio, so I add this kind of testcase.

Comment on attachment 781086 [details] [diff] [review] test for record audiocontext Review of attachment 781086 [details] [diff] [review]: ----------------------------------------------------------------- Closer. Still needs some cleanup. ::: content/media/test/test_mediarecorder_record_audiocontext.html @@ +10,5 @@ > +<pre id="test"> > +<script class="testbody" type="text/javascript"> > +var manager = new MediaTestManager; > + > +function startTest(test, token) { Because you are providing your own media source generated from the web audio API, you don't need to run this test as part of the media test manager. I'd modify this mochitest just to do exactly what you are doing below outside of running within media test manager. @@ +39,5 @@ > + mMediaRecorder.onerror = function() { > + ok(false, 'onerror unexpectedly fired'); > + }; > + > + mMediaRecorder.onstop = function() { What you really should do here is set this up such that onstop shouldn't fire upon calling start. When you are about to call stop, setup the actual onstop handler. @@ +44,5 @@ > + info('onstop fired successfully'); > + }; > + mMediaRecorder.ondataavailable = function (e) { > + if (mMediaRecorder.state == 'recording') { > + mMediaRecorder.stop(); For anything below this stop call, I'd move it into the onstop handler. @@ +52,5 @@ > + } > + }; > + try { > + mMediaRecorder.start(1000); > + is('recording', mMediaRecorder.state, "check record state recording"); You need to check the mime type here. What mime type should we be expecting here?

test case v3, add more check for media Recorder's status, onstop event.

carry reviewer, Fix try server mochitest-test mediaRecorder crash issue.

Should this one. try server result. https://tbpl.mozilla.org/?tree=Try&rev=7bb0e9092669

I will add another bug for the mimeType may retuen null after call the mediaRecorder Start(). It's timing issue. Let the mimeType check at blob event first.

Comment on attachment 779096 [details] record-tone-gen.html ><!DOCTYPE HTML> ><html> ><head> > <title>Media test: mediaRecorder</title> > <meta charset='utf-8'> > <script type="text/javascript" src="manifest.js"></script> ></head> ><body> ><pre id="test"> ><audio id="audioelem"></audio> ><script class="testbody" type="text/javascript"> > > var context = new AudioContext(); > var buffer = context.createBuffer(1, 204800, context.sampleRate); > for (var i = 0; i < 204800; ++i) { > buffer.getChannelData(0)[i] = Math.sin(440 * 2 * Math.PI * i / context.sampleRate); > } > > var source = context.createBufferSource(); > source.buffer = buffer; > > var dest = context.createMediaStreamDestination(); > source.connect(dest); > > var elem = document.getElementById('audioelem'); > elem.mozSrcObject = dest.stream; > elem.onloadedmetadata = function() { > setTimeout(function() { > document.mr = new MediaRecorder(dest.stream); > document.mr.ondataavailable = function(e) {dump(e);}; > document.mr.start(1000); > > }, 1000); > }; > > source.start(0); > elem.play(); > ></script> ></pre> ></body> ></html> > >

follow bug Bug 898396 - Media Recording - MediaRecorder's mimeType attribute maybe null after call the start()

Hi Ryan, Sorry causing the test case fail. Could you help this again? Thanks a lot. Need to check-in addPrincipal-v3.patch, check-in.patch, test case by sequence.

https://hg.mozilla.org/projects/birch/rev/f8f855f0b7dc https://hg.mozilla.org/projects/birch/rev/4fe81f58dbef https://hg.mozilla.org/projects/birch/rev/f2862e93b710

Backed out for asserting like crazy in the new tests. Note that your Try run was opt-only, so you never would have caught it there. https://hg.mozilla.org/projects/birch/rev/5aa02ee02f4b https://tbpl.mozilla.org/php/getParsedLog.php?id=25784353&tree=Birch 10:49:26 INFO - [Parent 2296] ###!!! ASSERTION: Slice out of range: 'aStart >= 0 && aEnd <= aSource.mDuration', file ../../../content/media/MediaSegment.h, line 240 10:49:26 INFO - mozilla::MediaSegmentBase<mozilla::AudioSegment, mozilla::AudioChunk>::AppendSliceInternal(mozilla::MediaSegmentBase<mozilla::AudioSegment, mozilla::AudioChunk> const&, long long, long long) [content/media/MediaSegment.h:239] 10:49:26 INFO - mozilla::TrackUnionStream::CopyTrackData(mozilla::StreamBuffer::Track*, unsigned int, long long, long long, bool*) [content/media/TrackUnionStream.h:248] 10:49:26 INFO - mozilla::TrackUnionStream::ProduceOutput(long long, long long) [content/media/TrackUnionStream.h:71] 10:49:26 INFO - mozilla::MediaStreamGraphImpl::ProduceDataForStreamsBlockByBlock(unsigned int, int, long long, long long) [obj-firefox/dist/include/nsAutoPtr.h:880] 10:49:26 INFO - mozilla::MediaStreamGraphImpl::RunThread() [content/media/MediaStreamGraph.cpp:1081] 10:49:26 INFO - mozilla::::MediaStreamGraphThreadRunnable::Run [content/media/MediaStreamGraph.cpp:1230] 10:49:26 INFO - nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:622] 10:49:26 INFO - NS_ProcessNextEvent(nsIThread*, bool) [obj-firefox/xpcom/build/nsThreadUtils.cpp:238]

Let me take it because there are still bugs need to solve. This issue can't reproduce on my Ubuntu 13.04 debug build and still find a way to reproduce & solve this issue. sorry for back out twice.

Sometimes the mDuration is less than aEnd in AppendSliceInternal function.. (ex aEnd = 113665, mDuration = 113664), so this assertion hits. NS_ASSERTION(aStart >= 0 && aEnd <= aSource.mDuration, "Slice out of range"); Repo step: use Firefox, load this test html and reload by F5 can find this problem.

Hi Roc, I add this work-around in TrackUnionStream.h can avoid this problem. // We'll take the latest samples we can. TrackTicks inputEndTicks = TimeToTicksRoundUp(rate, inputEnd); + if (inputEndTicks > aInputTrack->GetSegment()->GetDuration()) + inputEndTicks = aInputTrack->GetSegment()->GetDuration(); But I'm not sure what reason cause the inputEndTicks is equal to the GetDuration()+1. Is timing issue?

I encountered a similar issue in bug 856361. There's something fundamentally broken here, and it's tricky, but I think I've fixed it by rewriting the code. I'll submit a patch in bug 856361.

Thanks a lot. If there is a patch I can try, please also notify me. :)

Test with this patch https://bugzilla.mozilla.org/attachment.cgi?id=783698 (Part 7: Fix copying of track data from input streams to output streams in TrackUnionStream ) and it can solve this NS_ASSERTION(aStart >=....) problem.

Randy - With the assertions fixed in the dependent bug, this is ready to land again, right?

I will have a try again and check-in it. :)

try server. https://tbpl.mozilla.org/?tree=Try&rev=808280c3b190

Please help to check-in the addPrincipal-v3.patch (2.02 KB, patch) check-in.patch (2.86 KB, patch) test case (3.18 KB, patch) Thanks a lot.

https://hg.mozilla.org/integration/b2g-inbound/rev/9a466a6b1f46 https://hg.mozilla.org/integration/b2g-inbound/rev/980905e0308e https://hg.mozilla.org/integration/b2g-inbound/rev/6ca6b7563842

https://hg.mozilla.org/mozilla-central/rev/9a466a6b1f46 https://hg.mozilla.org/mozilla-central/rev/980905e0308e https://hg.mozilla.org/mozilla-central/rev/6ca6b7563842

Verified via successful landing of mochitest.

This bug has no relation to bug 932880.