382429 - Hang or crash when changing text dynamically with bidi and new textframe.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Simon Montagu :smontagu]

Attachment: Testcase

With new textframe, clicking on the button in the attached testcase causes a hang in nsSplittableFrame::IsInPrevContinuationChain(). Note that the change to the text from "ע 3/4" to "ע 3 ד 4" makes bidi resolution change the number of same-direction text runs in the textframes.

There are no assertions before the hang, but there is a message in the console:
WARNING: REASSIGNING MULTIFLOW TEXT RUN (not append)!: file /home/smontagu/mozwork/debugtree/mozilla/layout/generic/nsTextFrameThebes.cpp, line 1632

Comment 1

[Simon Montagu :smontagu]

Attachment: Top of stack at the hang

Comment 2

[Simon Montagu :smontagu]

Attachment: Thinner testcase

There seems to have been a change in the last few days. This version of the testcase originally caused lots of
###!!! ASSERTION: redo line on totally empty line: 'aState.IsImpactedByFloat()'
###!!! ASSERTION: unconstrained height on totally empty line: 
'NS_UNCONSTRAINEDSIZE != aState.mAvailSpaceRect.height'
followed by "yikes! spinning on a line over 1000 times!" and crash.
Today it hangs just like the previous version.

Comment 3

[Simon Montagu :smontagu]

Attachment: patch

We need to clear text runs when doing bidi resolution. I'm not sure if this is the ideal place to do it, though.

Comment 4

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

Comment on attachment 266733 [details] [diff] [review]
patch

I think this is a good place to do it. Maybe add a comment that this is called during bidi resolution, from the block container, so we shouldn't be holding a local reference to a textrun anywhere.

Comment 5

[Simon Montagu :smontagu]

Checked in.

Comment 6

[Simon Montagu :smontagu]

Attachment: mochitest

This is my first mochitest, so I'd like to have it reviewed

Comment 7

[Simon Montagu :smontagu]

Attachment: With the makefile change this time

Comment 8

[Simon Montagu :smontagu]

Ouch! I seem to have missed checking in the test, and it fails in current builds. To make matters worse, I wondered whether roc's change in bug 385270 might break this, but assumed it would be OK since I "knew" there was a unit test. (I'm not saying that it did break it, but it's a likely candidate)

Comment 9

[Simon Montagu :smontagu]

Attachment: Fix the hang again

The crash that I thought was caused by this patch happens even without it, so I'll file a separate bug for it.

Comment 10

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

This bug should be a blocker, so nominating as such, but I'll also approve your patch for checkin so you can check it in anytime you want.

Comment 11

[Simon Montagu :smontagu]

Checked in patch and test.