Skip to content

Commit 95963c6

Browse files
kaltinrilvchelaruclaude
authored
Fix duplicate-key crash on BBCode IsBold font resolution (#3530) (#3531)
* Fix duplicate-key crash on BBCode IsBold font resolution (#3530) Assigning a TextRuntime.Text containing a BBCode [IsBold=true] run could crash with "An item with the same key has already been added" for the bold font's FontCache .fnt key. The inline-BBCode font resolver (CustomSetPropertyOnRenderable. GetAndCreateFontIfNecessary) resolves the same font once per open/close tag by design and dedups only via GetDisposable(...) as BitmapFont. When an earlier resolution cached a value that cast cannot recover (a null or shared Text.DefaultBitmapFont fallback cached because no .fnt was on disk yet), the guard sees an empty slot, falls through, and AddDisposable collides with the still-occupied key. AddDisposable defaulted to ExistingContentBehavior.ThrowException, so it threw. Pass ExistingContentBehavior.Replace at both AddDisposable calls in GetAndCreateFontIfNecessary so a re-resolution heals the poisoned slot instead of crashing, matching the protection already present on the raylib and non-BBCode MonoGame font paths. Replace is used rather than Dispose-then-add because the occupying value can be null or the shared default font, which LoaderManager.Dispose cannot safely handle. Adds MonoGameGum.Tests regression coverage. * Cover both BBCode bold font-cache Replace paths (#3530) Supplements the regression test: adds a second test exercising the in-memory font-creation AddDisposable site (the disk/DefaultBitmapFont fallback site was already covered), plus a heal assertion proving the poisoned slot is replaced so the test can't pass vacuously if the computed cache key drifts. Verified red-for-the-right-reason per line; full MonoGameGum.Tests suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Victor Chelaru <VicChelaru@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent fcbe239 commit 95963c6

2 files changed

Lines changed: 182 additions & 2 deletions

File tree

Gum/Wireframe/CustomSetPropertyOnRenderable.cs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1318,7 +1318,9 @@ BitmapFont GetAndCreateFontIfNecessary()
13181318
font = InMemoryFontCreator.TryCreateFont(bmfcSave);
13191319
if (font != null)
13201320
{
1321-
global::RenderingLibrary.Content.LoaderManager.Self.AddDisposable(fontFileName, font);
1321+
// #3530: Replace so re-adding an already-occupied key heals it instead of throwing.
1322+
global::RenderingLibrary.Content.LoaderManager.Self.AddDisposable(fontFileName, font,
1323+
global::RenderingLibrary.Content.LoaderManager.ExistingContentBehavior.Replace);
13221324
}
13231325
}
13241326
catch
@@ -1395,7 +1397,9 @@ BitmapFont GetAndCreateFontIfNecessary()
13951397
// This can happen when closing tags are encountered at the end of a font. If no font exists, we can just go to the default
13961398
font = Text.DefaultBitmapFont;
13971399
}
1398-
global::RenderingLibrary.Content.LoaderManager.Self.AddDisposable(fontFileName, font);
1400+
// #3530: Replace so re-adding an already-occupied key heals it instead of throwing.
1401+
global::RenderingLibrary.Content.LoaderManager.Self.AddDisposable(fontFileName, font,
1402+
global::RenderingLibrary.Content.LoaderManager.ExistingContentBehavior.Replace);
13991403
}
14001404
}
14011405

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
using Gum.GueDeriving;
2+
using Gum.Wireframe;
3+
using Microsoft.Xna.Framework.Graphics;
4+
using RenderingLibrary.Content;
5+
using RenderingLibrary.Graphics;
6+
using RenderingLibrary.Graphics.Fonts;
7+
using Shouldly;
8+
using System;
9+
using System.IO;
10+
using ToolsUtilities;
11+
using Xunit;
12+
13+
namespace MonoGameGum.Tests.Runtimes;
14+
15+
// Regression coverage for issue #3530: assigning a TextRuntime.Text that contains a BBCode
16+
// [IsBold=true] run crashed a shipping FlatRedBall game with
17+
// "System.ArgumentException: An item with the same key has already been added. Key: ...\fontcache\font12garet_book_bold.fnt".
18+
//
19+
// The inline-BBCode font resolver (CustomSetPropertyOnRenderable.GetAndCreateFontIfNecessary)
20+
// resolves the same bold font once per open tag by design and relies on
21+
// LoaderManager.GetDisposable(...) as BitmapFont to dedup. That guard is not robust to a poisoned
22+
// cache slot: when an earlier resolution cached a value the "as BitmapFont" cast cannot recover
23+
// (e.g. a null/placeholder cached under the bold key because no .fnt existed on disk and the
24+
// DefaultBitmapFont fallback was used), the next lookup sees an empty slot, falls through, and its
25+
// AddDisposable(...) call — which defaulted to ExistingContentBehavior.ThrowException — collides with
26+
// the still-occupied key and throws. The fix passes ExistingContentBehavior.Replace so the poisoned
27+
// slot heals instead of crashing.
28+
//
29+
// The resolver has TWO AddDisposable calls that were changed to Replace, reached by different font
30+
// sources, so each has its own test:
31+
// * disk / DefaultBitmapFont fallback (no InMemoryFontCreator) -> the crash's exact line.
32+
// * in-memory font creation (InMemoryFontCreator set) -> the sibling line, covered so a
33+
// regression that heals only one of the two lines still turns a test red.
34+
// Each test also asserts the poisoned slot was actually REPLACED with a BitmapFont afterward. Without
35+
// that, the test could pass vacuously if the key the resolver computes ever drifted away from the key
36+
// this test poisons: the resolver would then add under a fresh (empty) key, never collide, and
37+
// Should.NotThrow would be satisfied without the fix under test running at all.
38+
public class TextRuntimeBbCodeFontCachingRegressionTests : BaseTestClass
39+
{
40+
// Uses a font NOT in the test harness's stubbed embedded resources (which only cover Arial-18)
41+
// and a size (12) matching the real crash's Font12garet_book_bold key, so the resolution actually
42+
// reaches the disk / DefaultBitmapFont fallback path instead of being satisfied by an embedded font.
43+
private const string UnstubbedFontName = "Garet";
44+
private const int UnstubbedFontSize = 12;
45+
46+
[Fact]
47+
public void Text_WithBbCodeBoldRun_WhenBoldCacheSlotPoisoned_ShouldNotThrowDuplicateKey()
48+
{
49+
bool savedCacheTextures = LoaderManager.Self.CacheTextures;
50+
string savedRelativeDirectory = FileManager.RelativeDirectory;
51+
try
52+
{
53+
LoaderManager.Self.CacheTextures = true;
54+
// A unique relative directory makes the (absolute) font-cache key unique to this run so no
55+
// prior test can mask the collision and this test cannot poison a shared slot.
56+
FileManager.RelativeDirectory = MakeUniqueRelativeDirectory();
57+
58+
// Reproduce the poisoned slot the real first resolution left behind: a cached value the
59+
// "as BitmapFont" cast cannot recover (so the dedup guard sees an empty slot and falls
60+
// through to AddDisposable) under the exact bold key the inline-BBCode resolver computes.
61+
string boldKey = GetBoldFontCacheKey();
62+
LoaderManager.Self.AddDisposable(boldKey, new NonFontDisposable(),
63+
LoaderManager.ExistingContentBehavior.Replace);
64+
65+
TextRuntime textRuntime = new();
66+
textRuntime.Font = UnstubbedFontName;
67+
textRuntime.FontSize = UnstubbedFontSize;
68+
69+
// Assigning BBCode with a bold run re-resolves the (already poisoned) bold key. Before the
70+
// fix this threw ArgumentException from AddDisposable's default ThrowException behavior. With
71+
// no InMemoryFontCreator set, resolution falls to the disk / DefaultBitmapFont path (the
72+
// exact line in the #3530 crash stack).
73+
Should.NotThrow(() =>
74+
textRuntime.Text = "normal [IsBold=true]bold[/IsBold] normal");
75+
76+
// The resolver reached the poisoned key and replaced the non-font placeholder with the
77+
// resolved font, proving the fixed AddDisposable actually ran (not skipped via key drift).
78+
LoaderManager.Self.GetDisposable(boldKey).ShouldBeAssignableTo<BitmapFont>();
79+
}
80+
finally
81+
{
82+
LoaderManager.Self.CacheTextures = savedCacheTextures;
83+
FileManager.RelativeDirectory = savedRelativeDirectory;
84+
}
85+
}
86+
87+
[Fact]
88+
public void Text_WithBbCodeBoldRun_WhenBoldSlotPoisonedAndResolvedByInMemoryCreator_ShouldNotThrowDuplicateKey()
89+
{
90+
bool savedCacheTextures = LoaderManager.Self.CacheTextures;
91+
string savedRelativeDirectory = FileManager.RelativeDirectory;
92+
IInMemoryFontCreator? savedCreator = CustomSetPropertyOnRenderable.InMemoryFontCreator;
93+
try
94+
{
95+
LoaderManager.Self.CacheTextures = true;
96+
FileManager.RelativeDirectory = MakeUniqueRelativeDirectory();
97+
// With an in-memory creator, the bold run resolves to a freshly created font and caches it
98+
// via the resolver's OTHER AddDisposable call (the in-memory branch), which the fix also
99+
// changed to Replace. The disk-fallback test above never reaches this branch because the
100+
// harness leaves InMemoryFontCreator null.
101+
CustomSetPropertyOnRenderable.InMemoryFontCreator = new AbcInMemoryFontCreator();
102+
103+
string boldKey = GetBoldFontCacheKey();
104+
LoaderManager.Self.AddDisposable(boldKey, new NonFontDisposable(),
105+
LoaderManager.ExistingContentBehavior.Replace);
106+
107+
TextRuntime textRuntime = new();
108+
textRuntime.Font = UnstubbedFontName;
109+
textRuntime.FontSize = UnstubbedFontSize;
110+
111+
// Only glyphs the stub font defines (space + A/B/C) so measurement stays valid; only "BB" is
112+
// bold, so it re-resolves the poisoned bold key.
113+
Should.NotThrow(() =>
114+
textRuntime.Text = "AA [IsBold=true]BB[/IsBold] CC");
115+
116+
LoaderManager.Self.GetDisposable(boldKey).ShouldBeAssignableTo<BitmapFont>();
117+
}
118+
finally
119+
{
120+
CustomSetPropertyOnRenderable.InMemoryFontCreator = savedCreator;
121+
LoaderManager.Self.CacheTextures = savedCacheTextures;
122+
FileManager.RelativeDirectory = savedRelativeDirectory;
123+
}
124+
}
125+
126+
// A unique relative directory makes the (absolute) font-cache key unique to this run so no prior
127+
// test can mask the collision and this test cannot poison a shared slot.
128+
private static string MakeUniqueRelativeDirectory() =>
129+
Path.Combine(Path.GetTempPath(), "GumBbCodeBoldFont_" + Guid.NewGuid().ToString("N"))
130+
.Replace('\\', '/') + "/";
131+
132+
// Mirrors CustomSetPropertyOnRenderable.GetFontFileName: the same GetFontCacheFileNameFor call
133+
// followed by the same RemoveDotDotSlash(Standardize(name, false, true)) normalization.
134+
private static string GetBoldFontCacheKey()
135+
{
136+
string cacheFileName = BmfcSave.GetFontCacheFileNameFor(
137+
UnstubbedFontSize, UnstubbedFontName, outline: 0, useFontSmoothing: true,
138+
isItalic: false, isBold: true, fontFilePath: null);
139+
return FileManager.RemoveDotDotSlash(
140+
FileManager.Standardize(cacheFileName, preserveCase: false, makeAbsolute: true));
141+
}
142+
143+
// A cached IDisposable that is not a BitmapFont, so the resolver's "GetDisposable(...) as
144+
// BitmapFont" guard yields null and treats the (occupied) slot as empty. Safe to Dispose when the
145+
// shared cache is later cleared, unlike a null cache value.
146+
private sealed class NonFontDisposable : IDisposable
147+
{
148+
public void Dispose()
149+
{
150+
}
151+
}
152+
153+
// Minimal in-memory font creator: returns a valid BitmapFont (space + A/B/C glyphs, no disk I/O)
154+
// for any request, so the resolver takes the in-memory AddDisposable branch instead of the disk /
155+
// DefaultBitmapFont fallback.
156+
private sealed class AbcInMemoryFontCreator : IInMemoryFontCreator
157+
{
158+
public BitmapFont? TryCreateFont(BmfcSave bmfcSave)
159+
{
160+
BitmapFont font = new BitmapFont((Texture2D)null!, AbcFontData);
161+
font.SetFontPattern(256, 256);
162+
return font;
163+
}
164+
165+
private const string AbcFontData =
166+
@"info face=""Arial"" size=-18 bold=0 italic=0 charset="""" unicode=1 stretchH=100 smooth=1 aa=1 padding=0,0,0,0 spacing=1,1 outline=0
167+
common lineHeight=18 base=18 scaleW=256 scaleH=256 pages=1 packed=0 alphaChnl=0 redChnl=4 greenChnl=4 blueChnl=4
168+
page id=0 file=""x.png""
169+
chars count=4
170+
char id=32 x=0 y=0 width=9 height=13 xoffset=0 yoffset=4 xadvance=9 page=0 chnl=15
171+
char id=65 x=0 y=0 width=9 height=13 xoffset=0 yoffset=4 xadvance=9 page=0 chnl=15
172+
char id=66 x=0 y=0 width=9 height=13 xoffset=0 yoffset=4 xadvance=9 page=0 chnl=15
173+
char id=67 x=0 y=0 width=9 height=13 xoffset=0 yoffset=4 xadvance=9 page=0 chnl=15
174+
";
175+
}
176+
}

0 commit comments

Comments
 (0)