87610 - Crash when viewing Arabic or hebrew content - Trunk [@ nsCaret::DrawCaret]

Status: RESOLVED FIXED

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

Description

[Olivier Cahagne]

This follows my previous bug #83694 (itself dup of #83448).

The page:
http://nocc.sf.net/demo/?lang=he (Hebrew) or http://nocc.sf.net/demo/?lang=ar
(Arabic)
 used to work with releases between the closing date of previous bug (June 10th)
and today (don't know when exactly).
Using build 20010624 on Win2k, the crash is more serious than #83694 as it
systematically crashes the browser.
I don't have time right now to reproducible smaller test-case but I will
investigate later.

Comment 1

[Olivier Cahagne]

Sorry, forgot the talkback ID:
TB32144761X

Comment 2

[Tuukka Tolvanen (sp3000)]

Attachment: a text input in a rtl body

Comment 3

[Tuukka Tolvanen (sp3000)]

Attached simplified version of the crashing page. Attachment crashes when text
input is focused. The page http://nocc.sf.net/demo/?lang=he has inline
javascript that gives focus to a text input, apparently causing the crash. No
crash without dir=rtl in body tag. Language makes no difference. (win98
2001-06-22-21-0.9.2)

Comment 4

[Diego Biurrun]

WFM, linux, build 2001062506

Comment 5

[Simon Montagu :smontagu]

Attachment: suggested patch

Comment 6

[Simon Montagu :smontagu]

This is caused by the fix for bug 84031. We were calling |sizeof| with a pointer
to a PRUint8 without dereferencing it, and GetBidiProperty was trashing the stack.

Comment 7

[Mike Kaply [:mkaply]]

In looking at:

http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsCaret.cpp#579

It seems that there are other places where we are passing the sizeof(PRUint8)

Comment 8

[Simon Montagu :smontagu]

mkaply, I don't understand what problem you have with passing the
sizeof(PRUint8). The stack crash is caused by passing the sizeof the address
instead of the sizeof the data.

Comment 9

[Mike Kaply [:mkaply]]

Sorry, the ** threw me off.

I get it now. aNextLevel is the pointer, *aNextLevel is the actual PRUint8.

r=mkaply

Comment 10

[kinmoz]

sr=[e-mail]

Please ask drivers if this can get into the trunk and the MOZILLA_0_9_2_BRANCH.

Simon, did you do a sweep over all GetBidiProperty() calls to make sure there 
weren't anymore places that needed to be changed?

Also why does GetBidiProperty() take a void** instead of a void*? It seems that 
most of the time you are passing in a pointer to data, not a pointer to a 
pointer?

Comment 11

[Jay Patel [:jay]]

Adding [@ nsCaret::DrawCaret] to summary and topcrash keyword.  This is showing
up on the Trunk topcrash report from Talkback.  Here is the latest data:

nsCaret::DrawCaret   21 
     First BBID :32144385
     Last BBID  :32165625
     Min Runtime :11
     Max Runtime :51468
     First Appearance Date : 2001-06-25
     Last Appearance Date : 2001-06-25
     First BuildID : 2001062206
     Last BuildID : 2001062509

Stack Trace: 

	 nsCaret::DrawCaret
[d:\builds\seamonkey\mozilla\layout\base\src\nsCaret.cpp  line 904]
 
 	Source File :
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/src/nsCaret.cpp
line : 904
     (32165625)	URL: http://nocc.sf.net/demo/?lang=he
(32165625)
Comments: Bugzilla Bug 87610  Crash when viewing Arabic or hebrew content
     (32161463)	URL: http://nocc.sf.net/demo/?lang=he
(32161463)
Comments: Loading and crashes when JS input
     (32151362)	URL: http://tinderbox.mozilla.org
(32147259)
Comments: arabic
     (32146241)	Comments: arabic
     (32146184)	Comments: hebrew
     (32145863)	Comments: hebrew test url
     (32145091)	URL: http://nocc.sf.net/demo/?lang=ar
(32144761)
URL: http://nocc.sf.net/demo/?lang=he
(32144385)
URL: http://nocc.sf.net/demo/?lang=ar
(32144385)
Comments: Just loading crashes the page every time

I'll post a stack trace when I find one in the Talkback data.  

I also think this might be a dup of bug 87813, but I'll leave it to QA to
confirm that.

Comment 12

[Mike Kaply [:mkaply]]

This change doesn't fix the crash for me.

When I double click the entry field, the browser closes with no crash info or 
anything. Still looking.

Comment 13

[Simon Montagu :smontagu]

Checked in to trunk, because Tinderbox says I don't need driver approval to do
that. Sent request to drivers to check into branch.

Comment 14

[Simon Montagu :smontagu]

>Simon, did you do a sweep over all GetBidiProperty() calls to make sure there 
>weren't anymore places that needed to be changed?

Yes and yes.

>Also why does GetBidiProperty() take a void** instead of a void*? It seems that 
>most of the time you are passing in a pointer to data, not a pointer to a 
>pointer?

Even if numerically there are more cases where it's a pointer to a data, the
method was primarily intended for use with an *nsIFrame to return the new frames
created by bidi reordering.

Comment 15

[Simon Montagu :smontagu]

mkaply, I can't reproduce the crash on double click, and the crash on setting
focus is fixed. Do you want to leave this bug open, or close it and open a new one?

Comment 16

[Christopher Blizzard (:blizzard)]

I've checked this into the 0.9.2 branch for you since we're getting down to the
wire.  I hope you don't mind.

Comment 17

[Simon Montagu :smontagu]

*** Bug 88022 has been marked as a duplicate of this bug. ***

Comment 18

[Simon Montagu :smontagu]

blizzard, I don't mind anything that saves me work :-)

Comment 19

[Christopher Blizzard (:blizzard)]

Can you close this?

Comment 20

[rubydoo123]

shouldn't this be marked fix since it is in trunk and branch? 

marking as such

Comment 21

[Simon Montagu :smontagu]

*** Bug 87813 has been marked as a duplicate of this bug. ***