126457 - Occurences of uninitialized variables being used before being set (in content/)

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Aleksey Nogin]

This bug is just for the warnings in various source files in content. Currently
(http://tinderbox.mozilla.org/SeaMonkey/warn1014136200.29882.html - Tue, 19 Feb
2002 11:30 EST) TBox shows the following warnings:

content/base/src/nsSelection.cpp:2250
 `class nsIFrame * thisBlock' might be used uninitialized in this function

content/html/style/src/nsCSSParser.cpp:2860
 `enum nsCSSUnit units' might be used uninitialized in this function

content/html/style/src/nsRuleNode.cpp:1804
 `enum nsSystemFontID sysID' might be used uninitialized in this function

content/html/style/src/nsRuleNode.cpp:4192
 `enum nsStyleContentType type' might be used uninitialized in this function

content/shared/src/nsStyleUtil.cpp:415
 `PRInt32 * column' might be used uninitialized in this function

content/xul/templates/src/nsTemplateMatchSet.cpp:257
 `class nsTemplateMatch ** last' might be used uninitialized in this function

Comment 1

[Aleksey Nogin]

Bug 59652 is the meta-bug tracking the fight against these (potentially very
nasty) warnings.

Comment 2

[harishd]

And why is this a parser problem?

Comment 3

[Aleksey Nogin]

Because http://www.mozilla.org/owners.html does not give enouh information to
make a better guess as to which module the content/... code belongs :-( Sorry if
I guessed wrong.

Comment 4

[harishd]

no worries.

The best way to find the responsible party is by using lxr.mozilla.org.

Comment 5

[Aleksey Nogin]

OK, 1 warning at a time:

1) content/base/src/nsSelection.cpp:2250
 `class nsIFrame * thisBlock' might be used uninitialized in this function

This warning seems to be a case of compiler stupidity (IMHO it would still be
nice to get rid of the warning). The check-in message is:

revision 3.87
date: 2001/03/21 01:13:06;  author: erik%netscape.com;  state: Exp;  lines: +964 -0
bug 71314; author=[e-mail]; r=mjudge,anthonyd; sr=erik; changes
from IBM bidi project (Arabic, Hebrew, etc); some in ifdef for now

2) content/html/style/src/nsCSSParser.cpp:2860
 `enum nsCSSUnit units' might be used uninitialized in this function

Another case of compiler stupidity.

revision 3.34
date: 1998/10/26 23:22:39;  author: peterl%netscape.com;  state: Exp;  lines:
+1492 -818
added CSS2 property handling

3) content/base/src/nsRuleNode.cpp:1804
`enum nsSystemFontID sysID' might be used uninitialized in this function

Here the problem seems to be that switch (aFontData.mFamily.GetIntValue()) does
not have a default clause (or is missing a case?).

This code was added while the file was still in content/html/style/src/

revision 3.1
date: 2001/05/31 22:19:28;  author: hyatt%netscape.com;  state: Exp;  lines:
+4490 -0
Fix for 78695 (rule matching improvements). r/sr=attinasi, jst, waterson

4) nsRuleNode.cpp:4192
 `enum nsStyleContentType type' might be used uninitialized in this function

Exactly the same as above (with the only difference that ``defaut:
NS_ERROR(...'' is present).

5) content/shared/src/nsStyleUtil.cpp:415
 `PRInt32 * column' might be used uninitialized in this function

revision 3.11
date: 2000/02/24 12:51:28;  author: pierre%netscape.com;  state: Exp;  lines:
+375 -27
Bug 18136/21950 "Fixing the font size mess". Implemented Todd Farhner's system
in nsStyleUtil. Disabled the font size rounding code on Windows (see bug 24005).
r=erik, a=rickg

6) content/xul/templates/src/nsTemplateMatchSet.cpp:257
 `class nsTemplateMatch ** last' might be used uninitialized in this function

Again, compiler not smart enough.

