89964 - crash on form of visual hebrew with dir=rtl

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Tzafrir Cohen]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.2) Gecko/20010628
BuildID:    2001062815

That page is encoded as ISO-8859-8 . It includes a form with a textarea element
that has dir=rtl and wrap=virtual . 

Generally working with this form I got some unexpected behaviours (including
strange behaviours of the selection) and some crashes.
Procedure below produces a reporducable crash (at least in this machine)

I seem to have had this problem from at least 0.9.1, but couldn't isolate it before.

Reproducible: Always
Steps to Reproduce:
1. start a new mozilla with that page
2. type a hebrwe word of at least two letters in that form (this requires hebrew
keyboard layout installed)
3. press enter
4. press backspace twice

Comment 1

[Simon Montagu :smontagu]

Attachment: suggested patch

Comment 2

[Simon Montagu :smontagu]

Commentary on the attached patch:

The IBMBIDI block in |nsCaret::SetupDrawingFrameAndOffset| needs to be moved
before the code that sets NS_FRAME_EXTERNAL_REFERENCE on the caret frame.

The crash was caused by the bidi code changing |theFrame| to a frame without
NS_FRAME_EXTERNAL_REFERENCE, so when that frame was deleted by backspacing
|nsCaret::ClearFrameRefs| wasn't being called and |mLastCaretFrame| was still
pointing to the deleted frame.

Comment 3

[Simon Montagu :smontagu]

Attachment: possibly a better patch

Comment 4

[Simon Montagu :smontagu]

I guess it makes sense to move the IBMBIDI block even earlier, as in the second
patch. What would be a good testcase?

Comment 5

[kinmoz]

sr=[e-mail]

Comment 6

[Keyser Sose]

Marking NEW.

Comment 7

[Blake Ross]

mkaply, do you think this is something that should go on the 6.1 branch?

Comment 8

[Mike Kaply [:mkaply]]

I think this would be a good candidate for the branch, although it is not even 
on the trunk right now.

Comment 9

[Simon Montagu :smontagu]

I will check this into the trunk as soon as I get r=

Comment 10

[Frank Tang]

r=ftang, you already got sr=kin . Check into trunk ASAP, please.

Comment 11

[selmer (gone)]

How common is the case that causes this crash?  Since it's not a simple
null-check, I'd like you to describe the testing that's been done and to have an
extra reviewer looking for possible interactions outside the immediate scope of
the change.  I can't afford any regressions right now, so I need to be able to
judge both the value and the risk before making the decision whether this goes
in the branch.  

Frank, can you be the local representative for this bug?  Please bring an update
to the PDT meeting today.

Comment 12

[Simon Montagu :smontagu]

fix checked into trunk

Comment 13

[selmer (gone)]

PDT+.  Pls check into the branch as soon as possible.  Thanks.

Comment 14

[Blake Ross]

I checked this into the branch.

Comment 15

[lchiang]

[e-mail] or gila, can you verify this bug?

Comment 16

[Simon Montagu :smontagu]

VERIFIED FIXED

Comment 17

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).