Open
Bug 777703
Opened 12 years ago
Updated 2 years ago
Abort in cairo_scaled_font_glyph_extents due to failure in _cairo_scaled_glyph_lookup when loading a pdf in pdf.js in metrofx
Categories
(Core :: Graphics: Text, defect)
Tracking
()
NEW
People
(Reporter: jimm, Unassigned)
References
Details
(Whiteboard: completed-elm)
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
jrmuizel
:
review-
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
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.
Reporter | ||
Updated•12 years ago
|
OS: Windows 8 Metro → Windows 8
Comment 3•12 years ago
|
||
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.
OS: Windows 8 → Windows 8 Metro
Comment 4•12 years ago
|
||
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•12 years ago
|
||
*humorous
Comment 6•12 years ago
|
||
Brian, if you get around to a patch, bas is probably the best person to review it.
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
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.
Reporter | ||
Comment 13•12 years ago
|
||
ping - jmuizelaar
Comment 14•12 years ago
|
||
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•12 years ago
|
||
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.
Flags: needinfo?(jmuizelaar)
Comment 17•12 years ago
|
||
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.
Attachment #646560 -
Flags: review?(jmuizelaar) → review-
Flags: needinfo?(jmuizelaar)
Comment 18•12 years ago
|
||
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.
Flags: needinfo?(joe)
Updated•11 years ago
|
Assignee: netzen → nobody
Assignee | ||
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
Updated•8 years ago
|
Comment 20•6 years ago
|
||
No assignee, updating the status.
Comment 21•6 years ago
|
||
No assignee, updating the status.
Comment 22•6 years ago
|
||
No assignee, updating the status.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•