75026 - Bidi: Selection on US system causes a crash in GKGFXWIN.DLL

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Maha Abou El-Rous]

When opening an Arabic htm page and trying to make a selection beyond the 
browser display area, Mozilla crashes in GKGFXWIN.DLL

Reproducability: Every time

Steps to reproduce:
1. Open file CP1256.htm
2. while on the Arabic text, press the left button of the mous and move up to 
make a selection
3. move up until you pass the browser area into where the URL name goes to be 
loaded or to the title bar of the window
4. move back towards the text

Actual Results:
MOZILLA caused an invalid page fault in
module GKGFXWIN.DLL at 015f:00cccbd8.
Registers:
EAX=0070f164 CS=015f EIP=00cccbd8 EFLGS=00010213
EBX=0000074e SS=0167 ESP=0070ee48 EBP=0070ee74
ECX=00000e9c DS=0167 ESI=0184aca0 FS=2c7f
EDX=00000000 ES=0167 EDI=0185ffc0 GS=0000
Bytes at CS:EIP:
66 8b 04 01 8b 4d e8 66 89 45 e4 0f b7 51 0e 8b 
Stack dump:
00000000 0184aca0 00000000 01858074 00700000 01842020 00000e9c 0000074e 
0185ffc0 00000384 00003480 0070f2f0 014f5ba5 0000074c 0070f164 ffffffff 

Expected Results:
You should have a selection from the point you started to the top of the file, 
not crashing.

Comment 1

[Maha Abou El-Rous]

Attachment: Arabic 1256 file

Comment 2

[Frank Tang]

add crash keyword

Comment 3

[Frank Tang]

we should fix this in moz0.9. crash is not good. 

Comment 4

[Frank Tang]

erik resign. reassign all his bug to ftang for now.

Comment 5

[Frank Tang]

mark it as assign

Comment 6

[Andreas Becker]

Changing QA contact to [e-mail].

Comment 7

[Frank Tang]

take out 0.9.1

Comment 8

[Frank Tang]

I cannot reproduce this right now. Either because we have not turn on 
FULL_ARABIC_SHAPING yet or because the fix I checked in for bug 81078

Comment 9

[Frank Tang]

I am not sure this is the same crash but I got a crash when I select up/down 
www.hotmail.co.il/login.asp

