950026 - Bail out safely when we get an unexpected value from AudioClient::IsFormatSupported

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Paul Adenot (:padenot)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> roc, are you talking about bug 833161? That appears to be a linux-only bug,
> and this is Windows, so it's not clear to me that it is the same.
> 
> cpearce the assert location is well-known in this case:
> http://hg.mozilla.org/releases/mozilla-aurora/annotate/731470a7c4ca/media/
> libcubeb/src/cubeb_wasapi.cpp#l665
> 
> See e.g. the stack at
> https://crash-stats.mozilla.com/report/index/83dcae38-7539-41ea-a342-
> c10902131204 or
> https://crash-stats.mozilla.com/report/index/a4999c2b-0cca-4f0d-ab1f-
> 7ffe02131205

----

(In reply to Matthew Gregan [:kinetik] from comment #6)
> Crashes in cubeb_wasapi need a different bug, this one has always been about
> cubeb_winmm code.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Bail out when we don't get an expected return value from AudioClient::IsFormatSupported, instead of asserting. r=

This will fall back to our own channel mapping code, instead of trying to use
the platform's.

Comment 2

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1ca8f7cf88

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c1ca8f7cf88

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Worth to nominate it for uplifting?

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8347305 [details] [diff] [review]
Bail out when we don't get an expected return value from AudioClient::IsFormatSupported, instead of asserting. r=

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 866675 (compiled in from firefox 27)
User impact if declined: Crashes on an assert (even in opt/non-debug) on some specific sound hardware/OS configuration.
Testing completed (on m-c, etc.): Baked for a while on m-c.
Risk to taking this patch (and alternatives if risky): Not risky, we just fallback to some other part of the code that is used a lot instead of trying to rely on system facilities.
String or IDL/UUID changes made by this patch: none.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d92422d01e6d
https://hg.mozilla.org/releases/mozilla-beta/rev/076d817a69f1

Comment 7

[u279076]

Do we have tests to ensure this is working correctly?

Comment 8

[Paul Adenot (:padenot)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #7)
> Do we have tests to ensure this is working correctly?

The test would require specific hardware/driver combination, I'm afraid. But since we were crashing on our own assertion, and because we already had a fallback path for this exact case, I'm confident with uplifting/shipping this.

Comment 9

[u279076]

Marking in-testsuite- based on comment 8. Please let me know if there's something QA can do manually to raise confidence that the fallback mechanism is working correctly.

Comment 10

[Danial Horton]

I do not believe this is fixed,

I can crash 26, 27, 28 and 29 by simply  opening https://vine.co/v/ManB7AzZjtP

The issue also occurs with tumblr video's.

Comment 11

[Danial Horton]

Issue is confirmed NOT resolved with Creative Soundblaster and Realtek hardware when vendor specific enhancements are enabled.

Crash can be reproduced 100% until "Disable [Product name] Enhancements" is ticked in Speaker device properties

I expect the enhancement modules provided with Recon and Z(x/R) series soundblaster cards will exhibit the same issue.

Comment 12

[Danial Horton]

Was this specific to certain html5 players?

Youtube's works without issue whilst tumblrs and vines just assert

https://crash-stats.mozilla.com/report/index/84243c37-eb98-44ea-9278-097f32140208
https://crash-stats.mozilla.com/report/index/da35a63d-c06c-4206-91a5-be4b12140208
https://crash-stats.mozilla.com/report/index/d83e9d96-e411-43c5-916d-d87d32140208
https://crash-stats.mozilla.com/report/index/03601244-ce1c-4f0e-ad45-ee1f22140208
https://crash-stats.mozilla.com/report/index/5b76fb4f-4ed0-4827-a934-005412140208
https://crash-stats.mozilla.com/report/index/ac8e8f41-020c-49db-8182-b2c6e2140208
https://crash-stats.mozilla.com/report/index/157893f1-dcef-4c93-aedb-31bf32140208
https://crash-stats.mozilla.com/report/index/6810e793-a3b8-4095-9678-fd0e22140208

Comment 13

[Matthew Gregan [:kinetik]]

(In reply to Danial Horton from comment #10)
> I do not believe this is fixed,
> 
> I can crash 26, 27, 28 and 29 by simply  opening
> https://vine.co/v/ManB7AzZjtP
> 
> The issue also occurs with tumblr video's.

Note that the fix in this bug is only present in Firefox 27 onwards, so it is expected that 26 would crash if you were hitting the same bug.

However, looking at the stacks, you're hitting a different assert and thus have a different bug.  I'll file a new bug and CC you.

Comment 14

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #13)
> I'll file a new bug and CC you.

Bug 970147.

Comment 15

[Michail Pappas]

Matthew, my apologies. Opened up bug# 970519, and closed it (incorrectly) as a duplicate of bug# 791112. Reading the latter more carefully, I understand that my issue is more likely related to either bug# 950026 or bug# 970147.

On which bug report should I continue to provide crash reports/feedback, considering my own problem is as listed in bug# 970519?

Apologies to all for littering the bugtracked not with one, but with 3 posts, and your patience pls; I'll try to be more careful next time...

Comment 16

[Matthew Gregan [:kinetik]]

No problem, thanks for the bug reports Michail.  It looks like bug 970147 is the correct one for your issue.