982315 - More maybe-used-uninitialized warning fixes

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: layout-used-uninitialized.diff

Following on from bug 978903, here are some more maybe-uninitialized warnings that can be addressed by code restructuring.  Case by case analysis:

FrameLayerBuilder::GetThebesLayerScaleForFrame: if aFrame is null on entry, the body of the for-loop is not executed, and we pass an uninitialized 'last' to PredictScaleForContent.  It appears that this should be impossible, but if it ever happens we should pass null for 'last' to ensure a *safe* crash => last should be initialized to nullptr.

ElementRestyler::RestyleChildren: the control flow can be made more comprehensible by factoring out the 'mHintsHandled & nsChangeHint_ReconstructFrame' condition; once that's done, the situation is evidently the same as above => 'lastContinuation' should be initialized to nullptr.

nsDisplayBackgroundImage::AppendBackgroundItemsToTop: currently structured like

   if (cond)
     var = value;
   ...
   if (cond && var != constant)
     use var;

which is easier for both humans and the compiler to understand if restructured as

   var = constant;
   if (cond)
      var = value;
   ...
   if (var != constant)
      use var;

MathMLTextRunFactory::RebuildTextRun: mathVar is used uninitialized when the text run length is zero; it appears that this case genuinely can happen; NS_MATHML_MATHVARIANT_NONE is a safe default.

Selection::Modify: add a missing "return" in an error case.

nsTreeBoxObject::GetTreeBody: same refactoring as for AppendBackgroundItemsToTop.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: addl-frame-patch.diff

One more piece for this bug - I'm manually breaking up a much larger diff and I missed some.  (Will be merged into the other patch for commit.)

nsIFrame::PeekOffsetParagraph: the blockFrameOrBR object is a 2-tuple, and either both or neither of its fields are meaningfully initialized.  We're relying on default-initialization of one field to guard uses of both, which is sound, but the compiler does not understand it.  Because the object is set in its entirety when it's set to a meaningful value, there is no plausible scenario where default-initializing both fields will lead to missed bugs in the future, so let's just go ahead and do that.

Comment 2

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

(In reply to Zack Weinberg (:zwol) from comment #0)
> ElementRestyler::RestyleChildren: the control flow can be made more
> comprehensible by factoring out the 'mHintsHandled &
> nsChangeHint_ReconstructFrame' condition; once that's done, the situation is
> evidently the same as above => 'lastContinuation' should be initialized to
> nullptr.

This is a substantive change (and I believe is incorrect), since mHintsHandled can get added to over the course of the function.

Comment 3

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

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #2)
> This is a substantive change (and I believe is incorrect), since
> mHintsHandled can get added to over the course of the function.

That said, refactoring to multiple early returns would be fine, since the changes to mHintsHandled should be only additive.

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8389425 [details] [diff] [review]
layout-used-uninitialized.diff

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3434,5 @@
>  gfxSize
>  FrameLayerBuilder::GetThebesLayerScaleForFrame(nsIFrame* aFrame)
>  {
> +  // This initialization only matters in the shouldn't-happen case
> +  // where aFrame is null on entry.  Then it ensures a safe crash.

Can you add an assertion for this?

@@ +3447,5 @@
>        // which is an ancestor of the popup.
>        break;
>      }
> +
> +    nsTArray<DisplayItemData*> *array =

Put "*" next to type while you're changing this line.

::: layout/base/RestyleManager.cpp
@@ +2523,5 @@
>    // kids would use mFrame->StyleContext(), which is out of date if
>    // mHintsHandled has a ReconstructFrame hint; doing this could trigger
>    // assertions about mismatched rule trees.
> +  if (mHintsHandled & nsChangeHint_ReconstructFrame)
> +    return;

Braces around this please.

@@ +2545,1 @@
>    }

Yes, we know we will have crashed in ElementRestyler's constructor if mFrame is null.

I was going to suggest that we can reword the loop so that we don't need the assignment to lastContinuation, and also skip the first |f| loop condition check:

  nsIFrame* lastContinuation;
  {
    nsIFrame* f = mFrame;
    do {
      lastContinuation = f;
      RestyleContentChildren(f, aChildRestyleHint);
      f = GetNextContinuationWithSameStyle(f, f->StyleContext());
    } while (f);
  }

or:

  nsIFrame* lastContinuation;
  for (nsIFrame* f = mFrame;;) {
    nsIFrame* next =
      GetNextContinuationWithSameStyle(f, f->StyleContext());
    if (!next) {
      lastContinuation = f;
      break;
    }
    RestyleContentChildren(f, aChildRestyleHint);
    f = next;
  }

