Closed Bug 1358372 Opened 7 years ago Closed 7 years ago

Volume Mixer processes being created for each tab

Categories

(Core :: DOM: Content Processes, defect, P1)

55 Branch
Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: rodrigo.mcunha, Assigned: handyman, NeedInfo)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [e10s-multi:-])

Attachments

(4 files, 2 obsolete files)

Windows 10 v1703, Firefox 55.0a1 (20170420030346)

Steps to reproduce:
1. Go to www.youtube.com (or any website that contains audio) and open a video.
2. Check on the volume mixer, if the process that is handling the audio on that tab is maxed.
3. If it is change it to another value.
4. Open some more audio tabs.
5. Notice new processes being created on the volume mixer with maxed volume (each one of those correspond to each opened tab, or not, because when it reaches 4 processes, tabs will start using each others processes).
6. Close all the tabs and notice the processes are still there and will take a while to clear up but one of them will remain with the saved volume value from step 3.

If you need more info, please ask.
Attached file about:support
I want to correct something on step 6: the volume value which was saved was the main Firefox process one, not the other tabs' processes'.
But apparently once you set all the 4 processes' volume values, after the processes clear up on the volume mixer when you create new tabs the old volume values will be saved for the session.

I'm very sorry for the spam, but I'm trying to give you the most information I have on this.
Component: Untriaged → Audio/Video
Product: Firefox → Core
Component: Audio/Video → Audio/Video: Playback
Summary: Volume Mixer processes being created for each tab and maxing out the volume for those → Volume Mixer processes being created for each tab (max of 4) with maxed out volume
This is caused by having 4 content processes.
Blocks: e10s-multi
Component: Audio/Video: Playback → Audio/Video: cubeb
Priority: -- → P3
We're shipping 4 content process with 54. Is this bug p3 because it's not important enough to fix it before 4 content processes reaching the release population? To me it sounds like this bug can cause an annoying and confusing user experience.
Flags: needinfo?(ajones)
Whiteboard: [e10s-multi:?]
Component: Audio/Video: cubeb → DOM: Content Processes
OS: Unspecified → Windows
Whiteboard: [e10s-multi:?] → [e10s-multi:-]
Looks like the proper fix is to implement session grouping using [0].