revision 1.2
date: 2001/04/04 04:59:55;  author: waterson%netscape.com;  state: Exp;  lines:
+268 -116
Bug 68213. Require users of nsFixedSizeAllocator to specify object size at
Free() time to avoid 8 byte overhead per allocation. r=harishd, brendan, shaver,
hyatt; sr=scc

Comment 6

[harishd]

Aleksey: Could you assign the bug to the appropriate engineers? Thanx

Comment 7

[harishd]

I'm not sure what to do with this bug. Will start from layout since html/style
sounds like layout :-/

Comment 8

[timeless]

.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <nsTemplateMatchSet.cpp> [Superseded by bug 285631]

See bug 132141 comment 4:
Rewrite of this code part,
which should fix the warning, and, hopefully, improve performance too.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 146143 [details] [diff] [review]
(Av1) <nsTemplateMatchSet.cpp>
[Superseded by bug 285631]


I have no compiler: Could you compile/test/review this patch ? Thanks.

Also, if this |Remove()| part is good, should I update |Contains()| and |Add()|
in the same way (array[] -> *pointer) ?

Comment 11

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

Comment on attachment 146143 [details] [diff] [review]
(Av1) <nsTemplateMatchSet.cpp>
[Superseded by bug 285631]

This changes the semantics of the function from removing *all* elements equal
to |aMatch| to removing *the first*.  Does that matter?

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> (From update of attachment 146143 [details] [diff] [review])
> This changes the semantics of the function from removing *all* elements equal
> to |aMatch| to removing *the first*.  Does that matter?

Good question ... for which I have a triple answer :-)

1) per |Add()|
{{
192         // Check for duplicates
193         for (PRInt32 i = PRInt32(count) - 1; i >= 0; --i) {
194             if (*(mStorageElements.mInlineMatches.mEntries[i]) == *aMatch)
195                 return PR_FALSE;
196         }
}}
there should be 1 matching entry at most !

2) As a confirmation, <nsTemplateMatchSet.h> reads:
{{
349     /**
350      * Remove a match from the set.
351      * @return PR_TRUE if the match was removed from the set; PR_FALSE
352      *   if the match was not present in the set.
353      */
354     PRBool Remove(const nsTemplateMatch* aMatch);
}}

3) Anyway, the existing code would *not* cope well with multiple matches,
despite what one could think at first:
{{
268             if (*match == *aMatch) {
269                 found = PR_TRUE;
270             }
271             else if (found)
272                 *last = match;
}}
handles only one of 'match' *or* 'found' for a given entry :-<

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 146143 [details] [diff] [review]
(Av1) <nsTemplateMatchSet.cpp>
[Superseded by bug 285631]

(I believe you just volonteered ;-))

Comment 14

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

Comment on attachment 146143 [details] [diff] [review]
(Av1) <nsTemplateMatchSet.cpp>
[Superseded by bug 285631]

http://mozilla.org/hacking/reviewers.html#seeking suggests you should test
before requesting superreview.	Superreviewers are overloaded enough as is, and
don't need to waste their time reviewing untested patches that have significant
probability of not working correctly.

Comment 15

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

(In reply to comment #12)
> 3) Anyway, the existing code would *not* cope well with multiple matches,

Ah, right.

> handles only one of 'match' *or* 'found' for a given entry :-<

No, that's not why.  It doesn't because |last| is always the immediate
predecessor of |entry| -- incrementing two pointers while traversing the list
would allow it to work fine.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> It doesn't because |last| is always the immediate
> predecessor of |entry|

Exactly: we agree anyway.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 146143 [details] [diff] [review]
(Av1) <nsTemplateMatchSet.cpp>
[Superseded by bug 285631]


(In reply to comment #14)

Clearing sr, rather than minus-ing...

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 146143 [details] [diff] [review]
(Av1) <nsTemplateMatchSet.cpp>
[Superseded by bug 285631]

This file was removed by bug 285631.

Comment 19

[Atul Aggarwal]

There is no warning at all in the layout/ code (using gcc4.6.1).
Hence closing this bug.