crash in usp10.dll(ScriptShape API)

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: martijn.martijn, Assigned: masayuki)

Tracking

({crash, testcase})

Trunk
x86
Windows XP
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

13 years ago
Posted file testcase

Comment 3

13 years ago
Posted patch crash fix 1 (obsolete) — Splinter Review
this is the crash I'm seeing.. Not sure whats up with it.  It isn't the one you were seeing though.
(Reporter)

Comment 4

13 years ago
I can retest when that patch is checked in.

Comment 6

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

yeah, that is 342922 apparently
Attachment #228177 - Attachment is obsolete: true
(Reporter)

Comment 7

13 years ago
Posted file testcase2
Hmm, currently I crash even with this testcase, it only contains &#1593;
It also crashes all older cairo builds. I'm pretty it didn't use to crash my trunk builds.
(Reporter)

Comment 8

13 years ago
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.
if fx fails to detect the character encoding, fx sometimes crashes. But the crash occurs randomly. This crash is serious...
Flags: blocking1.9?
Posted patch disable script cache patch (obsolete) — Splinter Review
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.)
Attachment #247160 - Flags: review?(pavlov)
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

13 years ago
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 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...
Attachment #247160 - Flags: review?(pavlov) → review-
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".
Assignee: nobody → masayuki
Attachment #247160 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #247320 - Flags: review?(pavlov)
I'll file a new bug for perf issue. This bug should fix only the crash.
Keywords: perf
Summary: A case where Mozilla is very slow, show garbage in text input an likely to crash in usp10.dll → crash in usp10.dll(ScriptShape API)

Comment 17

13 years ago
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 :/
(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?
Kimura-san, do you have an idea?

Comment 21

13 years ago
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.
Attachment #247329 - Flags: review?(pavlov) → review+
checked-in for a2.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Flags: blocking1.9?
Resolution: --- → FIXED

Comment 23

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

Updated

12 years ago
Depends on: 393630
You need to log in before you can comment on or make changes to this bug.