95228 - When entring Hebrew text into a form, Hitting ENTER twice crashes Mozilla

Status: RESOLVED FIXED

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

Description

[Shoshannah Forbes]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    20010801

An easy way to crash Mozilla 0.93 :
* Go to any Hebrew web site
* Type Hebrew text into a form textarea
* Hit the ENTER key on the keyboard twice

The result:
Mozilla crashes without giving any error message (it just dissapears)

I am using Milestone 0.93 on Windows 2000 Server which has Hebrew installed.

Example URLs:
* http://www.ynet.co.il/home/1,7340,L-875-258,FF.html - a logical page.
Click on "add a message" to get the form
* http://www.fisheye.co.il/story.php3?id=514 - a visual page. Click on "add
reply" to get the form.



Reproducible: Always
Steps to Reproduce:
1. Go to http://www.ynet.co.il/home/1,7340,L-875-258,FF.html 
2. Click on the "add messge" button
3. Type Hebrew text into a form textarea
4. Hit the ENTER key on the keyboard twice


Actual Results:  Mozilla crashes without giving any error message (it just 
dissapears)

Expected Results:  Enter two CRLF into the form

This happens both on logical and visual pages, as seen from the URLs above

Comment 1

[Shoshannah Forbes]

Talkback Incident IDs for this bug:
TB34085776H
TB34082393Z

Comment 2

[Simon Montagu :smontagu]

I can't reproduce this on NT or 2000. Can someone with access to talkback data
attach them to the bug?

Comment 3

[Shoshannah Forbes]

Tried again to repro- Didn't work when the messge was Hebrew only, but when I 
wrote Hebrew and then Egnlish, hit Enter twice, I got a crash again.
Talkback ID is:
TB34095309E

I saved the info from that talkback as a file- I am trying to attach it here

Comment 4

[Shoshannah Forbes]

Attachment: last talkback info

Comment 5

[Kathleen :Brade]

Here is the stack from the most recent talkback ID above:

