341500 - crash in usp10.dll(ScriptShape API)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Martijn Wargers (dead)]

See upcoming case. It's a bit messy one, I'm afraid.
Basically it consists of <nobr><input type="text" value="a lot of text, also some utf-8">

Loading that page is quite often causing a crash in Mozilla.
TB19857249X
USP10.dll + 0x3475f (0x74d4475f)
UniscribeItem::Shape   gfxWindowsTextRun::MeasureOrDrawUniscribe   gfxWindowsTextRun::Draw   nsThebesFontMetrics::DrawString   nsThebesRenderingContext::DrawString   nsLayoutUtils::SafeDrawString   nsTextFrame::PaintAsciiText   0x03d99020

When it hasn't crashed and then I select some of the garbage in the text input, I crash quite often:
TB19857402H
USP10.dll + 0x3ab05 (0x74d4ab05)
USP10.dll + 0x19a08 (0x74d29a08)
USP10.dll + 0x12d44 (0x74d22d44)
Uniscribe::Itemize  

I would expect that cairo builds behave the same as non-cairo builds, the page would load instantly without using 100%cpu, that it would not display garbage in the text input and that it would not crash.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: Original from which testcase is more or less derived from

Comment 3

[Stuart Parmenter]

Attachment: crash fix 1

this is the crash I'm seeing.. Not sure whats up with it.  It isn't the one you were seeing though.

Comment 4

[Martijn Wargers (dead)]

I can retest when that patch is checked in.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

that crash is bug 342922 is it not?

Comment 6

[Stuart Parmenter]

Comment on attachment 228177 [details] [diff] [review]
crash fix 1

yeah, that is 342922 apparently

Comment 7

[Martijn Wargers (dead)]

Attachment: testcase2

Hmm, currently I crash even with this testcase, it only contains ع
It also crashes all older cairo builds. I'm pretty it didn't use to crash my trunk builds.

Comment 8

[Martijn Wargers (dead)]

This doesn't crash anymore, but the performance of selecting something in the input in the first testcase is not good. Also, the text tends to disappear when selecting.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

if fx fails to detect the character encoding, fx sometimes crashes. But the crash occurs randomly. This crash is serious...

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: disable script cache patch

I think that this crash bug is a bug of Windows. Even if we send the broken string to ScriptShape, it should not crash. But the crash doesn't always occur the same testing. I think that the script cache may be broken sometimes and it makes crash by the testing. If so, we can explain why the crash is random. However of course, I'm not sure. I hope that we can test this patch in nightly build. (Of course, this patch has performance regression.)

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If we can fix this crash by my patch, we can use another way that is caching glyph informations for drawing/measuring in textRun. By we cache it in textRun, we can cut down the count of calling the uniscribe functions.

Comment 12

[Stuart Parmenter]

Comment on attachment 247160 [details] [diff] [review]
disable script cache patch

I really don't want to disable the script cache.  I'd prefer to find some other way to reproduce this so we know what to look out for and _then_ maybe clear the cache.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 247160 [details] [diff] [review]
disable script cache patch

I have a feedback that is a crash report with this patch. We need another idea of this cause...

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Disable "disable shpaing" patch

I found the cause of this bug.

If the string has surrogate pair and disabled the shaping, ScriptShape API crashes in sometimes. We should not use "disable shaping".

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'll file a new bug for perf issue. This bug should fix only the crash.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I filed the perf issue in bug 362631.

Comment 17

[Stuart Parmenter]

We have to disable shaping in order for ScriptShape to succeed at all in cases where we don't have an appropriate font to render the text we're trying to render.  This isn't going to work either :/

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #17)
> We have to disable shaping in order for ScriptShape to succeed at all in cases
> where we don't have an appropriate font to render the text we're trying to
> render.  This isn't going to work either :/

Really? What's the case?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Kimura-san, do you have an idea?

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Replace the surrogate pair at disabling the shaping

Another approach, how about this?

Comment 21

[Stuart Parmenter]

Comment on attachment 247329 [details] [diff] [review]
Replace the surrogate pair at disabling the shaping

ok -- lets get this in for now to stop the crash.  I'll ping microsoft and see if we can come up with a better fix.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in for a2.

Comment 23

[shutdown]

(In reply to comment #20)
> Created an attachment (id=247329) [edit]
> Replace the surrogate pair at disabling the shaping
> Another approach, how about this?

| +        mAlternativeString = (PRUnichar *)malloc(mLength * sizeof(PRUnichar));
| +        memcpy((void *)mAlternativeString, (const void *)mString,
| +               mLength * sizeof(PRUnichar));

This can cause crash on OOM.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #23)
> (In reply to comment #20)
> > Created an attachment (id=247329) [edit]
> > Replace the surrogate pair at disabling the shaping
> > Another approach, how about this?
> 
> | +        mAlternativeString = (PRUnichar *)malloc(mLength *
> sizeof(PRUnichar));
> | +        memcpy((void *)mAlternativeString, (const void *)mString,
> | +               mLength * sizeof(PRUnichar));
> 
> This can cause crash on OOM.
> 

Wow, you're right. I'll file and fix it.(after a business trip).