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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Layout
--
critical
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 1 bug, {crash, fixed1.8.1, verified1.8.0.8})

Trunk
mozilla1.9alpha1
PowerPC
Mac OS X
crash, fixed1.8.1, verified1.8.0.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 -
blocking1.8.0.8 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse null-deref], crash signature)

Attachments

(3 attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 195480 [details]
testcase (not reduced)

Crashes while the status bar counter says 1411.
(Reporter)

Comment 2

12 years ago
Created attachment 195481 [details]
apple crash report with stack trace
(Reporter)

Comment 3

12 years ago
Created attachment 198548 [details]
reduced testcase
(Reporter)

Updated

12 years ago
Summary: RandomStyles crash [@ nsGridRowLayout::GetGrid] → Crash involving nested elements with style="display: -moz-grid-group;" [@ nsGridRowLayout::GetGrid]
(Reporter)

Updated

12 years ago
Blocks: 306940
Created attachment 198651 [details] [diff] [review]
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.
Attachment #198651 - Flags: superreview?(dbaron)
Attachment #198651 - Flags: review?(bryner)
(Reporter)

Updated

12 years ago
No longer blocks: 306940
Blocks: 314502
Attachment #198651 - Flags: superreview?(dbaron)
Attachment #198651 - Flags: superreview+
Attachment #198651 - Flags: review?(bryner)
Attachment #198651 - Flags: review+
Assignee: nobody → bzbarsky
Target Milestone: --- → mozilla1.9alpha
Fixed.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: testcase+
Any reason not to take this safe fix on the 1.8/1.8.0 branches?
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Whiteboard: [sg:nse null-deref] reveals 306939 testcase

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Main reason is that I have no idea what this code really does and what the implications of this change really are...
Flags: blocking1.8.0.6? → blocking1.8.0.6+

Comment 8

11 years ago
DBaron - any thoughts on risk for 1.8 branch?
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.
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+

Comment 10

11 years ago
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.. 
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?
No, as long as someone understands why this is generally the right thing to do, I'm happy.
Attachment #198651 - Flags: approval1.8.1?

Comment 13

11 years ago
Comment on attachment 198651 [details] [diff] [review]
Proposed fix

a=schrep for 181drivers.
Attachment #198651 - Flags: approval1.8.1? → approval1.8.1+
Checked in to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
Restoring lost blocking flag
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9? → blocking1.8.0.8+
Comment on attachment 198651 [details] [diff] [review]
Proposed fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #198651 - Flags: approval1.8.0.8+
Fixed on 1.8.0 branch for 1.8.0.8.
Keywords: fixed1.8.0.8

Comment 18

11 years ago
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.
Keywords: fixed1.8.0.8 → verified1.8.0.8
Group: security
Whiteboard: [sg:nse null-deref] reveals 306939 testcase → [sg:nse null-deref]

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?
Crash Signature: [@ nsGridRowLayout::GetGrid]
You need to log in before you can comment on or make changes to this bug.