1381638 - Severe audio distortion/slowdown/delay sharing mic in multiple tabs (regression)

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

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

STRs:
 1. Open https://jsfiddle.net/jib1/6oy8dmg4/ + share mic, in 5 tabs
 2. Snap fingers

Expected result: Responsive feedback in speakers.

Actual result: After 5th tab opens, severe distortion of audio where audio slows down to half speed, and there's a multi-second delay.

Alternative STRs:
 1. In about:config, set dom.ipc.processCount = 1 and close all tabs
 2. Open https://jsfiddle.net/jib1/6oy8dmg4/ + share mic, in 2 tabs

Reproduced on OSX and Windows 10.

Regression range:

Last good revision: 15cc8dd663627ec12ecd4be11a69f0caad8e4e11
First bad revision: 29ac12c81bcda10a981dfe3371f70a9b17a19edd

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=15cc8dd663627ec12ecd4be11a69f0caad8e4e11&tochange=29ac12c81bcda10a981dfe3371f70a9b17a19edd

Workaround:

Increase dom.ipc.processCount to prolong the inevitable (more tabs needed to reproduce).

Comment 1

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1381638 - Prevent sharing multiple input devices in the same process.

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

Comment 3

[Paul Adenot (:padenot)]

Attachment: Bug 1381638 - Disable msg-per-document. r?jib r?jesup

Comment 4

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

Comment on attachment 8902296 [details] [diff] [review]
Bug 1381638 - Disable msg-per-document. r?jib r?jesup

Review of attachment 8902296 [details] [diff] [review]:
-----------------------------------------------------------------

Wfm.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8902296 [details] [diff] [review]
Bug 1381638 - Disable msg-per-document. r?jib r?jesup

r+, but hold on landing it for the moment until we coordinate with billm

Comment 6

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

Comment on attachment 8902242 [details]
Bug 1381638 - Prevent sharing multiple input devices in the same process.

clearing r? as we went a different direction.

Comment 7

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 8

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

[Tracking Requested - why for this release]: Severe audio regression when used concurrently in tabs.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't dynamically switch to/from fast-path gUM audio

Comment 10

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

Comment on attachment 8902242 [details]
Bug 1381638 - Prevent sharing multiple input devices in the same process.

https://reviewboard.mozilla.org/r/173770/#review185556

Discussed with jesup, and we've decided to take this patch for now and try to get it to 56. It's got a better chance of being approved at this point than "regress quantum".

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8908773 [details] [diff] [review]
don't dynamically switch to/from fast-path gUM audio

wrong bug

Comment 12

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

Comment on attachment 8902242 [details]
Bug 1381638 - Prevent sharing multiple input devices in the same process.

https://reviewboard.mozilla.org/r/173770/#review185620

Nevermind, it broke applyConstraints.

Comment 13

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

Attachment: Bug 1381638 - Prevent sharing multiple input devices in the same process.

Comment 14

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

Attachment: Bug 1381638 - Prevent sharing multiple input devices in the same process.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8908933 [details] [diff] [review]
Bug 1381638 - Prevent sharing multiple input devices in the same process.

Review of attachment 8908933 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comment change and optional code change.  File a new bug to resolve the issue, or land this one under leave-open and put that number in there

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +471,5 @@
>    }
>  
> +  // Until we fix (or wallpaper) support for multiple mic input
> +  // (Bug 1238038) fail allocation for a second device by blocking
> +  // sources with different Graphs

// Until we fix bug XXXXX we need to block a second tab (OuterWindow)
// from opening an already-open device.  If it's the same tab, they
// will share a Graph(), and we can allow it.

