diff --git a/HellionChat/Configuration.cs b/HellionChat/Configuration.cs index 3ef8a62..b46cb68 100755 --- a/HellionChat/Configuration.cs +++ b/HellionChat/Configuration.cs @@ -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(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. diff --git a/HellionChat/Plugin.cs b/HellionChat/Plugin.cs index a460488..fda84d3 100755 --- a/HellionChat/Plugin.cs +++ b/HellionChat/Plugin.cs @@ -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(); 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. diff --git a/HellionChat/Ui/Components/Settings/ChatColourPicker.cs b/HellionChat/Ui/Components/Settings/ChatColourPicker.cs index 62f1e24..575650b 100644 --- a/HellionChat/Ui/Components/Settings/ChatColourPicker.cs +++ b/HellionChat/Ui/Components/Settings/ChatColourPicker.cs @@ -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(); } diff --git a/HellionChat/Ui/Components/Settings/Tabs/ChatTab.cs b/HellionChat/Ui/Components/Settings/Tabs/ChatTab.cs index 647d11a..ae89425 100644 --- a/HellionChat/Ui/Components/Settings/Tabs/ChatTab.cs +++ b/HellionChat/Ui/Components/Settings/Tabs/ChatTab.cs @@ -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(); } diff --git a/HellionChat/Ui/Components/Settings/Tabs/DataPrivacyTab.cs b/HellionChat/Ui/Components/Settings/Tabs/DataPrivacyTab.cs index 307adef..403be48 100644 --- a/HellionChat/Ui/Components/Settings/Tabs/DataPrivacyTab.cs +++ b/HellionChat/Ui/Components/Settings/Tabs/DataPrivacyTab.cs @@ -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(); } diff --git a/HellionChat/Ui/FirstRunWizard.cs b/HellionChat/Ui/FirstRunWizard.cs index 568a42d..7506d14 100644 --- a/HellionChat/Ui/FirstRunWizard.cs +++ b/HellionChat/Ui/FirstRunWizard.cs @@ -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