Closed Bug 1041599 Opened 5 years ago Closed 4 years ago

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

Categories

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

x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s m9+ ---
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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
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.
Duplicate of this bug: 1196082
See Also: → e10s-rename
Priority: -- → P1
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.
Assignee: nobody → jmathies
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #8729633 - Attachment is obsolete: true
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.
Attachment #8729637 - Flags: review?(aklotz) → review+
Attached patch patchSplinter Review
Attachment #8729637 - Attachment is obsolete: true
Attachment #8729965 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/bcff334f8746
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Jim, should we uplift this fix to Aurora 47 or even Beta 46?
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
Duplicate of this bug: 1215803
(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 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.
Attachment #8729965 - Flags: approval-mozilla-aurora?
Comment on attachment 8729965 [details] [diff] [review]
patch

e10s related, stabilized on Nightly for a week, Aurora47+
Attachment #8729965 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.