653295 - nsFrameSetFrame.cpp:1391 warning: comparison between signed and unsigned integer expressions (or: Why are numCells/mNumRows/mNumCols signed?)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Filing bug on this build warning:
> nsFrameSetFrame.cpp:1391:27: warning: comparison between signed and unsigned integer expressions
for this code:

> 1351 void
> 1352 nsHTMLFramesetFrame::RecalculateBorderResize()
> 1353 {
[...]
> 1359   PRInt32 numCells = mNumRows * mNumCols; // max number of cells
[...]
> 1362   nsAutoArrayPtr<PRInt32> childTypes(new PRInt32[numCells]);
[...]
> 1366   PRUint32 childIndex, childTypeIndex = 0;
[...]
> 1387   for (; childTypeIndex < numCells; ++childTypeIndex) {

I think we can safely assume numCells is non-negative.  If it were negative, then "new PRInt32[numCells])" will convert it to a huge positive number and probably run out of memory.

So the most straightforward fix for this warning is probably to make numCells a PRUint32.  The only reason it's signed right now is presumably because mNumRows and mNumCols are signed... but I'm not sure why we'd want them to be signed, either.  If we somehow end up with a negative number of rows/cols, we're in trouble due to things like the "new" expression above overflowing.

Comment 1

[Daniel Holbert [:dholbert]]

(Based on nsHTMLFrameSetElement::ParseRowCol() which actually initialize the values of mNumRows / mNumCols, there's no way for them to end up with a negative value, at first glance...  So IMHO, we might want to make them unsigned too, and make ParseRowCol() etc. take an unsigned outparam)

Comment 2

[Atul Aggarwal]

Attachment: Patch v1

It is much easier to change the Unsigned to signed to avoid this warning. Change the signed to unsigned will require to change a lot of instances of variable declaration in the file. Moreover there is no danger of variable overflow as stated by the bug reporter as well. However I agree that making mNumRows and mNumCols is more appropriate solution but current patch might be best solution in this case (in order to make several changes in the file).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 558151 [details] [diff] [review]
Patch v1

As long as you're touching this code, could you move it away from GetChildCount()/GetChildAt() to using GetFirstChild()/GetNextSibling()?

Comment 4

[Atul Aggarwal]

Attachment: Patch v2

Improvements suggested by reviewer.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 562323 [details] [diff] [review]
Patch v2

r=me

Comment 6

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d983fc8a13c

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/7d983fc8a13c