[0]: https://msdn.microsoft.com/en-us/library/windows/desktop/dd370796(v=vs.85).aspx
(In reply to Paul Adenot (:padenot) (On PTO until the 1st of June) from comment #6)
> Looks like the proper fix is to implement session grouping using [0].
> 
> [0]:
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd370796(v=vs.85).
> aspx

We already do this, the interface we work with is a bit buggy though. What we should do is have content processes set up a session only when audio playback occurs and then destroy that session after audio playback is complete.
Flags: needinfo?(ajones)
Priority: P3 → P2
Depends on: 1362220
Could somebody change priority of this bug to highest one? This bug is more than annoying, one needs to open the Windows volume mixer several time per Firefox session in order to adjust volume for new tab(s) playing a video.
I want to point out that this is a problem on Windows 10 (1709 / 16299.64) x64.  It's NOT a problem on Windows 7-SP1 x64.  (No clue about Windows 8.1).  On Win7, the volume mixer properly shows only one entry and honors the user's volume/mute preferences for Firefox.
To enable or disable multi-process Firefox, do the following
1.    Type about:config in the browser's address bar
2.    Confirm that you will be careful
3.    Search for browser.tabs.remote.autostart
4.    Double-click on the preference
5.    Set it to False
6.    Do the same for the other browser.tabs.remote.autostart2
If you're going to go into about:config, try setting dom.ipc.processCount to 1 first, instead of turning off e10s.  That's not recommended for ideal performance, but it should be safer than trying to use the unsupported non-e10s mode.
(In reply to Hackwell from comment #15)
> To enable or disable multi-process Firefox, do the following
> 1.    Type about:config in the browser's address bar
> 2.    Confirm that you will be careful
> 3.    Search for browser.tabs.remote.autostart
> 4.    Double-click on the preference
> 5.    Set it to False
> 6.    Do the same for the other browser.tabs.remote.autostart2

This solution is the best for the moment :-)

Thank you ! You made my day ^^
(In reply to Jed Davis [:jld] (⏰UTC-7) from comment #17)
> If you're going to go into about:config, try setting dom.ipc.processCount to
> 1 first, instead of turning off e10s.  That's not recommended for ideal
> performance, but it should be safer than trying to use the unsupported
> non-e10s mode.

I'm sorry but i tried this solution but no results :-(
What do you mean about unsupported non e10s mode ?
Cheers !
Summary: Volume Mixer processes being created for each tab (max of 4) with maxed out volume → Volume Mixer processes being created for each tab
Assignee: nobody → davidp99
Priority: P2 → P1
This bug is crazy.  These patches don't completely fix things but they help a lot.  That's probably not good enough.  I go into way too much detail below but the top-level points are:

* We currently can end up with an unbounded number of extra sliders.
* This is an issue with sndvol.exe on Windows (10).  MS may or may not be interested in fixing it but probably won't be in a hurry to do so.
* The content process sandbox exacerbates the issue.  Patch 1 mitigates the damage but does not completely fix it.  Specifically, it seems to limit the number of SndVol.exe sliders to 2 (or 3 if sndvol was already open when the browser launched).  (The correct answer is '1'.)
* The issue is also present, albeit more random, with the sandbox off.  Patch 2 seems to reduce the scale of the issue in that case.  With it, behavior seems to be about the same as with the sandbox on with the patch 1 fix.

---------------------------

* We can end up with an unbounded number of extra sliders.

The bug is at its worst behavior when:
* sndvol.exe is opened _before_ launching the browser
* We open and close and open and close a bunch of tabs.  The closed tabs tend to leave behind ghost volume sliders.
* I have confirmed, by giving the various processes' sliders different labels, that these additional sliders definitely come from our AudioSession class.  This isn't e.g. something unknown done by cubeb code.

---------------------------

* This is an issue with Windows (10).  MS may or may not be interested in fixing it but I doubt they would be in a hurry.

Here's a breakdown of how this looks with other browsers:

* I suspect that Chrome is brokerking all audio so that it runs from one process.  If so then they can still set the title/icon with the AudioSessionManager but they don't need its grouping parameter feature.  If not... then I don't know what they're doing.  But their behavior wrt sndvol.exe is pristine.  I didn't see Chromium code about this (and didn't check a build of it).
* Internet Explorer (which I didn't even realize existed in Windows 10) has the same bug that we do.  This is telling.
* Edge doesn't seem to connect to sndvol at all.  It shows no application-specific slider.  Is this the behavior you get when you don't initialize the AudioSessionManager?  If so, that may be a wise more forward for us (unless/until we remote audio from one process like Chrome).

The docs suggest that the AudioSessionManager's grouping parameter was created for IE.  If MS isn't using this grouping parameter behavior anymore then we may not want to rely on it working going forward.

Note: IAudioSessionManager::GetAudioSessionControl [1] has another facility for unifying volume controls across processes.  Its not well documented but, if I understood it correctly, then it doesn't work anymore either.  The reason we used the grouping parameter instead of this is that the grouping parameter would work with external applications like Windows Media Player (and maybe Flash?) but thats not much concern for us.  But it didn't work anyway so...

--------------------------------

* The content process sandbox exacerbates the issue.  Patch 1 in this series ameliorates the issue but does not completely fix it.

Setting the grouping parameter is the first thing we do after lowering (ie activating) the content sandbox.  This patch moves it to be the last thing we do before lowering the sandbox.  This _mostly_ fixes the issue when the sandbox is on.
If sndvol.exe isn't open before launching the browser, sndvol seems to fluctuate between 1 and 2 sliders (randomly, based on opening/closing tabs).
If sndvol is open first then we tend to get three sliders.  Its unclear if there is a reliable pattern but I think there is usually the volume slider created for the chrome process and two more for all of the content procs.

--------------------------------

* The issue is also present, albeit more random, with the sandbox off.  Which is surprising.  Patch 2 seems to reduce the scale of the issue in that case.  With it, behavior seems to be about the same as with the sandbox on.

Clearly, there is some kind of timing issue at play here.  There doesn't seem to be a reliable way to time things to completely fix the issue but delaying the setting of the grouping parameter (dispatching the operation to the main thread) seems to help alot.  Without this patch, we usually get 4 dead volume sliders on launch (whether sndvol was already open or not) _and_ any number of extra sliders, based on opening/closing tabs.  The grouping parameter basically did nothing.

----------------------------------

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd370963(v=vs.85).aspx
Attachment #8941657 - Flags: review?(jmathies)
Attachment #8941657 - Attachment description: Part 1. Move Windows content process AudioSession init before sandbox lowering → 1. Move Windows content process AudioSession init before sandbox lowering
I always wondered if the grouping stuff was broken, it just never behaved like the docs claimed.
Attachment #8941657 - Flags: review?(jmathies) → review+
Comment on attachment 8941658 [details] [diff] [review]
2. Run Windows AudioSessionControl operations on main thread

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

Have you tested disconnecting one audio output device and then plugging in another? I remember there were issues with reentrancy on the disconnect that caused problems.
Attachment #8941658 - Flags: review?(jmathies) → review+
I didn't check the default device switch issue -- I remember that as well.  I'll check it.

On a broader note, I've written up a simple test case for this for MS (attached)... but it doesn't fail.  I have no idea why yet but this behaves like a timing deal and I can imagine it has a completely different performance profile than the browser.  Firefox has more in common with IE, which does fail (IE seems to max out at 2 volume controls if you have 2+ tabs open and playing sound).  Or maybe its that my test never actually plays a sound.  Who knows.

My instinct is still to report this to MS, submit these work-arounds and hope for the best but I could spend more time trying to isolate the difference in behavior here.  Remoting audio is coming soon and should fix all of this anyway.  Got an opinion or does that sound good to you?

(FYI, the test case is easy to use -- when you launch it, it opens and audio session.  Select the menu option "Action/Launch Child Process" and it will launch another instance with an audio session in the same group.  This bug was supposed to cause that to create a second volume slider in sndvol.)
Flags: needinfo?(jmathies)
(In reply to David Parks (dparks) [:handyman] from comment #24)
> Created attachment 8941955 [details]
> WIP: TestGroupingParam.zip
> 
> I didn't check the default device switch issue -- I remember that as well. 
> I'll check it.
> 
> On a broader note, I've written up a simple test case for this for MS
> (attached)... but it doesn't fail.  I have no idea why yet but this behaves
> like a timing deal and I can imagine it has a completely different
> performance profile than the browser.  Firefox has more in common with IE,
> which does fail (IE seems to max out at 2 volume controls if you have 2+
> tabs open and playing sound).  Or maybe its that my test never actually
> plays a sound.  Who knows.
> 
> My instinct is still to report this to MS, submit these work-arounds and
> hope for the best but I could spend more time trying to isolate the
> difference in behavior here.  Remoting audio is coming soon and should fix
> all of this anyway.  Got an opinion or does that sound good to you?

I think you should definitely land anything that improves things for e10s. 

I tested with your test app, first open I don't see a volume slider created, first child process launch I get an audio slider. Then additional child launches don't change anything, and closing every one of the application results in the audio slider going away. AFAICT this app is acting exactly the way you would expect it too.
Flags: needinfo?(jmathies)
(In reply to David Parks (dparks) [:handyman] from comment #24)
> My instinct is still to report this to MS, submit these work-arounds and

They'll want a test case that reproduces though. We have Firefox but that's not a great test case.
(In reply to Jim Mathies [:jimm] from comment #23)
> Have you tested disconnecting one audio output device and then plugging in
> another? I remember there were issues with reentrancy on the disconnect that
> caused problems.

I checked it.  Changing the default audio device works fine.
Keywords: checkin-needed
Windows audio data guy here - can someone repro this and file a problem report with logs?

https://blogs.msdn.microsoft.com/matthew_van_eerde/2016/09/26/report-problems-with-logs-and-suggest-features-with-the-feedback-hub/

Once filed, grab a direct link from Feedback Hub / Feedback / My feedback / (click on the problem report) / Share

Post the direct link here or email me
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b2bbebb2f0
Part 1 - Move Windows content process AudioSession init before sandbox lowering r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/76e48321127d
Part 2 - Run Windows AudioSessionControl operations on main thread r=jimm
Keywords: checkin-needed
Backed out 2 changesets (bug 1358372) for mochitest failures on AudioSession, AudioSession::CommitAudioSessionData, Mutex, nsStringBuffer.

Here's a log with the fail: https://treeherder.mozilla.org/logviewer.html#?job_id=156836288&repo=mozilla-inbound&lineNumber=16162

Treeherder push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3b3d07447b8203e7370a51972d34064d701fb44e

Backout push: https://hg.mozilla.org/integration/mozilla-inbound/rev/871fecd7db1d0847e0b4c48c17eda859f14254f7
Flags: needinfo?(davidp99)
Hey Matthew,

Big thanks for the help.  I've filed the bug here: https://aka.ms/Bqbdp5
I used Firefox for the logs but I could use IE if it makes a difference.  Let me know if there's something else I can do to assist.  Thanks again!
Flags: needinfo?(davidp99) → needinfo?(matthew.van.eerde)
We'll take a look (by the way I haven't worked at Hispanic Business Inc. since 2006)
Flags: needinfo?(mvaneerde)
Hi Jim,

    I see that Firefox on RS3 creates 5 processes but only one create an AudioSession. Whereas in RS4, each of the 5 processes creates a seperate AudioSession. What determines if a process should call AudioSession::GetSingleton()or not. 

Thanks,
Arthi.
(Clearing the messed up NI.)

Arthi, I'm not sure what you are talking about when you mention RS3 and RS4 but, if you are working on this bug, I'm probably the one you want.  I can tell you that Firefox creates a number of processes that would not create an AudioSession -- for example, the graphics GPU process, the launcher process and any plugin processes.  The only processes that _do_ create them are the main process (this is the root process once firefox has finished init) and any content/tab processes (identifiable in proc monitor as their command line parameters end with the word 'tab').  Is that what you are looking for?
Flags: needinfo?(matthew.van.eerde)
This happens in Windows 7 too, and not only in Windows 10 as some users posted.
Has annything changed in the FirFox implementation of using audio sessions recently?
(In reply to David Parks (dparks) [:handyman] from comment #35)
> (Clearing the messed up NI.)
> 
> Arthi, I'm not sure what you are talking about when you mention RS3 and RS4
> but, if you are working on this bug, I'm probably the one you want.  I can
> tell you that Firefox creates a number of processes that would not create an
> AudioSession -- for example, the graphics GPU process, the launcher process
> and any plugin processes.  The only processes that _do_ create them are the
> main process (this is the root process once firefox has finished init) and
> any content/tab processes (identifiable in proc monitor as their command
> line parameters end with the word 'tab').  Is that what you are looking for?

Hi David,

   I see firefox creating multiple processes and each process calling GetAudioSessionControl. For the first process, GetAudioSessionControl succeeds and I see a call to SetGroupingParam.

   From all the other processes also I see calls to GetAudioSessionControl. However, GetAudioSessionControl is failing with ERROR_ACCESS_DENIED. How are these processes being created? This failure seems to prevent the calls to SetGroupingParam.

   OpenSCManager is failing with ERROR_ACCESS_DENIED.

Thanks,
Arthi.
coffinz: Good to know.  I was definitely operating on the belief that this didn't happen outside of Win10.

Arthi: Nothing should have changed.  If something does happen, it would probably be in AudioSession.cpp so you would be able to see it here [1].

Strange -- I'm not seeing the failures you are.  In the AudioSession.cpp source, a failed call to GetAudioSessionControl would stop the call to SetGroupingParam [2].  I should mention that I never saw it do that.  In the event of that failure, I would also expect no volume slider to be created (since there is no audio) but I know that this is not this bug because I had instrumented the firefox code to set the grouping param as we do now but also to change the display name (on IAudioSessionControl::SetDisplayName) for each process (I added the PID to the end).  In that case, the Windows docs say that one process' display name should 'win' and the others would be ignored.  But instead, since I was already getting multiple sliders anyway, it just set their display names so that each had a different one.  And setting the display name comes after getting the audio session (and after setting the grouping param).

The child processes are created with a sandbox.  Its all pretty involved.  Is there something specific you need to know?

[1] https://hg.mozilla.org/mozilla-central/log/tip/widget/windows/AudioSession.cpp
[2] https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/widget/windows/AudioSession.cpp#257
WRT the hack in these patches:

The test failure turned out to be due to the ThreadManager not initializing in the NPAPI process.  I'd already fixed this in bug 1382251, which is landing.

Checking tests with the patches from 1382251:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8acf9dbd07062898aeb9c094fddd25ab0419d134
Depends on: 1382251
Firefox 58.0.1

I confirm that happens with Windows 8.1 AND Windows 7.

With these 2 OS, 3 processes appear in "Windows mixer", the first one with the level defined previously and the others at 100 (max).

Thanks
Previous tests were failing due to a race between the Stop and CommitAudioSessionData operations.  Fix was just to keep CommitAudioSessionData from running if Stop already had.  The AudioSession is refcounted so this is safe.

New tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ae410ede1b0ab037a5672ead3b3fdaabc9a392e&selectedJob=160524920
Attachment #8941658 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f865a069f8eb
Part 1 - Move Windows content process AudioSession init before sandbox lowering r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef538003bf44
Part 2 - Run Windows AudioSessionControl operations on main thread r=jimm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f865a069f8eb
https://hg.mozilla.org/mozilla-central/rev/ef538003bf44
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Should we consider this for beta59?
Flags: needinfo?(davidp99)
(In reply to Dorel Luca [:dluca] from comment #45)
> https://hg.mozilla.org/mozilla-central/rev/f865a069f8eb
> https://hg.mozilla.org/mozilla-central/rev/ef538003bf44

On Win7, I now see sliders getting created briefly and then destroyed for each new content process. Other than this minor polish issue, everything looks good.
Well sort of, sometimes I'm left with two sliders when the browser is open, not sure about STR for this yet. Will keep an eye on it.
Comment on attachment 8948869 [details] [diff] [review]
1. Move Windows content process AudioSession init before sandbox lowering (r=jimm)

I meant to post this earlier but I wanted to make sure there weren't more thread issues before trying for uplift.  Also, jimm, remember that this isn't exactly a fix, just a hack to make the problem less terrible.  If this gets fixed, it'll have been the engineers at Microsoft.  comment 20 has the details.

Approval Request Comment
[Feature/Bug causing the regression]:
N/A (this is the OS) but the content sandbox does make the issue worse

[User impact if declined]:
Without the fix, firefox creates an unbounded number of sliders as the user opens and closes tabs with audio.  With the fix, the max number of sliders is 2 or 3.  (The ideal answer would be '1').

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

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

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
Very mildly

[Why is the change risky/not risky?]:
The code is run constantly and has had time to gel on Nightly.  The only likely issues would be concurrency-based (in patch #2) and we'd expect to have seen them by now.

[String changes made/needed]:
N/A
Flags: needinfo?(davidp99)
Attachment #8948869 - Flags: approval-mozilla-beta?
Approval Request Comment
[Feature/Bug causing the regression]:
N/A (this is the OS) but the content sandbox does make the issue worse

[User impact if declined]:
Without the fix, firefox creates an unbounded number of sliders as the user opens and closes tabs with audio.  With the fix, the max number of sliders is 2 or 3.  (The ideal answer would be '1').

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

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

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
Very mildly

[Why is the change risky/not risky?]:
The code is run constantly and has had time to gel on Nightly.  The only likely issues would be concurrency-based and we'd expect to have seen them by now.

[String changes made/needed]:
N/A
Comment on attachment 8948870 [details] [diff] [review]
2. Run Windows AudioSessionControl operations on main thread (r=jimm)

see comment 50
Attachment #8948870 - Flags: approval-mozilla-beta?
Comment on attachment 8948869 [details] [diff] [review]
1. Move Windows content process AudioSession init before sandbox lowering (r=jimm)

This is a reasonably severe issue and would be good to fix. We are still a month away from go live, the risk is acceptable as this fix would get a good bit of testing from beta end-users, beta59+
Attachment #8948869 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8948870 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This is due to the ThreadManager not being initialized.  It was dealt with as part of bug 1382251.  That change is less safe than the patches here and this bug isn't a killer so we are spiking the uplift.
Flags: needinfo?(davidp99)
Comment on attachment 8948869 [details] [diff] [review]
1. Move Windows content process AudioSession init before sandbox lowering (r=jimm)

clearing flag as this had to be backed out of beta
Attachment #8948869 - Flags: approval-mozilla-beta+
Attachment #8948870 - Flags: approval-mozilla-beta+
(In reply to lanovoy from comment #57)
> How is it resolved as fixed when latest Firefox Nightly (61.0a1 (2018-05-01)
> (64-bit)) with 8 tabs opened has about 20 (!) audio mixer sliders for me
> (Windows 10 1709 latest as well)?

You might be interested in bug 1458034. If that bug is not applicable to you, then please file a new one.
Blocks: 1385207
for nightly 20181007100131 and also for release version there are multiple entries for Firefox in Windows 10 1803/1809 under --> Preferences --> System --> Sound --> Extended soundoptions
But only one entry have sound-function, the others are dead.
See Also: → 1458034
See Also: → 1579843
See Also: → 1722238
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: