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

Status: VERIFIED FIXED

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

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

See:
 - https://www.reddit.com/r/Windows10/comments/7kzt7s/psa_if_you_use_firefox_dont_upgrade_to_17063_it/
 - https://www.reddit.com/r/Windows10/comments/7l9xfs/alright_so_the_latest_insider_build_completely/

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Microsoft engineer suggest a patch on reddit (sic!): https://www.reddit.com/r/Windows10/comments/7kzt7s/psa_if_you_use_firefox_dont_upgrade_to_17063_it/drkfxmk/

Comment 2

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Serious issue that may also affect the january win10 update flights.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1426719 - Workaround a bug in Windows Insider build 17063 which breaks sound in Firefox.

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

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

[:Gijs (he/him)]

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).

Comment 6

[Chris Peterson [:cpeterson]]

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.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

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=

Comment 8

[:Gijs (he/him)]

(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.)

Comment 9

[Alex Chronopoulos [:achronop]]

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.

Comment 10

[:Gijs (he/him)]

(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.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 12

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1426719 - Update cubeb from upstream to e1e8337.

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

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8938468 [details]
Bug 1426719 - Update cubeb from upstream to e1e8337.

https://reviewboard.mozilla.org/r/209176/#review214978

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6217d397dc08
Update cubeb from upstream to e1e8337. r=jesup

Comment 15

[Maire Reavy [:mreavy]]

Nils -- Like we talked about on our call earlier today, can you verify that this patch solves the problem?

Comment 16

[Maire Reavy [:mreavy]]

If we can verify this patch solves the problem, we should get it into 58 and esr.

Comment 20

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1426719 - Pick cubeb e1e8337 to beta.

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]:

Comment 21

[Nils Ohlmeier [:drno]]

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.

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6217d397dc08

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks all - verified as fixed as per:
 - https://www.reddit.com/r/Windows10/comments/7kzt7s/psa_if_you_use_firefox_dont_upgrade_to_17063_it/drm8287/
 - https://www.reddit.com/r/Windows10/comments/7kzt7s/psa_if_you_use_firefox_dont_upgrade_to_17063_it/drm7bg2/

Comment 24

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

Tracking for now since this is a new Windows release potentially coming out in January. 
We can look at the uplifts tomorrow.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

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)

Comment 27

[Julien Cristau [:jcristau]]

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+

Comment 28

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cb9a0bfe28ea476c6299d7684dab4b6f2a0b4e0f

Comment 30

[Alex Chronopoulos [:achronop]]

Attachment: update-cubeb-to-e1e8337-release.patch

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

Comment 31

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1426719 - Pick cubeb e1e8337 to esr52

[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.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Do we know when this new version of Windows is going to be released?

Comment 33

[Jacob W. Klein]

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

Comment 34

[Sylvestre Ledru [:Sylvestre]]

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

[Gustavo Silva]

(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.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

ok, so, we would not even need to take the patch in esr52 then...

Comment 37

[Matthew van Eerde]

Heads-up, Windows fix has reached mainline as of build 17073

Comment 38

[Sylvestre Ledru [:Sylvestre]]

OK, so, let's wontfix that for previous versions.

Comment 39

[Julien Cristau [:jcristau]]

Comment on attachment 8939517 [details] [diff] [review]
update-cubeb-to-e1e8337-release.patch

per comment 38

Comment 40

[Jacob W. Klein]

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.