Windows: Fix unbounded growth of stored scoped attributes (#259)

Author: melekr

Runtime/Native/Windows/NativeClient.cs

@@ -333,6 +333,7 @@ private string GetPluginDirectoryPath()
             const string pluginDir = "Plugins";
             return Path.Combine(Application.dataPath, pluginDir);
         }
+
         /// <summary>
         /// Generate path to Crashpad handler binary
         /// </summary>
@@ -367,12 +368,24 @@ internal static void CleanScopedAttributes()
             {
                 return;
             }
-            var attributes = JsonUtility.FromJson<ScopedAttributesContainer>(attributesJson);
-            foreach (var attributeKey in attributes.Keys)
+
+            try
+            {
+                var attributes = JsonUtility.FromJson<ScopedAttributesContainer>(attributesJson);
+                    foreach (var attributeKey in attributes.Keys)
+                    {
+                        PlayerPrefs.DeleteKey(string.Format(ScopedAttributesPattern, attributeKey));
+                    }
+            }
+            catch (Exception e)
+            {
+                Debug.LogWarning($"Backtrace Failed to parse scoped attributes for cleanup: {e.Message}");
+            }
+            finally
             {
-                PlayerPrefs.DeleteKey(string.Format(ScopedAttributesPattern, attributeKey));
+                PlayerPrefs.DeleteKey(ScopedAttributeListKey);
+                PlayerPrefs.Save();
             }
-            PlayerPrefs.DeleteKey(ScopedAttributeListKey);
         }
 
         internal static IDictionary<string, string> GetScopedAttributes()
@@ -382,8 +395,25 @@ internal static IDictionary<string, string> GetScopedAttributes()
             {
                 return new Dictionary<string, string>();
             }
-            var result = new Dictionary<string, string>();
-            var attributes = JsonUtility.FromJson<ScopedAttributesContainer>(attributesJson);
+
+            var result = new Dictionary<string, string>(StringComparer.Ordinal);
+            ScopedAttributesContainer attributes = null;
+
+            try
+            {
+                attributes = JsonUtility.FromJson<ScopedAttributesContainer>(attributesJson);
+            }
+            catch (Exception e)
+            {
+                Debug.LogWarning($"Backtrace Failed to parse scoped attributes at read: {e.Message}");
+                return result;
+            }
+
+            if (attributes?.Keys == null)
+            {
+                return result;
+            }
+
             foreach (var attributeKey in attributes.Keys)
             {
                 var value = PlayerPrefs.GetString(string.Format(ScopedAttributesPattern, attributeKey), string.Empty);
@@ -411,7 +441,7 @@ internal static IDictionary<string, string> GetScopedAttributes()
         }
 
         /// <summary>
-        /// Adds attributes to scoped registry and to native clietn
+        /// Adds attributes to scoped registry and to native client
         /// </summary>
         /// <param name="key">attribute key</param>
         /// <param name="value">attribute value</param>
@@ -424,23 +454,100 @@ private void AddAttributes(string key, string value)
             AddScopedAttribute(key, value);
         }
 
+        /// <summary>Load a deduped set of scoped attribute keys from PlayerPrefs.</summary>
+        private HashSet<string> LoadScopedKeys()
+        {
+            var keys = new HashSet<string>(StringComparer.Ordinal);
+            var attributesJson = PlayerPrefs.GetString(ScopedAttributeListKey);
+            if (HasScopedAttributesEmpty(attributesJson))
+            {
+                try
+                {
+                    var container = JsonUtility.FromJson<ScopedAttributesContainer>(attributesJson);
+                    foreach (var k in container.Keys)
+                    {
+                        if (!string.IsNullOrEmpty(k))
+                        {
+                            keys.Add(k);
+                        }
+                    }
+                }
+                catch (Exception e)
+                {
+                    Debug.LogWarning($"Backtrace Failed to parse scoped attributes list. {e.Message}");
+                    keys.Clear();
+                }
+            }
+            return keys;
+        }
+
+        /// <summary>Persist a deduped list of keys back to PlayerPrefs.</summary>
+        private void SaveScopedKeys(HashSet<string> keys)
+        {
+            var container = new ScopedAttributesContainer
+            {
+                Keys = keys.ToList()
+            };
+            PlayerPrefs.SetString(ScopedAttributeListKey, JsonUtility.ToJson(container));
+            PlayerPrefs.Save();
+        }
+
+        /// <summary>Return the currently stored "value" for a scoped key or "null" if missing.</summary>
+        private string GetScopedValue(string key)
+        {
+            var prefsKey = string.Format(ScopedAttributesPattern, key);
+            return PlayerPrefs.HasKey(prefsKey) ? PlayerPrefs.GetString(prefsKey) : null;
+        }
+
+        /// <summary>Set the stored value for a scoped key.</summary>
+        private void SetScopedValue(string key, string value)
+        {
+            PlayerPrefs.SetString(string.Format(ScopedAttributesPattern, key), value ?? string.Empty);
+        }
+
         /// <summary>
         /// Adds dictionary of attributes to player prefs for windows crashes captured by unity crash handler
         /// </summary>
