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 on attachment 710444 [details] [diff] [review] fix Review of attachment 710444 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks for catching this dholbert!

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)

(fixed a typo in commit message)

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.