Closed
Bug 843695
Opened 12 years ago
Closed 12 years ago
Make some DataChannels assertions fatal in opt/release builds, at least for now
Categories
(Core :: WebRTC: Networking, defect, P1)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: jesup, Assigned: jesup)
Details
(Whiteboard: [webrtc][blocking-webrtc+][qa-])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
derf
:
review+
ted
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #716662 -
Flags: review?(tterribe)
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #716662 -
Attachment is obsolete: true
Attachment #716662 -
Flags: review?(tterribe)
Assignee | ||
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
Target Milestone: --- → mozilla22
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/01de5071069b
Silly mistake while merging
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a4ae083f9caf
https://hg.mozilla.org/mozilla-central/rev/01de5071069b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•