1041599 - Plugin-container does not share system volume slider with chrome process

Status: RESOLVED FIXED

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

Description

[Jim Mathies [:jimm]]

str:

1) run some audo tests or visit youtube in an e10s window
2) play some video
3) check the system volume mixer

result: plugin-container is listed individually.

We had the same problem with plugins. There's some initialization work we need to do in the child.

Comment 1

[jmd-bugzilla]

This happens to me since enabling e10s in Firefox Dev Edition
Currently using Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0
Also the Win7 volume mixer is not remembering the volume setting for the additional plugin container process when closing and re-opening the browser. Volume setting for the normal Firefox process is remembered even after closing and re-opening.

Interestingly it seems this only applies to non-flash media content. Two processes appear in the volume mixer since enabling e10s. One "Firefox Developer Edition" with the Dev Edition logo. With this volume slider you can control the volume for flash content. The second process appearing is named "Plugin Container for FirefoxDeveloperEdition" and with the corresponding volume slider you can control the volume on non-flash videos

Noticed it first on youtube after enabling e10s. Using the new HTML5 player

str:
1. Start playing HTML5 Video, for example on YouTube
2. Check OS volume mixer

result: plugin container is listed in addition to the normal firefox process

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Bug 1104619 would fix this as a side-effect, I think (by making the content process's audio output actually come from the main process), but from comment #0 it sounds like there's a much lower-effort solution.

Comment 4

[Benjamin Smedberg]

Renominating for triage: I think this needs to block and should be in M9: when we shipped the equivalent bug in OOP plugins, it gather dups and angry users rather quickly.

Comment 5

[Jim Mathies [:jimm]]

Attachment: wip

This addresses the same problem for content that we had with plugins.

There are issues with removal and grouping of volume controls in the defasult system voluem control Sndvol is open for our entire session. We've had these problems since this code originally landed. I experimented around with a number of different fixes but wasn't able to find something that worked 100% of the time, but I think I've improved things somewhat on browser close. I'll file a follow up on remaining issues Iv'e seen which aren't e10s specific.

Comment 6

[Jim Mathies [:jimm]]

Attachment: MozReview Request: Bug 1041599 - Maintain a single volume control session between browser, content, and plugins on Windows. r?aklotz

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

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8729637 [details]
MozReview Request: Bug 1041599 - Maintain a single volume control session between browser, content, and plugins on Windows. r?aklotz

https://reviewboard.mozilla.org/r/39529/#review36221

::: dom/ipc/ContentChild.cpp:3047
(Diff revision 1)
>    if (os) {
>      os->NotifyObservers(static_cast<nsIContentChild*>(this),
>                            "content-child-shutdown", nullptr);
>    }
>  
> +#if defined XP_WIN

General nit for entire patch: s/defined foo/defined(foo)/ for consistency with other code

::: dom/ipc/ContentChild.cpp:3179
(Diff revision 1)
> +#if !defined XP_WIN
> +    NS_RUNTIMEABORT("Not Reached!");
> +    return false;
> +#else
> +    nsresult rv = mozilla::widget::RecvAudioSessionData(aId, aDisplayName, aIconPath);
> +    NS_ENSURE_SUCCESS(rv, true); // Bail early if this fails

NS_ENSURE_SUCCESS is banned in new code, please replace with explicit return

::: widget/windows/AudioSession.cpp:190
(Diff revision 1)
>  
>    HRESULT hr;
>  
> -  // Don't check for errors in case something already initialized COM
> -  // on this thread.
> +  // There's a matching CoUninit in Stop() for this tied to a state of
> +  // UNINITIALIZED.
>    CoInitialize(nullptr);

I'm a little bit uneasy about our omission of return value check in this call.

if FAILED(CoInitialize(nullptr)) then we shouldn't call CoUnitialize() in Stop(), as that would mess up the balancing of Init to Unit calls.

Of course, the only time that could feasibly happen is if the current thread was previously CoInitializeEx'd with the multithreaded apartment, so maybe that is a non-issue.

Actually, yeah, let's just add an assertion that CoInitialize SUCCEEEDED here.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch

Comment 9

[Jim Mathies [:jimm]]

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcff334f8746

Comment 11

[(PTO June 19-July 7) Ryan VanderMeulen]

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

Comment 12

[Chris Peterson [:cpeterson]]

Jim, should we uplift this fix to Aurora 47 or even Beta 46?

Comment 13

[Jim Mathies [:jimm]]

(In reply to Chris Peterson [:cpeterson] from comment #12)
> Jim, should we uplift this fix to Aurora 47 or even Beta 46?

I'm planning on uplifting to 47 in a bit.

Comment 15

[Rodrigo]

(In reply to Jim Mathies [:jimm] from comment #13)
> (In reply to Chris Peterson [:cpeterson] from comment #12)
> > Jim, should we uplift this fix to Aurora 47 or even Beta 46?
> 
> I'm planning on uplifting to 47 in a bit.

Is it still gonna be uplifted to 47?

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 8729965 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]:
e10s
[User impact if declined]:
polishy issue with the system volume applet, content processes get get their own entry independent of firefox proper.
[Describe test coverage new/current, TreeHerder]:
on mc for a week.
[Risks and why]: 
low, well understood code.
[String/UUID change made/needed]:
none.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8729965 [details] [diff] [review]
patch

e10s related, stabilized on Nightly for a week, Aurora47+

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/261a56c9e348