Closed Bug 1331683 Opened 6 years ago Closed 6 years ago

Crash in CoreText when opening dropbox PNG


(Core :: Graphics: Text, defect, P3)




Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 + fixed
firefox54 --- fixed


(Reporter: jdm, Assigned: jfkthame)




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

Crash Data


(2 files)

Keywords: crash
Looks like only crash on MacOS 10.9
Priority: -- → P3
Whiteboard: [gfx-noted]
This looks linked to the support for variation fonts (bug 1321031), which touches the font backend code even though it's preffed off at the CSS level.

We should probably put the back-end support here behind an OS version check so that it's completely bypassed on pre-Sierra systems. I have heard that Core Text is buggy in this area...
[Tracking Requested - why for this release]:
Crashing on older MacOS releases.
Tracking 53+ so perhaps we can implement an OS version check.
Josh, there's a build in progress at ... when it's ready, could you please give it a try and confirm whether it avoids the crash for you? Thanks!
Flags: needinfo?(josh)
I got this crash using that build:
Flags: needinfo?(josh)
Huh, interesting. Maybe it's not directly due to the variation-font APIs, then, or maybe I missed a place where I should have added a version check. I'll look again.
Could you please use mozregression to pinpoint the Nightly build that first exhibits a crash on this URL? Note that the exact crash signature will probably have changed with the landing of bug 1321031 (landed on mozilla-central 2017-01-07), assuming it was happening then, but did the URL load OK before that and start crashing afterwards, or did it start some other time?
Flags: needinfo?(josh)
Josh, I've started another try build with an attempt to work around this; when it's ready, could you please give it a spin and see whether the crash still happens? Thanks!
Flags: needinfo?(josh)
I crash when loading the URL, but with a JS GC-related stack this time:
Flags: needinfo?(josh)
Sigh...trivial typo (lack of a negation) in the patch last time. OK, here's another try push:

I confirmed that this prevents the crash for me on a system running 10.10, at least; I don't have access to a 10.9 machine here at the moment.

Josh, when the try build is ready, could you please check it on your 10.9 system? Thanks.
Flags: needinfo?(josh)
It works!
Flags: needinfo?(josh)
This is ugly, but appears to be necessary; even though with the CSS feature preffed off, we never set any variation values on the fonts we're using, merely calling a function like CGFontCopyVariations on older MacOSX versions can result in a crash deep inside CoreText. :( So this patch checks that we're running on Sierra (or later), and if not, short-circuits those calls altogether. Exposing a version of nsCocoaFeatures::OnSierraOrLater() as a global C function seems particularly hacky here, but we need something like that for the Cairo callsite; I didn't want to entirely re-implement the OS version check there.
Attachment #8835363 - Flags: review?(jmuizelaar)
Assignee: nobody → jfkthame
Duplicate of this bug: 1338028
Comment on attachment 8835363 [details] [diff] [review]
Don't attempt to use any Core Text and Core Graphics variation-font APIs on pre-Sierra systems

Review of attachment 8835363 [details] [diff] [review]:

Looks ok to me. Getting additional review from Lee because of Skia
Attachment #8835363 - Flags: review?(lsalzman)
Attachment #8835363 - Flags: review?(jmuizelaar)
Attachment #8835363 - Flags: review+
Duplicate of this bug: 1335522
Attachment #8835363 - Flags: review?(lsalzman) → review+
Bug 1331683 - Don't attempt to use any Core Text and Core Graphics variation-font APIs on pre-Sierra systems. r=jrmuizel,lsalzman
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Here's a minimal crashtest for this bug: with a build prior to the patch here, it consistently dies with an exception deep inside CoreText on 10.9/10.10, and with a patched build it loads fine.
Attachment #8836317 - Flags: review?(jmuizelaar)
Comment on attachment 8835363 [details] [diff] [review]
Don't attempt to use any Core Text and Core Graphics variation-font APIs on pre-Sierra systems

Approval Request Comment
[Feature/Bug causing the regression]: bug 1321031
[User impact if declined]: crashes on certain pages for users on older MacOSX versions
[Is this code covered by automated tests?]: crashtest just posted, will uplift together with the patch
[Has the fix been verified in Nightly?]: yes
[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?]: just reverts to pre-existing code paths on older OS versions, to avoid touching buggy APIs
[String changes made/needed]: none
Attachment #8835363 - Flags: approval-mozilla-aurora?
Attachment #8836317 - Flags: review?(jmuizelaar) → review+
The issue is resolved for Nightly 54.0a1 (2017-02-11) on OSX 10.9.5. The tab/window does not crash on anymore.
Crash Signature: [@ libsystem_kernel.dylib@0x15866 ] → [@ libsystem_kernel.dylib@0x15866 ] [@ CrashReporter::TerminateHandler ]
Comment on attachment 8835363 [details] [diff] [review]
Don't attempt to use any Core Text and Core Graphics variation-font APIs on pre-Sierra systems

Fix a crash. Aurora53+.
Attachment #8835363 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.