-        /// <param name="atributes">Attributes</param>
+        /// <param name="attributes">Attributes</param>
         internal void AddScopedAttributes(IDictionary<string, string> attributes)
         {
             if (!_configuration.SendUnhandledGameCrashesOnGameStartup)
             {
                 return;
             }
-            var attributesContainer = new ScopedAttributesContainer();
-            foreach (var attribute in attributes)
+            if (attributes == null || attributes.Count == 0)
             {
-                attributesContainer.Keys.Add(attribute.Key);
-                PlayerPrefs.SetString(string.Format(ScopedAttributesPattern, attribute.Key), attribute.Value);
+                return;
+            }
+
+            var keys = LoadScopedKeys();
+            bool listChanged = false;
+
+            foreach (var kv in attributes)
+            {
+                var key = kv.Key;
+                var value = kv.Value ?? string.Empty;
+
+                // skip when unchanged
+                var current = GetScopedValue(key);
+                if (current == null || !string.Equals(current, value, StringComparison.Ordinal))
+                {
+                    SetScopedValue(key, value);
+                }
+
+                // deduplication
+                if (keys.Add(key))
+                {
+                    listChanged = true;
+                }
             }
-            PlayerPrefs.SetString(ScopedAttributeListKey, JsonUtility.ToJson(attributesContainer));
+
+            if (listChanged)
+            {
+                SaveScopedKeys(keys);
+            }
+
+            PlayerPrefs.Save();
         }
 
         /// <summary>
@@ -454,14 +561,29 @@ private void AddScopedAttribute(string key, string value)
             {
                 return;
             }
-            var attributesJson = PlayerPrefs.GetString(ScopedAttributeListKey);
-            var attributes = HasScopedAttributesEmpty(attributesJson)
-                ? JsonUtility.FromJson<ScopedAttributesContainer>(attributesJson)
-                : new ScopedAttributesContainer();
+            if (string.IsNullOrEmpty(key))
+            {
+                return;
+            }
+
+            var normalized = value ?? string.Empty;
+
+            // skip when unchanged
+            var current = GetScopedValue(key);
+            if (current != null && string.Equals(current, normalized, StringComparison.Ordinal))
+            {
+                return;
+            }
+
+            SetScopedValue(key, normalized);
+
+            var keys = LoadScopedKeys();
+            if (keys.Add(key))
+            {
+                SaveScopedKeys(keys);
+            }
 
-            attributes.Keys.Add(key);
-            PlayerPrefs.SetString(ScopedAttributeListKey, JsonUtility.ToJson(attributes));
-            PlayerPrefs.SetString(string.Format(ScopedAttributesPattern, key), value);
+            PlayerPrefs.Save();
         }
 
         private static bool HasScopedAttributesEmpty(string attributesJson)

Tests/Runtime/Native/Windows/ScopedNativeAttributesTests.cs