@@ +476,5 @@
> +  {
> +    auto* graph = aStream->Graph();
> +
> +    for (auto& source: mSources) {
> +      if (source->Graph() != graph) {

I don't mind checking them all.... but it's useless since this code ensures they all match, so you only need to check one.  You could code it as "if (!mSources.Empty() && aStream->Graph() != mSource[0].Graph()) { return NS_ERROR....}"

But I'll also take this

Comment 16

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

Attachment: Bug 1381638 - Prevent sharing multiple input devices in the same process.

Carrying forward r=jesup.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62efa118ea7f
Prevent sharing multiple input devices in the same process. r=jesup

Comment 18

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

Comment on attachment 8908945 [details] [diff] [review]
Bug 1381638 - Prevent sharing multiple input devices in the same process.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1330360 
[User impact if declined]:

Sharing microphone in multiple tabs causes severe distortion of mic audio where it slows down to half speed (Vader voice), and there's continual build-up of multi-second delay, rendering microphone unusable in both tabs until both are closed, when the two tabs are in the same content process. Does not happen if tabs are in separate content processes.

Workaround is to close mic-access in both tabs and try again, or open a new tab and hope to get a different process, if  available.

In contrast, with this band-aid patch, we instead fail getUserMedia() mic requests with "NotReadableError: Concurrent mic process limit.", but only when this would happen, to prevent the audio distortion. The first tab continues to work, but the second page sees getUserMedia failing, which sites generally check for already (users denying permission), and the workaround is the same. The thinking here is this is less embarrassing, as severely distorted audio is a poor user experience, and is directly attributable to the browser.

We chose this band-aid fix to avoid regressing quantum (which reverting the original patch would have done).

[Is this code covered by automated tests?]: No. Testing use across content processes is difficult in the tree.
[Has the fix been verified in Nightly?]: Verified locally. Still on inbound.

[Needs manual test from QE? If yes, steps to reproduce]: 

Use steps in comment 0.
I also verified multiple getUserMedia calls from the same page still works here: https://jsfiddle.net/jib1/wemt0fvs/

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]:

It is not risky, because the patch reuses an existing failure path.

We also only fail when there already are one or more active streams using MediaEngineWebRTCMicrophoneSource in the same content process, AND our MSG is different from theirs (bug 1330360 has different MSGs per document).

[String changes made/needed]: none

Comment 19

[Liz Henry (:lizzard)]

Tracking since this sounds like a bad regression. 
I'll come back to approve this for m-r (56) tomorrow to make sure it has landed on m-c successfully. 

Right now, m-c and beta (both 57) are still going to be synced periodically, so we don't need special uplift to beta.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_devices_get_user_media_in_frame.js failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/13aea0c07147bf77029652944a6b4c4d3936e8f4

https://treeherder.mozilla.org/logviewer.html#?job_id=131524595&repo=mozilla-inbound

Comment 21

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Backed out for browser_devices_get_user_media_in_frame.js failures.

Green locally. That test has known problems like bug 1369731 (intermittent leaked docshell) which we don't yet understand. If a second in-process (leaked) document has active gUM, it would mean a second MSG in the mix which would rightly trigger the error we've added (or distort microphone audio absent our patch) here. This test is already turned off for debug, I'm tempted to turn it off further to land this.

This error guards against mic distortion. I suppose there may be real-world cases broken by this, like sites opening a popup to continue an already running customer-service webrtc chat, that will now cause an error, but these are cases that previously would have been ruined by unusable microphone distortion. At least with this patch web developers might learn about it directly from errors instead users switching browser over unusable mic, without maybe even reporting it to the site.

> https://treeherder.mozilla.org/logviewer.html#?job_id=131524595&repo=mozilla-
> inbound

For context:

[task 2017-09-16T22:50:50.361Z] 22:50:50     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js | received ok - Got error: NotReadableError: Concurrent mic process limit., expected ok
[task 2017-09-16T22:50:50.367Z] 22:50:50     INFO - Stack trace:
[task 2017-09-16T22:50:50.369Z] 22:50:50     INFO - chrome://mochikit/content/browser-test.js:test_is:1011
[task 2017-09-16T22:50:50.373Z] 22:50:50     INFO - chrome://mochitests/content/browser/browser/base/content/test/webrtc/head.js:promiseMessage/promise</<:278
[task 2017-09-16T22:50:50.375Z] 22:50:50     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-09-16T22:50:50.379Z] 22:50:50     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js | Uncaught exception - 
[task 2017-09-16T22:50:50.385Z] 22:50:50     INFO - Leaving test bound test
[task 2017-09-16T22:50:50.387Z] 22:50:50     INFO - GECKO(1756) | MEMORY STAT | vsize 910MB | residentFast 197MB | heapAllocated 77MB
[task 2017-09-16T22:50:50.390Z] 22:50:50     INFO - TEST-OK | browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js | took 17561ms
[task 2017-09-16T22:50:50.392Z] 22:50:50     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-09-16T22:50:50.395Z] 22:50:50     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js | Found an unexpected tab at the end of test run: https://example.com/browser/browser/base/content/test/webrtc/get_user_media_in_frame.html -