0x02ff55a1 
nsCaret::SetupDrawingFrameAndOffset [d:\builds\seamonkey\mozilla\layout\base\src\
nsCaret.cpp, line 570] 
nsCaret::DrawCaret [d:\builds\seamonkey\mozilla\layout\base\src\nsCaret.cpp, line 
898] 
nsCaret::StartBlinking [d:\builds\seamonkey\mozilla\layout\base\src\nsCaret.cpp, 
line 456] 
nsCaret::SetCaretVisible [d:\builds\seamonkey\mozilla\layout\base\src\
nsCaret.cpp, line 210] 
PresShell::SetCaretEnabled [d:\builds\seamonkey\mozilla\layout\html\base\src\
nsPresShell.cpp, line 2996] 
PresShellViewEventListener::RestoreCaretVisibility [d:\builds\seamonkey\mozilla\
layout\html\base\src\nsPresShell.cpp, line 6757] 
PresShellViewEventListener::DidRefreshRegion [d:\builds\seamonkey\mozilla\layout\
html\base\src\nsPresShell.cpp, line 6792] 
nsViewManager::Refresh [d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, 
line 938] 
nsViewManager::DispatchEvent [d:\builds\seamonkey\mozilla\view\src\
nsViewManager.cpp, line 1960] 
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 68] 
nsWindow::DispatchEvent [d:\builds\seamonkey\mozilla\widget\src\windows\
nsWindow.cpp, line 725] 
nsWindow::DispatchWindowEvent [d:\builds\seamonkey\mozilla\widget\src\windows\
nsWindow.cpp, line 747] 
nsWindow::OnPaint [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, 
line 4051] 
nsWindow::ProcessMessage [d:\builds\seamonkey\mozilla\widget\src\windows\
nsWindow.cpp, line 3038] 
nsWindow::WindowProc [d:\builds\seamonkey\mozilla\widget\src\windows\
nsWindow.cpp, line 990] 
USER32.DLL + 0x2e98 (0x77e12e98) 
USER32.DLL + 0x39a3 (0x77e139a3) 
USER32.DLL + 0x395f (0x77e1395f) 
ntdll.dll + 0x2032f (0x77fa032f) 
nsEditor::EndPlaceHolderTransaction [d:\builds\seamonkey\mozilla\editor\base\
nsEditor.cpp, line 715] 
nsPlaintextEditor::TypedText [d:\builds\seamonkey\mozilla\editor\base\
nsPlaintextEditor.cpp, line 550] 
nsPlaintextEditor::HandleKeyPress [d:\builds\seamonkey\mozilla\editor\base\
nsPlaintextEditor.cpp, line 524] 
nsTextEditorKeyListener::KeyPress [d:\builds\seamonkey\mozilla\editor\base\
nsEditorEventListeners.cpp, line 264] 
nsEventListenerManager::HandleEvent [d:\builds\seamonkey\mozilla\content\events\
src\nsEventListenerManager.cpp, line 1598] 
nsGenericElement::HandleDOMEvent [d:\builds\seamonkey\mozilla\content\base\src\
nsGenericElement.cpp, line 1675] 
nsHTMLTextAreaElement::HandleDOMEvent [d:\builds\seamonkey\mozilla\content\html\
content\src\nsHTMLTextAreaElement.cpp, line 588] 
PresShell::HandleEventInternal [d:\builds\seamonkey\mozilla\layout\html\base\src\
nsPresShell.cpp, line 5632] 
PresShell::HandleEvent [d:\builds\seamonkey\mozilla\layout\html\base\src\
nsPresShell.cpp, line 5559] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 377] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 350] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 350] 
nsViewManager::DispatchEvent [d:\builds\seamonkey\mozilla\view\src\
nsViewManager.cpp, line 2058] 
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 68] 
nsWindow::DispatchEvent [d:\builds\seamonkey\mozilla\widget\src\windows\
nsWindow.cpp, line 725] 
nsWindow::DispatchWindowEvent [d:\builds\seamonkey\mozilla\widget\src\windows\
nsWindow.cpp, line 742] 
nsWindow::DispatchKeyEvent [d:\builds\seamonkey\mozilla\widget\src\windows\
nsWindow.cpp, line 2391] 
nsWindow::OnChar [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, 
line 2516] 
nsWindow::ProcessMessage [d:\builds\seamonkey\mozilla\widget\src\windows\
nsWindow.cpp, line 3510] 
nsWindow::WindowProc [d:\builds\seamonkey\mozilla\widget\src\windows\
nsWindow.cpp, line 990] 
USER32.DLL + 0x2e98 (0x77e12e98) 
USER32.DLL + 0x30e0 (0x77e130e0) 
USER32.DLL + 0x5824 (0x77e15824) 
nsAppShellService::Run [d:\builds\seamonkey\mozilla\xpfe\appshell\src\
nsAppShellService.cpp, line 425] 
main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1297] 
main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1602] 
WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1620] 
WinMainCRTStartup() 
KERNEL32.DLL + 0x17d08 (0x77e97d08)

Comment 6

[Simon Montagu :smontagu]

I have managed to reproduce this occasionally, but not reliably enough to debug
it. Shoshannah, can you send me a sequence of keystrokes which infallibly causes
the crash?

Comment 7

[timeless]

My guess is theFrame is garbage at
569                    theFrame->GetOffsets(start, end);
does that seem likely?

Comment 8

[Simon Montagu :smontagu]

Could this be related to bug 96604 and/or bug 96841?

Comment 9

[Simon Montagu :smontagu]

Shoshanna, can you still reproduce this bug now that those other textarea
crashes have been fixed?

Comment 10

[Shoshannah Forbes]

i don't have access anymore to the same machine where I orginally filed the bug on.
On a diffent w2k machine (this time with no service pack while the original had
SP2), I got 1 freeze and 1 crash, but I don't have a clear repro scanrio.
Since that machine has no Internet connection, I am attaching here the deatiled
saved from the talkback.
Is there any better way to get crash info?

