Remove redundant NS_WARNING in nsMixedContentBlocker.cpp, and replace MOZ_NOT_REACHED with MOZ_ASSERT

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Bug 782654 added a back-to-back MOZ_NOT_REACHED & NS_WARNING with the same message:
> 265       MOZ_NOT_REACHED("Mixed content of unknown type");
> 266       NS_WARNING("Mixed content of unknown type");
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#264

(link to cset: https://hg.mozilla.org/mozilla-central/rev/eef87ef15814#l1.146 )

I think we can just drop the NS_WARNING -- it will literally never display its message, because any builds that honor it will also honor the MOZ_NOT_REACHED and abort just before they reach it.
(Assignee)

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 1

6 years ago
Created attachment 710444 [details] [diff] [review]
fix
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #710444 - Flags: review?(tanvi)

Comment 2

6 years ago
Comment on attachment 710444 [details] [diff] [review]
fix

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

Looks good to me.  Thanks for catching this dholbert!
Attachment #710444 - Flags: review?(tanvi) → review+

Updated

6 years ago
Blocks: 815321
(Assignee)

Comment 3

6 years ago
Created attachment 710458 [details] [diff] [review]
fix v2 (also swap out MOZ_NOT_REACHED for MOZ_ASSERT)

Actually, it turns out MOZ_NOT_REACHED is generally something we shouldn't use, since it triggers undefined behavior when it's hit.

Of course, by definition, we're not _expecting_ to hit it -- but in the unexpected case that we do hit it (e.g. due to forgetting a case statement in the switch block in this patch's contextual code), we presumably want the defined behavior that MOZ_ASSERT(false, "...") provides, instead of explicitly-undefined behavior.

For reference / background, see bug 820686 comment 0, bug 820686 comment 8, and bug 820686 comment 10.

So: This swaps MOZ_NOT_REACHED out for MOZ_ASSERT (as bug 838349 did for some places code, too)
Attachment #710444 - Attachment is obsolete: true
Attachment #710458 - Flags: review?(tanvi)
(Assignee)

Comment 4

6 years ago
Created attachment 710459 [details] [diff] [review]
fix v3

(fixed a typo in commit message)
Attachment #710458 - Attachment is obsolete: true
Attachment #710458 - Flags: review?(tanvi)
Attachment #710459 - Flags: review?(tanvi)

Comment 5

6 years ago
Comment on attachment 710459 [details] [diff] [review]
fix v3

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

Thanks for digging into this!  MOZ_ASSERT() seems like the right thing here, as we only want a crash in a debug build to alert us that there is a new content type that the mixed content blocker doesn't know about.

r+'ing the patch and asking additional review from smaug since he is a module peer.
Attachment #710459 - Flags: review?(tanvi)
Attachment #710459 - Flags: review?(bugs)
Attachment #710459 - Flags: review+

Updated

6 years ago
Attachment #710459 - Flags: review?(bugs) → review+
(Assignee)

Updated

6 years ago
Summary: Remove redundant NS_WARNING in nsMixedContentBlocker.cpp → Remove redundant NS_WARNING in nsMixedContentBlocker.cpp, and replace MOZ_NOT_REACHED with MOZ_ASSERT
https://hg.mozilla.org/mozilla-central/rev/d53ff47e3297
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.