This bug was filed from the Socorro interface and is report bp-d00634f2-0c72-4a98-9b57-175652150819. ============================================================= 0 xul.dll _moz_cairo_scaled_font_get_font_options gfx/cairo/cairo/src/cairo-scaled-font.c 1 xul.dll gfxContext::GetRoundOffsetsToPixels(bool*, bool*) gfx/thebes/gfxContext.cpp 2 xul.dll gfxHarfBuzzShaper::SetGlyphsFromRun(gfxContext*, gfxShapedText*, unsigned int, unsigned int, wchar_t const*, hb_buffer_t*, bool) gfx/thebes/gfxHarfBuzzShaper.cpp 3 xul.dll gfxHarfBuzzShaper::ShapeText(gfxContext*, wchar_t const*, unsigned int, unsigned int, int, bool, gfxShapedText*) gfx/thebes/gfxHarfBuzzShaper.cpp 4 @0x7a516060 this graphics crash is already around for a few years, but it is spiking to #5 on the top score board for firefox 40.0.2 at the moment. esr 38.2.0 seems to be affected heavily as well. some of the crash comments seem to point out reproducible cases to crash by visiting certain urls. https://crash-stats.mozilla.com/search/?signature=~_moz_cairo_scaled_font_get_font_options&_facets=signature&_facets=app_notes&_facets=adapter_device_id&_facets=adapter_driver_version&_facets=useragent_locale&_facets=version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

This is a null dereference crash within cairo code when attempting to dereference a null cairo_scaled_font_t ptr. http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-scaled-font.c#2981 This is called from the code in gfxContext::GetRoundOffsetsToPixels: cairo_t *cr = GetCairo(); cairo_scaled_font_t *scaled_font = cairo_get_scaled_font(cr); // Sometimes hint metrics gets set for us, most notably for printing. cairo_font_options_t *font_options = cairo_font_options_create(); cairo_scaled_font_get_font_options(scaled_font, font_options); cairo_hint_metrics_t hint_metrics = cairo_font_options_get_hint_metrics(font_options); cairo_font_options_destroy(font_options); So the question is why is cairo_get_scaled_font() returning null.

Not sure of the exact cause for this condition but I don't think it's critical to fix. For now, just avoid the crash by null checking the returned scaled font object and use the fallback setting for that case.

[Tracking Requested - why for this release]: tracking to get this on the radar of relman as the issue may have quite a straightforward fix. due to crashing on startup around 50% of the time, this is on #5 of the firefox 40.0.2 crash score list. also on firefox 38.2.0esr with 5.65% of all crashes this signature is second on the over all topcrash list.

Thanks, unfortunately, this is too late for 40. However, we would probably be happy to uplift it to beta. John, could you fill the uplift request to aurora & beta? Thanks

Tracking for 41 and 42. I don't see that 42 is affected from crash-stats, though.

