777703 - Abort in cairo_scaled_font_glyph_extents due to failure in _cairo_scaled_glyph_lookup when loading a pdf in pdf.js in metrofx

Status: NEW

(Core :: Graphics: Text, defect)

Description

[Jim Mathies [:jimm]]

I'll attach the stack.

pdf:

http://www.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=5&ved=0CHYQFjAE&url=http%3A%2F%2Fwww.irs.gov%2Fpub%2Firs-pdf%2Ffw4.pdf&ei=HksRUO2FGaH06AH2qoCYAg&usg=AFQjCNHdJoiSxm8m_ff8SZ6jMDWjFVFBMQ

Comment 1

[Jim Mathies [:jimm]]

Attachment: stack

Comment 2

[Brian R. Bondy [:bbondy]]

I can't reproduce this with a release build, but I can reproduce this with a debug build on m-c on Windows 8 only.  A debug build on Windows 7 doesn't reproduce.

I debugged this and the error that triggers this is:

cairo-dwrite-font.cpp: 

HRESULT hr=  font_face->GetDesignGlyphMetrics(&charIndex, 1, &metrics); 


I noticed that charIndex = 40 when this fails.  It fails with an invalid arg error. I tried changing charIndex = 0 and set the debug cursor to re-run that line and it succeeds. Setting it to charIndex = 1 fails.  

So I think this is just related to some font difference in Windows 8 compared to Windows 7.  And error handling in debug mode leading to a crash.

Comment 3

[Brian R. Bondy [:bbondy]]

I'm still looking into this, the reason for the abort is that failure, but the abort happens in a debug assertion where we check that the cairo error is an error code != 0 and smaller than the max known cairo error version.

I was actually looking for such a case previously as I suspected it exists.  
Anyway I'll post more findings soon.

Comment 4

[Brian R. Bondy [:bbondy]]

Found the problem, we're using an init status as an error code, which is wrong.
It's a little humerus from the below comment:

/* Sure wish C had a real enum type so that this would be distinct
 * from #cairo_status_t. Oh well, without that, I'll use this bogus 100
 * offset.  We want to keep it fit in int8_t as the compiler may choose
 * that for #cairo_status_t */
typedef enum _cairo_int_status {
    CAIRO_INT_STATUS_UNSUPPORTED = 100,
    CAIRO_INT_STATUS_DEGENERATE,
    CAIRO_INT_STATUS_NOTHING_TO_DO,
    CAIRO_INT_STATUS_FLATTEN_TRANSPARENCY,
    CAIRO_INT_STATUS_IMAGE_FALLBACK,
    CAIRO_INT_STATUS_ANALYZE_RECORDING_SURFACE_PATTERN,

    CAIRO_INT_STATUS_LAST_STATUS
} cairo_int_status_t;

Comment 5

[Brian R. Bondy [:bbondy]]

*humorous

Comment 6

[John Daggett (:jtd)]

Brian, if you get around to a patch, bas is probably the best person to review it.

Comment 7

[Brian R. Bondy [:bbondy]]

There are a lot of instances (at least a few dozen) in cairo where _cairo_int_status errors are treated as cairo_status_t.
This is mostly fine except (see below for why it's not) in some cases there are errors and debug assertions will be thrown. 

The main problem with this is not the debug assertions in those errors but with _cairo_create_in_error.
Those create a cairo nil object from an array of objects.
The problem with this is that the array is only of length CAIRO_STATUS_LAST_STATUS which is I think 40.

But since status sometimes holds _cairo_int_status, it can return an object of some random object on the stack at array offset CAIRO_INT_STATUS_LAST_STATUS (160).

This leads to crashes like in bug 614772

static cairo_t *
_cairo_create_in_error (cairo_status_t status)

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1

I can file a follow up bug to try and catch all the other instances. In particular this one happens when viewing pdfs in pdfjs a lot in Windows 8.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 646560 [details] [diff] [review]
Patch v1

Review of attachment 646560 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/cairo/cairo/src/cairo-scaled-font.c
@@ +1509,3 @@
>  					     glyphs[i].index,
>  					     CAIRO_SCALED_GLYPH_INFO_METRICS,
>  					     &scaled_glyph);

What is the internal status that we're getting and can we handle it properly instead of just using GLYPH_ERROR?

Comment 10

[Brian R. Bondy [:bbondy]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> Comment on attachment 646560 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 646560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/cairo/cairo/src/cairo-scaled-font.c
> @@ +1509,3 @@
> >  					     glyphs[i].index,
> >  					     CAIRO_SCALED_GLYPH_INFO_METRICS,
> >  					     &scaled_glyph);
> 
> What is the internal status that we're getting 


Inside the _cairo_scaled_glyph_lookup call, 
This call is failing with: CAIRO_INT_STATUS_UNSUPPORTED
status = scaled_font->backend->scaled_glyph_init (scaled_font,
 scaled_glyph, info | CAIRO_SCALED_GLYPH_INFO_METRICS);



> and can we handle it properly
> instead of just using GLYPH_ERROR?

I have no idea, but returning a value within the range of the enum seems better to me (especially when we know that those error codes are indexed into an array of size max enum value for the proper status).  And the PDF looks fine that it loads as far as I can tell. 

If you know how it can be handled better let me know and I can fix that way instead.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to Brian R. Bondy [:bbondy] from comment #10)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> > Comment on attachment 646560 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 646560 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/cairo/cairo/src/cairo-scaled-font.c
> > @@ +1509,3 @@
> > >  					     glyphs[i].index,
> > >  					     CAIRO_SCALED_GLYPH_INFO_METRICS,
> > >  					     &scaled_glyph);
> > 
> > What is the internal status that we're getting 
> 
> 
> Inside the _cairo_scaled_glyph_lookup call, 
> This call is failing with: CAIRO_INT_STATUS_UNSUPPORTED
> status = scaled_font->backend->scaled_glyph_init (scaled_font,
>  scaled_glyph, info | CAIRO_SCALED_GLYPH_INFO_METRICS);

What code is returning the CAIRO_INT_STATUS_UNSUPPORTED and why?

Comment 12

[Brian R. Bondy [:bbondy]]

Please let me know why the previous answer wasn't sufficient. This call returns it: scaled_font->backend->scaled_glyph_init(...) and I don't know why.

Comment 13

[Jim Mathies [:jimm]]

ping - jmuizelaar

Comment 14

[Brian R. Bondy [:bbondy]]

Looks like the font was fixed with win8 RTM or the resource linked by the URL above changed. So we'll need another URL to reproduce again.  In any case this is still a valid patch and I'll leave the review as is. 

Since I don't know when it will be reviewed, I'll just revert this on elm and will eventually re-land on m-c if it gets reviewed.

https://hg.mozilla.org/projects/elm/rev/a8541284045d

Comment 15

[Brian R. Bondy [:bbondy]]

This has been waiting on a response for over 2 months even with extra pings in between. Marking feedback requested to see if that will help.

Comment 16

[Brian R. Bondy [:bbondy]]

ping

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 646560 [details] [diff] [review]
Patch v1

Review of attachment 646560 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about the long delay. Pinging me on IRC is more likely to have me noticeable effect as I missed the earlier pings in my bug mail.

Anyways, if the backend is returning UNSUPPORTED we should either be attempting to draw the glyphs another way or not drawing them at all or the backend should be returning a different error code instead of UNSUPPORTED. UNSUPPORTED is not supposed to mean any kind of error condition.

Comment 18

[Brian R. Bondy [:bbondy]]

I was just attempting to fix using the wrong enum for the error, one that is out of range of valid values.  Since I no longer get that error though I'll leave it as is using the wrong enum values.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.