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

RESOLVED FIXED in mozilla0.9.3

Status

()

Core
Internationalization
--
critical
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Maha Abou El-Rous, Assigned: smontagu)

Tracking

({crash, intl})

Trunk
mozilla0.9.3
x86
Windows 98
crash, intl
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: add bandaid code to turn crash into assertion gracefully.)

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 29956 [details]
Arabic 1256 file

Comment 2

17 years ago
add crash keyword
Keywords: crash

Comment 3

17 years ago
we should fix this in moz0.9. crash is not good. 
Target Milestone: --- → mozilla0.9

Updated

17 years ago
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 4

17 years ago
erik resign. reassign all his bug to ftang for now.
Assignee: erik → ftang

Comment 5

17 years ago
mark it as assign
Status: NEW → ASSIGNED

Comment 6

17 years ago
Changing QA contact to mahar@eg.ibm.com.
QA Contact: andreasb → mahar

Comment 7

17 years ago
take out 0.9.1
Target Milestone: mozilla0.9.1 → ---

Comment 8

17 years ago
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
Target Milestone: --- → mozilla0.9.1

Comment 9

17 years ago
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

Updated

17 years ago
Keywords: intl

Comment 10

17 years ago
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

17 years ago
another selection crash

Updated

17 years ago
Whiteboard: crash

Comment 12

17 years ago
I think the problem is we use the right length here.

Comment 13

17 years ago
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

17 years ago
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

17 years ago
Created attachment 35418 [details] [diff] [review]
add assertion and fix the crash

Comment 16

17 years ago
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

17 years ago
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

17 years ago
Created attachment 35454 [details] [diff] [review]
2nd try

Comment 19

17 years ago
Created attachment 35455 [details] [diff] [review]
2nd patch in -c form- easier to see the changes

Comment 20

17 years ago
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

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

simon- do you have a better fix?

Comment 22

17 years ago
kin is right, my attempt is wrong.

Comment 23

17 years ago
Created attachment 35479 [details] [diff] [review]
stop bleeding patch which prevent crash untill simon have the right fix.

Comment 24

17 years ago
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

17 years ago
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

17 years ago
check in temp fix and reassign to simon@softel.co.il to produce real fix. 
Since we no longer crash, take out the moz0.9.1 milestone. and crash keyword.
Assignee: ftang → simon
Status: ASSIGNED → NEW
Keywords: crash
Whiteboard: crash → add bandaid code to turn crash into assertion gracefully.
Target Milestone: mozilla0.9.1 → ---

Comment 27

17 years ago
mark as moz0.9.2
Target Milestone: --- → mozilla0.9.2
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 28

17 years ago
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.
Keywords: crash, nsbeta1
(Assignee)

Comment 29

17 years ago
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

17 years ago
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

17 years ago
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.
(Assignee)

Comment 32

17 years ago
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.

Updated

17 years ago
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 33

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

Updated

17 years ago
Depends on: 92797

Comment 34

17 years ago
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. 
You need to log in before you can comment on or make changes to this bug.