82556 - hang in nsTextFrame::GetPosition in BIDI code

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Christopher Blizzard (:blizzard)]

I'm getting an occasional hang in this bit of code in nsTextFrame.cpp:

#ifdef IBMBIDI
        if (aContentOffset >= mContentOffset) {
          PRInt32 stopOffset = mContentOffset + mContentLength;
          while (aContentOffset < stopOffset &&
                 IS_BIDI_DIACRITIC(text[ip[aContentOffset - mContentOffset]]) )
		  {
            aContentOffset++;
          }
        }
#endif // IBMBIDI

Unfortunately, my debugger is giving me trouble so I'm having problems catching
the values.

(gdb) print ip
$14 = (PRInt32 *) 0xbfffe878
(gdb) print text
$15 = (PRUnichar *) 0xbfffea18
(gdb) print *ip
$16 = 7542
(gdb) print *text
$17 = 43
(gdb) print aContentOffset
$18 = (PRInt32 &) @0xbfffedf8: 7591
(gdb) print mContentOffset
$19 = 7542
(gdb) print stopOffset
$9 = 7592

Comment 1

[kinmoz]

Cc the BIDI and selection crew. This is probably related to the UMR in bug 
81078.

Steps to repro from blizzard:

<blizzard> http://bugzilla.mozilla.org/showattachment.cgi?attach_id=35891
<blizzard> load that doc
<blizzard> start selecting from the top
<blizzard> and start going down
<blizzard> just after the first page ends it hangs every time

Comment 2

[Mike Kaply [:mkaply]]

I can't recreate this on Windows.

Has anyone seen it on anything but Linux?

Comment 3

[mjudge]

Attachment: Fix for the crash/hang.

Comment 4

[kinmoz]

Ooops, the last attatchment was made by me ([e-mail]), I forgot to reset
the login in my browser.

In any case this was my bonehead regression caused by my patch in bug 81078
which [e-mail] checked in for me.

This lastest patch subtracts off mContentOffset to remap the index in the ip[]
array back to the range in the text[] array.

Comment 5

[Simon Fraser [no longer active]]

sr=sfraser

Comment 6

[Mike Kaply [:mkaply]]

r=mkaply

Comment 7

[mjudge]

r=mjudge.

Comment 8

[rubydoo123]

a=beppe, and kin is pinging forces of power for checkin nod

Comment 9

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9.1

Please make my pain go away.

Comment 10

[kinmoz]

Looks like I'm set for checkin when the tree opens.

Comment 11

[kinmoz]

Attachment: An even better patch.

Comment 12

[kinmoz]

Sorry to do this again ... I figured out the cause of the last UMR in bug 81078, 
and it has to do with the same piece of code we are touching to fix this bug, so 
I thought I'd kill 2 birds with one checkin.

Can I get an sr= from sfraser, r= from anyone, and an a= blizzard again?

Comment 13

[Simon Fraser [no longer active]]

The logic here could be improved slightly:

+            if (aContentOffset < bidiStopOffset)
+              curindx = ip[aContentOffset - mContentOffset] - mContentOffset;
+            else
+              break;

to

  if (aContentOffset == bidiStopOffset)
    break;

  curindx = ip[aContentOffset - mContentOffset] - mContentOffset;

Either way, sr=sfraser

Comment 14

[mjudge]

r=mjudge with a change simon and kin will do. >= not ==

Comment 15

[kinmoz]

Attachment: Patch incorporating sfraser and mjudge suggestions.

Comment 16

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9.1 again

Comment 17

[kinmoz]

Fix checked in to Trunk:

    mozilla/layout/html/base/src/nsTextFrame.cpp  revision 1.305

Fix should appear in 05/24/01 QA bits built after 8am.

Comment 18

[Chris Petersen]

Marking verified in the May 30th build.