860913 - Fix MSVC build warnings in content/html/content/src/ and mark it as FAIL_ON_WARNINGS-for-all-compilers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

bug 857669 tracks fixing a GCC warning in content/html/content/src/, making it warning-free on GCC and clang again.

However, we still have MSVC warnings in that directory (even with bug 857863 fixed). Filing this bug on fixing those.

The warnings are at least the following:
{
HTMLAnchorElement.cpp
/dom/HTMLAnchorElement.h(29) : warning C4355: 'this' : used in base member initializer list

HTMLAreaElement.cpp(17) : warning C4355: 'this' : used in base member initializer list

HTMLInputElement.cpp(1231) : warning C4003: not enough actual parameters for macro 'MOZ_NOT_REACHED'

HTMLInputElement.cpp(1432) : warning C4003: not enough actual parameters for macro 'MOZ_NOT_REACHED'

HTMLInputElement.cpp(1491) : warning C4003: not enough actual parameters for macro 'MOZ_NOT_REACHED'

HTMLInputElement.cpp(6071) : warning C4003: not enough actual parameters for macro 'MOZ_NOT_REACHED'

HTMLInputElement.cpp(6089) : warning C4003: not enough actual parameters for macro 'MOZ_NOT_REACHED'

HTMLLinkElement.cpp(34) : warning C4355: 'this' : used in base member initializer list

HTMLSelectElement.cpp(107) : warning C4355: 'this' : used in base member initializer list

HTMLTextAreaElement.cpp(62) : warning C4355: 'this' : used in base member initializer list
}
(taken from the build log of a recent try build with FAIL_ON_WARNINGS=1 for all compilers in this directory:
https://tbpl.mozilla.org/php/getParsedLog.php?id=21696976&tree=Try )

Comment 1

[Daniel Holbert [:dholbert]]

The MOZ_NOT_REACHED usages almost certainly want to be MOZ_CRASH instead. As stated in various places in bug 820686, MOZ_NOT_REACHED triggers _undefined (potentially unsafe) behavior_, and it's intended as a compiler hint rather than as an assertion.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 1: use ALLOW_THIS_IN_INITIALIZER_LIST

This silences the existing warnings for 'this' used in init list, in this directory.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 2: replace MOZ_NOT_REACHED with MOZ_CRASH

As noted in comment 1, MOZ_NOT_REACHED is probably not what we want here. (It's deceptively-named -- it's *not* the same as NS_NOT_REACHED. bug 820686 goes into more detail.)

For the purposes of this bug -- MSVC is warning because MOZ_NOT_REACHED requires an argument. But conveniently, if we replace it with MOZ_CRASH (which likely provides the behavior that was actually intended), the warning goes away, since MOZ_CRASH doesn't require an argument.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 3: Mark directory as FAIL_ON_WARNINGS

...and this restores the FAIL_ON_WARNINGS, without the "if (not MSVC)" wrapper that used to be there.  As long as I haven't missed any warnings (which is possible, if the log quoted in comment 0 terminated before finishing the directory), this should be valid.

Try server run validating this:
  https://tbpl.mozilla.org/?tree=Try&rev=11589b64e012

Comment 5

[Daniel Holbert [:dholbert]]

For historical purposes:
 - This directory was originally marked as FAIL_ON_WARNINGS in bug 716338 (before we honored that variable for MSVC)
 - Then, this was given a "ifndef _MSC_VER" wrapper in Bug 824247 (the bug that made us start honoring FAIL_ON_WARNINGS for MSVC)
 - Then, this was all ripped out in https://hg.mozilla.org/mozilla-central/rev/c4cec4613cac , partly due to frustration caused by the MSVC-exemption (and tbpl bustage that would have been caught if the exemption were removed)
 - Finally, this bug is intended to add the FAIL_ON_WARNINGS back, without any MSVC exemption.

Comment 6

[Daniel Holbert [:dholbert]]

Try run is green!

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 736521 [details] [diff] [review]
part 2: replace MOZ_NOT_REACHED with MOZ_CRASH

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

I disagree with the semantic here. MOZ_NOT_REACHED() seems to be the correct thing to use. I see MOZ_CRASH to be used when we clearly can't do anything without putting the user at risk and stopping the application is the best thing to do. Here, we can return something and move on.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Mounir Lamouri (:mounir) from comment #7)
> Here, we can return something and move on.

I completely agree, which is *precisely why* MOZ_NOT_REACHED is wrong.  It's not an assertion, like NS_NOT_REACHED is -- it's a compiler hint which triggers undefined runtime behavior.  As noted in bug 820686 comment 8, it is *strictly less safe* than MOZ_CRASH & all of our other assertions / aborts.

If you prefer, I can swap it out for NS_NOT_REACHED (which is an assertion), with "shouldn't get here" or something as the warning message.

Comment 9

[Daniel Holbert [:dholbert]]

Oh, I thought MOZ_CRASH was debug-only, though -- in skimming the Assertions.h, I've now remembered that it crashes both debug + opt builds, which isn't what we want here.

So -- we want or MOZ_ASSERT(false, ...), probably, if we want the strictness of the new MOZ_* assertions.  Or perhaps NS_NOTREACHED(...) if a softer assert-and-keep-running was intended.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: replace most of the MOZ_NOT_REACHED with MOZ_ASSERT(false)

OK -- this removes the MOZ_NOT_REACHED with MOZ_ASSERT(false), which is the actual-assertion component of MOZ_NOT_REACHED (without the undefined-behavior-afterwards baggage).

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 736850 [details] [diff] [review]
part 2 v2:  replace most of the MOZ_NOT_REACHED with MOZ_ASSERT(false)

A few clarifications on the new patch:

>diff --git a/content/html/content/src/HTMLInputElement.cpp b/content/html/content/src/HTMLInputElement.cpp
>--- a/content/html/content/src/HTMLInputElement.cpp
>+++ b/content/html/content/src/HTMLInputElement.cpp
>@@ -1222,19 +1222,16 @@ HTMLInputElement::ConvertStringToNumber(
>     default:
>       return false;
>   }
>-
>-  MOZ_NOT_REACHED();
>-  return false;
> }