Comment 22

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

To add to manual test steps:

I also verified multiple getUserMedia calls from page and an iframe still works here: https://jsfiddle.net/jib1/90rfbspz/

Comment 23

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

Ok, it's perma-orange which should make it possible to find
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcfb3ae56925a3d5e685593207c3ab6697416a7c&selectedJob=131609408

Comment 24

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

Trying fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24b42ac70b4f845d4ac31ba1e81cd9c32eb6b25

Comment 25

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

Ok I misunderstood the test. I get it now.

2nd try fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=946cfdd6c4a4ccc18fe51976577ff90914e72c5b

Comment 26

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

That worked.

Comment 27

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

Attachment: Bug 1381638 - Change browser_devices_get_user_media_in_frame.js to test cam then cam+mic instead of mic then cam+mic, to avoid bug 1400488.

Comment 28

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8909152 [details] [diff] [review]
Bug 1381638 - Change browser_devices_get_user_media_in_frame.js to test cam then cam+mic instead of mic then cam+mic, to avoid bug 1400488.

Review of attachment 8909152 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7488798c5d862eee2ae63a6206fd7d4c924d267
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0af528ff3654b8c2ea728863369dea698ad99e1

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e7488798c5d8
https://hg.mozilla.org/mozilla-central/rev/c0af528ff365

Comment 31

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

Please see comment 18 for approval uplift request which still stands (subsequent activity was to fix a test that prevented landing).

Comment 32

[Liz Henry (:lizzard)]

Comment on attachment 8908945 [details] [diff] [review]
Bug 1381638 - Prevent sharing multiple input devices in the same process.

This makes a workaround possible for users with multiple tabs sharing audio. 
Let's take it for the 56 RC build later today.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fd583aba7c2b (FIREFOX_56b13_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-beta/rev/47fa81c799b4 (FIREFOX_56b13_RELBRANCH)

https://hg.mozilla.org/releases/mozilla-release/rev/f5d255813d92
https://hg.mozilla.org/releases/mozilla-release/rev/c2d1f91a9227

Comment 34

[Hani Yacoub, Desktop QA]

Build ID: 20170921220243
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.

Comment 35

[Timea Zsoldos [:tzsoldos], Desktop QA]

I have reproduced this issue using Firefox nightly from 2017.07.17 on Mac OS X 10.12.
I can confirm this issue is fixed, I verified using Firefox 56.0-build4 on Mac OS X 10.12, Windows 7 x64 (build x86), Windows 10 x64 and Ubuntu 16.04 x64

Comment 36

[Alexandru Simonca [:asimonca], Release Desktop QA]

Verified fixed in Nightly 57.0a1 (20170920220431) on Windows 10x64, MacOSX 10.12.6 and Ubuntu 16.04 x64.

Comment 37

[mroberts]

This makes testing a little annoying. I used to be able to test between two Firefox tabs. Is the plan to eventually reinstate the previous behavior?