Comment on attachment 8653241 [details] [diff] [review] patch, null-check cairo scaled font to prevent crash Approval Request Comment [Feature/regressing bug #]: frequent crasher for 41.b builds [User impact if declined]: crash [Describe test coverage new/current, TreeHerder]: fix landed on central last week [Risks and why]: low risk, fix is limited to error handling [String/UUID change made/needed]: none

Comment on attachment 8653241 [details] [diff] [review] patch, null-check cairo scaled font to prevent crash Patch includes an assert and null check so it seems safe to uplift to Aurora42 and Beta41. Also this crash shows up in 41.0b4, 41.0b3, etc.

John, can you request approval for uplift to ESR38 as well? Sounds like it was a bad crash for esr 38.2.0.

Comment on attachment 8653241 [details] [diff] [review] patch, null-check cairo scaled font to prevent crash [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: crashes will occur Fix Landed on Version: 43 Risk to taking this patch (and alternatives if risky): patch simply catches the error and handles it benignly String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment on attachment 8653241 [details] [diff] [review] patch, null-check cairo scaled font to prevent crash Approved for esr38, should be a low-risk stability fix.

the signature is still showing up in 41 builds: https://crash-stats.mozilla.com/search/?product=Firefox&version=41.0&signature=%3D_moz_cairo_scaled_font_get_font_options&_facets=signature&_facets=user_comments&_facets=platform&_facets=version&_facets=adapter_vendor_id&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Turns out |scaled_font| is not nullptr but rather 0xe0.

(That was also the case in the original crashes)

(In reply to David Major [:dmajor] from comment #17) > Turns out |scaled_font| is not nullptr but rather 0xe0. Does that mean we should reopen this bug?

According to crash stats, we have this bug for a while, it is low volume (#60 in the list of crashes), I am going to untracking but happy to take a patch for it.

The fix from comment 15 is in esr 38.3.0. I don't see this crash signature appearing for 38.3.0 so I'm marking it as fixed on that branch. Philipp if you think i'm wrong do let me know.

Aaaaand I'm wrong. Thanks Philipp! I was searching on 38.3.0 rather than version: 38.3.0esr. We have the problem with bugs like this that we've uplifted, set an approval+ flag, but if we simply re-set the status flag to affected, this bug will keep showing up in the sheriff's lists of bugs that need uplift.

I don't see any crashes reported over the last month that would indicate this affects Firefox 42 or later. Can someone please confirm if this is still an issue?

This isn't pretty but since we haven't been able to clearly identify why/how this occurs, work around the problem by looking for bad address values for the cairo_scaled_font_t ptr returned from cairo_get_scaled_font.

Humm, this doesn't seem to occur on Firefox 42+. Philipp, could you find same signature after Firefox 42?

yes, this particular signature has totally ceased in firefox 42 and above when looking at crash stats.

Comment on attachment 8691776 [details] [diff] [review] patch, work around bad scaled font pointer Review of attachment 8691776 [details] [diff] [review]: ----------------------------------------------------------------- I am not sure that this becomes workaround forever. This issue may be compiler issue. If we should fix this on ESR tree, I cannot find good way to fix this. But if m-c/m-i, we should investigate generated code by compiler.

Since this doesn't appear to be occurring on 42+, I'm not going to land this fix as it's a hack that would be best to avoid. If we see this again and can't track down the root cause, it may be something to consider. Marking the bug as 'leave-open' for now.

(In reply to Makoto Kato [:m_kato] from comment #28) > Comment on attachment 8691776 [details] [diff] [review] > patch, work around bad scaled font pointer > > Review of attachment 8691776 [details] [diff] [review]: > ----------------------------------------------------------------- > > I am not sure that this becomes workaround forever. This issue may be > compiler issue. > > If we should fix this on ESR tree, I cannot find good way to fix this. But > if m-c/m-i, we should investigate generated code by compiler. John, Makoto does that mean i should wait with uplifting this to esr38 ?

(In reply to Carsten Book [:Tomcat] from comment #30) > (In reply to Makoto Kato [:m_kato] from comment #28) > > Comment on attachment 8691776 [details] [diff] [review] > > patch, work around bad scaled font pointer > > > > Review of attachment 8691776 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I am not sure that this becomes workaround forever. This issue may be > > compiler issue. > > > > If we should fix this on ESR tree, I cannot find good way to fix this. But > > if m-c/m-i, we should investigate generated code by compiler. > > John, Makoto does that mean i should wait with uplifting this to esr38 ? This issue still occurs on 38.5.2esr and there is 5-6 crashes per day. Humm, although it will be fixed by this fix, root cause may be another issue due to no crash on 44+. Now since I don't have top 50 crash list on 38.5.2esr, I cannot know this issue is top 50. :Tomcat, is this high volume crash? If so, we should land to esr too.

(In reply to Makoto Kato [:m_kato] from comment #31) > :Tomcat, is this high volume crash? If so, we should land to esr too. oh good question, ritu might know better :)

As such this signature is a low volume crash. In 28 days of data, I see total ~175 instances of this crash. This link shows ESR38.5 crashes https://crash-stats.mozilla.com/search/?product=Firefox&version=38.5.0&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature and I don't see this bug/signature in that list. I think if the patch is not risky, we could uplift to esr38.6. Otherwise we do not have any crash data that suggests we uplift a risky patch to esr38 for this one. Hope that helps!

I am closing this bug as WONTFIX as this signature is only significant for users on Firefox ESR 38 which I believe is now EOL. Please reopen this bug report if you are experiencing this crash on Firefox 45 or later. If you are on Firefox 44 or earlier, particularly Firefox ESR 38, please update to Firefox 45.

I hit this on nightly just now. https://crash-stats.mozilla.com/report/index/4164ae98-5bbe-4422-aac8-eba272160430

Crash volume for signature '_moz_cairo_scaled_font_get_font_options': - nightly (version 50): 54 crashes from 2016-06-06. - aurora (version 49): 66 crashes from 2016-06-07. - beta (version 48): 0 crash from 2016-06-06. - release (version 47): 1 crash from 2016-05-31. - esr (version 45): 0 crash from 2016-04-07. Crash volume on the last weeks: Week N-1 Week N-2 Week N-3 Week N-4 Week N-5 Week N-6 Week N-7 - nightly 0 1 1 46 0 4 1 - aurora 18 6 4 7 13 14 0 - beta 0 0 0 0 0 0 0 - release 0 0 1 0 0 0 0 - esr 0 0 0 0 0 0 0 Affected platform: Windows

Crash volume for signature '_moz_cairo_scaled_font_get_font_options': - nightly (version 51): 1 crash from 2016-08-01. - aurora (version 50): 20 crashes from 2016-08-01. - beta (version 49): 287 crashes from 2016-08-02. - release (version 48): 0 crashes from 2016-07-25. - esr (version 45): 0 crashes from 2016-05-02. Crash volume on the last weeks (Week N is from 08-22 to 08-28): W. N-1 W. N-2 W. N-3 - nightly 1 0 0 - aurora 9 3 1 - beta 113 86 32 - release 0 0 0 - esr 0 0 0 Affected platform: Windows Crash rank on the last 7 days: Browser Content Plugin - nightly - aurora #200 #80 - beta #163 #247 - release - esr

hi, this crash signature is reappearing in firefox 49 in greater numbers again. it's currently making up 0.3% of all browser crashes on 49.01. do we need the patch from comment #28 after all or is it a different issue now & it's better to open a new bug report for this?

(In reply to [:philipp] from comment #38) > hi, this crash signature is reappearing in firefox 49 in greater numbers > again. it's currently making up 0.3% of all browser crashes on 49.01. > > do we need the patch from comment #28 after all or is it a different issue > now & it's better to open a new bug report for this? I cannot find root cause for this yet. I think that this seems to be compiler issue (PGO?) and we turn on SSE2 from Firefox 49. When I look cairo code for this, there is no condition that this pointer is invalid access. If this is our code bug, Firefox x64 will crash as same signature. But there is x86 only. OK, I will investigate this (including compiler bug)...

Crash volume for signature '_moz_cairo_scaled_font_get_font_options': - nightly (version 52): 0 crashes from 2016-09-19. - aurora (version 51): 4 crashes from 2016-09-19. - beta (version 50): 73 crashes from 2016-09-20. - release (version 49): 1400 crashes from 2016-09-05. - esr (version 45): 2 crashes from 2016-06-01. Crash volume on the last weeks (Week N is from 10-03 to 10-09): W. N-1 W. N-2 - nightly 0 0 - aurora 4 0 - beta 60 13 - release 1090 310 - esr 0 0 Affected platform: Windows Crash rank on the last 7 days: Browser Content Plugin - nightly - aurora #588 #392 - beta #255 #1224 - release #46 #76 - esr

0:052> dd xul!_cairo_scaled_font_nil_objects+0x190 5b0d8288 0000c000 0000c000 000000e0 000000e0 Maybe, cairo_status_t parameter of _cairo_scaled_font_create_in_error is CAIRO_INT_STATUS_*, not CAIRO_STATUS_*. So it causes overflow access, so we cannot get correct dummy scaled_font.

So, this crash depends on compiler optimization...

Comment on attachment 8797915 [details] [diff] [review] Part 1. cairo_scaled_font_create_in_error should return nil font even if status is cairo_int_status_t status of cairo_scaled_font_t isn't often the range of cairo_status_t such as CAIRO_INT_STATUS_UNSUPPORTED. If status is CAIRO_INT_STATUS_*, _cairo_scaled_font_create_in_error will return invalid pointer instead of nil font object. So If so, we should return nil font.

Comment on attachment 8797915 [details] [diff] [review] Part 1. cairo_scaled_font_create_in_error should return nil font even if status is cairo_int_status_t Review of attachment 8797915 [details] [diff] [review]: ----------------------------------------------------------------- If the value isn't guaranteed to be within the range of cairo_status_t, then shouldn't we also change the parameter to _cairo_scaled_font_create_in_error to be a cairo_int_status_t instead, to make it clear that it may have the larger range? And following that trail a little further, whatever the source of that value was (the status field in a cairo_font_face_t?) ought to be declared as cairo_int_status_t, too. In principle, I don't think it's safe to store/pass cairo_int_status_t values as cairo_status_t variables/fields/parameters, as the compiler would be within its rights to discard the high bits as being outside the enum's range.

How about crashing deliberately in _cairo_scaled_font_create_in_error when this happens, so that we get a stack for the source of these values?

(In reply to Jonathan Kew (:jfkthame) from comment #46) > Comment on attachment 8797915 [details] [diff] [review] > Part 1. cairo_scaled_font_create_in_error should return nil font even if > status is cairo_int_status_t > > Review of attachment 8797915 [details] [diff] [review]: > ----------------------------------------------------------------- > > If the value isn't guaranteed to be within the range of cairo_status_t, then > shouldn't we also change the parameter to _cairo_scaled_font_create_in_error > to be a cairo_int_status_t instead, to make it clear that it may have the > larger range? And following that trail a little further, whatever the source > of that value was (the status field in a cairo_font_face_t?) ought to be > declared as cairo_int_status_t, too. In principle, I don't think it's safe > to store/pass cairo_int_status_t values as cairo_status_t > variables/fields/parameters, as the compiler would be within its rights to > discard the high bits as being outside the enum's range. cairo_status_t's maximum is 46, but cairo_int_status_t is from 100. So it isn't good to add nil table for cairo_int_status_t simply. _cairo_scaled_glyph_lookup and backend->text_to_glyphs can return CAIRO_INT_STATUS_UNSUPPORTED as backend error. But some codes set it to _cairo_scaled_font_set_error as cairo_status_t. Also, on debug build, it will hit assertion by _cairo_error() / _cairo_status_is_error(). Actually, there is no generic error code for backend in cairo_status_t. Should I convert to CAIRO_STATUS_FONT_TYPE_MISMATCH or new error code such as CAIRO_STATUS_GLYPH_ERROR like bug 777703? Or Should I generate nil table for cairo_int_status_t too?

(In reply to :dmajor from comment #47) > How about crashing deliberately in _cairo_scaled_font_create_in_error when > this happens, so that we get a stack for the source of these values? _cairo_create_in_error (not scaled_font) can already crash by bug 614772. But it is clear that some code sets CAIRO_INT_STATUS_UNSUPPORTED to scaled_font's status. Also, on debug build, it will hit assertion.

It seems like it would be better to find why the code is setting CAIRO_INT_STATUS_UNSUPPORTED, and to prevent the situation that leads to it, than to bandage around it at a later point in time.

Crash volume for signature '_moz_cairo_scaled_font_get_font_options': - nightly (version 52): 2 crashes from 2016-09-19. - aurora (version 51): 16 crashes from 2016-09-19. - beta (version 50): 218 crashes from 2016-09-20. - release (version 49): 3855 crashes from 2016-09-05. - esr (version 45): 2 crashes from 2016-07-25. Crash volume on the last weeks (Week N is from 10-17 to 10-23): W. N-1 W. N-2 W. N-3 W. N-4 - nightly 0 2 0 0 - aurora 4 4 4 0 - beta 59 57 60 13 - release 1068 1010 1090 310 - esr 0 0 0 0 Affected platform: Windows Crash rank on the last 7 days: Browser Content Plugin - nightly - aurora #637 #164 - beta #341 #228 - release #58 #85 - esr

:jmuizelaar, I have a question since this is related to bug 777703. cairo glyph's backend sometimes use CAIRO_INT_STATUS_UNSUPPORTED as backend error, then it sets CAIRO_INT_STATUS_UNSUPPORTED as cairo_status_t unfortunately. After that, Firefox crashes by return value by _cairo_scaled_font_create_in_error() since cairo_status_t is cairo_int_status_t. Should we support cairo_int_status_t's error code in _cairo_scaled_font_create_in_error? Or should we store new error status like bug 777703's proposal fix?

(In reply to Makoto Kato [:m_kato] from comment #52) > :jmuizelaar, I have a question since this is related to bug 777703. > > cairo glyph's backend sometimes use CAIRO_INT_STATUS_UNSUPPORTED as backend > error, then it sets CAIRO_INT_STATUS_UNSUPPORTED as cairo_status_t > unfortunately. After that, Firefox crashes by return value by > _cairo_scaled_font_create_in_error() since cairo_status_t is > cairo_int_status_t. We should figure out when CAIRO_INT_STATUS_UNSUPPORTED is happening, and see if we can better handle the error there. > Should we support cairo_int_status_t's error code in > _cairo_scaled_font_create_in_error? No > Or should we store new error status > like bug 777703's proposal fix? Possibly, but I'd like to better understand what's causing the error.

Crash volume for signature '_moz_cairo_scaled_font_get_font_options': - nightly (version 53): 26 crashes from 2016-11-14. - aurora (version 52): 65 crashes from 2016-11-14. - beta (version 51): 657 crashes from 2016-11-14. - release (version 50): 6800 crashes from 2016-11-01. - esr (version 45): 2 crashes from 2016-07-06. Crash volume on the last weeks (Week N is from 01-02 to 01-08): W. N-1 W. N-2 W. N-3 W. N-4 W. N-5 W. N-6 W. N-7 - nightly 0 1 8 17 0 0 0 - aurora 2 27 7 11 5 13 0 - beta 86 86 83 132 112 101 30 - release 981 1063 1153 1144 990 859 275 - esr 0 0 0 0 0 0 0 Affected platform: Windows Crash rank on the last 7 days: Browser Content Plugin - nightly #2359 #127 - aurora #296 #114 - beta #206 #95 - release #62 #100 - esr

STR: open the PDF attached. CR: https://crash-stats.mozilla.com/report/index/adb76950-b5e4-4e26-a96a-12f412170217

Too late for firefox 52, mass-wontfix.

The leave-open keyword is there and there is no activity for 6 months. :m_kato, maybe it's time to close this bug?

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #58) > The leave-open keyword is there and there is no activity for 6 months. > :m_kato, maybe it's time to close this bug? This was already fixed by another bugs. No crash on 52+ from crash reporter