291702 - Crash if a XUL page contains flex summing up to zero - Integer divide by 0 [@ nsSprocketLayout::ComputeChildSizes ]

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Sergiu Dumitriu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

If a XUL page has two element, one with flex="n" and the other with flex="-n",
the browser crashes.

<window title="Negative flex bug"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  <button label="Button" flex="2"/>
  <label value="This is a label" flex="-2"/>
</window>

Reproducible: Always

Steps to Reproduce:
1. Write a simple XUL with two elements.
2. Set flex="1" for the first element.
3. Set flex="-1" for the second element.
4. Open the XUL file.

Actual Results:  
The browser crashes.

Expected Results:  
The browser doesn't crash.

If my view on the free space distribution algorithm isn't wrong, then a division
by 0 exception occurs, because the sum of all flex values is 0.

I didn't try it, but I think that any flex values that sum to a total of 0
causes this problem.

Comment 1

[Sergiu Dumitriu]

I've tested it with more elements, and I was right. If the flex attributes give
a total of 0, then there is a crash.

<window title="Negative flex bug #2"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  <button label="Button" flex="2"/>
  <label value="This is a label" flex="1"/>
  <label value="This is the second label" flex="-2"/>
  <label value="This is another label" flex="-1"/>
</window>

Comment 2

[Joel Nackman]

I could reproduce the problem on Gentoo Linux:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050413 Firefox/1.0.2

I'll confirm this bug.

Comment 3

[Joel Nackman]

Attachment: Testcase

Comment 4

[Martijn Wargers (dead)]

Also happens with current trunk build.

Comment 5

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050423
Talkback TB5323330E
As there are over 3100 reports waiting to be processed, I assume it will take
about 7 hours till this one gets ready:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB5323330E

Comment 6

[Hermann Schwab]

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB5323330E
Trigger Reason	Integer divide by zero

1256                           // ----- look at our min and max limits make sure
we aren't too small or too big -----

1257 evaughan   1.12           if (!computedBoxSizes->valid) {

1258 jaggernaut 1.26             PRInt32 newSize = pref +
sizeRemaining*flex/spacerConstantsRemaining;
//NSToCoordRound(float((sizeRemaining*flex)/spacerConstantsRemaining));




Source File, Line No.
c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,
line 1258
Stack Trace 	
nsSprocketLayout::ComputeChildSizes 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,
line 1258]
nsSprocketLayout::Layout 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,
line 279]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1104]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1104]
nsRootBoxFrame::Reflow 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsRootBoxFrame.cpp,
line 227]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsContainerFrame.cpp,
line 887]
ViewportFrame::Reflow 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsViewportFrame.cpp,
line 240]
PresShell::InitialReflow 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 2851]
nsXULDocument::StartLayout 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/document/src/nsXULDocument.cpp,
line 2157]
nsXULDocument::ResumeWalk 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/document/src/nsXULDocument.cpp,
line 3176]
nsXULDocument::EndLoad 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/document/src/nsXULDocument.cpp,
line 742]
XULContentSinkImpl::DidBuildModel 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/document/src/nsXULContentSink.cpp,
line 407]

Comment 7

[Hermann Schwab]

In this function the sum is made and used to divide, so crashing if it sums to zero.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsSprocketLayout.cpp&mark=1258&rev=#1162
1162               void
1163 evaughan 1.2  nsSprocketLayout::ComputeChildSizes(nsIBox* aBox,

1211 jaggernaut 1.26           spacerConstantsRemaining += boxSizes->flex;

1258 jaggernaut 1.26             PRInt32 newSize = pref +
sizeRemaining*flex/spacerConstantsRemaining;
//NSToCoordRound(float((sizeRemaining*flex)/spacerConstantsRemaining));

Comment 8

[Stefan [:stefanh]]

Hmm, the testcase seems to wfm with Seamonkey 2005042309 on Mac OS 10.3.9.

Comment 9

[Martijn Wargers (dead)]

Attachment: patch

This fixes the crash for me. Not sure if this is the correct way.

Comment 10

[[e-mail]]

Comment on attachment 219857 [details] [diff] [review]
patch

I guess it's a start, but what if you have four boxes with flex="1073741824"?

Comment 11

[Martijn Wargers (dead)]

Attachment: testcase2

(In reply to comment #10)
> (From update of attachment 219857 [details] [diff] [review] [edit])
> I guess it's a start, but what if you have four boxes with flex="1073741824"?
Hmm, yes, that still crashes with the patch.

Comment 12

[Martijn Wargers (dead)]

Attachment: patch2

This patch would also fix the crash in testcase2.

Comment 13

[[e-mail]]

Comment on attachment 220162 [details] [diff] [review]
patch2

I'm not going to argue about your choice of constants as you're going to need layout review anyway.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 220162 [details] [diff] [review]
patch2

So why the 16-bit limitation here?  And why to FFFE?

Comment 15

[Martijn Wargers (dead)]

Attachment: patch3

I should use nscoord_MAX, right?
This works right, Mozilla doesn't crash with it, although the testcase acts funny with the patch.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 220663 [details] [diff] [review]
patch3

I'm still not sure what the check against nscoord_MAX is doing....

sr=bzbarsky, but what we should _really_ do, imo is:

1)  Not parse negative flex values in CSS (use ParsePositiveVariant instead of ParseVariant).

2)  Not allow negative values from attributes.

3)  Deal with some of flexes overflow somehow (perhaps the nscoord_MAX check here does that?)

Comment 17

[Martijn Wargers (dead)]

Attachment: testcase3

Some extra tests, I've done. With patch3, Mozilla doesn't crash.

Comment 18

[Martijn Wargers (dead)]

Attachment: testcase3

Sorry, these tests make actually more sense.

Comment 19

[Martijn Wargers (dead)]

Checking in nsBox.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBox.cpp,v  <--  nsBox.cpp
new revision: 1.117; previous revision: 1.116
done

Checked into trunk, I'll file a follow-up bug on Boris' comments in comment 16.

Comment 20

[Martijn Wargers (dead)]

I've filed bug 345709 as a follow-up.

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

Crash is Verified FIXED using build 2006-07-27-09 of SeaMonkey trunk under Windows XP.

Comment 22

[Bob Clary [:bc] (inactive)]

layout/xul/base/src/crashtests/291702-1.xul
layout/xul/base/src/crashtests/291702-2.xul
layout/xul/base/src/crashtests/291702-3.xul
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3