1364627 - NativeFontResourceFontconfig leak when using DrawTargetRecording on Linux

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

https://treeherder.mozilla.org/logviewer.html#?job_id=98542458&repo=try&lineNumber=37676

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: fix leaking of Cairo scaled font when creating ScaledFontFontconfig from a recording

The issue here is that the Cairo scaled font passed to the ScaledFontFontconfig constructor then gets ref'd by cairo_scaled_font_reference inside ScaledFontBase. ScaledFontBase is thus expecting that something else either owned the Cairo scaled font or was going to release the reference to fully hand it off to it if that was the intention.

Thus, we need to to unref (via cairo_scaled_font_destroy) after we pass it in to the constructor. We weren't doing this, so the Cairo scaled fonts never got destroyed, and all bad stuff followed from this.

I'm not sure whether or not to consider this a "regression", since while this was introduced in bug 1309205, we never even supported font recording at all prior to that bug. None the less, this affects versions 53+ when using print-via-parent on Linux, so uplifting this fix is probably a good thing to not leak memory during printing.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e7872cb3b5c
fix leaking of Cairo scaled font when creating ScaledFontFontconfig from a recording. r=jrmuizel

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e7872cb3b5c

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this at your earliest convenience. The RC build is happening on Monday.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: fix leaking of Cairo scaled font when creating ScaledFontFontconfig from a recording (54)

Just a rebased version of the patch for 54, since the surrounding code changed a little bit.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1309205
[User impact if declined]: Memory leaks when printing on Linux with E10s.
[Is this code covered by automated tests?]: Currently printing is not covered by automated testing, which was why this leak managed to sneak in. Tobias Schneider is currently working on fixing that scenario.
[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]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: Just a simple one line fix to free a Cairo font that should have been freed, which gets rid of the leak.
[String changes made/needed]: none

Comment 6

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

Comment on attachment 8873261 [details] [diff] [review]
fix leaking of Cairo scaled font when creating ScaledFontFontconfig from a recording (54)

Seems like a low risk fix for a memleak issue, Beta54+

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a85ff8f8399d

Comment 8

[Andrei Vaida [:avaida]]

(In reply to Lee Salzman [:lsalzman] from comment #5)
> [Is this code covered by automated tests?]: Currently printing is not
> covered by automated testing, which was why this leak managed to sneak in.
> Tobias Schneider is currently working on fixing that scenario.
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no 

Setting qe-verify- based on Lee's assessment on manual testing needs.