but I think both of these make the loop logic too obscure, so I guess I'm fine with additional assignment.

::: layout/base/nsCounterManager.cpp
@@ +73,4 @@
>      aResult.Truncate();
> +    if (!mAllCounters || !mScopeStart) {
> +        // only one counter
> +        bool dummy;

I would prefer keeping the isTextRTL variable name, even though it's not used.

@@ +75,5 @@
> +        // only one counter
> +        bool dummy;
> +        nsBulletFrame::AppendCounterText(style, mValueAfter, aResult, dummy);
> +        return;
> +    }

If we're splitting up this function into two separate "one counter" or "multiple counters" cases, I think it would be clearer to get the counter style in the two branches too.  So do:

  int32_t style = mCounterStyle->Item(1).GetIntValue();

in this if-body, and then

  int32_t style = mCounterStyle->Item(2).GetIntValue();

below.

::: layout/generic/MathMLTextRunFactory.cpp
@@ +586,5 @@
>        }
>      }
>    }
>  
> +  uint8_t mathVar = NS_MATHML_MATHVARIANT_NONE;

I have a feeling we can return early if |length == 0|, though I'd want fredw to confirm.  Still, I don't mind the init here.

::: layout/generic/nsSelection.cpp
@@ +5670,5 @@
>             aGranularity.LowerCaseEqualsLiteral("paragraph") ||
>             aGranularity.LowerCaseEqualsLiteral("paragraphboundary") ||
>             aGranularity.LowerCaseEqualsLiteral("documentboundary")) {
>      aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
> +    return;

This one looks like a real bug!

Comment 5

[Zack Weinberg (:zwol)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #3)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #2)
> > This is a substantive change (and I believe is incorrect), since
> > mHintsHandled can get added to over the course of the function.
> 
> That said, refactoring to multiple early returns would be fine, since the
> changes to mHintsHandled should be only additive.

Yuck.  Will correct.  Would it be correct to return early in the middle of
the RestyleContentChildren loop, i.e.

  for (...) {
    lastContinuation = f;
    RestyleContentChildren(f, aChildRestyleHint);
    if (mHintsHandled & nsChangeHint_ReconstructFrame) {
      return;
    }
  }

?

(In reply to Cameron McCormack (:heycam) from comment #4)
> > +  // This initialization only matters in the shouldn't-happen case
> > +  // where aFrame is null on entry.  Then it ensures a safe crash.
> Can you add an assertion for this?

Sure.

> > +    nsTArray<DisplayItemData*> *array =
> Put "*" next to type while you're changing this line.

Done.

> > +  if (mHintsHandled & nsChangeHint_ReconstructFrame)
> > +    return;
> Braces around this please.

Done.

> I was going to suggest that we can reword the loop so that we don't need the
> assignment to lastContinuation, and also skip the first |f| loop condition
> check:

I think a slight modification of your first option is reasonably clear:

  // If mFrame were null at this point we would have crashed in the
  // constructor.
  nsIFrame* f = mFrame;
  nsIFrame* lastContinuation;
  do {
    RestyleContentChildren(f, aChildRestyleHint);

    lastContinuation = f;
    f = GetNextContinuationWithSameStyle(f, f->StyleContext());
  } while (f);


> ::: layout/base/nsCounterManager.cpp
> @@ +73,4 @@
> >      aResult.Truncate();
> > +    if (!mAllCounters || !mScopeStart) {
> > +        // only one counter
> > +        bool dummy;
> 
> I would prefer keeping the isTextRTL variable name, even though it's not
> used.

OK.

> If we're splitting up this function into two separate "one counter" or
> "multiple counters" cases, I think it would be clearer to get the counter
> style in the two branches too.

That unfortunately doesn't get us out of "mCounterStyle->Item(mAllCounters ? 2 : 1)" in the upper branch, because the upper branch is taken when !mScopeStart, as well as when !mAllCounters.  But I think it's cleaner that way anyway :)

> ::: layout/generic/MathMLTextRunFactory.cpp
> > +  uint8_t mathVar = NS_MATHML_MATHVARIANT_NONE;
> 
> I have a feeling we can return early if |length == 0|, though I'd want fredw
> to confirm.  Still, I don't mind the init here.

I'll see if anything breaks in the testsuite if I add an early return.  However, there are excellent odds that the compiler will not understand that |length > 0| means mathVar is initialized :-/

> ::: layout/generic/nsSelection.cpp
> @@ +5670,5 @@
> >             aGranularity.LowerCaseEqualsLiteral("paragraph") ||
> >             aGranularity.LowerCaseEqualsLiteral("paragraphboundary") ||
> >             aGranularity.LowerCaseEqualsLiteral("documentboundary")) {
> >      aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
> > +    return;
> 
> This one looks like a real bug!

Ayep, uh huh...

Comment 6

[Zack Weinberg (:zwol)]

Attachment: 982315-layout-uninitialized-warnings.diff

Current tentatively-revised patch.  No need to jump on review, let's see how the tryserver does: https://tbpl.mozilla.org/?tree=Try&rev=56bb8a78841a

Comment 7

[Zack Weinberg (:zwol)]

Attachment: 982315-layout-uninitialized-warnings.diff

Nnrgh, forgot to save one file.

https://tbpl.mozilla.org/?tree=Try&rev=b2078500b0fb

Comment 8

[Frédéric Wang (:fredw)]

IIRC, this function only performs transforms on the MathML text (ssty feature, mathvariant etc), so in theory if the length of the string is zero, there is no work to do. However, I don't remember if the tasks at the end of the function are necessary. FYI, the code was copied from nsCaseTransformTextRunFactory::RebuildTextRun so the same reasoning applies.

Comment 9

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

(In reply to Zack Weinberg (:zwol) from comment #5)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #3)
> > (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #2)
> > > This is a substantive change (and I believe is incorrect), since
> > > mHintsHandled can get added to over the course of the function.
> > 
> > That said, refactoring to multiple early returns would be fine, since the
> > changes to mHintsHandled should be only additive.
> 
> Yuck.  Will correct.  Would it be correct to return early in the middle of
> the RestyleContentChildren loop, i.e.
> 
>   for (...) {
>     lastContinuation = f;
>     RestyleContentChildren(f, aChildRestyleHint);
>     if (mHintsHandled & nsChangeHint_ReconstructFrame) {
>       return;
>     }
>   }
> 
> ?

RestyleContentChildren shouldn't touch mHintsHandled.  Only RestyleBeforePseudo, RestyleAfterPseudo, and RestyleSelf (which runs before RestyleChildren) should, I think.  (It should also be fine to have the first early-return be before the RestyleUndisplayedChildren, and then another check after the RestyleBeforePseudo call.  I think the current checks around RestyleContentChildren and RestyleAfterPseudo could actually be one check.)

Comment 11

[Chris Peterson [:cpeterson]]

Zack, is your patch to fix these -Wmaybe-uninitialized warnings in layout/ still relevant?

Many of gcc's -Wmaybe-uninitialized warnings are false positive, so fixing them has been a low priority compared to other compiler warnings.

Comment 12

[Zack Weinberg (:zwol)]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> Zack, is your patch to fix these -Wmaybe-uninitialized warnings in layout/
> still relevant?

I would imagine that by now the patch has bitrotted to the point where it would be easier to start over.  I do not have time to do this for the foreseeable future (if I had time to hack on Firefox at all there are many other things I would do first).

> Many of gcc's -Wmaybe-uninitialized warnings are false positive, so fixing
> them has been a low priority compared to other compiler warnings.

I still believe quite strongly that uninitialized-variable warnings should be considered must-fix *even if* code inspection suggests that they are false positives, mostly because it's too easy to get into the habit of ignoring *all* the uninitialized-variable warnings and then you don't notice the new one that's real.  But I was unable to convince dbaron or roc of this while I still worked for MoCo, so *shrug*.

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Zack Weinberg (:zwol) from comment #12)
> (In reply to Chris Peterson [:cpeterson] from comment #11)
> > Zack, is your patch to fix these -Wmaybe-uninitialized warnings in layout/
> > still relevant?
> 
> I would imagine that by now the patch has bitrotted to the point where it
> would be easier to start over.  I do not have time to do this for the
> foreseeable future (if I had time to hack on Firefox at all there are many
> other things I would do first).

Since this particular bug is not moving forward and its patch has bitrotted, I think we should close it. If someone has a new fix for current warnings, they can file a new bug.

> I still believe quite strongly that uninitialized-variable warnings should
> be considered must-fix *even if* code inspection suggests that they are
> false positives, mostly because it's too easy to get into the habit of
> ignoring *all* the uninitialized-variable warnings and then you don't notice
> the new one that's real.  But I was unable to convince dbaron or roc of this
> while I still worked for MoCo, so *shrug*.

We're in a much better place now than when you filed this bug. :) As of Firefox 40 (bug 1001975), -Wuninitialized and (clang's) -Wsometimes-uninitialized warnings are treated as errors. Those warnings are nearly always real bugs. gcc's -Wmaybe-uninitialized warnings, however, are about 90% false positives, so we still treat them as non-fatal warnings.