This is actually-unreachable code, so IMHO it's clearer just to drop it rather than to have a never-visited "return false" statement.  (If it somehow becomes reachable, then we'll know because we'll get a build error for the function failing to return a boolean value like it's supposed to.)

Note that in other functions in this file (e.g. the final few chunks touched by this patch) we don't bother with an unreachable return statement after the switch; I think we should follow suit here as well.

> JS::Value
> HTMLInputElement::GetValueAsDate(JSContext* aCx, ErrorResult& aRv)
> {
>-  if (mType != NS_FORM_INPUT_DATE && mType != NS_FORM_INPUT_TIME) {
>-    return JS::NullValue();
>-  }
>-
>   switch (mType) {
>     case NS_FORM_INPUT_DATE:
>     {
>       uint32_t year, month, day;
>       nsAutoString value;
>       GetValueInternal(value);
>       if (!GetValueAsDate(value, &year, &month, &day)) {
>         return JS::NullValue();
[...]
>       JSObject* date = JS_NewDateObjectMsec(aCx, millisecond);
>       if (!date) {
>         JS_ClearPendingException(aCx);
>         return JS::NullValue();
>       }
> 
>       return JS::ObjectValue(*date);
>     }
>-  }
>-
>-  MOZ_NOT_REACHED();
>-  aRv.Throw(NS_ERROR_UNEXPECTED);
>-  return JS::NullValue();
>+    default:
>+      return JS::NullValue();
>+  }
> }

Here, the MOZ_NOT_REACHED was likewise unreachable, and the "aRv.Throw(NS_ERROR_UNEXPECTED);" line of code was impossible to hit.  We could only hit it if our type was neither DATE nor TIME, and we already explicitly check for those at the beginning of this method. So, I'm just compressing that check (and its silent return) into the switch statement, and dropping the NOT_REACHED stuff.
MXR link for reference, if that doesn't make sense and you want to see more contextual code: https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLInputElement.cpp#1438

