1686559 - [@ CFRelease.cold.1 | CFRelease | mozilla::gfx::NativeFontResourceMac::Create] - Firefox 84.0.2 Crash Report - Report ID: f027f97b-aec3-4b09-bc22-cdee00210113

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[⚜️Québec🇨🇦Canada🍁]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:84.0) Gecko/20100101 Firefox/84.0

Steps to reproduce:

[@ CFRelease.cold.1 | CFRelease | mozilla::gfx::NativeFontResourceMac::Create] - Firefox 84.0.2 Crash Report - Report ID: f027f97b-aec3-4b09-bc22-cdee00210113

Actual results:

https://crash-stats.mozilla.org/report/index/f027f97b-aec3-4b09-bc22-cdee00210113

Expected results:

Fx should not crash

Comment 1

[Daniel Veditz [:dveditz]]

All the crashes with this signature are using Firefox 84.0.x and either MacOS 10.15 or 10.16

Does the bad instruction mean the code wandered into something broken in the Mac libraries themselves, or are we handling the font wrong? Our code looks like we're correctly holding a reference so maybe this is an Apple problem:
https://hg.mozilla.org/releases/mozilla-release/file/7e22d68e1ebfc0839092237feeefad46cfbd8651/gfx/2d/NativeFontResourceMac.cpp#l110

Any ideas Jonathan?

Comment 2

[⚜️Québec🇨🇦Canada🍁]

(In reply to Daniel Veditz [:dveditz] from comment #1)

All the crashes with this signature are using Firefox 84.0.x and either MacOS 10.15 or 10.16

Does the bad instruction mean the code wandered into something broken in the Mac libraries themselves, or are we handling the font wrong? Our code looks like we're correctly holding a reference so maybe this is an Apple problem:
https://hg.mozilla.org/releases/mozilla-release/file/7e22d68e1ebfc0839092237feeefad46cfbd8651/gfx/2d/NativeFontResourceMac.cpp#l110

Any ideas Jonathan?

It took me so long😱 to wait and was so hard for me to get this crash report.
The Firefox would freeze forever😓 but never crash and generate a report.

Comment 3

[Jonathan Kew [:jfkthame]]

I think Lee and/or Jeff have been dealing with this code recently, so perhaps they will have some more insight.

One thing I notice about the code here is that we don't always check API calls for failure. Both CFDataCreateWithBytesNoCopy and CTFontManagerCreateFontDescriptorFromData, at least, could return NULL according to the Apple docs, so we ought to be checking them.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1686559 - Add null-checks for API calls that may be fallible. r?jrmuizel

Comment 5

[Jonathan Kew [:jfkthame]]

(Note that I have no idea whether adding these error checks will make any difference here.)

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 9197063 [details]
Bug 1686559 - Add null-checks for API calls that may be fallible. r?jrmuizel

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unclear; we have no STR and don't really know what is triggering the crash here. This patch just aims to improve robustness based on code inspection.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 84+
• If not all supported branches, which bug introduced the flaw?: 1674175 (maybe)
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should be trivial
• How likely is this patch to cause regressions; how much testing does it need?: Minimal regression risk, just adds error-checking to more safely bail out on possible failures

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9197063 [details]
Bug 1686559 - Add null-checks for API calls that may be fallible. r?jrmuizel

approved to land and request uplift

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/1a524fd3193cc7f3a1a8d04c164d8b91bdadf985

Comment 9

[Ryan VanderMeulen [:RyanVM]]

We're going to want a Beta uplift request on this I think since we're not likely to see any impact on crash reports until this patch gets to release.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 9197063 [details]
Bug 1686559 - Add null-checks for API calls that may be fallible. r?jrmuizel

Beta/Release Uplift Approval Request

• User impact if declined: Possible crashes due to failure within OS font APIs
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: No known STR, we just have crash-stats reports
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just adds some missing error-checks to handle possible failures.
• String changes made/needed:

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9197063 [details]
Bug 1686559 - Add null-checks for API calls that may be fallible. r?jrmuizel

approved for 85.0b9

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/81b247799f55

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a524fd3193c