here is the stack trace
nsRenderingContextWin::GetWidth(nsRenderingContextWin * const 0x041e45e0, const 
unsigned short * 0x0012dd10, unsigned int 0xffffffff, int & 0x000000d2, int * 
0x00000000) line 1771 + 6 bytes
nsTextFrame::PaintUnicodeText(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, nsIStyleContext * 0x03a98a70, nsTextFrame::TextStyle & {...}, int 
0x00000000, int 0x00000000) line 2244 + 38 bytes
nsTextFrame::Paint(nsTextFrame * const 0x02636a08, nsIPresContext * 0x041cf8a0, 
nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 1387
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x02636a08, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsBlockFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
5162
nsBlockFrame::Paint(nsBlockFrame * const 0x0263623c, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 5040
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x0263623c, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsContainerFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext 
& {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
173
nsTableCellFrame::Paint(nsTableCellFrame * const 0x026361e0, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 447
nsTableRowFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext 
& {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
547
nsTableRowFrame::Paint(nsTableRowFrame * const 0x02636198, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 500
nsTableRowGroupFrame::PaintChildren(nsIPresContext * 0x041cf8a0, 
nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 278
nsTableRowGroupFrame::Paint(nsTableRowGroupFrame * const 0x025bde50, 
nsIPresContext * 0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, 
nsFramePaintLayer eFramePaintLayer_Overlay) line 232
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x025bde50, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsContainerFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext 
& {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
173
nsTableFrame::Paint(nsTableFrame * const 0x025bdde8, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 1471
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x025bdde8, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsTableOuterFrame::Paint(nsTableOuterFrame * const 0x025bdd9c, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 368
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x025bdd9c, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsBlockFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
5162
nsBlockFrame::Paint(nsBlockFrame * const 0x025bdd10, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 5040
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x025bdd10, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsContainerFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext 
& {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
173
nsTableCellFrame::Paint(nsTableCellFrame * const 0x025bdcb4, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 447
nsTableRowFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext 
& {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
547
nsTableRowFrame::Paint(nsTableRowFrame * const 0x02571e88, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 500
nsTableRowGroupFrame::PaintChildren(nsIPresContext * 0x041cf8a0, 
nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 278
nsTableRowGroupFrame::Paint(nsTableRowGroupFrame * const 0x025db094, 
nsIPresContext * 0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, 
nsFramePaintLayer eFramePaintLayer_Overlay) line 232
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x025db094, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsContainerFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext 
& {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
173
nsTableFrame::Paint(nsTableFrame * const 0x025db02c, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 1471
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x025db02c, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsTableOuterFrame::Paint(nsTableOuterFrame * const 0x025dafe0, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 368
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x025dafe0, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsBlockFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
5162
nsBlockFrame::Paint(nsBlockFrame * const 0x025daf04, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 5040
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x025daf04, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsBlockFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
5162
nsBlockFrame::Paint(nsBlockFrame * const 0x025dae78, nsIPresContext * 
0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 5040
nsContainerFrame::PaintChild(nsIPresContext * 0x041cf8a0, nsIRenderingContext & 
{...}, const nsRect & {...}, nsIFrame * 0x025dae78, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 229
nsContainerFrame::PaintChildren(nsIPresContext * 0x041cf8a0, nsIRenderingContext 
& {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay) line 
173
nsHTMLContainerFrame::Paint(nsHTMLContainerFrame * const 0x025da10c, 
nsIPresContext * 0x041cf8a0, nsIRenderingContext & {...}, const nsRect & {...}, 
nsFramePaintLayer eFramePaintLayer_Overlay) line 122
CanvasFrame::Paint(CanvasFrame * const 0x025da10c, nsIPresContext * 0x041cf8a0, 
nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer 
eFramePaintLayer_Overlay) line 238
PresShell::Paint(PresShell * const 0x041a26b4, nsIView * 0x04c24370, 
nsIRenderingContext & {...}, const nsRect & {...}) line 5241 + 34 bytes
nsView::Paint(nsView * const 0x04c24370, nsIRenderingContext & {...}, const 
nsRect & {...}, unsigned int 0x00000080, int & 0x100283d5) line 275
nsViewManager::RenderDisplayListElement(DisplayListElement2 * 0x041e4390, 
nsIRenderingContext & {...}) line 1443
nsViewManager::RenderViews(nsIView * 0x04c63800, nsIRenderingContext & {...}, 
const nsRect & {...}, int & 0x00000000) line 1368
nsViewManager::Refresh(nsIView * 0x04c63800, nsIRenderingContext * 0x041e45e0, 
const nsRect * 0x0012f654, unsigned int 0x00000001) line 900
nsViewManager::DispatchEvent(nsViewManager * const 0x041a09b0, nsGUIEvent * 
0x0012f794, nsEventStatus * 0x0012f698) line 1962
HandleEvent(nsGUIEvent * 0x0012f794) line 68
nsWindow::DispatchEvent(nsWindow * const 0x04c67d04, nsGUIEvent * 0x0012f794, 
nsEventStatus & nsEventStatus_eIgnore) line 702 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f794, nsEventStatus & 
nsEventStatus_eIgnore) line 728
nsWindow::OnPaint() line 3847 + 28 bytes
nsWindow::ProcessMessage(unsigned int 0x0000000f, unsigned int 0x00000000, long 
0x00000000, long * 0x0012fbac) line 2852 + 17 bytes
nsWindow::WindowProc(HWND__ * 0x16230660, unsigned int 0x0000000f, unsigned int 
0x00000000, long 0x00000000) line 957 + 27 bytes
USER32! 77e42137()
USER32! 77e42183()
NTDLL! 77f663b3()

The aLength in PaintUnicodeText is 0xfffffff

Comment 10

[Frank Tang]

and the following is the iter
-	iter	{...}
+	mUniStr	0x0012dd10 "  00????????"
+	mCStr	0x0012dd10 " "
	mLength	0x00000001
	mCurrentIdx	0x00000000
	mCurrentLength	0xffffffff
+	mOldStyle	{...}
+	mDetails	0x04369ad0
	mDone	0x00000000
+	mTypes	0x00000000 ""
	mInit	0x00000001
	mSelectionStatus	0x0002
	mDisabledColor	0xffb0b0b0

what is mCurrentLength	? what is the difference between mCurrentLength	 and 
mLength	?

Comment 11

[Frank Tang]

another selection crash

Comment 12

[Frank Tang]

I think the problem is we use the right length here.

Comment 13

[Frank Tang]

easier test procedure
1. open http://www.hotmail.co.il/login.asp
2. click into the mid of "Microsoft"
3. drag mouse around to select text
4. it will crash,

Comment 14

[Frank Tang]

I think the problem is we use the wrong "length" to do BIDI swaping code.
textLength is not from the result of nsTextTransform and it will change depend 
on many thing- such as whitespace compression and text-transform. I believe the 
mDetails->mStart/mEnd are from the content code so we should minus with 
mContentLength instead. 

Here is my patch, it include some ASSERTION code also, 

Comment 15

[Frank Tang]

Attachment: add assertion and fix the crash

Comment 16

[kinmoz]

I think the code in question:

      while (sdptr){
        sdptr->mStart = ip[sdptr->mStart] - mContentOffset;
        sdptr->mEnd = ip[sdptr->mEnd]  - mContentOffset;
#ifdef IBMBIDI // Simon - display substrings RTL in RTL frame on non-Bidi platfo
rm
        if ((level & 1) && !(isRightToLeftOnBidiPlatform)) {
          PRInt32 swap  = sdptr->mStart;
          sdptr->mStart = textLength - sdptr->mEnd;
          sdptr->mEnd   = textLength - swap;
        }
#endif
        sdptr = sdptr->mNext;
      }

is mapping all the detail offsets to the range within the compressed string, so 
using textLength is the right thing to do ... that said, the BIDI code is making 
mStart >= mEnd, so there's a problem since I think the selection iterator code 
expects mStart <= mEnd.

Comment 17

[kinmoz]

Ok, I must've been on drugs when I first looked at this ... I take it back, the 
BIDI code is not making mStart >= mEnd ... it's still generating mStart <= mEnd 
which should be ok.

Comment 18

[Frank Tang]

Attachment: 2nd try

Comment 19

[Frank Tang]

Attachment: 2nd patch in -c form- easier to see the changes

Comment 20

[kinmoz]

Ok, here's what I'm seeing in the debugger, using ftang's URL at 
Microsoft/hotmail.

If I click right before the 't' in Microsoft, and then shift-click in the middle 
of the '2000' next to it, we will run across a call to PaintUnicodeText() where 
textLength == 2 ... when bidiUtils->FormatUnicodeText() is called, it modifies 
textLength so that it is now 1, probaby because the 2nd character in the string 
is a space (char 32) ... so when we get to the "while(sdptr)" loop in question, 
the ip[] array is now out of sync with the text[] array and textLength variable. 
In this particular case sdptr->mStart == 0 and sdptr->mEnd == 2 ... so after 
executing the BIDI code in that loop we get sdptr->mStart = -1 and sdptr->mEnd = 
1 ... so we crash.

If the BIDI code is going to resize the text array and adjust the textLength 
variable, it needs to regenerate the ip array to match!!

Comment 21

[Frank Tang]

kin- you are right, my patch didn't do that.

simon- do you have a better fix?

Comment 22

[Frank Tang]

kin is right, my attempt is wrong.

Comment 23

[Frank Tang]

Attachment: stop bleeding patch which prevent crash untill simon have the right fix.

Comment 24

[Frank Tang]

OK, here is a temp fix. It does not really fix the problem but at least it will 
stop the bleeding untill simon produce a right fix. 

Comment 25

[Simon Fraser [no longer active]]

sr=sfraser the last patch, as long as the comment is enhanced to reference this 
bug, and say why the values can become negative (whitespace compression).

Comment 26

[Frank Tang]

check in temp fix and reassign to [e-mail] to produce real fix. 
Since we no longer crash, take out the moz0.9.1 milestone. and crash keyword.

Comment 27

[Frank Tang]

mark as moz0.9.2

Comment 28

[Jaime Rodriguez, Jr.]

Ftang - This is now marked as M0.9.3; should we place a nsBranch keyword on this 
one. I think this is stop ship crashed in my book.

Comment 29

[Simon Montagu :smontagu]

Does anybody still see the assertions here? I haven't had any luck in
reproducing this and suspect that it has been fixed by another checkin

Comment 30

[Maha Abou El-Rous]

I Tried it again on US NT with build of 28/06/2001, still got the same problem.
note that Arabic shaping was disabled for that build, so it has nothing to do 
with the crash. I'm downloading the installable version to test on 98 US.

Comment 31

[Maha Abou El-Rous]

I have tried the same scenario with 98 US with 28/06 build, I still get the same crash.
Simon, are you trying this on Win2k with US locale as default? because with this case for 
Arabic, if I have one of the Arabic locales installed - even if it is not the default - 
Mozilla still detects the system as a bidi one !!! so I can't recreate the problem there.

Comment 32

[Simon Montagu :smontagu]

There are two distinct crash cases being discussed here. Maha's original
testcase was duped by bug 85813, and is fixed by attachment 41263 [details] [diff] [review].

The hotmail.co.il login page is another case, which I will now start working on.

Comment 33

[Maha Abou El-Rous]

Tested with the build of 6 Jul, it is fixed.

Comment 34

[Maha Abou El-Rous]

This problem is related to the Arabic Shaping. Resulting buffer from shaping is smaller in size than the original, which causes mEnd > textLength, and the crash. This is fixed in the patch submitted for bug 92797. added zero width space to cover for the shrinkage.

we get the same assertion when we have RLM & LRM in the HTML, then  NSBIDI_REMOVE_BIDI_CONTROLS is set and the buffer size coming out from FormatUnicodeText is less than the original.