82089 - nsBidiUtilsImp::ArabicShaping should be turn on

Status: RESOLVED FIXED

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

Description

[Frank Tang]

In intl/unicharutil/src/nsBidiUtilsImp.cpp
The function nsBidiUtilsImp::ArabicShaping is currently 
#ifdef FULL_ARABIC_SHAPING out. We need to turn that on.

This code is currently used by non bidi system to render arabic.
There are only one place call this function 
In layout/base/src/nsBidiPresUtils.cpp nsBidiPresUtils::FormatUnicodeText

I think this routine is more important than nsBidiUtilsImp::Conv_FE_06 
, nsBidiUtilsImp::Conv_FE_06_WithReverse and 
nsBidiUtilsImp::Conv_06_FE_WithReverse so I think we handle these cases
seperately will increase the speed of turn this one on.

Comment 1

[Frank Tang]

I. We need documentation for any if statement and while loop to explain what it 
try to do.
problem lines in that function
1. 
297     if(src-2 >= (aString)){
298       for(p=src-2; (eTr == leftNoTrJ) && (CHAR_IS_ARABIC(*(p+1))) /*&& (p >= 
(aString))*/; p--)  
299         leftNoTrJ = GetJoiningClass(*(p)) ;
300     }

p may be < aString which may cause ABR (array boundary read) in purify and crash

2.
305     if(src+2 <= (aString+aLen-1)){
306       for(p=src+2; (eTr == rightNoTrJ) && (CHAR_IS_ARABIC(*(src+1))); p++)
307         rightNoTrJ = GetJoiningClass(*(p)) ;
308     }

should this be (CHAR_IS_ARABIC(*(src+1))) or (CHAR_IS_ARABIC(*(p+1))) ?
p may > aString+aLen-1 which will cause ABR in purify


3.
319   if(src-2 >= (aString)){
320     for(p=src-2; (eTr == leftNoTrJ) && (CHAR_IS_ARABIC(*(p+1))); p--)
321       leftNoTrJ = GetJoiningClass(*(p)) ;
322   }
same thing, no code guarantee p+1 >= aString and p>= aString
this will cause ABR in purify and crash

One thing I don't understand is we are looking forward (src++) the buffer. and 

293     if ((eTr != leftJ) || ((leftJ == eTr) && 
294         ( ( (src-1) >= aString ) && !CHAR_IS_ARABIC(*(src-1)))))
295       leftNoTrJ = thisJ;
296 
297     if(src-2 >= (aString)){
298       for(p=src-2; (eTr == leftNoTrJ) && (CHAR_IS_ARABIC(*(p+1))) /*&& (p >= 
(aString))*/; p--)  
299         leftNoTrJ = GetJoiningClass(*(p)) ;
300     }

is finding leftNoTrJ based on p < src, Why we need to do this ? Why can we 
remember condictional assign thisJ to leftNoTrJ after we get formB for the next 
loop ?

leftNoTrJ = eNJ; // we know the character before calling ArabicShaping is not 
Arabic

before the while loop 
and add 

if(eTr != thisJ)
  leftNoTrJ = thisJ;

in the end of the while loop after

310     formB = PresentationFormB(*src, DecideForm(leftNoTrJ, thisJ, 
rightNoTrJ));


We can replace line 293-300 and 315-322 by adding

and adding 
if(eTr != thisJ)
  leftNoTrJ = thisJ;

Comment 2

[Frank Tang]

The other problem we have with this routine is that we didn't check the output 
buffer length in the while loop. we should add
PRUnichar* aBufEnd = aBuf + *aBufLen;
before the while loop and
add 
if(dest >= aBufEnd)
{
   NS_ASSERTION((dest < aBufEnd), "no enough output buffer");
   return NS_OUT_OF_MEMORY;
}

in both the beginning of the while loop and before the second call to 
PresentationFormB

Comment 3

[Frank Tang]

Attachment: patch in -u form

Comment 4

[Frank Tang]

Attachment: same patch in diff -c form (easier for review)

Comment 5

[Frank Tang]

sorry, the patch is bad. Ignore them

Comment 6

[Frank Tang]

Attachment: 2nd patch in -c form

Comment 7

[Frank Tang]

Attachment: same 2nd patch in -u form

Comment 8

[Frank Tang]

simon/mkaply- can you get some one in IBM Egypt to review and verify my 2nd 
patch ? If there are problem, please describe what the problem is and provide 
test cases. Thanks.

Comment 9

[Maha Abou El-Rous]

This depends on adding the ICU shaping in

Comment 10

[Frank Tang]

arabic shaping code change. Reassign to mkaply.

Comment 11

[Zach Lipton [:zach]]

Mass-move all BiDi Hebrew and Arabic qa to me, [e-mail]. 
Thank you Gilad for your service to this component, and best of luck to you 
in the future.

Sholom. 

Comment 12

[Katsuhiko Momoi]

Assign QA back to mahar.

Comment 13

[Simon Montagu :smontagu]

This bug has been superseded by bug 92797