The cubeb code has a lot of "#undef NDEBUG" to enable assert() in release code. On Windows, if the assertion fails, the CRT will pop up a dialog like the one below. This is bad for the user experience, especially for non-technical users, and it interferes with our crash stats as only some subset of those buttons (I forget which) lead to the crash reporter. For assertions in release code we should use MOZ_RELEASE_ASSERT. --------------------------- Microsoft Visual C++ Runtime Library --------------------------- Assertion failed! Program: c:\ff\36b9\xul.dll File: c:/builds/moz2_slave/rel-m-beta-w32_.../cubeb_wasapi.cpp Line: 1068 Expression: context && stream For information on how your program can cause an assertion failure, see the Visual C++ documentation on asserts (Press Retry to debug the application - JIT must be enabled) --------------------------- Abort Retry Ignore ---------------------------

It can't use MOZ_RELEASE_ASSERT because it's not Gecko code. We can change the assert to something that does what you want on Windows, but what's the argument for not using assert() on other platforms where this is not an issue?

I am not familiar with the assert-failed experience on other platforms. If you can assure me that those platforms don't say scary things to the user and don't mess up crash reports, that's fine by me.

Where "don't mess up crash reports" means "that they trigger the crash reporter properly" right?

Yes.

(In reply to Matthew Gregan [:kinetik] from comment #7) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b92a7e6d22c Note that I'm *expecting* this to fail (in the right way), since the try commit added a couple of test calls to FATAL() in wasapi_init.

Comment on attachment 8566365 [details] [diff] [review] Introduce a FATAL() macro to libcubeb rather than (ab)using assert() Review of attachment 8566365 [details] [diff] [review]: ----------------------------------------------------------------- f+ on the general approach given that it's what MOZ_REALLY_CRASH does under the hood. I see a bunch of other libcubeb files that use assert and/or #undef NDEBUG. Are those not affected for some reason? ::: media/libcubeb/src/cubeb_wasapi.cpp @@ +30,5 @@ > #ifndef STACK_SIZE_PARAM_IS_A_RESERVATION > #define STACK_SIZE_PARAM_IS_A_RESERVATION 0x00010000 // Threads only > #endif > > +#define FATAL(expr) do { \ It's Paul's call, but to me statements like |FATAL(stm)| read like they're always fatal. Maybe keep some form of the word assert? (or check, verify, etc.)

Yeah, FATAL() is a dumb name. I've changed it do XASSERT() and moved the code into a common header. I'm leaving the non-Windows platforms alone as I'm happy with how assert() behaves there for now.

The fail test run kinda worked, but I noticed my fprintf args were screwy (used %s for an int!), so fixed in the current patch and repushed: Fail test run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaf02e8cd796 Regular test run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be5d4933f5b

(In reply to Matthew Gregan [:kinetik] from comment #12) > Fail test run: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaf02e8cd796 Of course, I forgot to rename FATAL to XASSERT in the test patch before pushing... https://treeherder.mozilla.org/#/jobs?repo=try&revision=49bffd0af236

Update commit message too.

Comment on attachment 8566411 [details] [diff] [review] Introduce an XASSERT() macro to libcubeb rather than (ab)using assert() v2.1 Review of attachment 8566411 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the comment addressed. ::: media/libcubeb/src/cubeb_wasapi.cpp @@ +23,3 @@ > #include <cmath> > > +#ifndef DEBUG This changes the behaviour, it enables the recursive lock checking unconditionally.

I'll note that werbrtc code we import from upstream is littered with assert()s (close to 2000 of them at a quick "cd media/webrtc/trunk/webrtc; chfind 'assert(' | wc" test). Perhaps if we don't want assert() to fire C++ library code we should override it? And it's not just webrtc: "chfind 'assert(' | wc" gives >14000 hits. Now some surely are in code not actually part of the build, but still.... and only ~2K of those are actually static_assert(). Or we can find ways to #define it to something else like jemalloc does... or get upstream(s) to make mods to parameterize/#define it in N upstreams for different parts of the tree, and change all the non-upstream uses. Given this, I think that (on windows) it makes sense to override it...

How many of those are assert + "#undef NDEBUG"? That's the real problem.

(In reply to David Major [:dmajor] (UTC+13) from comment #18) > How many of those are assert + "#undef NDEBUG"? That's the real problem. No idea -- but there seem to be no undef's of NDEBUG in webrtc except in some standalone test code. Which builds do we do that set NDEBUG? CI builds? Nightlies? Aurora, Beta? Try? I see that in config.mk we set it for apparently optimized builds with debug symbols. Generally, I'd rather assert() than crash, except for a few perf-critical paths. I wonder how many serious issues in the field would be a lot less serious (and easier to find and fix!) if we left (most/many) assertions enabled...

NDEBUG is supposed to be set for non-debug builds. It turns assert() into a no-op in MSVC builds. The problem arises when individual files undefine NDEBUG in order to keep using assert() in release code. Using assert() on its own without messing with the defines is fine. AFAICT there's no action required for webrtc.