408904 - Crash [@ nsSprocketLayout::PopulateBoxSizes] with <xul:grid>

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase crashes Firefox [@ nsSprocketLayout::PopulateBoxSizes].

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like a regression from bug 384874.  The code sets |last = nsnull|, then enters the loop.  aBoxSizes is not null, but all the things in that list are bogus, so we skip over all of them.  So currentBox ends up null.  So now we have |!currentBox && aBoxSizes|, so at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/xul/base/src/nsSprocketLayout.cpp&rev=1.62&mark=815,818,822#815 we end up with a guaranteed crash (since |last| is still null).

We need to either null-check |last| here or set |last| when skipping over bogus boxes or something.  Not sure what the right thing is, esp. since we reuse aBoxSizes later so it needs to have all the box sizes in it or something.

Comment 2

[[e-mail]]

Attachment: Proposed patch

Yeah, we always need to set last. See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/xul/base/src/nsSprocketLayout.cpp&rev=1.62&mark=896-897#895

I suppose the alternate fix is wrap the block (except those lines) inside
if (!currentBox || !currentBox->bogus) {
}

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 293821 [details] [diff] [review]
Proposed patch

Looks good.

Comment 4

[[e-mail]]

Comment on attachment 293821 [details] [diff] [review]
Proposed patch

Crash fix.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 293821 [details] [diff] [review]
Proposed patch

a=beltzner for 1.9

Comment 6

[[e-mail]]

Fix checked in.

Comment 7

[Jesse Ruderman]

Crashtest checked in.