1444007 - browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js abuses promise returned by BrowserTestUtils.removeTab

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

Separated from bug 1442465.

In bug 1442465, I'm about to stop returning Promise from BrowserTestUtils.removeTab.
currently it returns a Promise that is resolved once SessionStore info is updated for the tab, but that's completely misleading and just makes the test slower (see bug 1441872 also),
and also the tab is actually removed synchronously, so there's no point of waiting if the consumer just want to remove the tab.

Then, in browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js,
if I remove `await` from BrowserTestUtils.removeTab,
the following assertion fails, because there's 2 active streams while it expects 1 active stream.

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js#199-200
>     await BrowserTestUtils.removeTab(tab);
> 
>     // Check that we still show the sharing indicators for the first tab's stream.
>     await promiseWaitForCondition(() => webrtcUI.showCameraIndicator);
> ...
>     is(webrtcUI.getActiveStreams(true).length, 1, "1 active camera stream");
>     is(webrtcUI.getActiveStreams(true, true, true).length, 1, "1 active stream");

With `await`, the test waits for the SessionStore update,
and then waits for the condition that camera indicator is shown for the current tab,
but it actually doesn't wait for the stream of the 2nd tab gets removed.

The stream of the 2nd tab is removed in the following code.

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/browser/modules/webrtcUI.jsm#125-130
> var webrtcUI = {
> ...
>   forgetStreamsFromProcess(aProcessMM) {
>     // stream.processMM is null when e10s is disabled.
>     this._streams =
>       this._streams.filter(stream => stream.processMM &&
>                                      stream.processMM != aProcessMM);
>   },

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/browser/modules/webrtcUI.jsm#257-259
> var ContentWebRTC = {
> ...
>   receiveMessage(aMessage) {
>     switch (aMessage.name) {
> ...
>       case "webrtc:UpdatingIndicators":
>         webrtcUI.forgetStreamsFromProcess(aMessage.target);
>         break;


The "webrtc:UpdatingIndicators" is dispatched in the following:

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/browser/modules/ContentWebRTC.jsm#299
> function updateIndicators(aSubject, aTopic, aData) {
> ...
>   Services.cpmm.sendAsyncMessage("webrtc:UpdatingIndicators");

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/browser/modules/ContentWebRTC.jsm#43-45
> var ContentWebRTC = {
> ...
>   observe(aSubject, aTopic, aData) {
>     switch (aTopic) {
> ...
>       case "recording-device-events":
>         updateIndicators(aSubject, aTopic, aData);
>         break;

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/browser/modules/ContentObservers.js#66-68
> function webRTCObserve(aSubject, aTopic, aData) {
>   ContentWebRTC.observe(aSubject, aTopic, aData);
> }


And "recording-device-events" is notified in the following:

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/dom/media/MediaManager.cpp#2144-2146
> /* static */ nsresult
> MediaManager::NotifyRecordingStatusChange(nsPIDOMWindowInner* aWindow)
> {
> ...
>   obs->NotifyObservers(static_cast<nsIPropertyBag2*>(props),
>                        "recording-device-events",
>                        nullptr);

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/dom/media/MediaManager.cpp#4491
> void
> GetUserMediaWindowListener::NotifyChrome()
> {
> ...
>   NS_DispatchToMainThread(NS_NewRunnableFunction("MediaManager::NotifyChrome",
>                                                  [windowID = mWindowID]() {
> ...
>     nsresult rv = MediaManager::NotifyRecordingStatusChange(window->AsInner());

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/dom/media/MediaManager.cpp#4468-4471
> void
> GetUserMediaWindowListener::ChromeAffectingStateChanged()
> {
> ...
>   nsCOMPtr<nsIRunnable> runnable =
>     NewRunnableMethod("GetUserMediaWindowListener::NotifyChrome",
>                       this,
>                       &GetUserMediaWindowListener::NotifyChrome);

I haven't yet figured out the remaining part between it and tab removal tho,
there's already a lot of async things.

basically the test should wait for the last one, "webrtc:UpdatingIndicators",
or just wait for the condition that the number of active stream decreases.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I guess this is browser thing.

CCing some browser folks.

Comment 2

[Dão Gottwald [:dao]]

Hmm, I can't seem to reproduce this. I replaced:

  await BrowserTestUtils.removeTab(tab);

with:

  gBrowser.removeTab(tab);

and the test passes just fine locally over here. I'm on Ubuntu.

Comment 3

[Tooru Fujisawa [:arai]]

here's the try for bug 1442465 and failed job:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22a797a139e0cd70983ba81821b75730d7ec5c91&selectedJob=166403122

Comment 4

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #2)
> Hmm, I can't seem to reproduce this. I replaced:
> 
>   await BrowserTestUtils.removeTab(tab);
> 
> with:
> 
>   gBrowser.removeTab(tab);
> 
> and the test passes just fine locally over here. I'm on Ubuntu.

Pushed this to try, still not failing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfe53fd10c49758e19ac9d05bfbe97e07f01ce6d

Comment 5

[Tooru Fujisawa [:arai]]

hmm, seems to be really intermittent issue.
now I cannot reproduce it locally reliably, but not 0%.

Comment 6

[Tooru Fujisawa [:arai]]

but, anyway, theoretically the test should wait for something else than SessionStore update, right?

Comment 7

[Dão Gottwald [:dao]]

If it has to wait for something, then yes, this should be something else than SessionStore notifications.

Comment 8

[Dão Gottwald [:dao]]

Attachment: Bug 1444007 - Remove race condition in browser_devices_get_user_media_multi_process.js.

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

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8961315 [details]
Bug 1444007 - Remove race condition in browser_devices_get_user_media_multi_process.js.

https://reviewboard.mozilla.org/r/230120/#review235760

Looks reasonable.
thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6b39a06d680
Remove race condition in browser_devices_get_user_media_multi_process.js. r=arai

Comment 11

[Noemi Erli[:noemi_erli]]

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