I'll be trying to create a more clear scanrio for the crash.

Comment 11

[Shoshannah Forbes]

Attachment: Saved talkback info from the last crash

Comment 12

[Shoshannah Forbes]

Arrghh- hate replaying to mayself.
Anyway, forgot to mention that this last test was with build 2001083103.
Sorry for the extra mail :-)

Comment 13

[Shoshannah Forbes]

This bug is starting to annoy  me...
Just go a crash on win98 (Hebrew enabled, 1st edition, Mozilla 2001083103).
I wrote two paragphs in a textarea at http://www.fisheye.co.il , when I
attempted to move to the next paragraph. Hitting Enter didn't seem to move the
crouser, so I hit Enter again a few times, and resoulted in a crash.
Talkback ID is TB3487230W
BTW, I didn't use English at all in that textarea.

On the other hand, I am unable to get a consistent crash :( I do not get any
crashes in english language forms.

/me going back to Fisheye to reconstroct the contents of the messege she lost :-(

Comment 14

[Shoshannah Forbes]

Opps... wrong talkback ID.
The Correct one is:
TB34880978G
Sorry about that...

Comment 15

[Katsuhiko Momoi]

Here's the trace stack from Shosh's recent crash:
(Mostly identical to the one above.)

Incident ID 34880978
Stack Signature nsSelection::GetFrameForNodeOffset d7ca2712
Bug ID
Trigger Time 2001-09-03 08:29:34
Email Address [e-mail]
User Comments Typing a form in Hebrew
Build ID 2001083109
Product ID MozillaTrunk
Platform ID Win32
Trigger Reason Access violation
Stack Trace
nsSelection::GetFrameForNodeOffset
[d:\builds\seamonkey\mozilla\content\base\src\nsSelection.cpp, line 2935]
nsCaret::SetupDrawingFrameAndOffset
[d:\builds\seamonkey\mozilla\layout\base\src\nsCaret.cpp, line 570]
nsCaret::DrawCaret [d:\builds\seamonkey\mozilla\layout\base\src\nsCaret.cpp,
line 898]
nsCaret::StartBlinking [d:\builds\seamonkey\mozilla\layout\base\src\nsCaret.cpp,
line 456]
nsCaret::SetCaretVisible
[d:\builds\seamonkey\mozilla\layout\base\src\nsCaret.cpp, line 210]
PresShell::SetCaretEnabled
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 3003]
PresShellViewEventListener::RestoreCaretVisibility
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6788]
PresShellViewEventListener::DidRefreshRegion
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6843]
nsViewManager::Refresh [d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp,
line 936]
nsViewManager::DispatchEvent
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1960]
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 68]
nsWindow::DispatchEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 732]
nsWindow::DispatchWindowEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 754]
nsWindow::OnPaint [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp,
line 4071]
nsWindow::ProcessMessage
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3054]
nsWindow::WindowProc
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 997]
KERNEL32.DLL + 0x363b (0xbff7363b)
KERNEL32.DLL + 0x242eb (0xbff942eb) 

Comment 16

[Shoshannah Forbes]

Not sure if this is related or not:
win98 Hebrew enabled, Mozilla 2001090408
http://www.haayal.co.il/reply.php3?id=681&rep=34140&LastView=2001-09-05%2017%3A06%3A00
(a logical page)
I wrote about 2-3 paragraphs in Heberew, until the textarea was verticly full.
Hit ENTER to move to a new paragraph (and create a vertical scroll bar at the
textarea)- nothing happened. Hit enter again, and got a crash, talkback ID#
TB34971995Q
(note to self- when writing long posts in Hebrew, write them first in notepad.
It is annoying to loose them in a crash...)

I haven't had any crashes in English forms.
I wonder if this is realted to bug 86262 or bug 93958?

Comment 17

[Keyser Sose]

marking NEW.

Comment 18

[kinmoz]

