Closed Bug 1027486 Opened 10 years ago Closed 10 years ago

Fix -Wsometimes-uninitialized warning in nsTextFrame.cpp


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

Not set



Tracking Status
firefox32 --- unaffected
firefox33 --- fixed


(Reporter: cpeterson, Assigned: cpeterson)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

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 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
Attachment #8442620 - Flags: review?(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 on attachment 8442620 [details] [diff] [review]

> bool
>+  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.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Attachment #8442620 - Flags: review?(dholbert)
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).
Attachment #8442620 - Attachment is obsolete: true
Attachment #8445705 - Flags: review?(dholbert)
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.
Attachment #8445708 - Flags: feedback?(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].)
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 on attachment 8445708 [details] [diff] [review]

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.
Attachment #8445708 - Flags: feedback?(dholbert) → feedback+
Comment on attachment 8445708 [details] [diff] [review]

Er, meant to + the request on the other copy of this patch.
Attachment #8445708 - Attachment is obsolete: true
Attachment #8445708 - Flags: feedback+
Comment on attachment 8445705 [details] [diff] [review]

(r=me with nits in comment 7 addressed)
Attachment #8445705 - Flags: review?(dholbert) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.