Fix memory leak in VerticalMetricsTable #409
Conversation
This PR fixes memory leak in VerticalMetricsTable
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #409 +/- ##
=====================================
Coverage 86% 86%
=====================================
Files 238 238
Lines 15255 15257 +2
Branches 2108 2108
=====================================
+ Hits 13234 13240 +6
+ Misses 1550 1546 -4
Partials 471 471
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -47,7 +47,10 @@ internal short GetTopSideBearing(int glyphIndex) | |||
| return null; | |||
| } | |||
|
|
|||
| return Load(binaryReader, headTable.NumberOfVMetrics, profileTable.GlyphCount); | |||
| using (binaryReader) | |||
There was a problem hiding this comment.
Nice job finding this! Did you find it profiling or just by chance?
There was a problem hiding this comment.
I've used Roslyn analyzer, there are some other issues, but due to used patterns in Fonts/ImageSharp I still investigating if it can be fixed (there are some non-sealed classes with non-virtual Dispose and some classes has a local variables, where class is not an owner of it). For. ex., FontReader class has a variable stream, but it has no Dispose, but some other classes implements it
There was a problem hiding this comment.
I'd be careful with that analyzer, I've seen many, many false positives.
FontReader doesn't own the input stream so shouldn't be responsible for its lifecycle. It's perfectly fine for the type to use a private variable to hold the reference.
There was a problem hiding this comment.
Yes, it'll should be used carefully, because of such FPs
There was a problem hiding this comment.
I'll prepare some interesting cases and will add an appropriate discussion if needed, still need some time to research all warnings...
This PR fixes memory leak in
VerticalMetricsTable