Closed Bug 1304699 Opened 4 years ago Closed 4 years ago

Firefox Crash @ gfxFont::SpaceMayParticipateInShaping

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- fixed
firefox51 + fixed
firefox52 + fixed

People

(Reporter: cbook, Assigned: jfkthame)

References

()

Details

(Keywords: crash, Whiteboard: [gfx-noted])

Crash Data

Attachments

(3 files)

Attached file 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
[Tracking Requested - why for this release]:

bughunter real world find + crashes opt builds
Tracking 52+ since this is a reproducible crash. Adding crash signature to crash field.
Crash Signature: [@ gfxFont::SpaceMayParticipateInShaping]
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.
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?
Flags: needinfo?(cbook)
Priority: -- → P3
Whiteboard: [gfx-noted]
(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
Flags: needinfo?(cbook)
Track for 51+ as the crash is still reproducible.
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.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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?
Attachment #8794782 - Flags: review?(jmuizelaar) → review+
(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....
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
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
https://hg.mozilla.org/mozilla-central/rev/ca3ca7a3befa
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi :jfkthame,
Since this also affects 51, do you think the patch is worth uplifting to 51?
Flags: needinfo?(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.
Flags: needinfo?(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]
Attachment #8794782 - Flags: approval-mozilla-esr45?
Attachment #8794782 - Flags: approval-mozilla-beta?
Attachment #8794782 - Flags: approval-mozilla-aurora?
Sorry but it doesn't match the esr criteria and the volume is very low anyway.
Attachment #8794782 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45-
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+
Attachment #8794782 - Flags: approval-mozilla-beta?
Attachment #8794782 - Flags: approval-mozilla-beta+
Attachment #8794782 - Flags: approval-mozilla-aurora?
Attachment #8794782 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.