I suspect MOZ_ASSERT(false, "...") was intended in these cases: # grep -r 'MOZ_ASSERT("' . ./dom/system/gonk/AudioManager.cpp: MOZ_ASSERT("unexpected audio channel for volume control"); ./dom/audiochannel/AudioChannelService.cpp: MOZ_ASSERT("unexpected audio channel for volume control"); ./dom/wifi/NetUtils.h: MOZ_ASSERT("Symbol not found in shared library : " #name); \

We should make that code not compile in the first place.

Argh, TypeTraits.h doesn't build in C code (of course!) https://tbpl.mozilla.org/?tree=Try&rev=d18e939391ce

Wrong patch! https://tbpl.mozilla.org/?tree=Try&rev=de72baafac43

bad bad js...

Hi Mats & Ehsan, Thanks for correcting this error.

Shouldn't it be changed to MOZ_CRASH or MOZ_ASSUME_UNREACHABLE (if performance sensitive)?

(In reply to Masatoshi Kimura [:emk] from comment #7) > Shouldn't it be changed to MOZ_CRASH or MOZ_ASSUME_UNREACHABLE (if > performance sensitive)? No, they both mean very different things than MOZ_ASSERT.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5) > Created attachment 809585 [details] [diff] [review] > Patch (v3) > > bad bad js... Hmm, so JS_ASSERT only takes one argument. I have no idea what those JS_ASSERTS should be.

Waiting to hear from Waldo on the patch before spending the time to green it up more.

Comment on attachment 809585 [details] [diff] [review] Patch (v3) Review of attachment 809585 [details] [diff] [review]: ----------------------------------------------------------------- Please use MOZ_ASSUME_UNREACHABLE in all the js/ files, at least. I'd use it everywhere, actually, but I don't hugely care about it outside js. ::: mfbt/Assertions.h @@ +282,5 @@ > * *only* during debugging, not "in the field". > */ > #ifdef DEBUG > + /* Define a helper which makes sure the MOZ_ASSERT("foo") pattern results in > + a build error. */ /* * Define a... * ..build error. */ @@ +285,5 @@ > + /* Define a helper which makes sure the MOZ_ASSERT("foo") pattern results in > + a build error. */ > +# ifdef __cplusplus > +namespace mozilla { > +namespace detail { Two-space indentation for the contents here. @@ +288,5 @@ > +namespace mozilla { > +namespace detail { > + template <class T> > + long IsStringLiteral(const T&); > + template <size_t N> template<typename T> and template<size_t N> @@ +294,5 @@ > +} > +} > +# define MOZ_ASSERT_NOT_STRING_LITERAL(cond) \ > + static_assert(!(mozilla::IsSame<decltype(mozilla::detail::IsStringLiteral(cond)), int>::value), \ > + "Cannot pass a string literal as the condition") Align the "s" in static under the "f" in define, please. We could also do this, less-precisely, by adding mozilla::IsArray to TypeTraits.h. I'm not too worried about people passing non-char arrays to MOZ_ASSERT, to be honest. :-) But it doesn't really matter.

So I just found out that we build the omx-plugin code in C++98 mode! :( <http://mxr.mozilla.org/mozilla-central/source/media/omx-plugin/Makefile.in#32> Chris(es), could we not just fix the headers that we're using here? This makes the omx-plugin code unable to use any of the C++11 features which are currently deemed allowed in our code base (static_assert in this case.)

FWIW removing the char16_t and char32_t typedefs from the Unicode.h and String{8,16}.h headers seems to fix the build in C++11 for me.

Alrighty, thanks, gcc 4.4. This seems like game over. In file included from /media/storage/moz/src/mfbt/decimal/../double-conversion/double-conversion.h:33, from /media/storage/moz/src/mfbt/decimal/moz-decimal-utils.h:14, from /media/storage/moz/src/mfbt/decimal/Decimal.cpp:33: /media/storage/moz/src/mfbt/decimal/../double-conversion/utils.h: In member function 'double_conversion::Vector<T> double_conversion::Vector<T>::SubVector(int, int)': /media/storage/moz/src/mfbt/decimal/../double-conversion/utils.h:151: sorry, unimplemented: unable to determine the declared type of expression 'mozilla::detail::IsStringLiteral((to <= ((double_conversion::Vector<T>*)this)->double_conversion::Vector<T>::length_))' /media/storage/moz/src/mfbt/decimal/../double-conversion/utils.h:151: error: type of 'mozilla::detail::IsStringLiteral((to <= ((double_conversion::Vector<T>*)this)->double_conversion::Vector<T>::length_))' is unknown /media/storage/moz/src/mfbt/decimal/../double-conversion/utils.h:151: error: template argument 1 is invalid

I'm going to check in the parts that do not touch mfbt/Assertions.h.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14) > Alrighty, thanks, gcc 4.4. This seems like game over. Ehsan, what do you mean by "game over"? (In reply to :Ehsan Akhgari (needinfo? me!) from comment #13) > FWIW removing the char16_t and char32_t typedefs from the Unicode.h and > String{8,16}.h headers seems to fix the build in C++11 for me. I don't see a big problem with editing our tree's copy of Android's header files if our definitions of char16_t and char32_t don't cause ABI changes in the Stagefright APIs we call.

(In reply to Chris Peterson (:cpeterson) from comment #17) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #14) > > Alrighty, thanks, gcc 4.4. This seems like game over. > > Ehsan, what do you mean by "game over"? That we can't land the interesting bit of my patch because of this compiler bug. > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #13) > > FWIW removing the char16_t and char32_t typedefs from the Unicode.h and > > String{8,16}.h headers seems to fix the build in C++11 for me. > > I don't see a big problem with editing our tree's copy of Android's header > files if our definitions of char16_t and char32_t don't cause ABI changes in > the Stagefright APIs we call. Great. I have a patch which does that over in bug 922664 which is green on try.

Hi Ehsan, The patch here did the change as below. http://hg.mozilla.org/mozilla-central/file/64b497e6f593/dom/audiochannel/AudioChannelService.cpp#l712 This will cause web content who has permission of mozSettings can be easy to crash B2G. It just need to set any audio_volume_XXX which is not in the check list. So I will modify it to NS_WARNING or others by Bug 923173. If you have any comment please leave the message there. Thanks.

(In reply to Marco Chen [:mchen] (Summit 10/03 ~10/08) from comment #20) > Hi Ehsan, > > The patch here did the change as below. > http://hg.mozilla.org/mozilla-central/file/64b497e6f593/dom/audiochannel/ > AudioChannelService.cpp#l712 > > This will cause web content who has permission of mozSettings can be easy to > crash B2G. > It just need to set any audio_volume_XXX which is not in the check list. > So I will modify it to NS_WARNING or others by Bug 923173. > > If you have any comment please leave the message there. Thanks. Yeah that's fine. I r+ed your patch in that bug.

The compiler error in comment 14 seems to be about decltype. Fortunately, there is no need for decltype here, we can implement what we want in a simple low-tech way that will please even b2g gcc 4.4: #include <iostream> #include <stdint.h> template <typename T> struct is_char_array { static const bool value = false; }; template <size_t N> struct is_char_array<char[N]> { static const bool value = true; }; template <size_t N> struct is_char_array<const char[N]> { static const bool value = true; }; template <typename T, bool allowed = !is_char_array<T>::value> struct static_assert_allowed_type_impl { static_assert(allowed, "This type is not allowed. Also, you suck."); }; template <typename T> void static_assert_allowed_type(const T&) { static_assert_allowed_type_impl<T>(); } using namespace std; int main() { static_assert_allowed_type(123); // uncomment me to see a nice error //static_assert_allowed_type("foo"); }

Note that this old patch uses MOZ_ASSUME_UNREACHABLE, but should probably use MOZ_ASSERT or MOZ_CRASH. See bug 990764 for discussion of removing or replacing MOZ_ASSUME_UNREACHABLE.

Yup and note that the present use of MOZ_ASSUME_UNREACHABLE is confirmed to be compiled quite dangerously by GCC 4.6 at least: if (condition) { ... } else if (other condition) { ... ... ... } else { MOZ_ASSUME_UNREACHABLE(); } is typically compiled by GCC -O3 by having the MOZ_ASSUME_UNREACHABLE jump to the very end of the function, and by "very end" I mean *after* any ret instruction, so execution happily continues into whatever is next in our address space :) For more details see https://raw.githubusercontent.com/bjacob/builtin-unreachable-study/master/notes

That said, the conditions here are based on "settings" that are not exposed to unprivileged Web content, only to certified Gaia apps, so I suppose that that's not a security issue. (But basically if B2G GCC is similar to what I've seen with GCC 4.6 on desktop linux, then Gaia certified apps can cause dangerous crashes by playing with settings).

In the end I settled for decltype instead of templates because it's much simpler, as the templates approach turned out to be trickier than expected (I could elaborate, but basically, it's not easy to actually reimplement decltype with just templates, without evaluating expressions more than once, without passing objects by value, and without binding references to things that don't want to be bound references to). As noted above, 'decltype', while in theory supported by all our compilers, in practice gives some trouble on B2G GCC, and also, I found, on MSVC 2010, when used in MOZ_ASSERT (because we assert on so many different things, it is quite the test for decltype support). I work around this by #ifdef'ing out bad compilers, i.e. GCC <= 4.4 and MSVC. In addition to guarding against literal strings, this also guards against all other arrays, as well as guarding against asserting on functions (forgetting to call them) and floating-point numbers (typical source of intermittent assertions).

Comment on attachment 8414648 [details] [diff] [review] Check the types of assert conditions Review of attachment 8414648 [details] [diff] [review]: ----------------------------------------------------------------- Thanks a lot!

This fixes the red B on emulator-JB on the above try push. Good thing that JB uses GCC 4.6, not GCC 4.4 like ICS does! (And that explains why ICS didn't catch it). Introduced in: changeset: 174468:8aa9ab187f81 user: Randy Lin <rlin@mozilla.com> date: Wed Mar 19 14:52:45 2014 +0800 summary: Bug 959490 - [MediaEncoder] Support *.3gp with AMR-NB audio format on certificated application. r=roc

Comment on attachment 8414648 [details] [diff] [review] Check the types of assert conditions Adding :froydnj as another possible review in the interest of load balancing.

Comment on attachment 8414648 [details] [diff] [review] Check the types of assert conditions Review of attachment 8414648 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, just some formatting nits and small fixups. ::: mfbt/Assertions.h @@ +281,5 @@ > */ > > +/* > + * Implement MOZ_VALIDATE_ASSERT_CONDITION_TYPE, which is used to guard against > + * accidentally passing something unintented in lieu of an assertion condition. Nit: "...something unintended" @@ +284,5 @@ > + * Implement MOZ_VALIDATE_ASSERT_CONDITION_TYPE, which is used to guard against > + * accidentally passing something unintented in lieu of an assertion condition. > + */ > + > +#ifdef __cplusplus Can you a) file a followup bug for MSVC support for this; and b) Add a comment about MSVC's (2010 at least) lack of proper decltype support pointing to that bug? Shiny bonus points if you go and determine if MSVC 2012 or 2013 has proper decltype support and add the appropriate conditionals here instead. But I don't think it's worth blocking the patch on determining that... @@ +285,5 @@ > + * accidentally passing something unintented in lieu of an assertion condition. > + */ > + > +#ifdef __cplusplus > + #if defined(__clang__) Everything in this __cplusplus block should be like so: # if defined(__clang) with the # at the beginning of line and space indents after. @@ +288,5 @@ > +#ifdef __cplusplus > + #if defined(__clang__) > + #define MOZ_SUPPORT_ASSERT_CONDITION_TYPE_VALIDATION > + #elif defined(__GNUC__) && MOZ_GCC_VERSION_AT_LEAST(4, 5, 0) > + #define MOZ_SUPPORT_ASSERT_CONDITION_TYPE_VALIDATION Did you check to see whether the polyfill for decltype in GCC (see mfbt/Types.h) was good enough? @@ +293,5 @@ > + #endif > +#endif > + > +#ifdef MOZ_SUPPORT_ASSERT_CONDITION_TYPE_VALIDATION > +#include "mozilla/TypeTraits.h" Nit: this should be |# include...| for consistency. @@ +297,5 @@ > +#include "mozilla/TypeTraits.h" > +namespace mozilla { > +namespace detail { > +template<typename T> > +struct IsFunction Whitespace line prior to this definition, please. @@ +299,5 @@ > +namespace detail { > +template<typename T> > +struct IsFunction > +{ > + static const bool value = false; Nit: four-space indent. @@ +302,5 @@ > +{ > + static const bool value = false; > +}; > +template<typename R, typename... A> > +struct IsFunction<R(A...)> Whitespace line prior to this definition, please. @@ +304,5 @@ > +}; > +template<typename R, typename... A> > +struct IsFunction<R(A...)> > +{ > + static const bool value = true; Nit: likewise. @@ +307,5 @@ > +{ > + static const bool value = true; > +}; > +template<typename T> > +void ValidateAssertConditionType() Whitespace line prior to this definition, please. @@ +319,5 @@ > + "It's often a bad idea to assert that a floating-point number is nonzero, " > + "because such assertions tend to intermittently fail. Shouldn't your code gracefully handle " > + "this case instead of asserting? Anyway, if you really want to " > + "do that, write an explicit boolean condition, like !!x or x!=0."); > +} Whitespace just after this too, please.

(In reply to Nathan Froyd (:froydnj) from comment #32) > @@ +284,5 @@ > > + * Implement MOZ_VALIDATE_ASSERT_CONDITION_TYPE, which is used to guard against > > + * accidentally passing something unintented in lieu of an assertion condition. > > + */ > > + > > +#ifdef __cplusplus > > Can you a) file a followup bug for MSVC support for this; Filed bug 1004028. > and b) Add a > comment about MSVC's (2010 at least) lack of proper decltype support > pointing to that bug? Done. > Shiny bonus points if you go and determine if MSVC > 2012 or 2013 has proper decltype support and add the appropriate > conditionals here instead. But I don't think it's worth blocking the patch > on determining that... As explained on that bug, I actually intentionally didn't try enabling this on newer MSVC versions for now and think we should wait until they are covered on TBPL before doing that. Otherwise, we'd worsen the problem that some build errors are only hit on these MSVC versions locally and there's no way to catch them on tryserver. > Did you check to see whether the polyfill for decltype in GCC (see > mfbt/Types.h) was good enough? No, but note that the problem is not GCC versions lacking the decltype keyword. GCC 4.4 (which is the problem here) does have decltype; it just has bugs; I don't expect __typeof__ to be better; anyway the polyfill in Types.h only kicks in in C++98 mode, which is not used on currently supported GCC versions afaik? (So in fact, I'm not sure that it's ever used at present?)

(In reply to Benoit Jacob [:bjacob] from comment #33) > (In reply to Nathan Froyd (:froydnj) from comment #32) > > @@ +284,5 @@ > > > + * Implement MOZ_VALIDATE_ASSERT_CONDITION_TYPE, which is used to guard against > > > + * accidentally passing something unintented in lieu of an assertion condition. > > > + */ > > > + > > > +#ifdef __cplusplus > > > > Can you a) file a followup bug for MSVC support for this; > > Filed bug 1004028. > > > and b) Add a > > comment about MSVC's (2010 at least) lack of proper decltype support > > pointing to that bug? > > Done. Thanks. > > Shiny bonus points if you go and determine if MSVC > > 2012 or 2013 has proper decltype support and add the appropriate > > conditionals here instead. But I don't think it's worth blocking the patch > > on determining that... > > As explained on that bug, I actually intentionally didn't try enabling this > on newer MSVC versions for now OK, that sounds fine. > > Did you check to see whether the polyfill for decltype in GCC (see > > mfbt/Types.h) was good enough? > > No, but note that the problem is not GCC versions lacking the decltype > keyword. GCC 4.4 (which is the problem here) does have decltype; it just has > bugs; I don't expect __typeof__ to be better; Fair enough.

https://hg.mozilla.org/mozilla-central/rev/1ddb30e14e82 https://hg.mozilla.org/mozilla-central/rev/6349837631e5

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #1) > We should make that code not compile in the first place. btw, clang -Wstring-conversion can warn if a string literal is used in a boolean expression like MOZ_ASSERT("literal string"): http://blog.llvm.org/2013/09/clang-warnings.html