Closed Bug 843695 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: jesup, Assigned: jesup)

Details

(Whiteboard: [webrtc][blocking-webrtc+][qa-])

Attachments

(1 file, 1 obsolete file)

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
Attachment #716662 - Flags: review?(tterribe)
Attachment #716662 - Attachment is obsolete: true
Attachment #716662 - Flags: review?(tterribe)
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.
Attachment #717955 - Flags: review?(tterribe)
Attachment #717955 - Flags: review?(ted)
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.
Attachment #717955 - Flags: review?(tterribe) → review+
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
Attachment #717955 - Flags: review?(ted) → review+
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.