fix(config): guard the shared config maps, restore lost fields in Tab.Clone

TabsListLock never covered ChatColours, PrivacyPersistChannels or
RetentionPerChannelDays, yet the settings UI mutates them from the draw thread
while the retention thread can be serializing the same config. Adding a new key
to a dictionary or a new element to a set invalidates a running enumeration, so
this could throw from inside JsonConvert.SerializeObject.

Not a corner case: the colour picker lists 66 channels but only 25 ship with a
default, so the first edit of any of the remaining ones inserts a new key -- and
the reset button removes a key, which makes the next edit a fresh insert again.

The readers matter as much as the writers. IsAllowedForStorage runs per message
on the worker thread and asks PrivacyPersistChannels whether a channel may be
stored; a Contains racing an Add that resizes buckets can answer wrong, and that
answer decides whether a message is written to disk. The retention sweep
enumerates RetentionPerChannelDays on the framework thread while the wizard can
clear it -- Clear does not throw there, it just cuts the enumeration short, so
the sweep would run on half a policy.

New ConfigMapsLock covers all of it. It sits inside TabsListLock (that edge is
real, AutoTellTabsService calls SaveConfig while holding the tabs lock), never
the other way round -- so every call site closes the lock before saving.

Tab.Clone silently dropped Icon and ChatCodes, both serialized. A reflection
test now walks the serialized fields so a future one cannot slip past.

