843695 - Make some DataChannels assertions fatal in opt/release builds, at least for now

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

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

Perhaps keep these fatal (or at least NS_ASSERTION and cleanup code) for production releases; or we could turn them off in Beta.

These provide boot-and-suspenders against sec bugs and get us better error reports in nightly/aurora

Comment 1

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

Attachment: Make some DataChannels assertions fatal in opt/release

Comment 2

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

Attachment: Make some DataChannels assertions fatal in opt/release

Comment 3

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

Comment on attachment 717955 [details] [diff] [review]
Make some DataChannels assertions fatal in opt/release

Now includes the control of the assertions from configure.in.
We'd leave them on by default through Aurora and likely beta, perhaps into the first release or two of production.  Some we make make hard MOZ_CRASHes permanently where the security implications are important enough.

Comment 4

[Timothy B. Terriberry (:derf)]

Comment on attachment 717955 [details] [diff] [review]
Make some DataChannels assertions fatal in opt/release

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

It's not really clear to me why these asserts and not others should be fatal. It would be helpful to document which ones are made so because of security implications, otherwise we'll never know if it's safe to make them non-fatal again.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 717955 [details] [diff] [review]
Make some DataChannels assertions fatal in opt/release

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

::: configure.in
@@ +5283,5 @@
> +dnl ========================================================
> +MOZ_ARG_DISABLE_BOOL(webrtc,
> +[  --disable-webrtc-assertions        Disable WebRTC non-DEBUG assertions],
> +    MOZ_WEBRTC_ASSERT_ALWAYS=,
> +    MOZ_WEBRTC_ASSERT_ALWAYS=1)

Do we really need this to be a configure option? Doesn't seem worthwhile. Maybe just change the initial assignment to only assign if unset?

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +60,5 @@
> +#define ASSERT_WEBRTC(x) do { if (!(x)) { MOZ_CRASH(); } } while 0
> +#endif
> +#else
> +#define ASSERT_WEBRTC(x) MOZ_ASSERT((x))
> +#endif

I think this would read more simply if you did:
#ifdef DEBUG
#define ASSERT_WEBRTC(x) MOZ_ASSERT((x))
#elif defined(MOZ_WEBRTC_ASSERT_ALWAYS)
#define ASSERT_WEBRTC(x) do { if (!(x)) { MOZ_CRASH(); } } while 0
#endif

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ae083f9caf

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/01de5071069b
Silly mistake while merging

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4ae083f9caf
https://hg.mozilla.org/mozilla-central/rev/01de5071069b