Closed Bug 969372 Opened 12 years ago Closed 11 years ago

Intermittent | test_mediarecorder_record_no_timeslice.html | Test timed out.

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32
Tracking Flags:
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
firefox32 --- fixed
firefox-esr24 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: cbook, Assigned: rlin)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(6 files, 11 obsolete files)

check-in patch
1.81 KB, patch
Details | Diff | Splinter Review
RecordingThenNavigate.patch
2.40 KB, patch
Details | Diff | Splinter Review
three call stacks
20.90 KB, text/plain
Details
bug-969372-part1-nsIDocumentActivity.patch
29.77 KB, patch
bechen
: review+
Details | Diff | Splinter Review
bug-969372-part2-MediaRecorder.patch
11.66 KB, patch
bechen
: review+
Details | Diff | Splinter Review
bug-969372-part3-testcase.patch
1.74 KB, patch
bechen
: review+
Details | Diff | Splinter Review
Ubuntu VM 12.04 x64 fx-team opt test mochitest-1 on 2014-02-06 18:35:42 PST for push 83865ff96a29 slave: tst-linux64-spot-352 https://tbpl.mozilla.org/php/getParsedLog.php?id=34255610&tree=Fx-Team 9790 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Test timed out.
Component: Video/Audio → Video/Audio: Recording
No longer blocks: MediaRecording
Assignee: nobody → rlin
Try to build pgo version and debug on local pc first.
Getting worse. Can reproduce on try server, insert some logs and debugging.
Weird, media recorder has fired event to js side but js doesn't get it.
Print log on try server and found the event has been dispatched to js side. https://tbpl.mozilla.org/php/getParsedLog.php?id=39009178&tree=Try&full=1 19:08:15 INFO - notify DispatchSimpleEvent....
A simple workaround is set the media recorder object to link a global object.
Hi Olli, We found the events have been fired by the DOM object but js side can't receive it. Do you have any ideas how to debug this? I found an reversion that can easy produce this problem on try server.
Flags: needinfo?(bugs)
If events are dispatches but listener for them isn't called, it is most probably because we have already unloaded the page which contains the listeners. (we have innerwindow check in webidl callback code. Only callbacks from the current inner window are called.)
Flags: needinfo?(bugs)
But for this test case, I think the window won't close until the test get finished. How can I ensure the inner window has been closed? BTW, If I move the object into global level, this problem has gone..
Flags: needinfo?(bugs)
Attached patch test if this affect this issue (obsolete) — — Splinter Review
Hi Jason, I want to test if this could avoid this problem first?
Attachment #8417949 - Flags: review?(jsmith)
Comment on attachment 8417949 [details] [diff] [review] test if this affect this issue Review of attachment 8417949 [details] [diff] [review]: ----------------------------------------------------------------- Can you push this to try to see if this fixes the issue? If it does, then reflag the review.
Attachment #8417949 - Flags: review?(jsmith)
Attached patch test if this affect this issue (obsolete) — — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=68c8bd3cf2f6 On this version can easy found timeout at OS X 10.6 Debug on this case without this kind of change. https://tbpl.mozilla.org/?tree=Try&rev=ae195cd6df52
Attachment #8418595 - Flags: review?(jsmith)
Comment on attachment 8418595 [details] [diff] [review] test if this affect this issue Review of attachment 8418595 [details] [diff] [review]: ----------------------------------------------------------------- Note - we'll want to file a followup to update the other tests to follow this format, as I think this problem could happen with other media recorder tests. ::: content/media/test/test_mediarecorder_record_no_timeslice.html @@ +9,5 @@ > <body> > <pre id="test"> > <script class="testbody" type="text/javascript"> > var manager = new MediaTestManager; > +info("Need to clarify why declare mediaRecorder in startTest cause timeout"); Nit - I don't think an info statement needed here - I'd use a comment in the code. @@ +10,5 @@ > <pre id="test"> > <script class="testbody" type="text/javascript"> > var manager = new MediaTestManager; > +info("Need to clarify why declare mediaRecorder in startTest cause timeout"); > +var mediaRecorder; Nit - can you add a blank line here?
Attachment #8418595 - Flags: review?(jsmith) → review+
Attached patch check-in patch — — Splinter Review
check-in patch. carry reviewer.
Attachment #8417949 - Attachment is obsolete: true
Attachment #8418595 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/92ea414327b6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Flags: needinfo?(bugs)
Hi Kyle, Before this bug's this patch, We declared the mediaRecorder object in startTest function. http://hg.mozilla.org/mozilla-central/annotate/4c4dcf7533cb/content/media/test/test_mediarecorder_record_no_timeslice.html#l31 Does this object would enter gc list if exit this function scope? And event handler function may not be invoked on this case.
Flags: needinfo?(khuey)
(In reply to Randy Lin [:rlin] from comment #193) > Does this object would enter gc list if exit this function scope? > And event handler function may not be invoked on this case. Perhaps element.stream holds a reference to the MediaRecorder for a while, but I assume that (if it exists) would be released when the audio element finishes playing. MediaRecorder needs to keep a reference to itself if it will need to fire an event.
For MediaRecorder which is generated by MediaRecorder.webidl, is it possible for the JS wrapper to be GCed while the native object is still alive?
(In reply to JW Wang [:jwwang] from comment #197) > For MediaRecorder which is generated by MediaRecorder.webidl, is it possible > for the JS wrapper to be GCed while the native object is still alive? Yes. But the JS wrapper will be recreated when it is needed again (say, when an event is fired).
As Karl says in comment 196, MediaRecorder needs to be holding itself alive on the C++ side somehow if it is possible for it to fire events later.
Flags: needinfo?(khuey)
Randy, can you spin off another bug for fixing the C++ rooting issue and then reverting the patch that landed here?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #198) > (In reply to JW Wang [:jwwang] from comment #197) > > For MediaRecorder which is generated by MediaRecorder.webidl, is it possible > > for the JS wrapper to be GCed while the native object is still alive? > > Yes. But the JS wrapper will be recreated when it is needed again (say, > when an event is fired). Will the property changes be lost? Eg: mediaRecorder.ondataavailable = function(evt) { ...} The ondataavailable property of the recreated object will be null.
As I tested before and I found the MediaRecorder c++ object didn't hit the destructor while this issue happened.
(In reply to JW Wang [:jwwang] from comment #201) > Will the property changes be lost? > Eg: mediaRecorder.ondataavailable = function(evt) { ...} > The ondataavailable property of the recreated object will be null. No. GCing a wrapper should not be detectable from JS
(In reply to JW Wang [:jwwang] from comment #201) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #198) > > (In reply to JW Wang [:jwwang] from comment #197) > > > For MediaRecorder which is generated by MediaRecorder.webidl, is it possible > > > for the JS wrapper to be GCed while the native object is still alive? > > > > Yes. But the JS wrapper will be recreated when it is needed again (say, > > when an event is fired). > > Will the property changes be lost? > Eg: mediaRecorder.ondataavailable = function(evt) { ...} > The ondataavailable property of the recreated object will be null. I started to write a response to this but it turned into a blog post. http://blog.kylehuey.com/post/85235994007/dom-object-reflection-how-does-it-work The short answer is that ondataavailable is an IDL property so the value of the property is actually stored in the C++ object and if the wrapper is later recreated it will still be there.
(In reply to Randy Lin [:rlin] from comment #202) > As I tested before and I found the MediaRecorder c++ object didn't hit the > destructor while this issue happened. That could just be a timing issue with when we call destructors. Did you see if we run all of the expected MediaRecorder code to fire all of the expected events?
Yes, I try to log the expect event (onstop, ondataavailable) and I found those event has been fired but js side didn't receiver those one. I wonder where I can put more bugs to clarify those one?
What is supposed to keep the MediaRecorder alive during the recording process?
MediaRecorder object hold the reference to DOMMediaStream. and it will stop recording when media stream enter ended state.
You did not answer my question. What holds the MediaRecorder alive?
You mean this object:MediaRecorderBinding?
Nothing keeps the MediaRecorder alive AFAIK. I'm pretty sure I mentioned in bug 973522 that this would happen once that patch was checked in (although the patch in that bug was still the right thing to do for that bug). Karl was right in comment #196. The MediaRecorder needs to keep itself alive as long as it might dispatch an event in the future. It must *stop* keeping itself alive when its owning Window is cleaned up (e.g. see how nsGlobalWindow tracks all its AudioContexts and calls Shutdown on them from nsGlobalWindow::CleanUp/FreeInnerObjects).
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #212) > It must *stop* keeping > itself alive when its owning Window is cleaned up (e.g. see how > nsGlobalWindow tracks all its AudioContexts and calls Shutdown on them from > nsGlobalWindow::CleanUp/FreeInnerObjects). Note that this signal is too early as a new object may be created after the window is closed (bug 933730), but better than nothing. HTML media elements do things differently. Perhaps that is better - I don't know.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #212) > Karl was right in comment #196. The MediaRecorder needs to keep itself alive > as long as it might dispatch an event in the future. It must *stop* keeping > itself alive when its owning Window is cleaned up (e.g. see how > nsGlobalWindow tracks all its AudioContexts and calls Shutdown on them from > nsGlobalWindow::CleanUp/FreeInnerObjects). It needs to stop keeping itself alive whenever the recording process ceases, not just when the window shuts down.
The logs from Windows 8 debug: https://tbpl.mozilla.org/php/getParsedLog.php?id=39657325&tree=Try&full=1#error0 https://hg.mozilla.org/try/file/63ea20a250a6/content/media/MediaRecorder.cpp#l671 2014-05-14 14:55:41.526000 UTC - 0[89e828]: MediaRecorder=418d80e8 CreateAndDispatchBlobEvent 1 2014-05-14 14:55:41.526000 UTC - 0[89e828]: MediaRecorder=418d80e8 CreateAndDispatchBlobEvent 2 It looks like CheckPrincipal() fails and returns immediately without dispatching the event.
(In reply to JW Wang [:jwwang] from comment #215) > The logs from Windows 8 debug: > https://tbpl.mozilla.org/php/getParsedLog. > php?id=39657325&tree=Try&full=1#error0 > > https://hg.mozilla.org/try/file/63ea20a250a6/content/media/MediaRecorder. > cpp#l671 > 2014-05-14 14:55:41.526000 UTC - 0[89e828]: MediaRecorder=418d80e8 > CreateAndDispatchBlobEvent 1 > 2014-05-14 14:55:41.526000 UTC - 0[89e828]: MediaRecorder=418d80e8 > CreateAndDispatchBlobEvent 2 > > It looks like CheckPrincipal() fails and returns immediately without > dispatching the event. I don't see this text in the log you linked ...
https://tbpl.mozilla.org/php/getParsedLog.php?id=39708227&tree=Try&full=1#error0 21:38:49 INFO - [Parent 973] WARNING: CheckPrincipal: mStream is null.: file /builds/slave/try-osx64-d-000000000000000000/build/content/media/MediaRecorder.cpp, line 766 21:38:49 INFO - [Parent 973] WARNING: CreateAndDispatchBlobEvent: CheckPrincipal failed.: file /builds/slave/try-osx64-d-000000000000000000/build/content/media/MediaRecorder.cpp, line 676 CheckPrincipal() failed for mStream was null. It looks like something wrong to the life cycle of mStream.
(In reply to JW Wang [:jwwang] from comment #218) > https://tbpl.mozilla.org/php/getParsedLog. > php?id=39708227&tree=Try&full=1#error0 > > 21:38:49 INFO - [Parent 973] WARNING: CheckPrincipal: mStream is null.: > file > /builds/slave/try-osx64-d-000000000000000000/build/content/media/ > MediaRecorder.cpp, line 766 > 21:38:49 INFO - [Parent 973] WARNING: CreateAndDispatchBlobEvent: > CheckPrincipal failed.: file > /builds/slave/try-osx64-d-000000000000000000/build/content/media/ > MediaRecorder.cpp, line 676 > > CheckPrincipal() failed for mStream was null. It looks like something wrong > to the life cycle of mStream. My guess would be that we've already unlinked the MediaRecorder, which will cause mStream to be set to null (http://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?force=1#36). So we're back to comment 196, 212, etc.
Can you log when unlink is called?
Do you mean to expand the macro |NS_IMPL_CYCLE_COLLECTION_INHERITED(MediaRecorder, DOMEventTargetHelper, mStream)| in MediaRecorder.cpp and add some logs in MediaRecorder::cycleCollection::Unlink()?
https://tbpl.mozilla.org/php/getParsedLog.php?id=39748360&tree=Try&full=1#error0 10:54:39 INFO - [Parent 1632] WARNING: Mediarecorder=3c35a430 ctor: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\media\MediaRecorder.cpp, line 498 10:54:39 INFO - detodos.opus-0: element.oncanplaythrough ended=false 10:54:42 INFO - [Parent 1632] WARNING: Mediarecorder=3c35a430 CheckPrincipal: mStream is null.: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\media\MediaRecorder.cpp, line 767 10:54:42 INFO - [Parent 1632] WARNING: Mediarecorder=3c35a430 CreateAndDispatchBlobEvent: CheckPrincipal failed: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\media\MediaRecorder.cpp, line 677 10:59:40 INFO - 4815 INFO TEST-INFO | dumping last 4 message(s) 10:59:40 INFO - 4816 INFO TEST-INFO | if you need more context, please use SimpleTest.requestCompleteLog() in your test 10:59:40 INFO - 4817 INFO TEST-INFO | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Started Thu May 15 2014 10:54:38 GMT-0700 (Pacific Standard Time) (1400176478.778s) 10:59:40 INFO - 4818 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | [started detodos.opus-0] Length of array should match number of running tests 10:59:40 INFO - 4819 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Media recorder should be recording 10:59:40 INFO - 4820 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Media recorder stream = element stream at the start of recording 10:59:40 INFO - TEST-INFO | screenshot: exit status 0 10:59:40 INFO - 4821 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Test timed out. ... 10:59:42 INFO - [Parent 1632] WARNING: Mediarecorder=3c35a430 dtor: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\media\MediaRecorder.cpp, line 476 Mediarecorder=3c35a430 was deleted after timeout happened. Since Mediarecorder holds a reference to mStream, if mStream is deleted by cycle collector, Mediarecorder should be also deleted by cycle collector, right? It is weird for Mediarecorder to live longer than mStream.
(In reply to JW Wang [:jwwang] from comment #221) > Do you mean to expand the macro > |NS_IMPL_CYCLE_COLLECTION_INHERITED(MediaRecorder, DOMEventTargetHelper, > mStream)| in MediaRecorder.cpp and add some logs in > MediaRecorder::cycleCollection::Unlink()? Yes.
(In reply to JW Wang [:jwwang] from comment #222) > Since > Mediarecorder holds a reference to mStream, if mStream is deleted by cycle > collector, Mediarecorder should be also deleted by cycle collector, right? > It is weird for Mediarecorder to live longer than mStream. You are right that mStream being deleted means that the MediaRecorder will be deleted, but it is more complicated than that and these can be deleted in either order. Cycle collection destruction has 2 phases. First, unlinking will call Unlink on the MediaRecorder, and, if the stream has no other references, Unlink() will also be called on the Stream. This will set mStream to null. Objects are still alive after this phase, but usually they cannot do much because they have been disconnected (Unlink()ed) from other objects. Both objects will be deleted in another phase but that happens later, and other things may happen in the period between these phases.
Print logs when MediaRecorder::cycleCollection::Unlink is called. https://tbpl.mozilla.org/php/getParsedLog.php?id=39792726&tree=Try&full=1#error0 00:51:37 INFO - [Parent 3168] WARNING: Mediarecorder=2392ecb0 ctor: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\media\MediaRecorder.cpp, line 522 00:51:37 INFO - detodos.opus-0: element.oncanplaythrough ended=false 00:51:38 INFO - --DOMWINDOW == 17 (2379ADC8) [pid = 3168] [serial = 4766] [outer = 00000000] [url = http://mochi.test:8888/tests/content/media/test/test_mediarecorder_record_immediate_stop.html] 00:51:39 INFO - [Parent 3168] WARNING: Mediarecorder=2392ecb0 MediaRecorder::cycleCollection::Unlink: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\media\MediaRecorder.cpp, line 44 00:51:40 INFO - [Parent 3168] WARNING: Mediarecorder=2392ecb0 CheckPrincipal: mStream is null.: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\media\MediaRecorder.cpp, line 791 00:51:40 INFO - [Parent 3168] WARNING: Mediarecorder=2392ecb0 CreateAndDispatchBlobEvent: CheckPrincipal failed: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\media\MediaRecorder.cpp, line 701 It looks like CC decided to collect the MediaRecorder object which broke the link from MediaRecorder to |mStream| and therefore |mStream| became nullptr. (The actual stream object could still live for a while for it was referenced by others) It is weird for MediaRecorder to live a long time (over 3 mins) after CC. I wonder if something happens in between the CC phases to resurrect MediaRecorder.
Attached patch bug-969372-mrleak.patch (obsolete) — — Splinter Review
If I understand correctly, now MR hold a raw pointer to Session (nsTArray<Session*> mSessions;) and Session hold a raw pointer to MR (MediaRecorder* mRecorder;). That's why nobody keep MR alive. So let Session holds a reference to Mr seems to resolve this. Keep MR alive until all Sessions stopped. But it breaks the design again in Bug 973522.
Attachment #8430489 - Flags: feedback?(rlin)
Attachment #8430489 - Flags: feedback?(jwwang)
Attachment #8430489 - Flags: feedback?(cku)
Thread safe issues 1. MediaRecorder::RemoveSession is called in mReadThread, this function change context of mSessions. 2. MediaRecorder::~MediaRecorder/ MediaRecorder::Start are called in main thread, these two function change context of mSessions. I suggest dispatch MediaRecorder::RemoveSession into main thread.
(In reply to C.J. Ku[:cjku] from comment #228) > Thread safe issues > 1. MediaRecorder::RemoveSession is called in mReadThread, this function > change context of mSessions. > 2. MediaRecorder::~MediaRecorder/ MediaRecorder::Start are called in main > thread, these two function change context of mSessions. > I suggest dispatch MediaRecorder::RemoveSession into main thread. Currently the RemoveSession fuction is only called on main thread so we won't have problem now.
After this patch, you need to modify original code as well dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#478 Call mSessions[i]->ForgetMediaRecorder() in MediaRecorder::~MediaRecorder is not right. Unless session been release, MeidaRecoder can't be free. My suggestion is remove #477 ~ 480 and remove ForgetMediaRecoder public function
dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#343 TracksAvailableCallback* tracksAvailableCallback = new TracksAvailableCallback(mRecorder->mSessions.LastElement()); Why don't we just use this here? TracksAvailableCallback* tracksAvailableCallback = new TracksAvailableCallback(this);
Flags: needinfo?(rlin)
Comment on attachment 8430489 [details] [diff] [review] bug-969372-mrleak.patch Review of attachment 8430489 [details] [diff] [review]: ----------------------------------------------------------------- f- because comment 230. Since you change object relation between MediaRecoder and Session, ForgetMediaRecoder is not required
Attachment #8430489 - Flags: feedback?(cku) → feedback-
(In reply to C.J. Ku[:cjku] from comment #231) > dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#343 > TracksAvailableCallback* tracksAvailableCallback = new > TracksAvailableCallback(mRecorder->mSessions.LastElement()); > > Why don't we just use this here? > TracksAvailableCallback* tracksAvailableCallback = new > TracksAvailableCallback(this); It could cause problem, session should register itself to get the trackavailable callback.
Flags: needinfo?(rlin)
Comment on attachment 8430489 [details] [diff] [review] bug-969372-mrleak.patch Review of attachment 8430489 [details] [diff] [review]: ----------------------------------------------------------------- 1. It is possible that Session::AfterTracksAdded is never called, then there is no way to destroy the session and there will be a leak. 2. It is possible that Session::Observe comes before Session::AfterTracksAdded, mEncoder would be a null.
Attachment #8430489 - Flags: feedback?(jwwang)
(In reply to JW Wang [:jwwang] from comment #234) > Comment on attachment 8430489 [details] [diff] [review] > bug-969372-mrleak.patch > > Review of attachment 8430489 [details] [diff] [review]: > ----------------------------------------------------------------- > > 1. It is possible that Session::AfterTracksAdded is never called, then there > is no way to destroy the session and there will be a leak. > 2. It is possible that Session::Observe comes before > Session::AfterTracksAdded, mEncoder would be a null. I'm using drx at home, so the code flow may not 100% correct. Here is the code flow I see: Session::Start -> Session::SetupStream -> DOMMediaStream::OnTracksAvailable -> DOMMediaStream::CheckTracksAvailable -> TracksAvailableCallback::NotifyTracksAvailable -> Session::AfterTracksAdded If the code flow is exactly the same as I traced, the Session::AfterTracksAdded is immediately called by MediaRecorder::Play.
http://hg.mozilla.org/mozilla-central/file/a3848fbadb16/content/media/DOMMediaStream.cpp#l360 It could return immediately when |mTrackTypesAvailable| is 0. A DOMMediaStream can start with no tracks inside and then tracks are added later. However, we won't hit the case for now when getUserMedia is considered. In the long term, we should support dynamic add/remove in tracks which is not handled for now.
Attached patch bug-969372-mrleak.patch (obsolete) — — Splinter Review
try server test: https://tbpl.mozilla.org/?tree=Try&rev=706eccb4572b
Attachment #8430489 - Attachment is obsolete: true
Attachment #8430489 - Flags: feedback?(rlin)
Attachment #8433009 - Flags: feedback?(rlin)
Attachment #8433009 - Flags: feedback?(jwwang)
Attachment #8433009 - Flags: feedback?(cku)
1. http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#563 http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#631 MOZ_ASSERT(mSessions.Length() > 0); 2. http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#433 if (mRecorder) { mRecorder->RemoveSession(this); mRecorder = nullptr; } Stop(); Please discuss with Randy. Session::Stop eventually submit a DestroyRunnable task into main thread, which calls RemoveSession, why we need to explicit RemoveSession at #433? 3. http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#197 http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#254 Duplicate code.
Attachment #8433009 - Flags: feedback?(cku) → feedback+
Comment on attachment 8433009 [details] [diff] [review] bug-969372-mrleak.patch Review of attachment 8433009 [details] [diff] [review]: ----------------------------------------------------------------- I prefer to have another for this patch.
Attachment #8433009 - Flags: feedback?(rlin) → feedback+
Comment on attachment 8433009 [details] [diff] [review] bug-969372-mrleak.patch Review of attachment 8433009 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks fine to me.
Attachment #8433009 - Flags: feedback?(jwwang) → feedback+
Attachment #8433009 - Flags: review?(roc)
Comment on attachment 8433009 [details] [diff] [review] bug-969372-mrleak.patch Review of attachment 8433009 [details] [diff] [review]: ----------------------------------------------------------------- Please document somewhere in the code how we manage the lifetime of the MediaRecorder and Sessions. The comment " // Hold the sessions pointer in media recorder and clean in the destructor of recorder." needs to be fixed. It's unclear what keeps the Session alive while we're recording. I *think* it's actually the shutdown observer! This is not obvious and must be documented, or possibly changed to more explicitly keep itself alive. If someone starts recording and then just closes the page, what causes the Session and MediaRecorder to be released? It seems to me they will leak. I think we need to Stop all MediaRecorders as soon as the page becomes inactive. We have a similar problem for media elements; for those, we register them as "Freezables" with nsIDocument, and then nsIDocument::EnumerateFreezableElements is called when the document goes inactive. Maybe we can change Freezables from being an nsIContent* to an nsISupports* and register Sessions as freezables too.
Attachment #8433009 - Flags: review?(roc) → review-
Attached patch bug-969372-mrleak.patch (obsolete) — — Splinter Review
Modify the comments. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #241) > If someone starts recording and then just closes the page, what causes the > Session and MediaRecorder to be released? It seems to me they will leak. I > think we need to Stop all MediaRecorders as soon as the page becomes > inactive. We have a similar problem for media elements; for those, we > register them as "Freezables" with nsIDocument, and then > nsIDocument::EnumerateFreezableElements is called when the document goes > inactive. Maybe we can change Freezables from being an nsIContent* to an > nsISupports* and register Sessions as freezables too. We will file another bug to track or solve it.
Attachment #8433009 - Attachment is obsolete: true
Attachment #8434743 - Flags: review?(roc)
Blocks: 1020822
How is bug 1020822 different from bug 973522? Why are we reintroducing bugs we've already fixed?
Yeah, this patch is effectively a backout of bug 973522. Why would we do that?
Comment on attachment 8434743 [details] [diff] [review] bug-969372-mrleak.patch Review of attachment 8434743 [details] [diff] [review]: ----------------------------------------------------------------- What Kyle said. We're going around in circles. One thing to help us stop going around in circles would be better tests. We should have a way to test that a MediaRecorder stops when its window is closed. This is actually relatively easy to test: have a same-origin <iframe> with a script in it that starts a MediaRecorder, and then navigate the iframe to about:blank. We should be able to detect that the MediaRecorder has stopped.
Attachment #8434743 - Flags: review?(roc) → review-
FWIW I'll be in Taipei next week if some of you want to sit down and talk about what needs to be done here.
This testcase tries to implement the scenario in comment 245. Open an iframe and the iframe starts a MediaRecorder, then navigate the iframe to "about:blank". During the navigation to "about:blank" the MediaRecorder doens't be destroyed until the process shout down. Randy, do you think this behaviour is correct or do we need to "listen some events" to release Session and MediaRecorder? As comment 241 said.
Flags: needinfo?(rlin)
As discussed with kyle, Benjamin, jwwang, We can register the "inner-window-destroyed" event and remember the window ID in session, change media recorder to be strong reference in session object. When session receives that event, we can compare the window ID and stop the recording thread as needed. It could cover the problem that roc said in comment 245.
Flags: needinfo?(rlin)
Attached patch wip-bug-969372-mrleak.v02.patch (obsolete) — — Splinter Review
Attachment #8434743 - Attachment is obsolete: true
(In reply to Benjamin Chen [:bechen] from comment #249) > Created attachment 8437426 [details] [diff] [review] > wip-bug-969372-mrleak.v02.patch Briefly explain the patch: 1. Session hold a reference to MediaRecorder. 2. Change the "shutdown observer" to "inner-window-destroyed" on Session object. 3. Add "nsIDocumentActivity" interface on MediaRecorder object to allow queryinterface fuction in nsDocument.cpp. 4. Change the mFreezableElements in nsDocumenp.cpp; from nsIContent to nsISupports. - some compiler errors, now using static_cast to resolve it.
Attachment #8437426 - Flags: feedback?(roc)
Attachment #8437426 - Flags: feedback?(rlin)
Attachment #8437426 - Flags: feedback?(jwwang)
Attachment #8437426 - Flags: feedback?(cku)
Comment on attachment 8437426 [details] [diff] [review] wip-bug-969372-mrleak.v02.patch Review of attachment 8437426 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsContentUtils.cpp @@ +6268,5 @@ > return principalsEqual; > } > > static void > +CheckForWindowedPlugins(nsISupports* aContent, void* aResult) s/aContent/aObject/ @@ +6270,5 @@ > > static void > +CheckForWindowedPlugins(nsISupports* aContent, void* aResult) > +{ > + if (!static_cast<nsIContent*>(aContent)->IsInDoc()) { Use do_QueryInterface. An nsISupports is not necessarily an nsIContent. ::: content/media/MediaRecorder.cpp @@ +42,5 @@ > NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper) > > NS_IMPL_ADDREF_INHERITED(MediaRecorder, DOMEventTargetHelper) > NS_IMPL_RELEASE_INHERITED(MediaRecorder, DOMEventTargetHelper) > +//NS_IMPL_QUERY_INTERFACE_INHERITED(MediaRecorder, nsIDocumentActivity) This line not needed? @@ +520,5 @@ > + nsPIDOMWindow* window = GetOwner(); > + if (window) { > + nsIDocument* doc = window->GetDoc(); > + if (doc) { > + doc->RegisterFreezableElement(static_cast<nsIDocumentActivity*>(this)); No need to cast. @@ +532,5 @@ > + nsPIDOMWindow* window = GetOwner(); > + if (window) { > + nsIDocument* doc = window->GetDoc(); > + if (doc) { > + doc->UnregisterFreezableElement(static_cast<nsIDocumentActivity*>(this)); No need to cast. @@ +610,1 @@ > if (mSessions.Length() > 0) { The assertion does the check for you. @@ +827,5 @@ > +NS_IMETHODIMP > +MediaRecorder::NotifyOwnerDocumentActivityChanged() > +{ > + nsPIDOMWindow* window = GetOwner(); > + NS_ENSURE_TRUE(window, NS_OK); Do you want to return OK when window is null? @@ +829,5 @@ > +{ > + nsPIDOMWindow* window = GetOwner(); > + NS_ENSURE_TRUE(window, NS_OK); > + nsIDocument* doc = window->GetDoc(); > + NS_ENSURE_TRUE(doc, NS_OK); Same as above. @@ +833,5 @@ > + NS_ENSURE_TRUE(doc, NS_OK); > + > + LOG(PR_LOG_DEBUG, ("MediaRecorder::NotifyOwnerDocumentActivityChanged %d\n" > + ,doc->IsActive())); > + if (doc->IsActive()) { http://dxr.mozilla.org/mozilla-central/source/content/base/public/nsIDocument.h#1686 "Note that this will return true for bfcached documents." It looks like not what we want. We should stop recording when the user navigate away from the current page. However, media element also checks IsActive in HTMLMediaElement::AddRemoveSelfReference(), the logic should be right for the media element. ::: layout/base/nsPresShell.cpp @@ +8200,5 @@ > return mStyleSet->RemoveStyleSheet(nsStyleSet::eOverrideSheet, aSheet); > } > > static void > +FreezeElement(nsISupports *aContent, void * /* unused */) s/aContent/aObject/ @@ +8274,5 @@ > } > } > > static void > +ThawElement(nsISupports *aContent, void *aShell) s/aContent/aObject/ @@ +10184,5 @@ > return true; > } > > static void > +SetPluginIsActive(nsISupports* aContent, void* aClosure) s/aContent/aObject/ @@ +10186,5 @@ > > static void > +SetPluginIsActive(nsISupports* aContent, void* aClosure) > +{ > + nsIFrame *frame = static_cast<nsIContent*>(aContent)->GetPrimaryFrame(); Query the interface instead of static casting. ::: layout/generic/nsObjectFrame.cpp @@ +1994,5 @@ > NS_PRECONDITION(aContent, ""); > > // This function is called from a document content enumerator so we need > // to filter out the nsObjectFrames and ignore the rest. > + nsIObjectFrame* obj = do_QueryFrame(static_cast<nsIContent*>(aContent)->GetPrimaryFrame()); Query interface. @@ +2011,5 @@ > NS_PRECONDITION(aContent, ""); > > // This function is called from a document content enumerator so we need > // to filter out the nsObjectFrames and ignore the rest. > + nsIObjectFrame* obj = do_QueryFrame(static_cast<nsIContent*>(aContent)->GetPrimaryFrame()); Query interface.
Attachment #8437426 - Flags: feedback?(jwwang) → feedback-
Comment on attachment 8437426 [details] [diff] [review] wip-bug-969372-mrleak.v02.patch Review of attachment 8437426 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDocument.cpp @@ +4375,2 @@ > if (domMediaElem) { > + HTMLMediaElement* mediaElem = static_cast<HTMLMediaElement*>(static_cast<nsIContent*>(aSupports)); Instead of this extra cast, just static_cast<HTMLMediaElement*>(domMediaElem.get()). ::: content/media/MediaRecorder.cpp @@ +262,5 @@ > CleanupStreams(); > + nsCOMPtr<nsIObserverService> obs = > + mozilla::services::GetObserverService(); > + if (obs) { > + obs->RemoveObserver(this, "inner-window-destroyed"); You shouldn't need an observer for inner-window-destroyed. That should be handled through the document-activity changing to false. @@ +833,5 @@ > + NS_ENSURE_TRUE(doc, NS_OK); > + > + LOG(PR_LOG_DEBUG, ("MediaRecorder::NotifyOwnerDocumentActivityChanged %d\n" > + ,doc->IsActive())); > + if (doc->IsActive()) { Just check doc->IsVisible() && doc->IsActive() ... that's what HTMLMediaElement does. nsDocument.cpp's NotifyActivityChanged gets called when IsVisible changes as well as when IsActive changes. ::: content/media/moz.build @@ +154,5 @@ > 'WebVTTListener.cpp', > ] > > +XPIDL_SOURCES += [ > + 'nsIDocumentActivity.idl', Don't use IDL for nsIDocumentActivity. Make it a plain .h file.
Attachment #8437426 - Flags: feedback?(roc) → feedback-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #252) > @@ +833,5 @@ > > + NS_ENSURE_TRUE(doc, NS_OK); > > + > > + LOG(PR_LOG_DEBUG, ("MediaRecorder::NotifyOwnerDocumentActivityChanged %d\n" > > + ,doc->IsActive())); > > + if (doc->IsActive()) { > > Just check doc->IsVisible() && doc->IsActive() ... that's what > HTMLMediaElement does. nsDocument.cpp's NotifyActivityChanged gets called > when IsVisible changes as well as when IsActive changes. In this case, seems that only check the IsActive is sufficient for MediaRecorder. And also window->GetExtentDoc() can replace window->GetDoc() if there is always an document exist.
Attached patch bug-969372-mrleak.v02.patch (obsolete) — — Splinter Review
try server: https://tbpl.mozilla.org/?tree=Try&rev=fdd8ec27f412 1. remove nsIDocumentActivity.idl file, add the "nsIDocumentActivity.h" to source. 2. Replace the static_cast to do_queryinterface. > > nsCOMPtr<nsIDOMHTMLMediaElement> domMediaElem(do_QueryInterface(aSupports)); > > if (domMediaElem) { > > + HTMLMediaElement* mediaElem = static_cast<HTMLMediaElement*>(static_cast<nsIContent*>(aSupports)); > > Instead of this extra cast, just > static_cast<HTMLMediaElement*>(domMediaElem.get()). Here we can not cast domMediaElem to HTMLMediaElement. Becuase the hierarchical tree is: HTMLAudioElement-->nsIDOMHTMLMediaElement --> nsISupports HTMLAudioElement-->HTMLMediaElement--> nsIContent --> nsISupports
Attachment #8437426 - Attachment is obsolete: true
Attachment #8437426 - Flags: feedback?(rlin)
Attachment #8437426 - Flags: feedback?(cku)
Attachment #8439140 - Flags: feedback?(roc)
Attachment #8439140 - Flags: feedback?(rlin)
Attachment #8439140 - Flags: feedback?(jwwang)
Attachment #8439140 - Flags: feedback?(cku)
(In reply to Benjamin Chen [:bechen] from comment #253) > In this case, seems that only check the IsActive is sufficient for > MediaRecorder. If you load a page that uses MediaRecorder, then in the same tab open another page, the MediaRecorder will keep recording. Right?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #255) > (In reply to Benjamin Chen [:bechen] from comment #253) > > In this case, seems that only check the IsActive is sufficient for > > MediaRecorder. > > If you load a page that uses MediaRecorder, then in the same tab open > another page, the MediaRecorder will keep recording. Right? I'm not exactly understand what the behavior "same tab open another page" is? Now I have test the patch in some ways, all on desktop build: 1. change the location by script: iframe.contentWindow.location.href = "about:blank"; // stop recording 2. when recording, type another url on the url bar. // stop recording 3. when recording, open another tab. // keep recording Is there any other ways I need to check? Or there exsist some testcases that I can refer. By the way, the HTMLMediaElement::AddRemoveSelfReference only refer the IsActive to control the self reference. The IsVisible() only used for suspending the HTMLMediaElement. And then, if there is a case that the document's stats is ACTIVE and NONVISIBLE, and we need to sop recording under this situation. I could modify the patch from |if (!doc->IsActive())| to |if (!doc->IsActive() || !doc->IsVisible())|
Flags: needinfo?(roc)
(In reply to Benjamin Chen [:bechen] from comment #256) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment > #255) > > (In reply to Benjamin Chen [:bechen] from comment #253) > > > In this case, seems that only check the IsActive is sufficient for > > > MediaRecorder. > > > > If you load a page that uses MediaRecorder, then in the same tab open > > another page, the MediaRecorder will keep recording. Right? > I'm not exactly understand what the behavior "same tab open another page" is? > > Now I have test the patch in some ways, all on desktop build: > 1. change the location by script: iframe.contentWindow.location.href = > "about:blank"; // stop recording > 2. when recording, type another url on the url bar. // stop recording > 3. when recording, open another tab. // keep recording > > Is there any other ways I need to check? Or there exsist some testcases that > I can refer. #1 and #2 are the cases I'm concerned about. How does your patch stop recording in those cases? As far as I know, the MediaRecorder document will still be IsActive in those cases. > By the way, the HTMLMediaElement::AddRemoveSelfReference only refer the > IsActive to control the self reference. The IsVisible() only used for > suspending the HTMLMediaElement. That's right, because we want to resume playback if the page becomes visible again. I think for MediaRecorder we are not going to resume recording when the page becomes visible again, at least not for now. > |if (!doc->IsActive())| to |if (!doc->IsActive() || !doc->IsVisible())| That's what I want.
Flags: needinfo?(roc)
Comment on attachment 8439140 [details] [diff] [review] bug-969372-mrleak.v02.patch Review of attachment 8439140 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/nsIDocument.h @@ +1684,5 @@ > * GetInnerWindow()->IsCurrentInnerWindow(). > */ > bool IsActive() const { return mDocumentContainer && !mRemovedFromDocShell; } > > + void RegisterFreezableElement(nsISupports* aSupports); For the naming of aSupports, How about aElement? And it's not plural. ::: content/media/MediaRecorder.h @@ +36,5 @@ > * Also extract the encoded data and create blobs on every timeslice passed from start function or RequestData function called by UA. > */ > > class MediaRecorder : public DOMEventTargetHelper > + , public nsIDocumentActivity class MediaRecorder : public DOMEventTargetHelper, public nsIDocumentActivity ::: content/media/nsIDocumentActivity.h @@ +1,4 @@ > +/* > + * DO NOT EDIT. THIS FILE IS GENERATED FROM ../../../dist/idl/nsIDocumentActivity.idl > + */ > + Mozilla License Headers? @@ +22,5 @@ > + {0x9b9f584e, 0xefa8, 0x11e3, \ > + { 0xbb, 0x74, 0x5e, 0xdd, 0x1d, 0x5d, 0x46, 0xb0 }} > + > +class NS_NO_VTABLE nsIDocumentActivity : public nsISupports { > + public: space @@ +45,5 @@ > +/* Use this macro to declare functions that forward the behavior of this interface to another object in a safe way. */ > +#define NS_FORWARD_SAFE_NSIDOCUMENTACTIVITY(_to) \ > + NS_IMETHOD NotifyOwnerDocumentActivityChanged(void) { return !_to ? NS_ERROR_NULL_POINTER : _to->NotifyOwnerDocumentActivityChanged(); } > + > +#if 0 Do we need those? ::: layout/base/nsPresShell.cpp @@ +10187,5 @@ > static void > +SetPluginIsActive(nsISupports* aSupports, void* aClosure) > +{ > + nsCOMPtr<nsIContent> content(do_QueryInterface(aSupports)); > + if (!content) { If it's unexcepted, assert it.
Attachment #8439140 - Flags: feedback?(rlin)
Comment on attachment 8439140 [details] [diff] [review] bug-969372-mrleak.v02.patch Review of attachment 8439140 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/nsIDocument.h @@ +1684,5 @@ > * GetInnerWindow()->IsCurrentInnerWindow(). > */ > bool IsActive() const { return mDocumentContainer && !mRemovedFromDocShell; } > > + void RegisterFreezableElement(nsISupports* aSupports); A generic term such as |aObject| should do the work. ::: content/base/src/nsDocument.cpp @@ +4375,2 @@ > if (domMediaElem) { > + nsCOMPtr<nsIContent> content(do_QueryInterface(domMediaElem)); Assert the query should always succeed. ::: layout/base/nsPresShell.cpp @@ +10187,5 @@ > static void > +SetPluginIsActive(nsISupports* aSupports, void* aClosure) > +{ > + nsCOMPtr<nsIContent> content(do_QueryInterface(aSupports)); > + if (!content) { Nah, we are changing to allow something other than nsIContent to register with Freezable.
Attachment #8439140 - Flags: feedback?(jwwang)
Attached file three call stacks —
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #257) > > Now I have test the patch in some ways, all on desktop build: > > 1. change the location by script: iframe.contentWindow.location.href = > > "about:blank"; // stop recording > > 2. when recording, type another url on the url bar. // stop recording > > 3. when recording, open another tab. // keep recording > > > > Is there any other ways I need to check? Or there exsist some testcases that > > I can refer. > > #1 and #2 are the cases I'm concerned about. How does your patch stop > recording in those cases? As far as I know, the MediaRecorder document will > still be IsActive in those cases. When I'm doing #1 and #2, the |MediaRecorder::NotifyOwnerDocumentActivityChanged()| always be invoked three times. Attachment includes the three call stacks. The first and second stacks seems normal both triggered by |nsDocument::OnPageHide| |nsDocumentViewer::PageHide|. At this time we get the Document is ACTIVE. But the third stack will change the Document into NON-ACTIVE. We can look into the function |nsDocumentViewer::Destroy()| http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?from=nsdocumentviewer.cpp#1659 The code snippet looks like even though we put the document into bfcache, the document will be NON-ACTIVE possibly. That's why my patch works only check the doc->IsActive .
Attached patch bug-969372-mrleak.v02.patch (obsolete) — — Splinter Review
Check the document's activity and visibility. |if (!doc->IsActive() || !doc->IsVisible()) | try server: https://tbpl.mozilla.org/?tree=Try&rev=2c740b2699b2
Attachment #8439140 - Attachment is obsolete: true
Attachment #8439140 - Flags: feedback?(roc)
Attachment #8439140 - Flags: feedback?(cku)
Attachment #8441231 - Flags: review?(roc)
(In reply to Benjamin Chen [:bechen] from comment #260) > When I'm doing #1 and #2, the > |MediaRecorder::NotifyOwnerDocumentActivityChanged()| always be invoked > three times. > Attachment includes the three call stacks. > > The first and second stacks seems normal both triggered by > |nsDocument::OnPageHide| |nsDocumentViewer::PageHide|. At this time we get > the Document is ACTIVE. > But the third stack will change the Document into NON-ACTIVE. We can look > into the function |nsDocumentViewer::Destroy()| > http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer. > cpp?from=nsdocumentviewer.cpp#1659 > The code snippet looks like even though we put the document into bfcache, > the document will be NON-ACTIVE possibly. > > That's why my patch works only check the doc->IsActive . I'm worried that this is just an artifact of your tests and in other cases the document will be hidden but still active. Anyway, you've fixed the patch.
Comment on attachment 8441231 [details] [diff] [review] bug-969372-mrleak.v02.patch Review of attachment 8441231 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. Please create a separate patch for the changes to FreezableElements, including the addition of nsIDocumentActivity in that patch. ::: content/base/public/nsIDocument.h @@ +1691,1 @@ > void EnumerateFreezableElements(FreezableElementEnumerator aEnumerator, We no longer are dealing with only elements. So I think we should change the names here from "freezable elements" to just "activity observers" and document in comments that we send notifications to these observers when document activity or visibility changes. Also document that the observers can be HTMLMediaElement, nsIObjectLoadingContent or nsIDocumentActivity. ::: content/media/nsIDocumentActivity.h @@ +8,5 @@ > + > +#include "nsISupports.h" > + > +/* starting interface: nsIDocumentActivity */ > +#define NS_IDOCUMENTACTIVITY_IID_STR "9b9f584e-efa8-11e3-bb74-5edd1d5d46b0" I don't think you need this #define @@ +21,5 @@ > + > + NS_DECLARE_STATIC_IID_ACCESSOR(NS_IDOCUMENTACTIVITY_IID) > + > + /* void NotifyOwnerDocumentActivityChanged (); */ > + NS_IMETHOD NotifyOwnerDocumentActivityChanged(void) = 0; just make this virtual void @@ +29,5 @@ > +NS_DEFINE_STATIC_IID_ACCESSOR(nsIDocumentActivity, NS_IDOCUMENTACTIVITY_IID) > + > +/* Use this macro when declaring classes that implement this interface. */ > +#define NS_DECL_NSIDOCUMENTACTIVITY \ > + NS_IMETHOD NotifyOwnerDocumentActivityChanged(void); trailing whitespace @@ +37,5 @@ > + NS_IMETHOD NotifyOwnerDocumentActivityChanged(void) { return _to NotifyOwnerDocumentActivityChanged(); } > + > +/* Use this macro to declare functions that forward the behavior of this interface to another object in a safe way. */ > +#define NS_FORWARD_SAFE_NSIDOCUMENTACTIVITY(_to) \ > + NS_IMETHOD NotifyOwnerDocumentActivityChanged(void) { return !_to ? NS_ERROR_NULL_POINTER : _to->NotifyOwnerDocumentActivityChanged(); } delete these forwarding macros
Attachment #8441231 - Flags: review?(roc) → review-
Attachment #8441231 - Attachment is obsolete: true
Attachment #8441966 - Flags: review?(roc)
Attachment #8441967 - Flags: review?(roc)
Attached patch bug-969372-part3-testcase.patch (obsolete) — — Splinter Review
Attachment #8441968 - Flags: review?(roc)
Rebased, r=roc
Attachment #8441966 - Attachment is obsolete: true
Attachment #8442567 - Flags: review+
Rebased, r=roc
Attachment #8441967 - Attachment is obsolete: true
Attachment #8442568 - Flags: review+
Rebased, r=roc Push to try server again because these patches have wide affection. https://tbpl.mozilla.org/?tree=Try&rev=de78da0ea78a
Attachment #8441968 - Attachment is obsolete: true
Attachment #8442570 - Flags: review+
Hi sheriff, Please help to check in the three patches: bug-969372-part1-nsIDocumentActivity.patch bug-969372-part2-MediaRecorder.patch bug-969372-part3-testcase.patch
Keywords: checkin-needed
Depends on: 1317805
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: