Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using a font resource with FontFamily will cause UnmanagedMemoryStream leaks #746

Closed
mgnslndh opened this issue Jul 12, 2017 · 8 comments
Closed
Labels
bug wiki This item has wiki changes
Milestone

Comments

@mgnslndh
Copy link
Contributor

Background

This is a known WPF issue but still affects applications created with Material Design XAML Toolkit since it relies on the Roboto font. See https://stackoverflow.com/a/31452979. The answer at StackOverflow has a broken link to a issue at Microsoft Connect. I have not been able to find out if this bug is going to be fixed. I have not found any official comments about this issue.

According to my observations the leaked object UnmanagedMemoryStream is very small in size, about 32 bytes. This makes it almost invisible to the memory footprint of many applications. I observed this leak in an application with many bindings which are updated frequently. The application is not expected to be restarted often. These traits would lead to huge amounts of UnmanagedMemoryStream being created over time.

How to reproduce

You need Visual Studio Diagnostics Tool, dotMemory or another memory profiler to observer the memory leak. I have used both Visual Studio Diagnostics Tool and dotMemory to track down the leak and confirm it has to do with font loading.

Visual Studio Diagnostics Tool

  1. Start demo project
  2. Take snapshot 1
  3. Navigate to "Fields" page
  4. Take snapshot 2
  5. Change text field "Name"
  6. Take snapshot 3
  • Snapshot 1 will have about 162 instances of UnmanagedMemoryStream
  • Snapshot 2 will have about 36 new instances of UnmanagedMemoryStream
  • Snapshot 3 will have about 3 new instances of UnmanagedMemoryStream

Workaround

The workaround suggested by Israel Altar is to use file based fonts instead of resource based fonts. I have confirmed that this workaround "works on my machine".

  1. Create a static FontFamily instance:
public class MaterialDesignFonts
{
    public static FontFamily Roboto { get; }

    static MaterialDesignFonts()
    {
        var fontPath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, @"Resources\Roboto\");            
        Roboto = new FontFamily(new Uri($"file:///{fontPath}"), "./#Roboto");            
    }
}
  1. Add the Roboto fonts to the project.

Add the fonts with "Build Action" None and "Copy to Output Directory" Copy if newer

<None Include="Resources\Roboto\Roboto-Black.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-BlackItalic.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-Bold.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-BoldItalic.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-Italic.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-Light.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-LightItalic.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-Medium.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-MediumItalic.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-Regular.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-Thin.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\Roboto-ThinItalic.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\RobotoCondensed-Bold.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\RobotoCondensed-BoldItalic.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\RobotoCondensed-Italic.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\RobotoCondensed-Light.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\RobotoCondensed-LightItalic.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None> 
<None Include="Resources\Roboto\RobotoCondensed-Regular.ttf"> 
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> 
</None>  
  1. Change the FontFamily attributes on your Window types. This is the only place I use the Roboto font in my application:
FontFamily="{x:Static local:MaterialDesignFonts.Roboto}"
TextElement.FontFamily="{x:Static local:MaterialDesignFonts.Roboto}"

Future

How do we handle this issue in Material Design XAML Toolkit? Possible actions below but I let you guys weigh in on the way forward.

  • Write a wiki page about font usage and describe the workaround for those in need of it
  • Let the library solve this issue:
    • Keep the font as a resource in the library
    • Add a MaterialDesignFonts type that takes care of writing temporary font files to disk before loading
    • The consumer of the library can decide if they want to use {StaticResource MaterialDesignFont} or {x:Static materialDesign:MaterialDesignFonts.Roboto}

Personally I think it would be nice if I could use the material design fonts without memory leakage out of the box :)

@mgnslndh
Copy link
Contributor Author

mgnslndh commented Sep 3, 2017

@ButchersBoy I know you've got a lot on your plate but if you think this is a good idea I can provide a PR:

  • Keep the font as a resource in the library
  • Add a MaterialDesignFonts type that takes care of writing temporary font files to disk before loading
  • The consumer of the library can decide if they want to use {StaticResource MaterialDesignFont} or {x:Static materialDesign:MaterialDesignFonts.Roboto}

I would also add some info about this behaviour in the wiki. Or I could just provide the info in the wiki.

@ButchersBoy
Copy link
Collaborator

Yes, good idea, so long as we keep all the existing stuff in tact as you have said.

@Keboo Keboo added this to the 3.0.0 milestone Jun 20, 2019
@Keboo Keboo added bug wiki This item has wiki changes labels Oct 1, 2019
@Keboo Keboo closed this as completed in 9d6dc34 Nov 28, 2019
@Keboo
Copy link
Member

Keboo commented Nov 28, 2019

