1443612 - Pre-start cubeb before content sandboxing if media.cubeb.sandbox is false

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P1)

Description

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

The cubeb preloading from bug 1259508, removed in bug bug 1405877, is still relevant if media.cubeb.sandbox is false; we should restore that code, conditional on the pref.  See bug 1434392 comment #7 for a longer explanation.

There seems to be something else going on here, because there are reports of a regression between 57 and 58 with similar effects — when pulseaudio isn't already started, it fails to start and complains about fork(); see bug 1422073 comment #7 for an example.  I haven't been able to find anything relevant that would have changed in 58.

(Also complicating things, there are systems where the PulseAudio client won't fork the daemon directly, but instead cause a service manager like systemd to do so; see bug 1434392 comment #9.  So this may not be reproducible on arbitrary distros just with strategic use of `pulseaudio --kill`.)

But I think this should still be fixed.

Comment 1

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

Attachment: Bug 1443612 - Pre-start cubeb before content sandboxing if not remoting audio.

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

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 8956611 [details]
Bug 1443612 - Pre-start cubeb before content sandboxing if not remoting audio.

https://reviewboard.mozilla.org/r/225540/#review231426

Thanks for fixing this!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94db8d320385
Pre-start cubeb before content sandboxing if not remoting audio. r=kinetik

Comment 4

[Matthew Gregan [:kinetik]]

It'd be great to get this into beta if we can.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

59 is already in the RC phase, so this would need a pretty compelling argument for uplift at this point.

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/94db8d320385

Comment 7

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> 59 is already in the RC phase, so this would need a pretty compelling
> argument for uplift at this point.

This patch also isn't enough in most cases; see bug 1434392 comment #10.  Uplifting this patch *and* backing out bug 1412464 and bug 1408497 should work; I can test that, in case it's worth trying to get this into a point release.

Comment 8

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

(In reply to Jed Davis [:jld] (⏰UTC-7) from comment #7)
> should work; I can test that

It works.

https://github.com/mozilla/gecko-dev/compare/release...jld:pulseaudio-for-59

Comment 9

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

Comment on attachment 8956611 [details]
Bug 1443612 - Pre-start cubeb before content sandboxing if not remoting audio.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1412464 and bug 1405877.
[User impact if declined]: Audio won't work in some less common environments (where pulseaudio isn't started when logging in).  This has been broken since the 58 release, but we did get several reports about it.  There is a workaround, but it's not immediately obvious.
[Is this code covered by automated tests?]: I don't think we test this particular case (where the pulseaudio daemon isn't already running) but sandboxing and WebAudio have tests.
[Has the fix been verified in Nightly?]: Nightly has audio remoting preffed on so it's not affected, and this patch doesn't do anything there.  I've manually verified the fix against mozilla-release (and against m-c with the pref flipped).
[Needs manual test from QE? If yes, steps to reproduce]: STR is to use a distribution that doesn't have systemd launch pulseaudio (I used Debian 8), run `pulseaudio --kill` if needed, and then start Firefox and try to use WebAudio.
[List of other uplifts needed for the feature/fix]: Back out bug 1412464 and bug 1408497.  This part is properly bug 1434392, but the patch I wrote there won't backport easily to 59, and it's lower-risk to back out the regressing changes instead.  The backouts may have merge conflicts; the GitHub link I posted in comment #8 shows how things should look.  I could attach patches for the backouts to bug 1434392 and flag them separately if that would work better.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The proposed changes restore how things worked in 58 (this patch) and 57 (the backouts to address bug 1434392).
[String changes made/needed]: None

If it's too late for 59.0, I'd like this to be considered for 59.0.1.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

It's too late for 59.0, but we can keep this open for a potential dot release. 
We could also describe the workaround, ideally in SUMO, and write a "known issue" release note. What do you think? Can you write the SUMO page and suggest wording?

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

On re-reading this, it seems a bit too complex and not high impact enough for a dot release. I would prefer that we give a good description of the workaround for affected users.

Comment 12

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

I've drafted a SUMO article, currently awaiting review: https://support.mozilla.org/en-US/kb/no-sound-firefox-59-linux

I tried to include some technical details for those who want them but still make the article readable otherwise, but it will probably need some editing.

Comment 13

[Alice Wyman]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #12)
> I've drafted a SUMO article, currently awaiting review:
> https://support.mozilla.org/en-US/kb/no-sound-firefox-59-linux
> 
> I tried to include some technical details for those who want them but still
> make the article readable otherwise, but it will probably need some editing.

I'll NeedInfo Joni Savage, the SUMO Content Manager.

Comment 14

[Joni Chan]

Thanks for looping me in, Alice, and for writing the article, Jed. I've published it with some minor edits.