Closed Bug 341500 Opened 15 years ago Closed 14 years ago

crash in usp10.dll(ScriptShape API)

Categories

(Core :: Graphics, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: masayuki)

References

Details

(Keywords: crash, testcase)

Attachments

(5 files, 2 obsolete files)

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.
Attached file testcase
Attached 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.
I can retest when that patch is checked in.
Comment on attachment 228177 [details] [diff] [review]
crash fix 1

yeah, that is 342922 apparently
Attachment #228177 - Attachment is obsolete: true
Attached 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.
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?
Attached 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 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)
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 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
Closed: 14 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
(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).
Depends on: 393630
You need to log in before you can comment on or make changes to this bug.