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

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: 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

[Bernd]

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

Comment 2

[Bernd]

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

Do you still think we need this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[juan becerra [:juanb]]

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

Comment 5

[Bernd]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

What bernd said.  This can definitely be used to cause a crash, and probably an exploitable one (esp. on trunk).

Comment 7

[Daniel Veditz [:dveditz]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Remove the comment
// keep this in sync  with IsSpecialContent
in ConstructXULFrame too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: patch merged to comments

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 270760 [details] [diff] [review]
patch merged to comments

approved for 1.8.1.5 and 1.8.0.13, a=dveditz

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in on both branches.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Er, backed out of the 1.8 branch.  It's closed.  :(

Comment 16

[Boris Zbarsky [:bzbarsky]]

Relanded.

Comment 17

[Jesse Ruderman]

Crashtest checked in.