711908 - Fix a bunch of GCC warnings in layout

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch fixes about 29 warnings that I get in layout/ on debug linux64
builds.  The fixed cases are mostly -Wunused-but-set-variable and -Wformat,
with a couple of -Wuninitialized ones where I was very confident I was doing
something reasonable.  There are still quite a few warnings left, mostly
-Wuninitialized ones.

Note: The |diffsquared| case nsStyleAnimation.cpp looks like it was a
genuine bug.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 582747 [details] [diff] [review]
patch

>diff --git a/layout/base/nsFrameManager.cpp b/layout/base/nsFrameManager.cpp
>--- a/layout/base/nsFrameManager.cpp
>+++ b/layout/base/nsFrameManager.cpp
>@@ -486,19 +486,20 @@ nsFrameManager::AppendFrames(nsIFrame*  
> }
> 
> nsresult
> nsFrameManager::InsertFrames(nsIFrame*       aParentFrame,
>                              ChildListID     aListID,
>                              nsIFrame*       aPrevFrame,
>                              nsFrameList&    aFrameList)
> {
>-  NS_PRECONDITION(!aPrevFrame || (!aPrevFrame->GetNextContinuation()
>-                  || IS_TRUE_OVERFLOW_CONTAINER(aPrevFrame->GetNextContinuation()))
>-                  && !IS_TRUE_OVERFLOW_CONTAINER(aPrevFrame),
>+  NS_PRECONDITION(!aPrevFrame ||
>+                  ((!aPrevFrame->GetNextContinuation()
>+                   || IS_TRUE_OVERFLOW_CONTAINER(aPrevFrame->GetNextContinuation()))
>+                   && !IS_TRUE_OVERFLOW_CONTAINER(aPrevFrame)),
>                   "aPrevFrame must be the last continuation in its chain!");

Are you sure this is what was intended?  I've delayed fixing this one for a while because I didn't have time to think about what the author meant.

>diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
>--- a/layout/base/nsLayoutUtils.cpp
>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -2520,16 +2520,19 @@ nsLayoutUtils::ComputeWidthValue(
>                          (aBoxSizingToMarginEdge + aContentEdgeToBoxSizing);
>           result = NS_MAX(min, NS_MIN(pref, fill));
>           NS_ASSERTION(result >= 0, "width less than zero");
>         }
>         break;
>       case NS_STYLE_WIDTH_AVAILABLE:
>         result = aContainingBlockWidth -
>                  (aBoxSizingToMarginEdge + aContentEdgeToBoxSizing);
>+      default:
>+        NS_NOTREACHED("unexpected coord value");
>+        result = 0;


Some "break" statements would be useful here.  (If this doesn't cause any test failures, please add some.)  That said, I really don't like adding code for unreachable cases; I prefer to let valgrind catch them.

>diff --git a/layout/base/nsPresContext.cpp b/layout/base/nsPresContext.cpp
>--- a/layout/base/nsPresContext.cpp
>+++ b/layout/base/nsPresContext.cpp
>@@ -501,16 +501,20 @@ nsPresContext::GetFontPreferences()
>     switch (eType) {
>       case eDefaultFont_Variable:  font = &mDefaultVariableFont;  break;
>       case eDefaultFont_Fixed:     font = &mDefaultFixedFont;     break;
>       case eDefaultFont_Serif:     font = &mDefaultSerifFont;     break;
>       case eDefaultFont_SansSerif: font = &mDefaultSansSerifFont; break;
>       case eDefaultFont_Monospace: font = &mDefaultMonospaceFont; break;
>       case eDefaultFont_Cursive:   font = &mDefaultCursiveFont;   break;
>       case eDefaultFont_Fantasy:   font = &mDefaultFantasyFont;   break;
>+      default:
>+        NS_NOTREACHED("bad font type");
>+        font = NULL;
>+        break;
>     }

nsnull, not NULL.

Also, there's absolutely no way to hit this code given the for loop just a few lines up.  I'd rather not have it.

>diff --git a/layout/base/tests/TestPoisonArea.cpp b/layout/base/tests/TestPoisonArea.cpp
>--- a/layout/base/tests/TestPoisonArea.cpp
>+++ b/layout/base/tests/TestPoisonArea.cpp

I think this was intentionally trying to avoid the code being optimized away, because it needs to trigger the memory access in order to test that frame poisoning causes crashes when expected -- are you sure your changes don't cause that?

>diff --git a/layout/generic/nsContainerFrame.cpp b/layout/generic/nsContainerFrame.cpp
>--- a/layout/generic/nsContainerFrame.cpp
>+++ b/layout/generic/nsContainerFrame.cpp
>@@ -835,17 +835,17 @@ nsContainerFrame::DoInlineIntrinsicWidth
>       NS_MAX(GetCoord(stylePadding->mPadding.Get(startSide), 0), 0) +
>       styleBorder->GetActualBorderWidth(startSide) +
>       GetCoord(styleMargin->mMargin.Get(startSide), 0);
>   }
> 
>   const nsLineList_iterator* savedLine = aData->line;
>   nsIFrame* const savedLineContainer = aData->lineContainer;
> 
>-  nsContainerFrame *lastInFlow;
>+  nsContainerFrame *lastInFlow = NULL;

nsnull.  But also obviously unreachable by inspection, since |this| is not null.

>diff --git a/layout/generic/nsLineLayout.cpp b/layout/generic/nsLineLayout.cpp
>--- a/layout/generic/nsLineLayout.cpp
>+++ b/layout/generic/nsLineLayout.cpp
>@@ -1796,30 +1796,36 @@ nsLineLayout::VerticalAlignFrames(PerSpa
>     // sanity check (see bug 105168, non-reproducible crashes from null frame)
>     NS_ASSERTION(frame, "null frame in PerFrameData - something is very very bad");
>     if (!frame) {
>       return;
>     }
> 
>     // Compute the logical height of the frame
>     nscoord logicalHeight;
>+#ifdef NOISY_VERTICAL_ALIGN
>     nscoord topLeading;
>+#endif
>     PerSpanData* frameSpan = pfd->mSpan;
>     if (frameSpan) {
>       // For span frames the logical-height and top-leading was
>       // pre-computed when the span was reflowed.
>       logicalHeight = frameSpan->mLogicalHeight;
>+#ifdef NOISY_VERTICAL_ALIGN
>       topLeading = frameSpan->mTopLeading;
>+#endif
>     }
>     else {
>       // For other elements the logical height is the same as the
>       // frames height plus its margins.
>       logicalHeight = pfd->mBounds.height + pfd->mMargin.top +
>         pfd->mMargin.bottom;
>+#ifdef NOISY_VERTICAL_ALIGN
>       topLeading = 0;
>+#endif 
>     }
> 
>     // Get vertical-align property
>     const nsStyleCoord& verticalAlign =
>       frame->GetStyleTextReset()->mVerticalAlign;
> #ifdef NOISY_VERTICAL_ALIGN
>     printf("  [frame]");
>     nsFrame::ListTag(stdout, frame);

The point of NOISY_VERTICAL_ALIGN is to print info that we have, not generate new stuff.  If we don't need it, drop it from the printing.


>diff --git a/layout/tables/nsTableFrame.cpp b/layout/tables/nsTableFrame.cpp
>--- a/layout/tables/nsTableFrame.cpp
>+++ b/layout/tables/nsTableFrame.cpp
>@@ -7053,19 +7051,16 @@ BCPaintBorderIterator::StoreColumnWidth(
> }
> /**
>  * Determine if a vertical segment owns the corder
>  */
> bool
> BCPaintBorderIterator::VerticalSegmentOwnsCorner()
> {
>   mozilla::css::Side cornerOwnerSide = NS_SIDE_TOP;
>-  bool bevel = false;
>-  nscoord cornerSubWidth;
>-  cornerSubWidth = (mBCData) ? mBCData->GetCorner(cornerOwnerSide, bevel) : 0;
>   // unitialized ownerside, bevel
>   return  (NS_SIDE_TOP == cornerOwnerSide) ||
>           (NS_SIDE_BOTTOM == cornerOwnerSide);
> }

This is pretty clearly wrong.  If it didn't cause tests to fail, please add such tests.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 582747 [details] [diff] [review]
patch

kicking r? over to dbaron, since he's already looked through it and is better-qualified to review changes to this code anyway. :)

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch v2

> >     switch (eType) {
> >       case eDefaultFont_Variable:  font = &mDefaultVariableFont;  break;
> >       case eDefaultFont_Fixed:     font = &mDefaultFixedFont;     break;
> >       case eDefaultFont_Serif:     font = &mDefaultSerifFont;     break;
> >       case eDefaultFont_SansSerif: font = &mDefaultSansSerifFont; break;
> >       case eDefaultFont_Monospace: font = &mDefaultMonospaceFont; break;
> >       case eDefaultFont_Cursive:   font = &mDefaultCursiveFont;   break;
> >       case eDefaultFont_Fantasy:   font = &mDefaultFantasyFont;   break;
> >+      default:
> >+        NS_NOTREACHED("bad font type");
> >+        font = NULL;
> >+        break;
> >     }
> 
> nsnull, not NULL.
> 
> Also, there's absolutely no way to hit this code given the for loop just a
> few lines up.  I'd rather not have it.

Using a switch here is an ugly way to do things, I've changed it to iterate over an array of font types.  If the enum is ever extended, the static assertion I added will catch it.  (Unlike the old switch code!)

> I think this was intentionally trying to avoid the code being optimized
> away, because it needs to trigger the memory access in order to test that
> frame poisoning causes crashes when expected -- are you sure your changes
> don't cause that?

I'm unconvinced that the original code is any safer against such optimizations, but I took an easier route and reverted and then added a |(void)scratch;| statement afterwards.

> >diff --git a/layout/generic/nsContainerFrame.cpp b/layout/generic/nsContainerFrame.cpp
> >--- a/layout/generic/nsContainerFrame.cpp
> >+++ b/layout/generic/nsContainerFrame.cpp
> >@@ -835,17 +835,17 @@ nsContainerFrame::DoInlineIntrinsicWidth
> >       NS_MAX(GetCoord(stylePadding->mPadding.Get(startSide), 0), 0) +
> >       styleBorder->GetActualBorderWidth(startSide) +
> >       GetCoord(styleMargin->mMargin.Get(startSide), 0);
> >   }
> > 
> >   const nsLineList_iterator* savedLine = aData->line;
> >   nsIFrame* const savedLineContainer = aData->lineContainer;
> > 
> >-  nsContainerFrame *lastInFlow;
> >+  nsContainerFrame *lastInFlow = NULL;
> 
> nsnull.  But also obviously unreachable by inspection, since |this| is not
> null.

This one struck me as an easy case to silence -- it's obviously a false positive, but there's an obvious initialization value to silence the warning, and if the code changes in the future so that the nsnull flows through we'll get an obvious nsnull access crash.  (And this is better than relying on Valgrind detecting use of an undefined variable, because people usually don't run under Valgrind.)

But I've reverted just to keep things moving.

dbaron, what's your general take on silencing warnings?  God's work, a fool's errand, or somewhere in between?  I ask because I've learnt that opinions vary greatly so I'm interested in getting extra data points.


> 
> >diff --git a/layout/generic/nsLineLayout.cpp b/layout/generic/nsLineLayout.cpp
> >--- a/layout/generic/nsLineLayout.cpp
> >+++ b/layout/generic/nsLineLayout.cpp
> >@@ -1796,30 +1796,36 @@ nsLineLayout::VerticalAlignFrames(PerSpa
> >     // sanity check (see bug 105168, non-reproducible crashes from null frame)
> >     NS_ASSERTION(frame, "null frame in PerFrameData - something is very very bad");
> >     if (!frame) {
> >       return;
> >     }
> > 
> >     // Compute the logical height of the frame
> >     nscoord logicalHeight;
> >+#ifdef NOISY_VERTICAL_ALIGN
> >     nscoord topLeading;
> >+#endif
> >     PerSpanData* frameSpan = pfd->mSpan;
> >     if (frameSpan) {
> >       // For span frames the logical-height and top-leading was
> >       // pre-computed when the span was reflowed.
> >       logicalHeight = frameSpan->mLogicalHeight;
> >+#ifdef NOISY_VERTICAL_ALIGN
> >       topLeading = frameSpan->mTopLeading;
> >+#endif
> >     }
> >     else {
> >       // For other elements the logical height is the same as the
> >       // frames height plus its margins.
> >       logicalHeight = pfd->mBounds.height + pfd->mMargin.top +
> >         pfd->mMargin.bottom;
> >+#ifdef NOISY_VERTICAL_ALIGN
> >       topLeading = 0;
> >+#endif 
> >     }
> > 
> >     // Get vertical-align property
> >     const nsStyleCoord& verticalAlign =
> >       frame->GetStyleTextReset()->mVerticalAlign;
> > #ifdef NOISY_VERTICAL_ALIGN
> >     printf("  [frame]");
> >     nsFrame::ListTag(stdout, frame);
> 
> The point of NOISY_VERTICAL_ALIGN is to print info that we have, not
> generate new stuff.  If we don't need it, drop it from the printing.

I can't tell if we need it or not.  But AFAICT |frameSpan| is never changed in this function so I've removed |topLeading| and used |frameSpan| directly in the code lower down.


> > bool
> > BCPaintBorderIterator::VerticalSegmentOwnsCorner()
> > {
> >   mozilla::css::Side cornerOwnerSide = NS_SIDE_TOP;
> >-  bool bevel = false;
> >-  nscoord cornerSubWidth;
> >-  cornerSubWidth = (mBCData) ? mBCData->GetCorner(cornerOwnerSide, bevel) : 0;
> >   // unitialized ownerside, bevel
> >   return  (NS_SIDE_TOP == cornerOwnerSide) ||
> >           (NS_SIDE_BOTTOM == cornerOwnerSide);
> > }
> 
> This is pretty clearly wrong.  If it didn't cause tests to fail, please add
> such tests.

I fixed this.  (And I'm reminded why I don't like using C++ references for outparams, oh well.)


I've reverted all the other changes you objected to, kicking the can down the road in order to make some progress.  24 warnings are still fixed in this version, if I've counted correctly.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> dbaron, what's your general take on silencing warnings?  God's work, a
> fool's errand, or somewhere in between?  I ask because I've learnt that
> opinions vary greatly so I'm interested in getting extra data points.

I think valgrind warnings are more important than compiler warnings, so I tend to dislike compiler warning fixes that would silence a potential future valgrind branch-on-uninitialized warning.

Beyond that I don't mind warning fixes -- having fewer compiler warnings is good -- as long as they don't conflict with other goals.

That said, I don't actually notice compiler warnings much because I have -Wshadow turned on in my build (since IMO it's one of the most important warnings gcc gives) since we turned it off because of issues it has in C (but not C++) that showed up in the JS engine back when it was in C, and people keep making -Wshadow very noisy.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 583001 [details] [diff] [review]
patch v2

>diff --git a/layout/base/nsPresContext.cpp b/layout/base/nsPresContext.cpp
>--- a/layout/base/nsPresContext.cpp
>+++ b/layout/base/nsPresContext.cpp
>@@ -486,32 +486,34 @@ nsPresContext::GetFontPreferences()
>   PRInt32 size = Preferences::GetInt(pref.get());
>   if (unit == eUnit_px) {
>     mMinimumFontSizePref = CSSPixelsToAppUnits(size);
>   }
>   else if (unit == eUnit_pt) {
>     mMinimumFontSizePref = CSSPointsToAppUnits(size);
>   }
> 
>+  nsFont* fontTypes[eDefaultFont_COUNT] = {

Leave this as [] rather than [eDefaultFont_COUNT]

>+    &mDefaultVariableFont,
>+    &mDefaultFixedFont,
>+    &mDefaultSerifFont,
>+    &mDefaultSansSerifFont,
>+    &mDefaultMonospaceFont,
>+    &mDefaultCursiveFont,
>+    &mDefaultFantasyFont
>+  };
>+  PR_STATIC_ASSERT(NS_ARRAY_LENGTH(fontTypes) == eDefaultFont_COUNT);

... so that this PR_STATIC_ASSERT will actually do something (rather than just ending up with a 0-padded array).

r=dbaron with that

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e25bdfb62c0

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1e25bdfb62c0