Closed Bug 860913 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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 )
Component: Disability Access APIs → DOM
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
This silences the existing warnings for 'this' used in init list, in this directory.
Attachment #736520 - Flags: review?(mounir)
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.
Attachment #736521 - Flags: review?(mounir)
...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
Attachment #736522 - Flags: review?
Attachment #736522 - Flags: review? → review?(mounir)
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.
Try run is green!
Attachment #736520 - Flags: review?(mounir) → review+
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.
Attachment #736521 - Flags: review?(mounir) → review-
Attachment #736522 - Flags: review?(mounir) → review+
(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.
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.
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).
Attachment #736521 - Attachment is obsolete: true
Attachment #736850 - Flags: review?(mounir)
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 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
Attachment #736850 - Flags: review?(mounir) → review-
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
Attachment #736850 - Flags: review- → review?
Attachment #736850 - Flags: review? → review?(mounir)
(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/
Attachment #736850 - Attachment is obsolete: true
Attachment #736850 - Flags: review?(mounir)
Attachment #737528 - Flags: review?(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?
Attachment #737528 - Flags: review?(mounir) → review+
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
Flags: in-testsuite-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.