1129247 - Stop using full release-mode assertions for media invariants

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

The media code is racey, and our test coverage is poor - so CI doesn't give us a lot of confidence that our code is bug-free. As such, I've recently gotten into the habit of asserting certain invariants with release-mode assertions so that I can find bugs in the wild. This has worked quite well, but isn't a good long-term strategy - for most of these bugs, we'd still rather just be buggy than crash for release builds. This strategy only makes sense as a diagnostic on branches where we can rapidly deploy fixes (i.e. Nightly and Aurora).

I'm going to add some infrastructure that lets us do that.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Introduce MOZ_DIAGNOSTIC_ASSERT. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use MOZ_DIAGNOSTIC_ASSERT instead of MOZ_RELEASE_ASSERT in media code. v1

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8558868 [details] [diff] [review]
Part 1 - Introduce MOZ_DIAGNOSTIC_ASSERT. v1

Review of attachment 8558868 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Assertions.h
@@ +387,5 @@
>  
> +#ifdef RELEASE_BUILD
> +#  define MOZ_DIAGNOSTIC_ASSERT(...) MOZ_ASSERT(__VA_ARGS__)
> +#else
> +#  define MOZ_DIAGNOSTIC_ASSERT(...) MOZ_RELEASE_ASSERT(__VA_ARGS__)

It's probably better to have these as

#  define MOZ_DIAGNOSTIC_ASSERT MOZ_ASSERT
#  define MOZ_DIAGNOSTIC_ASSERT MOZ_RELEASE_ASSERT

so that we can worry less about possibly triggering MSVC's variadic macro bugs.

Comment 4

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fee3301951
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c84373abd46

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8558870 [details] [diff] [review]
Part 2 - Use MOZ_DIAGNOSTIC_ASSERT instead of MOZ_RELEASE_ASSERT in media code. v1

Approval request for both patchesaurora - we've got several of these assertions that we have fatal on all builds, and we want to make them non-fatal for beta and release. ~Zero risk.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5fee3301951
https://hg.mozilla.org/mozilla-central/rev/8c84373abd46

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8558868 [details] [diff] [review]
Part 1 - Introduce MOZ_DIAGNOSTIC_ASSERT. v1

I agree with comment 5. These changes are also trivial. Aurora+

Comment 8

[Ralph Giles (:rillian)]

NB part two doesn't apply cleanly to aurora. I'm going through the backlog of changes which weren't critical for beta and preparing uplift requests.

Comment 9

[Ralph Giles (:rillian)]

Landed on 37 with some conflicts resolved manually.

https://hg.mozilla.org/releases/mozilla-aurora/rev/f2b73f65dad9
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4dbe6aaec36