Latest insider build of Windows 10 (17063) breaks sound playback completely

VERIFIED FIXED in Firefox 58

Status

()

defect
P1
normal
Rank:
10
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: gandalf, Assigned: achronop)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr5258+ wontfix, firefox57+ wontfix, firefox58+ fixed, firefox59+ fixed)

Details

Attachments

(5 attachments)

Comment 2

a year ago
[Tracking Requested - why for this release]:
Serious issue that may also affect the january win10 update flights.
Summary: Latest insider build of Windows 10 (17063) It breaks sound playback on Firefox Quantum → Latest insider build of Windows 10 (17063) breaks sound playback completely
(Reporter)

Comment 4

a year ago
The uploaded patch is an implementation of the workaround suggested by the person on reddit. I have no access to a Windows machine (I'm at the airport atm) and cannot verify if it fixes the issue.

Also, if this is the patch we may have to try to upstream it up to stable as it affects users in production right now.

Comment 5

a year ago
The MS response on their feedback hub is as follows:

Windows has several audio volume APIs - IAudioEndpointVolume, IChannelAudioVolume, ISimpleAudioVolume, and IAudioEndpointVolume. These APIs can be used to change the volume and/or mute state of the stream, app, or audio device.

In build 17063 a change was made to these APIs to have them return S_FALSE (1) if the requested change was a no-op.

This broke apps (like Firefox) which request changes (that may be no-ops) and then explicitly check the return value against S_OK (0).
Someone should probably audit the other uses of bare S_OK and S_FALSE in mozilla-central and determine whether they should use the SUCCEEDED or FAILED macros instead. And perhaps add a mach lint check.
(Reporter)

Comment 7

a year ago
In that case, there are a couple more places where we do this: https://searchfox.org/mozilla-central/search?q=%3D%3D+S_OK&case=false&regexp=false&path=

Updated

a year ago
Attachment #8938450 - Flags: review?(padenot)
Attachment #8938450 - Flags: review?(achronop)

Comment 8

a year ago
(Asking for review from whoever's still around - don't actually need more than 1 review. I tested this patch on my windows insider box, and it fixes the issue for me.)
(Assignee)

Comment 9

a year ago
Thanks for the report. That's true, I also see it in the documentation here: https://msdn.microsoft.com/en-us/library/windows/desktop/ff485842(v=vs.85).aspx

"All of the constants with the prefix "E_" are error codes. The constants S_OK and S_FALSE are both success codes. Probably 99% of COM methods return S_OK when they succeed; but do not let this fact mislead you. A method might return other success codes, so always test for errors by using the SUCCEEDED or FAILED macro."

I will patch it first on cubeb repo and when is landed I will import it in Gecko.

Updated

a year ago
Blocks: 1426722

Comment 10

a year ago
(In reply to Chris Peterson [:cpeterson] from comment #6)
> Someone should probably audit the other uses of bare S_OK and S_FALSE in
> mozilla-central and determine whether they should use the SUCCEEDED or
> FAILED macros instead. And perhaps add a mach lint check.

Filed bug 1426722 for this.
Assignee: nobody → achronop
Rank: 10
Priority: -- → P1
Comment on attachment 8938450 [details]
Bug 1426719 - Workaround a bug in Windows Insider build 17063 which breaks sound in Firefox.

https://reviewboard.mozilla.org/r/209154/#review214970

r+ though I'm not officially a peer.  Clear, obvious fix supplied by MS.

::: media/libcubeb/src/cubeb_wasapi.cpp:1495
(Diff revision 1)
>      XASSERT(mix_format->wFormatTag == WAVE_FORMAT_EXTENSIBLE);
>      *reinterpret_cast<WAVEFORMATEXTENSIBLE *>(mix_format.get()) = hw_mix_format;
>    } else if (hr == S_OK) {
>      LOG("Requested format accepted by WASAPI.");
>    } else {

This needs to change too, as achronop indicated
Attachment #8938450 - Flags: review+
Comment hidden (mozreview-request)
Comment on attachment 8938468 [details]
Bug 1426719 - Update cubeb from upstream to e1e8337.

https://reviewboard.mozilla.org/r/209176/#review214978
Attachment #8938468 - Flags: review?(rjesup) → review+

Comment 14

a year ago
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6217d397dc08
Update cubeb from upstream to e1e8337. r=jesup
(Assignee)

Updated

a year ago
Attachment #8938450 - Flags: review?(padenot)
Attachment #8938450 - Flags: review?(achronop)
Attachment #8938450 - Flags: review+
Nils -- Like we talked about on our call earlier today, can you verify that this patch solves the problem?
Flags: needinfo?(drno)
If we can verify this patch solves the problem, we should get it into 58 and esr.

Updated

a year ago
Duplicate of this bug: 1426550

Updated

a year ago
Duplicate of this bug: 1426524

Updated

a year ago
Duplicate of this bug: 1426606
(Assignee)

Comment 20

a year ago
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1197049
[User impact if declined]: No audio output on Windows 
[Is this code covered by automated tests?]: No (we don't have such a sound card in automation)
[Has the fix been verified in Nightly?]: Just landed
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not.
[Why is the change risky/not risky?]: It's very simple and it has been verified in Nightly 
[String changes made/needed]:
Attachment #8938509 - Flags: review?(rjesup)
Attachment #8938509 - Flags: approval-mozilla-beta?
Attachment #8938509 - Flags: review?(rjesup) → review+
I verified that the 17063 build broke audio playback for me and that the patch attached to this bug fixes the playback issue.

Unfortunately it does not fix the mic issue which we have since the earlier insider builds.

Note: there is one more comparison to S_OK in the cubeb wasapi code left. But since it only affect logging it's probably not worth fixing and uplifting.
Flags: needinfo?(drno)

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6217d397dc08
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Tracking for now since this is a new Windows release potentially coming out in January. 
We can look at the uplifts tomorrow.
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Alex, could you prepare a patch for esr and release (just in case)? I guess it will need some rebasing for esr?
Thanks
(this can wait for January 3rd)
Flags: needinfo?(achronop)

Updated

a year ago
Duplicate of this bug: 1426285
Comment on attachment 8938509 [details] [diff] [review]
Bug 1426719 - Pick cubeb e1e8337 to beta.

cubeb error handling update to fix sound on windows insider build, beta58+
Attachment #8938509 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

a year ago
Duplicate of this bug: 1427469
(Assignee)

Comment 30

a year ago
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1197049
[User impact if declined]: Playback and webrtc does not work
[Is this code covered by automated tests?]: No (we don't have such a sound card in automation)
[Has the fix been verified in Nightly?]: Just landed
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not.
[Why is the change risky/not risky?]: It's very simple and it has been verified in Nightly 
[String changes made/needed]:N/A
Attachment #8939517 - Flags: review?(rjesup)
Attachment #8939517 - Flags: approval-mozilla-release?
(Assignee)

Comment 31

a year ago
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Basic functionality does not work (all playbacks like youtube etc).
User impact if declined: Playback and webrtc does not work.
Fix Landed on Version: Nightly, beta (requested for release)
Risk to taking this patch (and alternatives if risky): No risk, it's very simple fix and it has been verified in Nightly 
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(achronop)
Attachment #8939522 - Flags: review?(rjesup)
Attachment #8939522 - Flags: approval-mozilla-esr52?
Do we know when this new version of Windows is going to be released?

Comment 33

a year ago
Microsoft is targeting March 2018 for the new version of Windows 10.

Windows 10 current release strategy is:
"Twice-per-year feature update release targeting around March and September"
https://technet.microsoft.com/en-us/windows/release-info.aspx?f=255&MSPPError=-2147217396

Insiders have access to the builds now, in the public realm, to test with. Fast and Slow rings.

Fast Ring includes many changes, including this API change, in Build 17063. It's unclear whether the change was intentional or a mistake. I tried asking in the Feedback Hub item (as Jacob K. in a comment), but have not received a response. And I doubt I'll get one.
https://aka.ms/G2cgew

Slow Ring is more stable, and if the API change landed there, I'd assume it was intentional at that point.

I'm happy that Mozilla is responsive in getting this change implemented, and if it doesn't break any existing audio, I'd personally like it to land in release Firefox 57.

Thanks,
Jacob
Thanks Jacob for the information.
As we are going to ship Firefox 58 in a bit more than 2 weeks (2018-01-23), maybe we could just wait...

Comment 35

a year ago
(In reply to Jacob W. Klein from comment #33)
> Fast Ring includes many changes, including this API change, in Build 17063.
> It's unclear whether the change was intentional or a mistake. I tried asking
> in the Feedback Hub item (as Jacob K. in a comment), but have not received a
> response. And I doubt I'll get one.
> https://aka.ms/G2cgew
> 
> Slow Ring is more stable, and if the API change landed there, I'd assume it
> was intentional at that point.

The engineer at Microsoft who proposed this fix also said on Reddit he had fixed it internally and added tests to prevent the bug from happening again, so even if nothing changed in Firefox, final users shouldn't experience this bug when the next version of Windows is released.
ok, so, we would not even need to take the patch in esr52 then...
Attachment #8939517 - Flags: review?(rjesup) → review+
Attachment #8939522 - Flags: review?(rjesup) → review+
Heads-up, Windows fix has reached mainline as of build 17073
OK, so, let's wontfix that for previous versions.
Comment on attachment 8939517 [details] [diff] [review]
update-cubeb-to-e1e8337-release.patch

per comment 38
Flags: needinfo?(lhenry)
Attachment #8939517 - Flags: approval-mozilla-release? → approval-mozilla-release-
Attachment #8939522 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-

Comment 40

a year ago
I believe Windows 10 Insider Build 17074, fixes the issue, by possibly reverting the Sound API change that broke 17063.
See here:
https://blogs.windows.com/windowsexperience/2018/01/11/announcing-windows-10-insider-preview-build-17074-pc
- We fixed an issue resulting in certain apps, like Firefox, might not have audio after upgrading to the previous flight. This issue also impacted the ability to record audio in Microsoft Edge.
You need to log in before you can comment on or make changes to this bug.