1027486 - Fix -Wsometimes-uninitialized warning in nsTextFrame.cpp

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: decorationsBlock_Wsometimes-uninitialized.patch

This clang warning is a regression from bug 727125. Even though the layout/generic directory is marked FAIL_ON_WARNINGS (except for MSVC), this warning was not detected because configure.in downgrades -Wsometimes-uninitialized warnings to not be errors.

I assert that decorationsBlock is not null because the called function UnionAdditionalOverflow() assumes its aBlock parameter is not null. Is it safe to assume decorationsBlock is always found and never null?

layout/generic/nsTextFrame.cpp:8603:30 [-Wsometimes-uninitialized] variable 'decorationsBlock' is used uninitialized whenever 'for' loop exits because its condition is false

Comment 1

[Daniel Holbert [:dholbert]]

I'm out this week, but I'll get to this next week, or you can have mats or dbaron or roc review if you'd like a quicker response.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8442620 [details] [diff] [review]
decorationsBlock_Wsometimes-uninitialized.patch

> bool
>+nsTextFrame::UpdateOverflow()
>+{
>+  const nsRect rect(nsPoint(0, 0), GetSize());

This part looks good.

>-  nsIFrame*decorationsBlock;
>+  nsIFrame* decorationsBlock = nullptr;
>   if (IsFloatingFirstLetterChild()) {
>     decorationsBlock = GetParent();
>   } else {
>     for (nsIFrame* f = this; f; f = f->GetParent()) {
>       nsBlockFrame* fBlock = nsLayoutUtils::GetAsBlock(f);
>       if (fBlock) {
>         decorationsBlock = fBlock;
>         break;
>       }
>     }
>   }
>+
>+  MOZ_ASSERT(decorationsBlock, "Couldn't find text-decoration block");

Hmm. So really, the part that clang is worried about is the "else" clause -- and there, decorationsBlock would only be left uninitialized if we exited the "for" loop by failing the loop condition.

And that would only happen if we iterated up to the root of the frame tree without ever finding a block frame, which I don't think is possible.  So, all should be well here. (aside from the compiler not knowing about this)

Anyway -- I'd rather we convert the "for" loop into a "while" loop, and instead of *breaking* when "f" is null, spam a NS_ERROR() and return false.  That way, we won't need any dummy initialization just to satisfy the compiler, and in the off chance that we do actually hit the scenario that the compiler is concerned about (never finding a block frame), we'll assert & bail out gracefully instead of proceeding & probably-crashing.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: without-initializing-decorationsBlock.patch

I couldn't see a simple way to use a while loop, as you had suggested, because the loop does not have one simple condition. Each loop iteration must check for both null GetAsBlock() and null GetParent().

This version of the patch is more complicated than the current code's for loop, but avoids unnecessary initialization of decorationsBlock and an unnecessary `f` null check for the first loop iteration (when f = this).

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: handle-null-decorationsBlock.patch

Alternatively, we could initialize decorationsBlock to null and check for null before calling UnionAdditionalOverflow(). This fix would handle the loop and also the case if GetParent() returned null in the IsFloatingFirstLetterChild() code path.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Chris Peterson (:cpeterson) from comment #3)
> Created attachment 8445705 [details] [diff] [review]
> without-initializing-decorationsBlock.patch
> 
> I couldn't see a simple way to use a while loop, as you had suggested,
> because the loop does not have one simple condition.

Oops -- sorry, my bad -- I meant to say "while (true)", not "while". It looks like you ended up at what I meant to say, using for (;;) instead of while (true), fortunately.

(In reply to Chris Peterson (:cpeterson) from comment #4)
> Created attachment 8445708 [details] [diff] [review]
> handle-null-decorationsBlock.patch
> 
> Alternatively, we could initialize decorationsBlock to null

I think you attached the wrong patch? (This looks the same as attachment 8445705 [details] [diff] [review].)

Comment 6

[Daniel Holbert [:dholbert]]

From your description, I can imagine the "handle-null-decorationsBlock.patch" patch would look like, though. (Similar to your original patch, but with an early-return instead of an assertion.)

I'd lean against that approach -- it still involves the unnecessary initialization [largely to pacify the compiler], and we also don't need to worry about the IsFloatingFirstLetterChild code-path -- there, we use GetParent(), which can only be null if we are the root frame. (and we'll never have a nsTextFrame as the root frame.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8445708 [details] [diff] [review]
handle-null-decorationsBlock.patch

Looks good! Just two prose nits:

>Bug 1027486 - Fix -Wsometimes-uninitialized warning in nsTextFrame.cpp. r?

Let's make this describe the real functional change (since there is a real functional change here). How about something like:

"Make nsTextFrame::UpdateOverflow bail out if it has no block ancestor (fixing -Wsometimes-uninitialized warning)."

>diff --git a/layout/generic/nsTextFrame.cpp b/layout/generic/nsTextFrame.cpp
>+      f = f->GetParent();
>+      if (!f) {
>+        NS_ERROR("Couldn't find text-decoration block frame in frame tree");

The phrase "text-decoration block frame" sounds like a special flavor of block frame (but there is no special flavoring here; just special usage).

Replace with something like:
 "Couldn't find any block ancestor (to use for text decorations)".

r=me with the above.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8445708 [details] [diff] [review]
handle-null-decorationsBlock.patch

Er, meant to + the request on the other copy of this patch.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8445705 [details] [diff] [review]
without-initializing-decorationsBlock.patch

(r=me with nits in comment 7 addressed)

Comment 10

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7228cafc3a2b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7228cafc3a2b