Comment 12

[Mounir Lamouri (:mounir)]

Comment on attachment 736850 [details] [diff] [review]
part 2 v2:  replace most of the MOZ_NOT_REACHED with MOZ_ASSERT(false)

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

IMO, we should fix MOZ_NOT_REACHED() to not warn on Windows when the optional argument isn't present. As we obviously do for MOZ_ASSERT.

::: content/html/content/src/HTMLInputElement.cpp
@@ +1425,5 @@
>  
>          return true;
>        }
>      default:
> +      MOZ_ASSERT(false, "Unrecognized input type");

I think MOZ_NOT_REACHED() is appropriate here: we actually help the compiler now this SHOULD NOT happen.

@@ -1438,5 @@
>  HTMLInputElement::GetValueAsDate(JSContext* aCx, ErrorResult& aRv)
>  {
> -  if (mType != NS_FORM_INPUT_DATE && mType != NS_FORM_INPUT_TIME) {
> -    return JS::NullValue();
> -  }

Actually, I would like to keep that because it should later be changed to "DoesDateApply()" and then the MOZ_NOT_REACHED() would make sense.

@@ +1479,5 @@
>  
>        return JS::ObjectValue(*date);
>      }
> +    default:
> +      return JS::NullValue();

Seems good if you call a MOZ_NOT_REACHED() before returning.

@@ +6058,5 @@
>        return kStepScaleFactorNumberRange;
>      case NS_FORM_INPUT_TIME:
>        return kStepScaleFactorTime;
>      default:
> +      MOZ_ASSERT(false, "Unrecognized input type");

I believe that MOZ_NOT_REACHED() was completely appropriate: we shouldn't be there if step applies.

@@ +6076,5 @@
>        return kDefaultStep;
>      case NS_FORM_INPUT_TIME:
>        return kDefaultStepTime;
>      default:
> +      MOZ_ASSERT(false, "Unrecognized input type");

ditto

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 736850 [details] [diff] [review]
part 2 v2:  replace most of the MOZ_NOT_REACHED with MOZ_ASSERT(false)

re-asking for review, after chatting on IRC

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: part 2 v3: replace most of the MOZ_NOT_REACHED with MOZ_ASSERT(false)

(In reply to Mounir Lamouri (:mounir) from comment #12)
> Actually, I would like to keep that because it should later be changed to
> "DoesDateApply()" and then the MOZ_NOT_REACHED() would make sense.

OK - that makes the existing logic make more sense (depending on what DoesDateApply checks).

I reverted my logic-shuffle here and just made this part a s/MOZ_NOT_REACHED/MOZ_ASSERT/

Comment 15

[Mounir Lamouri (:mounir)]

Comment on attachment 737528 [details] [diff] [review]
part 2 v3: replace most of the MOZ_NOT_REACHED with MOZ_ASSERT(false)

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

::: content/html/content/src/HTMLInputElement.cpp
@@ +1223,5 @@
>  
>        aResultValue = static_cast<double>(milliseconds);
>        return true;
>      default:
>        return false;

Could you add a MOZ_ASSERT here?

Comment 16

[Daniel Holbert [:dholbert]]

Yup. Fixed that and gave it a Try run (w/ unit tests just to be sure I was exercising the assertions in the HTMLInputElement.cpp code):
 https://tbpl.mozilla.org/?tree=Try&rev=060e2770bbbf

That's looking green, so I canceled its remaining jobs to save resources (those are the pink jobs), and I pushed to inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/98e99b17ce08
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e9352eddeb
  https://hg.mozilla.org/integration/mozilla-inbound/rev/fea489bb4f30

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98e99b17ce08
https://hg.mozilla.org/mozilla-central/rev/f2e9352eddeb
https://hg.mozilla.org/mozilla-central/rev/fea489bb4f30