549784 - Crash [@ _cairo_dwrite_scaled_show_glyphs] [@ _cairo_dwrite_scaled_show_glyphs(void*, _cairo_operator, _cairo_pattern const*, _cairo_surface*, int, int, int, int, unsigned int, unsigned int, cairo_glyph_t*, int, _cairo_region*, int*) ]

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[Makoto Kato [:m_kato]]

Some crashes in _cairo_dwrite_scaled_show_glyphs() is reported by enabling DirectWrite.

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=_cairo_dwrite_scaled_show_glyphs%28void*%2C%20_cairo_operator%2C%20_cairo_pattern%20const*%2C%20_cairo_surface*%2C%20int%2C%20int%2C%20int%2C%20int%2C%20unsigned%20int%2C%20unsigned%20int%2C%20cairo_glyph_t*%2C%20int%2C%20int*%29&version=Firefox%3A3.7a3pre

It seems that analysis in _cairo_dwrite_scaled_show_glyphs() becomes NULL because DWriteFactory::Instance()->CreateGlyphRunAnalysis() returns error.

We should check whether DWriteFactory::Instance()->CreateGlyphRunAnalysis() returns error and add correct error handling.

Comment 1

[Martijn Wargers (dead)]

Yeah, I was getting these crashes. But I haven't been able to get a testcase.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Properly check if analysis is succesfully created

This patch will check analysis, it will also make sure the function is receiving a Win32 surface. Which it requires. If either of these fail it will return unsupported and further fallback will occur.

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to comment #1)
> Yeah, I was getting these crashes. But I haven't been able to get a testcase.

A testcase would still be great! I would like to know why the analysis creation fails.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Properly check if analysis is succesfully created v2

Should not have added the check. The function -can- write to any surface type. The cast at the start is just done to make the first if clause more readable, that if-clause properly checks to see if it's a win32 surface though. So this patch is fine.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

I don't know if returning UNSUPPORTED is the best thing to do here.  Isn't _cairo_dwrite_scaled_show_glyphs sort of our last fallback resort here? Even if it's not the last if the analysis fails do we have any reason to believe that falling back will work? I think we should try to figure out why the CreateGlyphRunAnalysis is failing before we try to put a bandaid on it.

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to comment #5)
> I don't know if returning UNSUPPORTED is the best thing to do here.  Isn't
> _cairo_dwrite_scaled_show_glyphs sort of our last fallback resort here? Even if
> it's not the last if the analysis fails do we have any reason to believe that
> falling back will work? I think we should try to figure out why the
> CreateGlyphRunAnalysis is failing before we try to put a bandaid on it.