So anyone come up with exact steps to reproduce this? If so, I can probably help 
look into this.

FYI, I fixed a couple of cases where the editor delete code caused the 
textarea's caret to be placed in the browser content, which caused some strange 
crashes.

I mention this because I know that the BIDI code does some selection/caret 
manipulation ... perhaps that is something worth looking into?

Comment 19

[Alon Altman]

Another talkback ID: TB35497270Q

Comment 20

[Tzafrir Cohen]

See TB35901135E .

Note: I mentioned there "strange behaviour. I later learned that this is caused
by the fact that the font "Verdana" was used (platform: win98se/hebrew enabled
w/office97 fonts).

See, for instance, http://www.technion.ac.il/~tzafrir/form_hebrew_fonts.html .
Notice the placement of the '.' between the two hebrew words.

MSIE5 uses a different font there instead of Verdana.

This will probably have no effect on other platforms

Comment 21

[Tzafrir Cohen]

still happens with 20011005 win32 nightly build on win98se hebrew (enabled).

Currently it seems that every time I edit a long enough text in a textarea with
mixed hebrew/english text (either a UTF-8 or a ISO-8889-8-i page) I get a crash.
Can't think of a better way to reproduce

Comment 22

[Tzafrir Cohen]

I believe I have a reproducable sample:
http://www.technion.ac.il/~tzafrir/form_hebrew_fonts.html#problem1
(and follow the instructions from there)

causes mozilla to hang and take 100% CPU (but not to crash). Works both with
0.9.4 and with build 2001100503 (on win98/hebrew)

Comment 23

[Shoshannah Forbes]

I am getting this with Mac OS as well, build id  2002012903. Changing platform
and OS to "all"

Comment 24

[Shoshannah Forbes]

got 2 more crashes on mac 9.2.2.
Talkback IDs:
TB2767184g
tb2667064w

Comment 25

[Shoshannah Forbes]

