379418 - "ASSERTION: internal error: '!aListName'" with MathML and Hebrew text

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: internal error: '!aListName', file /Users/jruderman/trunk/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.h, line 373

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

aListName is nsGkAtoms::nextBidi

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

I think we should pass bidi frames through without asserting.
I've also added an assertion to SetInitialChildList(), in this method
I don't think the list can ever be nsGkAtoms::nextBidi though.
Also, I don't think we should ReLayoutChildren() if it's a bidi frame,
the base classes (nsBlockFrame/nsInlineFrame) doesn't do that:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.829&root=/cvsroot&mark=4626#4589

Comment 3

[Boris Zbarsky [:bzbarsky]]

I wouldn't bother with the ifdef.  We don't support building without bidi; the ifdef is just there because it marks code that should really get reviewed.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Ok, thanks, I didn't know that.

BTW, shouldn't all these virtual methods be moved to the .cpp file?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 263424 [details] [diff] [review]
Patch rev. 2

rbs should really review this (esp. the relayout changes)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 263424 [details] [diff] [review]
Patch rev. 2

Looks ok.

Comment 7

[Boris Zbarsky [:bzbarsky]]

karlt, do you think you're up to reviewing this in lieu of rbs?

Comment 8

[Karl Tomlinson (:karlt)]

bz, patch looks reasonable from what i can tell, but i'm only just starting to work out which way is North in these frames, and i haven't looked at bidi.

Deferring to roc seems a better idea now, or, if he doesn't get to it, hopefully i'll know which way is South in a few weeks time.

Comment 9

[Boris Zbarsky [:bzbarsky]]

roc, you want to take a stab at the review?

Comment 10

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

Comment on attachment 263424 [details] [diff] [review]
Patch rev. 2

let's do this. For the record, though, I wouldn't have bothered with this at this stage of the game just to fix an assertion.

Comment 11

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

BTW Simon, what is this "nextBidi" child list actually used for?

Comment 12

[Simon Montagu :smontagu]

nextBidi is an ugly hack. See bug 365132.

Comment 13

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

Comment on attachment 263424 [details] [diff] [review]
Patch rev. 2

rerequest approval when tree gets out of red-state

Comment 14

[Reed Loden [:reed]]

Attachment: unbitrotten patch

Comment 15

[Reed Loden [:reed]]

Checking in layout/mathml/base/src/nsMathMLContainerFrame.h;
/cvsroot/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.h,v  <--  nsMathMLContainerFrame.h
new revision: 1.76; previous revision: 1.75
done

Comment 16

[Jesse Ruderman]

Crashtest checked in.