Filed by: philringnalda@gmail.com https://treeherder.mozilla.org/logviewer.html#?job_id=31114285&repo=mozilla-inbound http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64-pgo/1467513070/mozilla-inbound_ubuntu64_vm_test_pgo-mochitest-browser-chrome-4-bm118-tests1-linux64-build2.txt.gz

Review commit: https://reviewboard.mozilla.org/r/69356/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69356/

This patch fixes the issue by ensuring that only a single recording-device-events "shutdown" event gets dispatched when a MediaStream stops. Previously, we would send the "shutdown" event for each track that was stopped. This makes sense as "starting" was only dispatched once even when starting a source with both audio and video tracks. With the patch applied on m3.large AWS instances, failures for non-e10s M-bc6 have gone from ~50% to 0; Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db162554c50ca89abb8a90276aae5a39659c7c37&filter-searchStr=bc6 After: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c687a518b9868264551e697843a9154bddfa462&filter-searchStr=bc6 Thanks!

Comment on attachment 8777964 [details] Use desktop-test-large instances. Re-assigning the review, because I'm not really an expert on MediaManager.cpp

Comment on attachment 8777964 [details] Use desktop-test-large instances. https://reviewboard.mozilla.org/r/69356/#review66524 I'm stealing this review. r- because it undoes Bug 1050802. Streams and tracks are tricky, so can we find a way to solve this without breaking that use case? What use-case specifically is this aimed at? ::: dom/media/MediaManager.cpp:605 (Diff revision 1) > if (mType == MEDIA_STOP) { > source->EndAllTrackAndFinish(); > - } > > - nsIRunnable *event = > + nsIRunnable *event = > - new GetUserMediaNotificationEvent(mListener, > + new GetUserMediaNotificationEvent(mListener, > - mType == MEDIA_STOP ? > + GetUserMediaNotificationEvent::STOPPING, This change makes the MEDIA_STOP_TRACK case into a no-op, which would seem to regress Bug 1050802. MEDIA_STOP_TRACK was added in that bug to let users end screensharing without also ending audio.

Also, I think we'd like to understand why this is intermittent, so we don't accidentally wallpaper over a potential problem.

Comment on attachment 8777964 [details] Use desktop-test-large instances. https://reviewboard.mozilla.org/r/69356/#review66652 What jib said. It's also important to note that an application is able to affect the UI the same way as the **stop sharing** action by stopping all tracks given to it by getUserMedia.

Thank you for the feedback; I wasn't aware that dispatching the event twice was expected behaviour for some cases. This makes more sense now. After some further investigation, the intermittent actually appears to be related to the timing of the garbage collection of the MediaStream- forcing a GC flush in closeStream() (found in browser/base/content/test/webrtc/get_user_media.html) causes the test suite to permafail. It looks like destroying the stream before the "STOPPING" MediaOperationTask can get dispatched causes multiple recording-device-events to be dispatched (one per stream). I'm going to continue looking into this.

Yes, this appears to indeed be the case- if the GC claims the DOMMediaStream before GetUserMediaCallbackMediaStreamListener::NotifyFinished gets called as a result of calling DOMMediaStream::Stop, we get two "recording-device-events" dispatched. The question is then; should we be consistent with the destructor and send two STOP_TRACK events in a call to Stop(), hold a strong reference to the DOMMediaStream while processing a call to Stop(), or something else entirely? Thoughts, jib?

stop() pretty much exists to avoid implicit actions on GC, so firing events on GC when stop() has already been called on all its tracks seems wrong. It also seems redundant, as I don't see who'd be interested in learning when an entirely inactive stream is destructed. Is there a way to detect this and not fire the events in this case? (I should say I have no idea what isApp is, and other code that seems to have grown around here, so hopefully this is doable.)

I haven't looked at the tests involved, but I think we'd also want to make sure tests call .stop() explicitly to avoid depending on GC.

It would make sense to not rely on stream stops since those are going away. We should really just look at the tracks. I *think* they correctly notifies MediaManager when they stop for any reason. There should even be asserts to make sure of it.

Here's a workaround for now; we should likely flag this test as TODO and discuss how to handle MediaStream.stop() GC raciness in a separate bug.

Comment on attachment 8778946 [details] Bug 1284038 - Workaround multiple recording-device-events being dispatched in browser/base/content/test/webrtc/browser_devices_get_user_media.js. https://reviewboard.mozilla.org/r/70032/#review68334 I'm willing to r+ such a patch just for the sake of no longer seeing so many intermittent failures on treeherder, but I would like follow-up but to be filed to figure out how we can avoid all these intermitent double recording-device-events notifications, and remove the promiseTodoObserverNotCalled thing entirely. I understand this bug will likely be low priority though. But I can't r+ this specific patch because the code it touches has just changed significantly with the landing of bug 1206233.

Comment on attachment 8778946 [details] Bug 1284038 - Workaround multiple recording-device-events being dispatched in browser/base/content/test/webrtc/browser_devices_get_user_media.js. https://reviewboard.mozilla.org/r/70032/#review68374 Thanks!

Pushed by acomminos@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cf473fd356c4 Workaround multiple recording-device-events being dispatched in browser/base/content/test/webrtc/browser_devices_get_user_media.js. r=florian