is any work being done here? The combaination of this bug and the lack of
copy/paste in  the mac build (bugs  #119877 and #119899) make it virtually
impossible to participate in meesage boards using mozilla. Yet another TB crash:
 tb3328264g

Comment 26

[Simon Montagu :smontagu]

Nominating nsbeta1 per comment 25

Comment 27

[Frank Tang]

crash bug. nsbeta1+ reassign to ftang

Comment 28

[Frank Tang]

could someone give me an exact url which have hebrew text area. I don't read
Hebrew so I cannot tell which link are link to "add a message" or "add reply".
Thanks

Comment 29

[Alon Altman]

Attachment: Minimal testcase - hebrew form

I have attached a hebrew form to test this bug.
To recreate the bug you must type a combination of hebrew and english text that
would be wrapped around the text entry box and then try to type enter.

Comment 30

[Frank Tang]

crash, so this is a p1

Comment 31

[Frank Tang]

simon- I can reproduce with the attach test case with HANG now- not crash
Looks like some thing are confused with the the caret position. 
It looks like a bug in layout with bidi but not a keyboard bug . 

could you take a look at this ? I can help you to reproduce it if you cannot.

Comment 32

[Shoshannah Forbes]

build 2002031708/ mac os 9.2: I get less crashes for this, but I still get some :-(
Latest talkback ID:
TB4167091E

Comment 33

[Frank Tang]

ok, I can easily reproduce the hang in the following way
1. on WinXP, install hebrew keyboard
2. view the attached form
http://bugzilla.mozilla.org/attachment.cgi?id=73799&action=view
3. in hebrew keyboard, type 'a' 10 times (which produce 10 hebrew) and Shift 'a'
time times (which produce 10 english A), repeate this 3 times without pressing
space, the mozilla will wrap the line 
4. now move the cursor to the first line between two English "A" and press
return, it now hang

Comment 34

[Frank Tang]

Attachment: put cursor between 2 "A" in the frist line and hit return, it will hang

Comment 35

[Frank Tang]

Attachment: put cursor between 2 "A" in the frist line and hit return, it will hang

Comment 36

[Frank Tang]

hang and crash, adt1
smontagu, please mark as assign, if you agree to work on it.

Comment 37

[Frank Tang]

smontagu- any estimation when can we fix this one ?
please put the date on the status whiteboard

Comment 38

[Jaime Rodriguez, Jr.]

Changing impact to ADT2, because Hebrew is not a Tier 1 language for the MachV
release.

Comment 39

[Frank Tang]

 Impact Platform: ALL
Impact language users: Arabic and Hebrew . total 6.3 M 1.125% of total internet
users
Probability of hitting the problem: HIGH, editing any text area in html form may
hit this problem.
Severity if hit the problem in the worst case: hang or crash
Way of recover after hit the problem: kill the app or reboot the machine
Risk of the fix: unknown
Potential benefit of fix this problem: unknown

Comment 40

[Simon Montagu :smontagu]

There are clearly two different scenarios here.

Bug 136552 has a detailed reproduction procedure which leads to a crash very
similar to the stack traces in comment 5 and comment 15 (compare attachment 78585 [details])

Following the procedure in comment 33 leads to a loop with the same assertions
as in http://bugzilla.mozilla.org/show_bug.cgi?id=129847#c2

Comment 41

[Simon Montagu :smontagu]

Attachment: Patch for the crash

Comment 42

[Simon Montagu :smontagu]

Lina, as the original author I think you are the best person to give r=. Can you
look at attachment 78771 [details] [diff] [review] and tell me if you think it is the correct fix, or
wallpaper over another problem?

Comment 43

[Lina Kemmel]

r=[e-mail]

However, there is one point that seems strange to me.
What happened before Simon's patch was deletion of a non-text frame (BRFrame in 
this particular case) -- unnecessary and maybe harmful thing, but not the one 
that should cause a crash. The crash happens because after the frame removing 
the corresponding content node still remains associated with the deleted frame, 
and 
    mTracker->GetPrimaryFrameFor(theNode, aReturnFrame)
in nsSelection::GetFrameForNodeOffset, returns the frame deleted..

Did I miss anything? Is this behaviour intentional?

Comment 44

[Simon Montagu :smontagu]

What I saw when debugging was that the problem was caused when an |nsTextFrame|
was deleted. Would it perhaps be better to test for the frame type and only
delete |nsContinuingTextFrame|s? I am assuming that an |nsContinuingTextFrame|
is never mapped to content in the frame manager.

Comment 45

[Oded Arbel]

latest nightly don't crash on me, or atleast - after testing today several pages
where I always had crashes, I'm willing to submit "works for me". the hang
problem decribed in Tzafrir's page, however, is still there (
http://www.technion.ac.il/~tzafrir/form_hebrew_fonts.html ).

Comment 46

[Oded Arbel]

Attachment: Talkback info of latest crash

Comment 47

[Oded Arbel]

Retraction: and then it crashes. sorry if I bothered anyone. I attached the
crash info. (http://bugzilla.mozilla.org/attachment.cgi?id=79417&action=view)

Comment 48

[Frank Tang]

are we still crashing ?
IF NO, do we still crash on branch build?
IF Yes, smontagu: should you ask sr= ?

Comment 49

[Simon Montagu :smontagu]

The crash continues in both trunk and branch, using the procedure described in
bug 136552. I have requested sr= from waterson.

Comment 50

[Chris Waterson]

Comment on attachment 78771 [details] [diff] [review]
Patch for the crash

sr=waterson (but avoid double-negative in comment; `only delete bidi frames')

Comment 51

[Simon Montagu :smontagu]

Attachment: Removed double negative per comment 50

Comment 52

[Simon Montagu :smontagu]

Comment on attachment 80436 [details] [diff] [review]
Removed double negative per comment 50

Transferring r/sr status

Comment 53

[Simon Montagu :smontagu]

Attachment 80436 [details] [diff] checked in on trunk. This fixes the crashing case that I was
able to reproduce (from bug 136552), but leaves the hanging case open. Unless I
obtain some evidence to the contrary, I am assuming that the hanging case is a
dupe of bug 129847 and working on it there. The trunk/branch checkin process is
complicated enough without having more than one issue per bug :-)

Comment 54

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 80436 [details] [diff] [review]
Removed double negative per comment 50

a=shaver for 1.0 branch.

Comment 55

[scottputterman]

adding adt1.0.0-. Let's get this in for RTM.

Comment 56

[Alon Altman]

Still happened in 2002042221. Not 100% reporoducible, but still did happen. It
was a hang not a crash, with multi-line Hebrew+English text.

Shall I reopen?

Comment 57

[Simon Montagu :smontagu]

See comment 53. Please only reopen this bug for a crash, not a hang.

Comment 58

[Simon Montagu :smontagu]

Is there anyone who can confirm or deny that the crash no longer happens? Please
specify build ID and platform tested on.

Comment 59

[Matt Printz]

I still hang on both win98 and win2k build ID 2002042510. Hang was achieved
using testcase submitted on 3/22/02.

Comment 60

[Shoshannah Forbes]

build id 2002041508 on mac 9.2.2 : this time I got a hang. at :
http://www.tapuz.co.il/tapuzforum/main/addmsg.asp?id=156&msgid=3877240 I wrote
some hebrew text, hit enter , hit the "add code" link (which addes some english
code), and then when looking for the insertion point to move to the new line (i
 hit a few times enter), i got a hang. 

Comment 61

[Simon Montagu :smontagu]

Read my lips: this bug is no longer about hanging. Please discuss hanging in bug
129847.

If anyone still *crashes* with the test case described in bug 136552 or a
similar scenario, please comment in this bug. If anyone can't reproduce that
crash with a build since 2002-04-23, then please comment _loudly_ in this bug.
Thank you.

Comment 62

[Ilya Konstantinov]

Can no longer bring it to crash on Linux Mozilla 1.0RC1.

Comment 63

[Simon Montagu :smontagu]

Sorry, I should have specified "a trunk build since 2002-04-23". The fix is not
in the branch, and certainly not in RC1

Comment 64

[Shoshannah Forbes]

RC1 on win98:
In a text area, I wrote an English word, and then about 3 Hebrew lines. Then I 
went back to the English word, and attempted to select it and type a new word 
on it- got a freeze and had to quit Mozilla from the task manager.

Comment 65

[Frank Tang]

If we want to land this crash case into the branch, we probably need to solve
the case of hang first. Otherwise, there are no way can verify we really fix the
case of crash because it is possible tester hang before they can hit the case
crash. Also, it is better to have crash than hang since we will get talkback
report about crash but no talkback report about hang. hang is worst than crash
since we won't know how many poeple hit that issue. 

smontagu- please continue to work on the case of hang. I don't think we can
really verify THIS bug untill we fix that one. 

Comment 66

[Simon Montagu :smontagu]

OK, the hang happens (at least in the test case in attachment 75635 [details]) because
after adding the line break we are not going through Bidi resolution again when
reflowing.

Comment 67

[Simon Montagu :smontagu]

Attachment: Patch for the hang

Comment 68

[Randell Jesup [:jesup] (needinfo me)]

Adding to RC2 Not Suck bug.  We really want this in 1.0 or mozilla1.0 will be
hard/impossible to use for BiDi locales

Comment 69

[Simon Montagu :smontagu]

Attachment: Better patch for the hang

Comment 70

[Simon Montagu :smontagu]

Compare
http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsBoxToBlockAdaptor.cpp#1021
Our problem was exactly what is described there: "blocks sometimes send resizes
even when its children are dirty!" Adding the check of NS_FRAME_IS_DIRTY and
NS_FRAME_HAS_DIRTY_CHILDREN will make us correctly do Bidi resolution in these
cases and prevent the hang.

Comment 71

[John Keiser (jkeiser)]

Comment on attachment 81953 [details] [diff] [review]
Better patch for the hang

r=jkeiser
Perhaps a short comment is in order as to what is going on here.  Like "// If
text content has changed, the BIDI direction of some text may have changed"

Comment 72

[Simon Montagu :smontagu]

Attachment: Attachment 81953 plus comment

Comment 73

[Simon Montagu :smontagu]

I should add that I retract what I said in comment 53: the hang here is not the
same as the hang in bug 129847.

Comment 74

[Asa Dotzler [:asa]]

waterson, attinasi, can one of you give this BiDi topcrash fix a super-review?
We're short on time and really need to get this into RC2 (landed before tomorrow).

Comment 75

[Marc Attinasi]

Comment on attachment 81963 [details] [diff] [review]
Attachment 81953 [details] [diff] plus comment

sr=attinasi, though I would prefer if the comment came _after_ the new line of
code, or before the whole test, slightly reworded to make sense in a slightly
different context.

Comment 76

[Simon Montagu :smontagu]

Attachment: Modified comment position and wording per comment 75

Comment 77

[Simon Montagu :smontagu]

Renominating for adt1.0.0, since there is now a fix for both the trunk and the
crash case, and drivers want this for rc2.

Comment 78

[Simon Montagu :smontagu]

Fix checked into trunk. In my previous comment, read "there is now a fix for
both the crash and the hang case"

Comment 79

[Asa Dotzler [:asa]]

Comment on attachment 82088 [details] [diff] [review]
Modified comment position and wording per comment 75

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 80

[Frank Tang]

the crash and hang combination in the text area make Mozilla 100% unusable with
arabic and hebrew users . 
we need to following test under the current shor-of-reaction time:
1. all the smoke test to make sure this patch won't cause any layout regression 
2. Look at the trunk tinderbox about the page load. make sure it does not crash
/ hang wiht the page load, neither cause any slow down
3. run the intl page load which they don't run on the trunk tinderbox. make sure
no crash / hang or different layout. 
4. test text area and composer editing with bidi text. 

Comment 81

[Frank Tang]

this is a easy to crash and hang bug with the user group. 

waterson and attinasi : can you measure the risk for this two patches? how risky
it is ? 

Comment 82

[Simon Montagu :smontagu]

1. Smoketests pass
2. Tinderboxen had no problems running the pageload tests. I don't see any
regressions in the graphs
4. I have been writing in Hebrew with the patch all afternoon without
encountering hangs or crashes.

Looking at intl pageload tests now

Comment 83

[Simon Montagu :smontagu]

intl pageload tests look OK too.

Comment 84

[Randell Jesup [:jesup] (needinfo me)]

I'm not waterson or attinasi, but I've spent a lot of hours in this code recently.

Risk on this patch is quite low, and only on pages with BIDI text or textareas.
 The textarea code is  in a BIDI-only routine, and the blockframe code is the
"if ()" leading to a block of BIDI-only code.  If the blockframe is not
BIDIenabled, or if BIDI is not compiled in, it has no effect.  Worst-case
problem is slight perf hit for over-dirtying lines even if things were totally
wrong (and it would still have to be bidienabled to happen).

Low risk, get it in.

Comment 85

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin to the 1.0 branch. Pls check
this one in tonight, then add the fixed1.0.0 keyword.

Comment 86

[Simon Montagu :smontagu]

Checked both patches into branch

Comment 87

[Frank Tang]

I want more verification be done on the trunk and branch today . simon, can you
send mail out and ask help?

Comment 88

[Lina Kemmel]

There are a few problems with the attachment 80436 [details] [diff] [review]:

1) A frame marked as a candidate for removal, but actually not deleted,
does not pass Bidi resolution.
(Easy to fix, I guess).

2) Moreover, if the unhandled frame is an |nsContinuingTextFrame|, content 
dupes may occur.
Can we always remove |nsContinuingTextFrame|s, even though are not Bidi?