Closed Bug 406137 Opened 12 years ago Closed 12 years ago

Crash loading www.redhat.com [@ nsIFrame::GetStyleDisplay] [@ nsIFrame::GetPosition]

Categories

(Core :: Layout: Floats, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: ajschult784, Assigned: roc)

References

()

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(3 files)

Attached file testcase
With trunk CVS, I crash loading www.redhat.com.  The attached testcase crashes during load unless the image is in the cache (in which case shift-reload will still crash).  www.redhat.com crashes in nsIFrame::GetStyleDisplay and the testcase crashes in nsIFrame::GetPosition.  Both are crashing due to floatFrame being NULL in nsBlockReflowState::RecoverFloats.

A build from this morning (9am PST) doesn't crash and the crashing CVS build is from 10pm PST, so this is from bug 320378.
Attached file stacktrace
This is the stacktrace from the testcase.
Attached patch fixSplinter Review
The problem is that we can have dangling frame pointers in a line's float cache. If we skip reflowing the line due to willReflowAgain, then in the skip path we do RecoverStateFrom on the line, which walks the float cache, which crashes on the dangling pointer.

This patch fixes the issue by forcing reflow of any dirty lines with floats even if willReflowAgain is true. That reflow might give bogus positions, but that's OK because we always dirty such a line after reflowing it. We also have to be sure not to RecoverStateFrom on a skipped line if it's a block; it might have descendant lines with float caches containing dangling pointers. This is OK because such a RecoverStateFrom call never moves frames, it just adds floats to the space manager, and it doesn't matter if the space manager is wrong during this doomed reflow.

It's really nasty that the float cache can contain dangling pointers like this. The solution is to remove the float cache but I don't want to have to do that for 1.9.
Attachment #290857 - Flags: superreview?(dbaron)
Attachment #290857 - Flags: review?(dbaron)
David, this is a very nasty regression that will cause a lot of crashes on trunk. If you aren't able to review this today perhaps we should back out the original patch?
Comment on attachment 290857 [details] [diff] [review]
fix

r+sr=dbaron
Attachment #290857 - Flags: superreview?(dbaron)
Attachment #290857 - Flags: superreview+
Attachment #290857 - Flags: review?(dbaron)
Attachment #290857 - Flags: review+
Flags: blocking1.9? → blocking1.9+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Depends on: 406484
Crash Signature: [@ nsIFrame::GetStyleDisplay] [@ nsIFrame::GetPosition]
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae62f4c006c2
Crash Signature: [@ nsIFrame::GetStyleDisplay] [@ nsIFrame::GetPosition] → [@ nsIFrame::GetStyleDisplay] [@ nsIFrame::GetPosition]
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.