@@ -10,20 +10,27 @@ namespace Backtrace.Unity.Tests.Runtime.Native.Windows
 {
     public sealed class ScopedNativeAttributesTests
     {
+        // PlayerPrefs key used by the Windows NativeClient to store the scoped list
+        private const string ScopedKeyList = "backtrace-scoped-attributes";
+        private const string ScopedValuePattern = "bt-{0}";
+
         [TearDown]
         public void Setup()
         {
             CleanLegacyAttributes();
             NativeClient.CleanScopedAttributes();
+            PlayerPrefs.DeleteAll();
         }
 
+        [Test]
         public void FreshStartup_ShouldntIncludeAnyAttributeFromPlayerPrefs_AttributesAreEmpty()
         {
             var attributes = NativeClient.GetScopedAttributes();
 
             Assert.IsEmpty(attributes);
         }
 
+        [Test]
         public void LegacyAttributesSupport_ShouldIncludeLegacyAttributesWhenScopedAttributesAreNotAvailable_AllLegacyAttributesArePresent()
         {
             string testVersion = "0.1.0";
@@ -82,7 +89,6 @@ public void NativeCrashUploadAttributesSetting_ShouldReadPlayerPrefsWithLegacyAt
         [Test]
         public void NativeCrashUploadAttributes_ShouldSetScopedAttributeViaNativeClientApi_AttributePresentsInScopedAttributes()
         {
-
             var configuration = ScriptableObject.CreateInstance<BacktraceConfiguration>();
             configuration.SendUnhandledGameCrashesOnGameStartup = true;
             const string testAttributeKey = "foo-key-bar-baz";
@@ -93,7 +99,6 @@ public void NativeCrashUploadAttributes_ShouldSetScopedAttributeViaNativeClientA
             var scopedAttributes = NativeClient.GetScopedAttributes();
 
             Assert.AreEqual(scopedAttributes[testAttributeKey], testAttributeValue);
-
         }
 
         [Test]
@@ -119,6 +124,59 @@ public void NativeCrashAttributesCleanMethod_ShouldCleanAllScopedAttribtues_Scop
             Assert.IsNotEmpty(attributesBeforeCleanup);
         }
 
+        [Test]
+        public void ScopedAttributes_ShouldNotDuplicateKeys_OnRepeatedAdds()
+        {
+            var configuration = ScriptableObject.CreateInstance<BacktraceConfiguration>();
+            configuration.SendUnhandledGameCrashesOnGameStartup = true;
+
+            const string k = "dup-key";
+            const string v = "v1";
+
+            var client = new NativeClient(configuration, null, new Dictionary<string, string>(), new List<string>());
+
+            client.SetAttribute(k, v);
+            client.SetAttribute(k, v);
+            client.SetAttribute(k, v);
+
+            var scoped = NativeClient.GetScopedAttributes();
+            Assert.IsTrue(scoped.ContainsKey(k));
+            Assert.AreEqual(v, scoped[k]);
+
+            var json = PlayerPrefs.GetString(ScopedKeyList);
+            var occurrences = json.Split('"');
+            int count = 0;
+            foreach (var s in occurrences)
+            {
+                if (s == k) count++;
+            }
+
+            Assert.AreEqual(1, count, "Key should be stored once in the scoped key list.");
+        }
+
+        [Test]
+        public void ScopedAttributes_ShouldSkipWrites_WhenValueUnchanged()
+        {
+            var configuration = ScriptableObject.CreateInstance<BacktraceConfiguration>();
+            configuration.SendUnhandledGameCrashesOnGameStartup = true;
+
+            const string k = "stable-key";
+            const string v = "same-value";
+
+            var client = new NativeClient(configuration, null, new Dictionary<string, string>(), new List<string>());
+
+            client.SetAttribute(k, v);
+            var json1 = PlayerPrefs.GetString(ScopedKeyList);
+            var val1 = PlayerPrefs.GetString(string.Format(ScopedValuePattern, k));
+
+            client.SetAttribute(k, v);
+            var json2 = PlayerPrefs.GetString(ScopedKeyList);
+            var val2 = PlayerPrefs.GetString(string.Format(ScopedValuePattern, k));
+
+            Assert.AreEqual(json1, json2, "Scoped key list JSON should not change when value is unchanged.");
+            Assert.AreEqual(val1, val2, "Stored value should remain the same.");
+        }
+
         private void CleanLegacyAttributes()
         {
             PlayerPrefs.DeleteKey(NativeClient.VersionKey);