Jesse Ruderman Reporter
	Description • 17 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 17 years ago

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.

	neil@parkwaycc.co.uk Assignee
	Comment 2 • 17 years ago

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) {
}

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 17 years ago

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

Looks good.

	neil@parkwaycc.co.uk Assignee
	Comment 4 • 17 years ago

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

Crash fix.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 5 • 17 years ago

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

a=beltzner for 1.9

	neil@parkwaycc.co.uk Assignee
	Comment 6 • 17 years ago

Fix checked in.

	Jesse Ruderman Reporter
	Comment 7 • 17 years ago

Crashtest checked in.