[FIX]"ASSERTION: Non-row-group primary frame list child of an nsTableFrame?" with <xul:menubar>

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
Layout: Tables
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla1.9alpha8
x86
Mac OS X
assertion, fixed1.8.0.13, fixed1.8.1.5, testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
Created attachment 269809 [details]
testcase

###!!! ASSERTION: Non-row-group primary frame list child of an nsTableFrame?  How come?: 'Not Reached', file nsTableFrame.cpp, line 2723

This assertion was added in bug 367706, "OrderRowGroups should be typesafe".

Comment 1

10 years ago
probably due to http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1358&mark=3364-3367#3350

Comment 2

10 years ago
Boris this is from your review comment https://bugzilla.mozilla.org/show_bug.cgi?id=311661#c16

Do you still think we need this?
Er... on non-OSX, menubar is special.  On OSX, sometimes it creates no frame... and the rest of the time it's special.  So the code is definitely wrong as-is.  It's probably fine to always treat it as special.  The other option is for the code here should do the same checks as the XUL frame construction code does, for OSX.
Flags: blocking1.9?
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?

Comment 4

10 years ago
Boris, should we fix this or must we fix it? We are trying to triage this bug as blocking/non-blocking.

Comment 5

10 years ago
From my experience I would say every violation of IsSpecialContent leads to a crash, this assumes people like Jesse who are really determined to find them. The crashes might not be exploitable but they are crashes.

What bernd said.  This can definitely be used to cause a crash, and probably an exploitable one (esp. on trunk).
Sounds like we'd like a branch fix, but given the lack of trunk progress (no assignee, for one) it's probably not realistic to actually "block" on it.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Created attachment 270352 [details] [diff] [review]
Proposed fix
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #270352 - Flags: superreview?(roc)
Attachment #270352 - Flags: review?
Attachment #270352 - Flags: review? → review?(bernd_mozilla)
Summary: "ASSERTION: Non-row-group primary frame list child of an nsTableFrame?" with <xul:menubar> → [FIX]"ASSERTION: Non-row-group primary frame list child of an nsTableFrame?" with <xul:menubar>
Target Milestone: --- → mozilla1.9beta1

Updated

10 years ago
Attachment #270352 - Flags: review?(bernd_mozilla) → review+
Comment on attachment 270352 [details] [diff] [review]
Proposed fix

Remove the comment
// keep this in sync  with IsSpecialContent
in ConstructXULFrame too.
Attachment #270352 - Flags: superreview?(roc) → superreview+
Created attachment 270760 [details] [diff] [review]
patch merged to comments
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Comment on attachment 270760 [details] [diff] [review]
patch merged to comments

I think we should get this in on branches too.  It's a simple and safe fix for a broken frame tree that could lead to nasty crashes.
Attachment #270760 - Flags: approval1.8.1.5?
Attachment #270760 - Flags: approval1.8.0.13?
Comment on attachment 270760 [details] [diff] [review]
patch merged to comments

approved for 1.8.1.5 and 1.8.0.13, a=dveditz
Attachment #270760 - Flags: approval1.8.1.5?
Attachment #270760 - Flags: approval1.8.1.5+
Attachment #270760 - Flags: approval1.8.0.13?
Attachment #270760 - Flags: approval1.8.0.13+
Checked in on both branches.
Keywords: fixed1.8.0.13, fixed1.8.1.5
Er, backed out of the 1.8 branch.  It's closed.  :(
Keywords: fixed1.8.1.5
Relanded.
Keywords: fixed1.8.1.5
Flags: blocking1.9?
Depends on: 368573
Flags: in-testsuite?
(Reporter)

Comment 17

10 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.