Closed Bug 126457 Opened 18 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect, trivial)

x86
Linux
defect
Not set
trivial

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: mozilla-bugs, Unassigned)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 obsolete file)

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
Bug 59652 is the meta-bug tracking the fight against these (potentially very
nasty) warnings.
Blocks: 59652
Keywords: mozilla1.0
And why is this a parser problem?
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.
no worries.

The best way to find the responsible party is by using lxr.mozilla.org.
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=simon@softel.co.il; 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
Aleksey: Could you assign the bug to the appropriate engineers? Thanx
Depends on: 132141
Depends on: 132145
I'm not sure what to do with this bug. Will start from layout since html/style
sounds like layout :-/
Assignee: harishd → attinasi
Component: Parser → Layout
QA Contact: moied → petersen
Priority: -- → P2
Target Milestone: --- → Future
Summary: Occurances of uninitialized variables being used before being set (in content/) → Occurences of uninitialized variables being used before being set (in content/)
.
Assignee: attinasi → misc
Component: Layout → Layout: Misc Code
Keywords: mozilla1.0helpwanted
QA Contact: petersen → nobody
No longer blocks: 179819
Depends on: 208868
No longer depends on: 132145
See bug 132141 comment 4:
Rewrite of this code part,
which should fix the warning, and, hopefully, improve performance too.
Assignee: core.layout.misc-code → gautheri
Status: NEW → ASSIGNED
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) ?
Attachment #146143 - Flags: review?(jag)
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?
(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 :-<
Severity: normal → trivial
Keywords: helpwanted
Priority: P2 → --
Target Milestone: Future → ---
Comment on attachment 146143 [details] [diff] [review]
(Av1) <nsTemplateMatchSet.cpp>
[Superseded by bug 285631]

(I believe you just volonteered ;-))
Attachment #146143 - Flags: superreview?(dbaron)
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.
Attachment #146143 - Flags: superreview?(dbaron) → superreview-
(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.
No longer depends on: 132141
(In reply to comment #15)
> It doesn't because |last| is always the immediate
> predecessor of |entry|

Exactly: we agree anyway.
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...
Attachment #146143 - Flags: superreview-
Depends on: 240562
No longer depends on: 240562
QA Contact: nobody → layout.misc-code
Comment on attachment 146143 [details] [diff] [review]
(Av1) <nsTemplateMatchSet.cpp>
[Superseded by bug 285631]

This file was removed by bug 285631.
Attachment #146143 - Attachment is obsolete: true
Attachment #146143 - Flags: review?(jag-mozilla)
Attachment #146143 - Attachment description: (Av1) <nsTemplateMatchSet.cpp> → (Av1) <nsTemplateMatchSet.cpp> [Superseded by bug 285631]
Assignee: sgautherie.bz → nobody
Status: ASSIGNED → NEW
Whiteboard: [good first bug]
There is no warning at all in the layout/ code (using gcc4.6.1).
Hence closing this bug.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.