323916 - nsCSSRendering.cpp:1077: warning: operation on 'temp' may be undefined

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

../../../../mozilla/layout/base/nsCSSRendering.cpp: In static member function 'static void nsCSSRendering::DrawDashedSides(PRIntn, nsIRenderingContext&, const nsRect&, const nsStyleColor*, const nsStyleBorder*, const nsStyleOutline*, PRBool, const nsRect&, const nsRect&, PRIntn, nsRect*)':
../../../../mozilla/layout/base/nsCSSRendering.cpp:1004: warning: operation on 'temp' may be undefined
../../../../mozilla/layout/base/nsCSSRendering.cpp:1011: warning: operation on 'temp' may be undefined
../../../../mozilla/layout/base/nsCSSRendering.cpp:1070: warning: operation on 'temp' may be undefined
../../../../mozilla/layout/base/nsCSSRendering.cpp:1077: warning: operation on 'temp' may be undefined

And the code does do odd things:
1004             temp = temp-= (dashRect.height-adjust);

Either the "temp =" part or the "temp -=" part should probably be removed...

Comment 1

[Atul Aggarwal]

Attachment: Patch v1 to fix warning

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Atul, is this the right patch?

Comment 3

[Atul Aggarwal]

Yes, this is the right patch. Actually now we have quite different warnings in the nsCSSRendering.cpp file. Current warnings are:

/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp: In static member function 'static void nsCSSRendering::PaintOutline(nsPresContext*, nsRenderingContext&, nsIFrame*, const nsRect&, const nsRect&, nsStyleContext*)':
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:699: warning: 'width' may be used uninitialized in this function
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp: In static member function 'static void nsCSSRendering::PaintBackgroundWithSC(nsPresContext*, nsRenderingContext&, nsIFrame*, const nsRect&, const nsRect&, nsStyleContext*, const nsStyleBorder&, PRUint32, nsRect*)':
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:1516: warning: 'color' may be used uninitialized in this function
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:1516: note: 'color' was declared here
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:2364: warning: 'isSolidBorder' may be used uninitialized in this function
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:2363: warning: 'currentBackgroundClip' may be used uninitialized in this function
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp: In static member function 'static void nsCSSRendering::PaintDecorationLine(gfxContext*, nscolor, const gfxPoint&, const gfxSize&, gfxFloat, gfxFloat, PRUint8, PRUint8, gfxFloat)':
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:3397: warning: 'oldLineWidth' may be used uninitialized in this function
nsFirstLetterFrame.cpp

Sorry, I forgot to mention those.

Comment 4

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

(In reply to Atul Aggarwal from comment #3)
> Yes, this is the right patch. Actually now we have quite different warnings
> in the nsCSSRendering.cpp file.


Then I suspect it probably should have been a different bug.

That said, I think these current warnings are mostly or entirely bogus, and the fixes make the code more confusing.

Comment 5

[Atul Aggarwal]

I completely agree that these are false warnings and make code tougher to read. But either there has a way to shut the gcc about false warnings or we have to fix them otherwise developers(including me) tends to overlook the warning if there are lots of them and serious bugs are missed. Otherwise there is no point of adding gcc flags in the builds for showing warnings which just fills build logs and make search difficult in case of any failure.

Comment 6

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

I think we should turn off these warnings instead.

Comment 7

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

Comment on attachment 557620 [details] [diff] [review]
Patch v1 to fix warning

I don't think initializing variables to nonsense values is a good idea.  It makes the code harder to understand.

We should depend on valgrind catching stuff like this, and I tend to think we should turn off this gcc warning if it's this noisy.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'd be more inclined to agree with you if it had caught things like <http://hg.mozilla.org/integration/mozilla-inbound/file/dd6e523b0b82/content/html/document/src/nsHTMLFragmentContentSink.cpp#l942> (which GCC did catch, but the warning was ignored because of the large number of unfixed warnings).

Comment 9

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

If you want to fix these warnings, the way to do it is to de-COM-taminate the methods and make them return things rather than take out parameters.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 557620 [details] [diff] [review]
Patch v1 to fix warning

># HG changeset patch
># Parent 84233d215c4e8d2f2f71e3d36cb912f88d3c1498
># User Atul Aggarwal <[e-mail]>
># Date 1314906994 -19800
>
>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
>--- a/layout/base/nsCSSRendering.cpp
>+++ b/layout/base/nsCSSRendering.cpp
>@@ -691,17 +691,17 @@ nsCSSRendering::PaintOutline(nsPresConte
>                              const nsRect& aBorderArea,
>                              nsStyleContext* aStyleContext)
> {
>   nscoord             twipsRadii[8];
> 
>   // Get our style context's color struct.
>   const nsStyleOutline* ourOutline = aStyleContext->GetStyleOutline();
> 
>-  nscoord width;
>+  nscoord width = 0;
>   ourOutline->GetOutlineWidth(width);

David, are we actually guaranteed that ourOutline->mHasCachedOutline is true here?

Comment 11

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

(In reply to Ms2ger from comment #10)
> David, are we actually guaranteed that ourOutline->mHasCachedOutline is true
> here?

Yes, since it's always true.  nsRuleNode::ComputeOutlineData calls nsStyleOutline::RecalcData which sets it to true for every legal type of mOutlineWidth.  We should probably clean that up at some point.

(Then again, *this bug* is WORKSFORME.)

Comment 12

[Atul Aggarwal]

Suggesting closing the bug as David suggested.

Comment 13

[:Cykesiopka]

Can this bug be closed per comment 12?

Comment 14

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

Yes.