1304699 - Firefox Crash @ gfxFont::SpaceMayParticipateInShaping

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

Found via bughunter and reproduced on the latest aurora opt build and tinderbox trunk debug windows build.

Steps to reproduce:
-> Load http://neko-san.fr/anime
--> debug and opt crash - affects also 51 

opt-crash id https://crash-stats.mozilla.com/report/index/dd080547-b5ad-49a1-94dc-d00122160922

Comment 1

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

bughunter real world find + crashes opt builds

Comment 2

[Carsten Book [:Tomcat]]

Attachment: about:support graphics section if needed

Comment 3

[Marcia Knous [:marcia]]

Tracking 52+ since this is a reproducible crash. Adding crash signature to crash field.

Comment 4

[Jonathan Kew [:jfkthame]]

Wow, that page (http://neko-san.fr/anime) sure is horrible.... it throws all kinds of scam/malware pages at me, trying to convince me to clear my computer of viruses ("if you close this page, your hard drive will be deleted") and "update" my Firefox with a "security improvement". Nasty.

So far, though, it hasn't crashed my browser. I'll try a debug build and see what shows up.

Comment 5

[Jonathan Kew [:jfkthame]]

My debug build doesn't crash either (or assert, or anything interesting really...)

Does it still reproducibly crash for you? I wonder if that page always delivers the same content, or if there's a bunch of dynamic junk in the ads etc that makes it unpredictable?

Comment 6

[Carsten Book [:Tomcat]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> My debug build doesn't crash either (or assert, or anything interesting
> really...)
> 
> Does it still reproducibly crash for you? I wonder if that page always
> delivers the same content, or if there's a bunch of dynamic junk in the ads
> etc that makes it unpredictable?

yeah still reproducible on aurora opt build like https://crash-stats.mozilla.com/report/index/390b16c2-d5bd-48ce-88db-fcf652160923

Comment 7

[Gerry Chang [:gchang]]

Track for 51+ as the crash is still reproducible.

Comment 8

[Jonathan Kew [:jfkthame]]

OK, I think I got it. This reproduces on Windows if h/w acceleration is disabled (either manually, or because of incompatible hardware/drivers), so that the GDI font backend is in use. The underlying bug isn't specific to that backend, though, it's just that the GDI subsystem makes it more likely to manifest as a crash.

The crash is occurring because we're trying to shape text with a null gfxFont, which is never going to end well. The null gfxFont is the |smallCapsFont| that we got at https://dxr.mozilla.org/mozilla-central/rev/29beaebdfaccbdaeb4c1ee5a43a9795ab015ef49/gfx/thebes/gfxFont.cpp#310, which is supposed to be a reduced-size version of the current font, for synthetic small-caps styling.

So as a first line of defence, we should probably include a check here that GetSmallCapsFont() didn't return null, and fall back to using |this| if it does (which would result in incorrect styling -- full-size caps instead of reduced-size ones -- but that's better than crashing).

It turns out that's not really enough, though; it prevents _this_ crash, but the page still ends up crashing, just in a different place, after firing various critical-sounding GFX assertions. Basically, GDI has run out of resources, and various graphics APIs start failing drastically.

The real question, then, is why we're exhausting GDI resources to the point that GetSmallCapsFont() is failing. And the answer is... unicode-range doesn't play well with the global font cache.

When we're using a @font-face resource where unicode-range was specified, we fail to pass the proper unicode-range when looking up the gfxFont in the cache at https://dxr.mozilla.org/mozilla-central/rev/29beaebdfaccbdaeb4c1ee5a43a9795ab015ef49/gfx/thebes/gfxFontEntry.cpp#283, with the result that we never find a hit, and always instantiate a new gfxFont.

That would be kinda OK (although inefficient), if it weren't for the fact that we don't immediately delete fonts when they're replaced in the cache; we use a deferred-expiration scheme so that recently-used fonts can be resurrected rather than having to create them afresh.

But in this case, where we've got GetSmallCapsFont() requesting the same font for each of numerous fragments of text, and the cache lookup failing because we don't pass the unicode-range, this means we keep instantiating new fonts long before the old copies have been freed, and in the end, GDI gives up on us.

The fix, then, is simple: gfxFontEntry::FindOrMakeFont needs to pass the proper unicode-range map when looking up the font. This avoids the reckless accumulation of identical font instances, and hence prevents the crash here.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Pass the proper unicode-range when looking up a font in the global font cache

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8794782 [details] [diff] [review]
Pass the proper unicode-range when looking up a font in the global font cache

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

Is there some additional defense that we should have in place to prevent this kind of thing from happening? Can we count how many open resources that we have and expose this in about:memory?

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> Comment on attachment 8794782 [details] [diff] [review]
> Pass the proper unicode-range when looking up a font in the global font cache
> 
> Review of attachment 8794782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there some additional defense that we should have in place to prevent
> this kind of thing from happening? Can we count how many open resources that
> we have and expose this in about:memory?

That would be nice, but I don't know of a clean/supported way to do it.... presumably getting the GdiSharedHandleTable and looking through its entries is a possibility, but that's undocumented and has changed (I believe) across Windows versions, so it could be a bit painful. Task Manager used to be able to show a count of GDI objects per process, but I don't see that option in Win10. (Maybe I'm just not looking in the right place?)

Presumably the font-cache figure in about:memory would have increased significantly in the particular example here, but the user isn't going to get a chance to even look at it once they're loading the page that leads to runaway font instantiation and a crash....

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66b8ea362db884bd5d74937cf294395fc436999a
Bug 1304699 - Pass the proper unicode-range when looking up a font in the global font cache. r=jrmuizel

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b0bc3eb907ae for https://treeherder.mozilla.org/logviewer.html#?job_id=36492319&repo=mozilla-inbound

Comment 14

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca3ca7a3befafc0c9eb189a9d08f305e368308c8
Bug 1304699 - Pass the proper unicode-range when looking up a font in the global font cache. r=jrmuizel

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca3ca7a3befa

Comment 16

[Gerry Chang [:gchang]]

Hi :jfkthame,
Since this also affects 51, do you think the patch is worth uplifting to 51?

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Gerry Chang [:gchang] from comment #16)
> Hi :jfkthame,
> Since this also affects 51, do you think the patch is worth uplifting to 51?

Yes, I think so; it's a very safe patch for a real-world crash.

This affects all current branches (not only 51), as the original bug comes from the unicode-range support landed in bug 475891 (FF36), and enabled by default in bug 1119062 (FF 44), so we may want to take it on beta and esr45 as well.

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8794782 [details] [diff] [review]
Pass the proper unicode-range when looking up a font in the global font cache

Approval Request Comment
[Feature/regressing bug #]: 1119062 (enabled unicode-range support in @font-face, which exposed the implementation bug from 475891)

[User impact if declined]: potential crash, particularly affecting Windows GDI users

[Describe test coverage new/current, TreeHerder]: crash reproduced locally with site from the original report, and patched build verified to no longer crash

[Risks and why]: minimal, simple patch to make the font cache work as intended instead of constantly creating new font instances

[String/UUID change made/needed]: none

[Approval Request Comment]

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Sorry but it doesn't match the esr criteria and the volume is very low anyway.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8794782 [details] [diff] [review]
Pass the proper unicode-range when looking up a font in the global font cache

This crash is very low volume but I agree with Jonathan's assessment that this is a low risk fix, Aurora51+, Beta50+

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5449a2057604

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6d739e6ff883