I have put in a "fix" for this that I am looking to get feedback on. It is an opt-in style feature.
Inside of your project file you can set <IncludeMaterialDesignFont>True</IncludeMaterialDesignFont>. This should be placed inside of your default PropertyGroup (the one without any Condition attribute). This will cause the font files to be added to your output directory (ie. bin\Debug) under Resources\Roboto. You can then make use of these font files by using the new static property MaterialDesignFonts.Roboto. Like this:

TextElement.FontFamily="{x:Static wpf:MaterialDesignFonts.Roboto}"

Now there are a couple things worth considering here:

  1. Is setting an MSBuild property too complicated?
  2. When the font files are not present there is no exception when accessing the MaterialDesignFonts.Roboto property. This matches the default behavior when you typo a FontFamily name. Is this intuitive?
  3. Should we keep this for the 3.0.0 release?

@mgnslndh
Copy link
Contributor Author

Nice work! I like it. It is simple and we can expand on it if we have to. Keep it!

@quicoli
Copy link
Contributor

quicoli commented Dec 6, 2019

Have you seen this different solution: using a markup extension to fix memory leak

With this:

  • the font can be kept in resources (no breaking changes here)
  • a markup extension is created (no breaking changes here)
  • The way to set the font in a Window would change (breaking change)

FontFamily="{local:FontExplorer Key=Roboto}">

Keboo added a commit that referenced this issue Dec 8, 2019
@Keboo
Copy link
Member

Keboo commented Dec 8, 2019

@quicoli I have not seen that. I think I like it better, though I think i am going to leave in the option for people to include the font files in their project if they want. I dropped the MaterialDesignFonts static class in favor of the MaterialDesignFont markup extension.

@quicoli
Copy link
Contributor

quicoli commented Dec 8, 2019 via email

GBSDS added a commit to GBSDS/MaterialDesignInXamlToolkit that referenced this issue Dec 11, 2019
* Fixes MaterialDesignInXAML#746

Allowing the option for projects to opt-into having the font files copied locally rather than used as an embedded resource.
No longer setting font for DailogHost
No longer setting font for Toolbar.

* Icons update from Azure pipeline (MaterialDesignInXAML#1534)

* Cleanup pipeline (MaterialDesignInXAML#1538)

* WIP cleaning up build pipeline since we no longer need AppVeyor.

* Parameterizing the nuget to make local builds easier.

* Fixing license url

Fixing framework dependency groups

* Script and nuspec updates to handle the icons

* Fixing dependency group

* Moving build script and adding all NuGets

* Adding netcore target to nuget package.

* Adding default dependency group

* Updating build pipeline to build NuGet packages.

* Update azure-pipelines.yml for Azure Pipelines

* Update azure-pipelines.yml for Azure Pipelines

* Update azure-pipelines.yml for Azure Pipelines

* Update azure-pipelines.yml for Azure Pipelines

* Update azure-pipelines.yml for Azure Pipelines

* Fixing TextAlignment for NumericUpDown control (MaterialDesignInXAML#1540)

* Change Calendar TodayBackground to Transparent with a circle around it (MaterialDesignInXAML#1446)

* Change Calendar TodayBackground to SecondaryAccentBrush

* Update Current date to a Transparent circle with black border like in https://material.io/components/pickers/

* Change TodayBackground Stroke color to Foreground color

* Fix the text for current date is always black

* Default IsMonitoring to true (MaterialDesignInXAML#1541)

This is needed for MahApps attached properties to register for events to respond to things like SelectAllOnFocus.

Fixes MaterialDesignInXAML#1539

* Filled and Outlined DatePicker styles (MaterialDesignInXAML#1501)

* Filled and Outlined DatePicker styles

* Removed redundant setter

* Forward attached properites

Inherited attached properties can have performance implications. So instead we simply forward them as needed.

* Icons update from Azure pipeline (MaterialDesignInXAML#1544)

* Custom Thumb Control and ControlTemplate to fix issue MaterialDesignInXAML#1531 (MaterialDesignInXAML#1542)

* Created Custom Thumb to override the cursor when dragging. Created ControlTemplate for Thumb that sets cursor to SizeWE

* Implemented code review suggested changes

GridViewColumnThumb is now internal.
Created a new style explicitly for the GridView Column.
GridViewColumnHeader now uses this new style.

* Fixed foreground brush on Light, Dark and Accent versions of CheckBox and RadioButton styles (MaterialDesignInXAML#1543)

* Fixing the underline behavior of the numeric up down control (MaterialDesignInXAML#1547)

* Fixing nuget minor version. (MaterialDesignInXAML#1548)

* Another attempted fix for MaterialDesignInXAML#746

* Updating demo to use the markup extension

* Renaming file to match class name
@quicoli
Copy link
Contributor

quicoli commented Dec 14, 2019

I just published a nuget package for this.
You can the source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug wiki This item has wiki changes
Projects
None yet
Development

No branches or pull requests

4 participants