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

RESOLVED FIXED in mozilla9



8 years ago
6 years ago


(Reporter: dholbert, Assigned: atulagrwl)


(Blocks: 1 bug)


Firefox Tracking Flags

(Not tracked)


(Whiteboard: [build_warning])


(1 attachment, 1 obsolete attachment)

1.53 KB, patch
: review+
Details | Diff | Splinter Review


8 years ago
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

8 years ago
(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)
Blocks: 187528

Comment 2

8 years ago
Created attachment 558151 [details] [diff] [review]
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).
Assignee: nobody → atulagrwl


8 years ago
Attachment #558151 - Flags: review?(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()?
Attachment #558151 - Flags: review?(bzbarsky) → review-

Comment 4

8 years ago
Created attachment 562323 [details] [diff] [review]
Patch v2

Improvements suggested by reviewer.
Attachment #558151 - Attachment is obsolete: true
Attachment #562323 - Flags: review?(bzbarsky)
Comment on attachment 562323 [details] [diff] [review]
Patch v2

Attachment #562323 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Target Milestone: --- → mozilla9

Comment 7

8 years ago
Last Resolved: 8 years ago
Resolution: --- → FIXED


6 years ago
Duplicate of this bug: 563226
You need to log in before you can comment on or make changes to this bug.