Crash in premultiply_data

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: calixte, Assigned: jfkthame)

Tracking

(Blocks 1 bug, {crash, regression})

57 Branch
mozilla58
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [clouseau][gfx-noted], crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is 
report bp-a7116c93-d902-4023-b592-dbf3c0170918.
=============================================================

There are 5 crashes in nightly 57 starting with buildid 20170918100058. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1400602.

[1] https://hg.mozilla.org/mozilla-central/rev?node=c4f1d0d1852b6e61c7a3e78ec8c0e33879c20d95
Flags: needinfo?(jfkthame)
I'd have no problem backing out to a previous version of FreeType.  A bit risky changing something that size this late.
Assignee: nobody → jfkthame
Flags: needinfo?(ryanvm)
Priority: -- → P1
Whiteboard: [clouseau] → [clouseau][gfx-noted]
Per our IRC discussion, we'll let this sit for a couple days and make the call on Monday before b3 gtb. Jonathan did say in the other bug that the patch was mostly documentation updates, so it looks a lot bigger than it actually was, FWIW.
The crash is happening in PNG-related code (meaning it most likely occurs when we're trying to use a color-emoji font, though my attempts to reproduce it with Noto Color Emoji have so far been fruitless; possibly it's being triggered by some other color font that some users have), and specifically it is in a chunk of code that was added in 2.8.1 to load color fonts faster thanks to a vectorized speedup.[1]

I haven't managed to reproduce the crash locally, so I'm not sure exactly what is triggering the problem, but I think our safest/best option is to back out the 2.8.1 update from Fennec 57 (even though that still leaves the possibility that users on Linux systems with the new FreeType installed could encounter the issue).


[1] http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/src/sfnt/pngshim.c?id=c9d7c03fa1b5c6244759ade814c546da04080646
Flags: needinfo?(jfkthame)
I filed https://savannah.nongnu.org/bugs/index.php?52092 to inform the FreeType project about this, although lacking a specific testcase it's a rather vague report. :(
I've identified what I think may be the problem in the freetype code, and submitted a suggested patch for the upstream devs to consider. If they agree with the diagnosis, it'll be a trivial fix and we could consider cherry-picking it as soon as it's confirmed from their side.
This has landed upstream, so will be in the next freetype release, but I'd suggest we cherry-pick it immediately to resolve the crash here.
Attachment #8911577 - Flags: review?(milan)
BTW, although the patch looks a bit big/messy in splinter, it's actually rather trivial -- it just adds an "if (row_info->rowbytes > 15) { ... }" condition around the vector code. All the rest is just the resulting whitespace changes because of the resulting indentation change.
Attachment #8911577 - Flags: review?(milan) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e63c4c186787
Cherry-pick patch 0d1262a41e019e4511071e339bb8aa018596a1fd from upstream freetype to avoid potential crash in premultiply_data. r=milan
https://hg.mozilla.org/mozilla-central/rev/e63c4c186787
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
No crashes reported from today's nightlies, and looking at the frequency we'd been seeing, I think we would have by now if this wasn't fixed. I think we're good to request Beta uplift on this :)
Flags: needinfo?(ryanvm) → needinfo?(jfkthame)
Comment on attachment 8911577 [details] [diff] [review]
Cherry-pick patch 0d1262a41e019e4511071e339bb8aa018596a1fd from upstream freetype to avoid potential crash in premultiply_data

Approval Request Comment
[Feature/Bug causing the regression]: freetype update in bug 1400602
[User impact if declined]: possible crash when trying to render color fonts (Android)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: crash-stats indicates crashes have stopped
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: adds a simple test to bypass the newly-added optimization when the glyph is too small to safely do it
[String changes made/needed]: none
Flags: needinfo?(jfkthame)
Attachment #8911577 - Flags: approval-mozilla-beta?
Comment on attachment 8911577 [details] [diff] [review]
Cherry-pick patch 0d1262a41e019e4511071e339bb8aa018596a1fd from upstream freetype to avoid potential crash in premultiply_data

Fix a crash, taking it.
Should be in 47b4, gtb tomorrow Thursday
Attachment #8911577 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.