166360 - Incorrect comparison in nsListBoxObject.cpp

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[tenthumbs]

In layout/xul/base/src/nsListBoxObject.cpp, gcc3.1.1 says:

nsListBoxObject.cpp: In function `void FindBodyContent(nsIContent*,
  nsIContent**)':
nsListBoxObject.cpp:201: warning: comparison of unsigned expression >= 0
  is always true

The code is:

    PRUint32 count;
    kids->GetLength(&count);
    // start from the end, cuz we're smart and we know the listboxbody 
is probably at the end
    for (PRUint32 i = count-1; i >= 0; --i) {     <<<===========
      nsCOMPtr<nsIDOMNode> childNode;
      kids->Item(i, getter_AddRefs(childNode));
      nsCOMPtr<nsIContent> childContent(do_QueryInterface(childNode));
      FindBodyContent(childContent, aResult);
      if (*aResult)
        break;
    }

so i>=0 is always true and the for loop has no termination test. You 
better hope you can always break out of the loop.

Comment 1

[tenthumbs]

Sorry about the spam but I have to correct a typo in the summary.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: signed! Signed!

jag?  hewitt? reviews?

Comment 3

[jag (Peter Annema)]

Comment on attachment 97747 [details] [diff] [review]
signed!  Signed!

Shouldn't you change

  kids->Item(i, ...)

to

  kids->Item(PRUint32(i), ...)

?

sr=jag if you fix it so you don't get a signed/unsigned warning there.

Comment 4

[Kevin McCluskey (gone)]

-> XPToolkit/Widgets

Comment 5

[John Morrison]

*** Bug 157155 has been marked as a duplicate of this bug. ***

Comment 6

[timeless]

.

Comment 7

[Boris Zbarsky [:bzbarsky]]

If I wanted this bug, I would have taken it.

Comment 8

[timeless]

mine

Comment 9

[timeless]

Attachment: change the logic

Comment 10

[Axel Hecht]

Comment on attachment 115319 [details] [diff] [review]
change the logic

r=[e-mail]

Comment 11

[tenthumbs]

Seems rather ugly to me. The original for loop expressed the functioning rather
clearly but the patch obscures it. Furthermore it is possible you'll confuse the
compiler into generating less optimal code because you're taking the address if
"i"  which is obviously just a temporary. What's wrong with 
  for (PRInt32 i = PRInt(count - 1); i >= 0; --i) ?

Comment 12

[tenthumbs]

Actually I think there's another problem. If kids->GetLength returns an unsigned
quantity then you should check that it's < INT_MAX before converting to a signed
quantity. Alternatively, run the loop from count-1 to 1 and test 0 separately.

Comment 13

[tenthumbs]

BTW, the simplest fix is
  for (PRUIint32 i = count - 1; i < count; --i)
which works for twos-complement arithmetic.

Comment 14

[tenthumbs]

Sorry about all the comments but let me be very clear. If GetLength 
operates on a PRUInt32 then the i<count method is the simplest solution. 
If GetLength operates on a PRInt32 (which I now think it does) then just 
s/PRUInt32/PRInt32/ will be better. Is there a spec on what GetLength is 
supposed to do, as opposed to reading the source?

Comment 15

[timeless]

Comment on attachment 115319 [details] [diff] [review]
change the logic

both interface methods GetLength/Item are unsigned
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/public/idl/core/nsIDOM
NodeList.idl&rev=1.4&mark=58-59#55>, all of the casts make me queezy.

Note that my change really isn't that big.

old flow:
i (count)	:=	kids->GetLength
loop:
if i < 1 goto done
i (count-1|--i) :=	i - 1
Item(i)
goto loop
done:

new flow:
i	:=	kids->GetLength
loop:
if i <= 0 goto done
i	:=	i - 1
Item(i)
goto loop
done:

Comment 16

[tenthumbs]

Since GetLength is unsigned I object strongly to using signed
quantities. A valid unsigned number can be equivalent a negative signed
one. The loop would never run in this case. You are implicitly asserting
a range constraint on count, namely that half the range of a uint is
disallowed. If this is the place to make such an assertion then you
should do it explicitly with suitable testing to catch errors.

I have no doubt that such a case would be a bug but I doubt greatly that 
this is the place to handle it.

Comment 17

[timeless]

eh? i'm using prUint32 ... 

Comment 18

[tenthumbs]

Yes, you're right. I looked at the patch a number of times and saw 
PRInt32. Sorry about that.

Comment 19

[timeless]

checked in