1284038 - Intermittent browser/base/content/test/webrtc/browser_devices_get_user_media.js | expected notification recording-device-events - Got 2, expected 1

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P3)

Description

[Treeherder Bug Filer]

Filed by: [e-mail]

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

Comment 1

[Andrew Comminos [:acomminos]]

Attachment: Use desktop-test-large instances.

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

Comment 2

[Andrew Comminos [:acomminos]]

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 3

[Nils Ohlmeier [:drno]]

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 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 6

[Andreas Pehrson [:pehrsons]]

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.

Comment 7

[Andrew Comminos [:acomminos]]

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.

Comment 8

[Andrew Comminos [:acomminos]]

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?

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.)

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 11

[Andreas Pehrson [:pehrsons]]

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.

Comment 12

[Andrew Comminos [:acomminos]]

Attachment: Bug 1284038 - Workaround multiple recording-device-events being dispatched in browser/base/content/test/webrtc/browser_devices_get_user_media.js.

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

Comment 13

[Andrew Comminos [:acomminos]]

Comment on attachment 8777964 [details]
Use desktop-test-large instances.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69356/diff/1-2/

Comment 14

[Andrew Comminos [:acomminos]]

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 15

[Florian Quèze [:florian]]

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 16

[Andrew Comminos [:acomminos]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70032/diff/1-2/

Comment 17

[Florian Quèze [:florian]]

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!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf473fd356c4

Comment 20

[Intermittent Failures Robot]

9 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 6
* autoland: 2
* try: 1

Platform breakdown:
* linux64: 8
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1284038&startday=2016-08-08&endday=2016-08-14&tree=all