Change MOZ_CRASH in nsFlexContainerFrame switch statements to MOZ_ASSERT_UNREACHABLE

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
nsFlexContainerFrame has some MOZ_CRASH statements for handling unexpected (unrecognized) style-system enumerated values, in switch statements. These are bad, because they'll abort in release builds. Really, they should be MOZ_ASSERT_UNREACHABLE.

HISTORY:
========
These were originally MOZ_NOT_REACHED(), which were intended as stronger forms of NS_NOTREACHED(), but it turned out that MOZ_NOT_REACHED was a dangerous footgun (bug 820686 comment 0), so it was converted to MOZ_CRASH as part of a mass-conversion here:
https://hg.mozilla.org/mozilla-central/diff/55c1f447549d/layout/generic/nsFlexContainerFrame.cpp#l1573

And then I later copypasted the MOZ_CRASH into a new switch statement for "align-content" when adding support for that:
https://hg.mozilla.org/mozilla-central/diff/47901ad6d20b/layout/generic/nsFlexContainerFrame.cpp#l1.176

Anyway -- there's no reason we need to abort in release builds if we somehow unexpectedly hit these lines -- these should just be MOZ_ASSERT_UNREACHABLE, which is what they wanted to be originally anyway (though that didn't exist at the time).
(Assignee)

Comment 1

3 years ago
Created attachment 8702815 [details] [diff] [review]
fix v1

This patch does the following:
 1) s/MOZ_CRASH/MOZ_ASSERT_UNREACHABLE/
 2) Fixes an incorrect assertion-message in one of the affected lines -- right now, it says "flex-flow" but means to say "flex-direction". (fixed here)
Attachment #8702815 - Flags: review?(mats)
Comment on attachment 8702815 [details] [diff] [review]
fix v1

r=mats
Attachment #8702815 - Flags: review?(mats) → review+
FYI, there is also a NS_NOTREACHED("Unexpected align-self value") that could be fixed.
(Assignee)

Comment 4

3 years ago
Thanks -- I'll upgrade that one to MOZ_ASSERT_UNREACHABLE for consistency with the rest of these.

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9059476e8881
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.