307809 - Crash involving nested elements with style="display: -moz-grid-group;" [@ nsGridRowLayout::GetGrid]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050908
Firefox/1.6a1

Filing as security-sensitive because the testcase includes code from bug 306939
and I didn't manage to make a simplified testcase.

Comment 1

[Jesse Ruderman]

Attachment: testcase (not reduced)

Crashes while the status bar counter says 1411.

Comment 2

[Jesse Ruderman]

Attachment: apple crash report with stack trace

Comment 3

[Jesse Ruderman]

Attachment: reduced testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

The nsGrid::GetScrolledBox will happily return non-boxes if it's passed one,
but if it gets a scrollframe for a non-box it'll return null.  So we need to
guard against it here.	Or change what nsGrid::GetScrolledBox does.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 6

[Daniel Veditz [:dveditz]]

Any reason not to take this safe fix on the 1.8/1.8.0 branches?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Main reason is that I have no idea what this code really does and what the implications of this change really are...

Comment 8

[Mike Schroepfer]

DBaron - any thoughts on risk for 1.8 branch?

Comment 9

[Daniel Veditz [:dveditz]]

Minusing for 1.8.0.7 based on Boris's discomfort (might be trading a safe null-deref for a worse crash elsewhere). We'll do more testing on the trunk and see if this can go into 1.8.0.8 safely.

Comment 10

[Mike Schroepfer]

DBaron/Bz any thoughts for 1.8.1?  If we are going to take this for 1.5.0.8 we'll need it for 1.8.1 as well.. 

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Boris:  I'm pretty happy with this patch.  The code being changed is determining if the child's layout manager is an nsIGridPart.  If it's not a box at all, then it's certainly doesn't have an nsIGridPart layout manager (nsFrame's box code is nothing like an nsIGridPart).  So it seems quite safe to me.

Are you still concerned with the patch?

Comment 12

[Boris Zbarsky [:bzbarsky]]

No, as long as someone understands why this is generally the right thing to do, I'm happy.

Comment 13

[Mike Schroepfer]

Comment on attachment 198651 [details] [diff] [review]
Proposed fix

a=schrep for 181drivers.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to MOZILLA_1_8_BRANCH.

Comment 15

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 198651 [details] [diff] [review]
Proposed fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.0 branch for 1.8.0.8.

Comment 18

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.8pre) Gecko/20061020 Firefox/1.5.0.8pre, no crash with reduced testcase.