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

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 2

[Tanvi Vyas[:tanvi]]

Comment on attachment 710444 [details] [diff] [review]
fix

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

Looks good to me.  Thanks for catching this dholbert!

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: 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)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v3

(fixed a typo in commit message)

Comment 5

[Tanvi Vyas[:tanvi]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d53ff47e3297