351393 - Crash [@ nsLineBox::IndexOf] with legend and fieldset ::first-letter and ::first-line

Status: VERIFIED WORKSFORME

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase which crashes on load with current trunk builds.

Talkback ID: TB22913034Z
nsLineBox::IndexOf  [mozilla\layout\generic\nslinebox.cpp, line 279]
nsBlockFrame::AddFrames  [mozilla\layout\generic\nsblockframe.cpp, line 5479]
nsBlockFrame::InsertFrames  [mozilla\layout\generic\nsblockframe.cpp, line 5438]
SplitInlineAncestors  [mozilla\layout\base\nsbidipresutils.cpp, line 139]
nsBidiPresUtils::Resolve  [mozilla\layout\base\nsbidipresutils.cpp, line 456]
nsBlockFrame::Reflow  [mozilla\layout\generic\nsblockframe.cpp, line 816]

This regressed between 2006-08-06 and 2006-08-07. I think a regression from bug 169139, somehow.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Uri Bernstein]

I reproduced this on Mac.
I'm pretty sure this would be fixed by fixing bug 351407.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 5

[Mats Palmgren (inactive)]

There are several issues here, the first problem is that
the second <legend> element is picked up as the legend for the
fieldset. The second is the fieldset specific test in IsValidSibling()
that was first added for bug 180805 in rev. 1.1153:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1153&root=/cvsroot&mark=8403-8410#8364
The test is basically the same today:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1260&root=/cvsroot&mark=8604-8611#8570

This doesn't look right to me. As far as I know, it's only the first
<legend> that is special in the sense that it's a child of the fieldset
frame and not the content area frame. Other <legend>s are valid siblings
of each other or any other content.

The third problem is needs to be illustrated with a bit of tracing for
the first testcase...

a. we restyle the legend and thus call RecreateFramesForContent
   which removes and reinserts the content node.
b. when reinserting the node we try to find a previous frame
   using FindPreviousSibling, which walks backwards from the old
   content index, calling IsValidSibling to test if this is a valid
   sibling frame. Because of problem 2 we find the first <legend>
   inside the ::first-line frame and now we adjust |parentFrame|
   to be the Line frame.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1260&root=/cvsroot&mark=9459#9430
c. then we enter the "haveFirstLetterStyle" block
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1260&root=/cvsroot&mark=9547,9584-9591#9546
   which causes us to recalculate |prevSibling| - this time the
   |parentFrame| isn't the Fieldset frame though so the special
   code in IsValidSibling isn't triggered, so we get the frame
   for the text content (0x8a37be8) preceeding the legend.
   Ironically, this is actually the correct previous frame, but
   |parentFrame| now have the wrong value.
   
I think it makes more sense to use the original parentFrame
when we call FindPreviousSibling() the second time, in the
"haveFirstLetterStyle" block.

The fourth problem is illustrated by Testcase #3:
1. click "insert legend"
2. click "display innerHTML of fieldset"
Note that the content order does not correspond to the frame order.
This last problem will be tricky to fix...

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This fixes the crash, the assertions, and the moving legends in Testcase #2.
It does not attempt to fix the first or fourth problem.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Frame dump (described in comment 5)

Comment 8

[Uri Bernstein]

Testcase #1 no longer crashes for me, starting from 2006-12-08 (that is, the reflow branch landing).

Testcase #2 also seems to be OK now, starting from some time between 2007-02-18 and 2007-02-24 (sorry, no time to narrow it right now).

WFM?

Comment 9

[Martijn Wargers (dead)]

Fine with me.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Testcase #2 started working between 2007-02-23-01 and 2007-02-24-01.  Most likely fixed by bug 370933?  If so, that's odd....  why did that help?

I'm not sure what to make of testcase #3.

Comment 11

[Martijn Wargers (dead)]

Mats, can this bug be marked worksforme?

Comment 12

[Martijn Wargers (dead)]

Mats, can this bug be marked worksforme?

Comment 13

[Martijn Wargers (dead)]

I'm going to mark this bug worksforme now.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified WFM for me too with Build identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090106 Shiretoko/3.1b3pre