It isn't our last resort I think. I believe it goes on to try and get bitmaps for each glyph, and then if that fails get paths for each glyph, right? There's two reasons I can think of this failing, a zero length glyph run, or an invalid glyph index (I don't think the latter even causes it to fail). In both cases we'll have to fail anyway.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Here's some information about the failing call:

run:

+		fontFace	0x0145e648	IDWriteFontFace *
		fontEmSize	727.27271	float
		glyphCount	0x0000000a	unsigned int
+		glyphIndices	0x0cc64da0 <Bad Ptr>	const unsigned short *
+		glyphAdvances	0x0fa84610	const float *
+		glyphOffsets	0x0d03e970 {advanceOffset=??? ascenderOffset=??? }	const DWRITE_GLYPH_OFFSET *
		isSideways	0x00000000	int
		bidiLevel	0x00000000	unsigned int

We can conclude that transform is false because fontEmSize is not 1.f

All of the other parameters are constants.

Here's a better stack:

>	xul.dll!_cairo_dwrite_scaled_show_glyphs(void * scaled_font=0x0ffa3300, _cairo_operator op=CAIRO_OPERATOR_OVER, const _cairo_pattern * pattern=0x001391a0, _cairo_surface * generic_surface=0x0a2a0380, int source_x=0x00000091, int source_y=0x00000000, int dest_x=0x00000091, int dest_y=0x00000000, unsigned int width=0x000001a8, unsigned int height=0x00000069, cairo_glyph_t * glyphs=0x00139bb8, int num_glyphs=0x0000000a, int * remaining_glyphs=0x00138dac)  Line 522 + 0xd bytes	C++
 	xul.dll!_cairo_scaled_font_show_glyphs(_cairo_scaled_font * scaled_font=0x0ffa3300, _cairo_operator op=CAIRO_OPERATOR_OVER, const _cairo_pattern * pattern=0x001391a0, _cairo_surface * surface=0x0a2a0380, int source_x=0x00000091, int source_y=0x00000000, int dest_x=0x00000091, int dest_y=0x00000000, unsigned int width=0x000001a8, unsigned int height=0x00000069, cairo_glyph_t * glyphs=0x00139bb8, int num_glyphs=0x0000000a)  Line 2012	C
 	xul.dll!_cairo_surface_old_show_glyphs_draw_func(void * closure=0x001390cc, _cairo_operator op=CAIRO_OPERATOR_OVER, const _cairo_pattern * src=0x001391a0, _cairo_surface * dst=0x0a2a0380, int dst_x=0x00000000, int dst_y=0x00000000, const _cairo_rectangle_int * extents=0x001390d8)  Line 1044 + 0x34 bytes	C
 	xul.dll!_clip_and_composite(_cairo_clip * clip=0x00000000, _cairo_operator op=CAIRO_OPERATOR_CLEAR, const _cairo_pattern * src=0x00000000, _cairo_status (void *, _cairo_operator, const _cairo_pattern *, _cairo_surface *, int, int, const _cairo_rectangle_int *)* draw_func=0x60cfc720, void * draw_closure=0x001390cc, _cairo_surface * dst=0x0a2a0380, const _cairo_rectangle_int * extents=0x00000000)  Line 398 + 0x17 bytes	C
 	xul.dll!_cairo_surface_fallback_show_glyphs(_cairo_surface * surface=0x0a2a0380, _cairo_operator op=CAIRO_OPERATOR_OVER, const _cairo_pattern * source=0x001391a0, cairo_glyph_t * glyphs=0x00139bb8, int num_glyphs=0x0000000a, _cairo_scaled_font * scaled_font=0x0ffa3300)  Line 1091 + 0x2f bytes	C
 	xul.dll!_cairo_surface_show_text_glyphs(_cairo_surface * surface=0x0a2a0380, _cairo_operator op=CAIRO_OPERATOR_OVER, const _cairo_pattern * source=0x001391a0, const char * utf8=0x00000000, int utf8_len=0x00000000, cairo_glyph_t * glyphs=0x00139bb8, int num_glyphs=0x0000000a, const cairo_text_cluster_t * clusters=0x001393b8, int num_clusters=0x00000000, _cairo_text_cluster_flags cluster_flags=0x00000000, _cairo_scaled_font * scaled_font=0x0ffa3300, _cairo_rectangle_int * extents=0x00000000)  Line 2991 + 0x19 bytes	C
 	xul.dll!_cairo_gstate_show_text_glyphs(_cairo_gstate * gstate=0x11e37820, const char * utf8=0x00000000, int utf8_len=0x00000000, const cairo_glyph_t * glyphs=0x0013a6e8, int num_glyphs=0x0000000a, const cairo_text_cluster_t * clusters=0x00000000, int num_clusters=0x00000000, _cairo_text_cluster_flags cluster_flags=0x00000000)  Line 1656 + 0x3e bytes	C
 	xul.dll!_moz_cairo_show_glyphs(_cairo * cr=0x11e37800, const cairo_glyph_t * glyphs=0x0013a6e8, int num_glyphs=0x0000000a)  Line 3307 + 0x15 bytes	C
 	xul.dll!GlyphBuffer::Flush(_cairo * aCR=0x11e37800, int aDrawToPath=0x00000000, int aReverse=0x00000000, int aFinish=0x00000001)  Line 853 + 0x5 bytes	C++
 	xul.dll!gfxFont::Draw(gfxTextRun * aTextRun=0x0fa1fac0, unsigned int aStart=0x00000000, unsigned int aEnd=0x0000000a, gfxContext * aContext=0x0f79d450, int aDrawToPath=0x00000000, gfxPoint * aPt=0x0013af38, gfxFont::Spacing * aSpacing=0x00000000)  Line 993	C++
 	xul.dll!gfxTextRun::Draw(gfxContext * aContext=0x0f79d450, gfxPoint aPt={...}, unsigned int aStart=0x00000000, unsigned int aLength=0x0000000a, const gfxRect * aDirtyRect=0x0013bd98, gfxTextRun::PropertyProvider * aProvider=0x0013bed8, double * aAdvanceWidth=0x0013be08)  Line 2785 + 0x72 bytes	C++
 	xul.dll!nsTextFrame::DrawText(gfxContext * aCtx=0x0f79d450, const gfxPoint & aTextBaselinePt={...}, unsigned int aOffset=0x00000000, unsigned int aLength=0x0000000a, const gfxRect * aDirtyRect=0x0013bd98, PropertyProvider * aProvider=0x0013bed8, double & aAdvanceWidth=1.127968084012e-309#DEN, int aDrawSoftHyphen=0x00000000)  Line 4816	C++
 	xul.dll!nsTextFrame::PaintOneShadow(unsigned int aOffset=0x00000000, unsigned int aLength=0x0000000a, nsCSSShadowItem * aShadowDetails=0x0f79d450, PropertyProvider * aProvider=0x0013bed8, const nsRect & aDirtyRect={...}, const gfxPoint & aFramePt={...}, const gfxPoint & aTextBaselinePt={...}, gfxContext * aCtx=0x0f79d430, const unsigned int & aForegroundColor=0xff000000)  Line 4517	C++
 	xul.dll!nsTextFrame::PaintText(nsIRenderingContext * aRenderingContext=0x14323940, nsPoint aPt={...}, const nsRect & aDirtyRect={...})  + 0x39d941 bytes	C++
 	xul.dll!nsDisplayText::Paint(nsDisplayListBuilder * aBuilder=0x0013c548, nsIRenderingContext * aCtx=0x14323940)  Line 3903	C++
 	xul.dll!nsDisplayList::PaintThebesLayers(nsDisplayListBuilder * aBuilder=0x0013c548, const nsTArray<nsDisplayList::LayerItems> & aLayers={...})  Line 831	C++
 	xul.dll!nsDisplayList::Paint(nsDisplayListBuilder * aBuilder=0x0013c548, nsIRenderingContext * aCtx=0x1315f800, unsigned int aFlags=0x00000000)  Line 772	C++
 	xul.dll!nsDisplayTransform::Paint(nsDisplayListBuilder * aBuilder=, nsIRenderingContext * aCtx=)  Line 1855	C++
 	xul.dll!nsDisplayList::PaintThebesLayers(nsDisplayListBuilder * aBuilder=0x0013c548, const nsTArray<nsDisplayList::LayerItems> & aLayers={...})  Line 831	C++
 	xul.dll!nsDisplayList::Paint(nsDisplayListBuilder * aBuilder=0x0013c548, nsIRenderingContext * aCtx=0x1048ef00, unsigned int aFlags=0x00000000)  Line 772	C++
 	xul.dll!nsLayoutUtils::PaintFrame(nsIRenderingContext * aRenderingContext=0x1048ef00, nsIFrame * aFrame=0x0a1f9b98, const nsRegion & aDirtyRegion={...}, unsigned int aBackstop=0x00000000, unsigned int aFlags=0x00000001)  Line 1197	C++
 	xul.dll!nsSVGForeignObjectFrame::PaintSVG(nsSVGRenderState * aContext=, const nsIntRect * aDirtyRect=)  Line 263 + 0x1e bytes	C++
 	xul.dll!nsSVGUtils::PaintFrameWithEffects(nsSVGRenderState * aContext=0x0013cbac, const nsIntRect * aDirtyRect=0x0013cbe8, nsIFrame * aFrame=0x0a1f9898)  Line 1062	C++
 	xul.dll!nsSVGDisplayContainerFrame::PaintSVG(nsSVGRenderState * aContext=0x0013cbac, const nsIntRect * aDirtyRect=0x0013cbe8)  Line 172 + 0xc bytes	C++
 	xul.dll!nsSVGUtils::PaintFrameWithEffects(nsSVGRenderState * aContext=0x0013cbac, const nsIntRect * aDirtyRect=0x0013cbe8, nsIFrame * aFrame=0x0a1f96d8)  Line 1062	C++
 	xul.dll!nsSVGDisplayContainerFrame::PaintSVG(nsSVGRenderState * aContext=0x0013cbac, const nsIntRect * aDirtyRect=0x0013cbe8)  Line 172 + 0xc bytes	C++
 	xul.dll!nsSVGUtils::PaintFrameWithEffects(nsSVGRenderState * aContext=0x0013cbac, const nsIntRect * aDirtyRect=0x0013cbe8, nsIFrame * aFrame=0x0a1f9548)  Line 1062	C++
 	xul.dll!nsSVGOuterSVGFrame::Paint(nsIRenderingContext & aRenderingContext={...}, const nsRect & aDirtyRect={...}, nsPoint aPt={...})  Line 602	C++
 	xul.dll!nsDisplaySVG::Paint(nsDisplayListBuilder * aBuilder=0x0013ce90, nsIRenderingContext * aCtx=0x1048ef00)  Line 454	C++
 	xul.dll!nsDisplayList::PaintThebesLayers(nsDisplayListBuilder * aBuilder=0x0013ce90, const nsTArray<nsDisplayList::LayerItems> & aLayers={...})  Line 831	C++
 	xul.dll!nsDisplayList::Paint(nsDisplayListBuilder * aBuilder=0x0013ce90, nsIRenderingContext * aCtx=0x00000000, unsigned int aFlags=0x00000001)  Line 772	C++
 	xul.dll!nsLayoutUtils::PaintFrame(nsIRenderingContext * aRenderingContext=0x00000000, nsIFrame * aFrame=0x0a21b2b0, const nsRegion & aDirtyRegion={...}, unsigned int aBackstop=0xffffffff, unsigned int aFlags=0x00000004)  Line 1197	C++
 	xul.dll!PresShell::Paint(nsIView * aDisplayRoot=, nsIView * aViewToPaint=, nsIWidget * aWidgetToPaint=, const nsRegion & aDirtyRegion=, int aPaintDefaultBackground=)  Line 5764 + 0x13 bytes	C++
 	xul.dll!nsViewManager::Refresh(nsView * aView=0x07d3be20, nsIWidget * aWidget=0x005828c0, const nsIntRegion & aRegion={...}, unsigned int aUpdateFlags=0x0013d298)  Line 428	C++
 	xul.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0013d378, nsIView * aView=0x07d3be20, nsEventStatus * aStatus=0x0013d2cc)  Line 877 + 0x12 bytes	C++
 	xul.dll!HandleEvent(nsGUIEvent * aEvent=0x00000001)  Line 161	C++
 	xul.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0013d378, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 3061 + 0x3 bytes	C++
 	xul.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0013d378, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 3090	C++
 	xul.dll!nsWindow::OnPaint(HDC__ * aDC=0x00000000)  Line 543	C++


We'll have to do more work if we need to get the indices, offsets, and advances arrays.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

It also looks like all of these crashes are the same user crashing multiple times.

Comment 9

[Martijn Wargers (dead)]

Yes, that would be me. I was trying to get a minimized testcase, but I'm not able to, thus far. It might be that once bug 549496 gets fixed, I might be able to reproduce this one again (I keep hitting bug 549496 now).

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #9)
> Yes, that would be me. I was trying to get a minimized testcase, but I'm not
> able to, thus far. It might be that once bug 549496 gets fixed, I might be able
> to reproduce this one again (I keep hitting bug 549496 now).

Do you have an non-minimized testcase?

Comment 11

[Martijn Wargers (dead)]

No, sorry, not at the moment.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

How about now?

Comment 13

[Martijn Wargers (dead)]

Sorry, no. I have managed to crash once with this stracktrace in a 20100429 build, so I guess the bug is still there.
But the crash doesn't seem to be catch-able in a nice, simple testcase.
Could this crash perhaps be the result of some low memory condition?

Comment 14

[Martijn Wargers (dead)]

Attachment: zipped up unminimized testcase

This is appr. closest what I can get with a reproducable testcase.
Unzip it, then open the file named 'parentframe - Copy.htm'. You need to give the testcase enhanced privileges, when it asks for that.
It might take a couple of minutes before it crashes. It might help if you open it in more than 1 tab, not sure.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

This test case also seems to cause us to leak memory.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #14)
> Created an attachment (id=443065) [details]
> zipped up unminimized testcase
> 
> This is appr. closest what I can get with a reproducable testcase.
> Unzip it, then open the file named 'parentframe - Copy.htm'. You need to give
> the testcase enhanced privileges, when it asks for that.
> It might take a couple of minutes before it crashes. It might help if you open
> it in more than 1 tab, not sure.

Are you using the html5 parser to reproduce this crash?

Comment 17

[Martijn Wargers (dead)]

Yes, and on windows 7, using a trunk build.

Comment 18

[Bas Schouten (:bas.schouten)]

I've been looking at the case and it won't crash for me.

It leaks pretty badly, but it does so as well in 3.6.4. Profiling seems to indicate most of the leaks are coming from JS land.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

I've been able to reproduce this. The font we hit this on is "PMingLiUBold" with a glyph run with a single glyph with id 18498.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

The string in question seems to be: "碚BY垩f?"

Comment 21

[Jeff Muizelaar [:jrmuizel]]

I hit this again, this time with a two glyph run {14560, 11514} with the font "MingLiU_HKSCSBold"

Comment 22

[Bas Schouten (:bas.schouten)]

(In reply to comment #21)
> I hit this again, this time with a two glyph run {14560, 11514} with the font
> "MingLiU_HKSCSBold"

Do we have any idea what error the analysis is returning?

Comment 23

[Jeff Muizelaar [:jrmuizel]]

0x80070008 Not enough storage is available to process this command.

Comment 24

[Bas Schouten (:bas.schouten)]

(In reply to comment #23)
> 0x80070008 Not enough storage is available to process this command.

That's an out of memory error, really weird.. I wonder if something about this font is just broken. Or if DirectWrite has some sort of weird bug here.

Comment 25

[Joe Drew (not getting mail)]

At bare minimum, we must take Bas's patch.

Comment 27

[Bas Schouten (:bas.schouten)]

What's the status here Jeff, can you still manage to repro this?

Comment 28

[Benoit Girard (:BenWa)]

I can no longer find this crash signature. We should close this bug.

Comment 29

[Sheila Mooney]

Benoit, your wish is coming true. Resolving as works for me.

Comment 30

[Martijn Wargers (dead)]

The testcase doesn't seem to crash anymore, fwiw.
Is the memory leak still there? And is it something to worry about, perhaps?

Comment 31

[Sheila Mooney]

I think if the memory leak is still there, we should log a separate bug and let the memshrink guys know. Who is the best person to confirm the mem leak stuff?