Also: CurrentTab read Count and [0] as two separate accesses.
This commit is contained in:
2026-08-17 07:27:48 +02:00
parent 2b4243599e
commit eaed0b13e0
6 changed files with 84 additions and 23 deletions
+16 -1
View File
@@ -79,7 +79,15 @@ public class Configuration : IPluginConfiguration
{
if (!PrivacyFilterEnabled)
return true;
if (PrivacyPersistChannels.Contains(type))
// Runs per message on the worker thread while the settings UI can Add to the
// same set from the draw thread. A HashSet.Contains racing an Add that
// resizes buckets can return the wrong answer -- and this answer decides
// whether a message is persisted. Lock kept tight, this is a hot path.
bool persists;
lock (Plugin.Instance.ConfigMapsLock)
persists = PrivacyPersistChannels.Contains(type);
if (persists)
return true;
// F3.2: log first occurrence of a ChatType the running build doesn't
@@ -619,9 +627,15 @@ public class Tab
public Tab Clone()
{
#pragma warning disable CS0618 // ChatCodes is obsolete but still serialized and must survive a clone
return new Tab
{
Name = Name,
// Both were missing: Icon feeds the sidebar glyph, ChatCodes carries
// legacy migration data that is still written to the JSON. A clone
// round-trip used to drop them silently.
Icon = Icon,
ChatCodes = new Dictionary<ChatType, ChatSource>(ChatCodes),
SelectedChannels = SelectedChannels.ToDictionary(pair => pair.Key, pair => pair.Value),
ExtraChatAll = ExtraChatAll,
ExtraChatChannels = ExtraChatChannels.ToHashSet(),
@@ -654,6 +668,7 @@ public class Tab
NotificationSoundId = NotificationSoundId,
IsGreeted = IsGreeted,
};
#pragma warning restore CS0618
}
/// Ordered message list with duplicate ID tracking, sorting and mutex protection.
+31 -4
View File
@@ -193,14 +193,36 @@ public sealed class Plugin : IAsyncDalamudPlugin
// MessageList's SemaphoreSlim inner — never the reverse.
internal readonly object TabsListLock = new();
// Guards the serialized config maps that the draw thread mutates while a
// background save may be serializing them: ChatColours, PrivacyPersistChannels
// and RetentionPerChannelDays. TabsListLock does not cover these.
// Ordering: ConfigMapsLock sits INSIDE TabsListLock (that edge is real, via
// AutoTellTabsService calling SaveConfig under the tabs lock). Never the other
// way round -- so SaveConfig must never be called while holding ConfigMapsLock.
internal readonly object ConfigMapsLock = new();
internal DateTime GameStarted { get; }
// Couples "current tab" to the real UI selection. The chat hooks are
// installed before MainWindow is Phase-1 resolved, so the null-conditional
// fallback to Tabs[0] is load-bearing — it keeps the pre-coupling behavior
// in that early window rather than being merely defensive.
internal Tab CurrentTab =>
MainWindow?.ActiveTab ?? (Config.Tabs.Count > 0 ? Config.Tabs[0] : new Tab());
// Read once into a local: Count and [0] as two separate accesses can be split
// by a removal on another thread. Only reachable before MainWindow exists.
internal Tab CurrentTab
{
get
{
if (MainWindow?.ActiveTab is { } active)
return active;
lock (TabsListLock)
{
var tabs = Config.Tabs;
return tabs.Count > 0 ? tabs[0] : new Tab();
}
}
}
public Plugin()
{
@@ -940,8 +962,13 @@ public sealed class Plugin : IAsyncDalamudPlugin
var policy = new Dictionary<int, int>();
foreach (var (type, days) in Privacy.PrivacyDefaults.DefaultRetentionDays)
policy[(int)(ushort)type] = days;
foreach (var (type, days) in Config.RetentionPerChannelDays)
policy[(int)(ushort)type] = days;
// This is the enumerator the wizard's Clear() cuts short. Reading under the
// same lock the writers take keeps the policy snapshot whole.
lock (ConfigMapsLock)
{
foreach (var (type, days) in Config.RetentionPerChannelDays)
policy[(int)(ushort)type] = days;
}
var defaultDays = Config.RetentionDefaultDays;
// IsBackground = true so a stuck sweep never blocks plugin unload.
@@ -71,7 +71,8 @@ internal sealed class ChatColourPicker
)
)
{
Plugin.Config.ChatColours.Remove(type);
lock (_plugin.ConfigMapsLock)
Plugin.Config.ChatColours.Remove(type);
commit = true;
}
@@ -86,7 +87,8 @@ internal sealed class ChatColourPicker
)
{
var gameColour = _plugin.Functions.Chat.GetChannelColor(type);
Plugin.Config.ChatColours[type] = gameColour ?? type.DefaultColor() ?? 0;
lock (_plugin.ConfigMapsLock)
Plugin.Config.ChatColours[type] = gameColour ?? type.DefaultColor() ?? 0;
commit = true;
}
@@ -97,7 +99,10 @@ internal sealed class ChatColourPicker
: ColourUtil.RgbaToVector3(type.DefaultColor() ?? 0);
if (ImGui.ColorEdit3(type.Name(), ref vec, ImGuiColorEditFlags.NoInputs))
{
Plugin.Config.ChatColours[type] = ColourUtil.Vector3ToRgba(vec);
// First edit of a channel without a default inserts a NEW key --
// that is the case that invalidates a running enumeration.
lock (_plugin.ConfigMapsLock)
Plugin.Config.ChatColours[type] = ColourUtil.Vector3ToRgba(vec);
liveOnly = true;
}
if (ImGui.IsItemDeactivatedAfterEdit())
@@ -155,7 +160,8 @@ internal sealed class ChatColourPicker
private void ApplyPreset(ChatColourPreset preset)
{
foreach (var (channel, colour) in preset.Colours)
Plugin.Config.ChatColours[channel] = colour;
lock (_plugin.ConfigMapsLock)
Plugin.Config.ChatColours[channel] = colour;
ApplyChatColourChange();
}
@@ -226,7 +232,8 @@ internal sealed class ChatColourPicker
if (ImGui.Button(HellionStrings.Settings_Themes_ApplyChatColors_Apply))
{
foreach (var kvp in themeChatColors.Channels)
Plugin.Config.ChatColours[kvp.Key] = kvp.Value;
lock (_plugin.ConfigMapsLock)
Plugin.Config.ChatColours[kvp.Key] = kvp.Value;
_applyDismissedFor = active.Slug;
ApplyChatColourChange();
}
@@ -84,13 +84,18 @@ internal sealed class ChatTab
var present = Plugin.Config.PrivacyPersistChannels.Contains(ct);
if (ImGui.Checkbox($"{label}##persist-{label}", ref present))
{
if (present)
// Lock closes before SaveConfig: taking ConfigMapsLock across a save
// would invert the lock order (SaveConfig can reach TabsListLock).
lock (_plugin.ConfigMapsLock)
{
Plugin.Config.PrivacyPersistChannels.Add(ct);
}
else
{
Plugin.Config.PrivacyPersistChannels.Remove(ct);
if (present)
{
Plugin.Config.PrivacyPersistChannels.Add(ct);
}
else
{
Plugin.Config.PrivacyPersistChannels.Remove(ct);
}
}
_plugin.SaveConfig();
}
@@ -81,13 +81,17 @@ internal sealed class DataPrivacyTab
var present = Plugin.Config.PrivacyPersistChannels.Contains(ct);
if (ImGui.Checkbox($"{label}##privacy-persist-{label}", ref present))
{
if (present)
// Lock closes before the save below, see ChatTab for the ordering.
lock (_plugin.ConfigMapsLock)
{
Plugin.Config.PrivacyPersistChannels.Add(ct);
}
else
{
Plugin.Config.PrivacyPersistChannels.Remove(ct);
if (present)
{
Plugin.Config.PrivacyPersistChannels.Add(ct);
}
else
{
Plugin.Config.PrivacyPersistChannels.Remove(ct);
}
}
_plugin.SaveConfig();
}
+4 -1
View File
@@ -655,7 +655,10 @@ public sealed class FirstRunWizard : Window
Plugin.Config.PrivacyPersistUnknownChannels = true;
Plugin.Config.RetentionEnabled = false;
Plugin.Config.RetentionPerChannelDays.Clear();
// Clear does not throw during enumeration, it just cuts it short -- the
// retention sweep would then run on half a policy.
lock (Plugin.ConfigMapsLock)
Plugin.Config.RetentionPerChannelDays.Clear();
}
// Test-only entry point so SelfTests/WizardStateSmokeStep can advance