From c50646ef913aca85d2d8b51d8f544dc58263e294 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Gu=C3=B0mundsson?= Date: Thu, 16 Jul 2026 20:37:22 +0000 Subject: [PATCH] fix: guard TransformHandle against reflection serialization GameObjectSerializer reflects over component properties and hands values to Json.NET. On Unity 6, a property returning UnityEngine.TransformHandle makes the serializer enumerate the handle's direct children - which throws NullReferenceException (AssertHandleIsValid) the moment the underlying object is destroyed, a common race when component data is read during play mode while scripts Destroy() objects. On live objects it would walk entire child hierarchies of handles instead of a value. Add a write-only TransformHandleConverter that emits null for the type, matched by full name so the package still compiles on Unity versions without the type (floor is 2021.3), avoiding version defines per the Unity*Compat guidance. Internal for the same converter-scanner reason as UnityEngineObjectConverter (#1138). Repro: read mcpforunity://scene/gameobject/{id}/components repeatedly in play mode while a spawner destroys and respawns tagged objects. Co-Authored-By: Claude Fable 5 --- .../Editor/Helpers/GameObjectSerializer.cs | 1 + .../Serialization/UnityTypeConverters.cs | 25 +++++ .../Helpers/TransformHandleConverterTests.cs | 106 ++++++++++++++++++ .../TransformHandleConverterTests.cs.meta | 11 ++ 4 files changed, 143 insertions(+) create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/TransformHandleConverterTests.cs create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/TransformHandleConverterTests.cs.meta diff --git a/MCPForUnity/Editor/Helpers/GameObjectSerializer.cs b/MCPForUnity/Editor/Helpers/GameObjectSerializer.cs index f97113618..4907b03b7 100644 --- a/MCPForUnity/Editor/Helpers/GameObjectSerializer.cs +++ b/MCPForUnity/Editor/Helpers/GameObjectSerializer.cs @@ -714,6 +714,7 @@ private static object ConvertJTokenToPlainObject(JToken token) new RectConverter(), new BoundsConverter(), new Matrix4x4Converter(), // Fix #478: Safe Matrix4x4 serialization for Cinemachine + new TransformHandleConverter(), // Unity 6 TransformHandle: enumerating a destroyed handle's children throws NRE new UnityEngineObjectConverter() // Handles serialization of references }, ReferenceLoopHandling = ReferenceLoopHandling.Ignore, diff --git a/MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs b/MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs index a96e9ad02..95522f748 100644 --- a/MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs +++ b/MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs @@ -266,6 +266,31 @@ public override Matrix4x4 ReadJson(JsonReader reader, Type objectType, Matrix4x4 } } + /// + /// Safe pass-by converter for UnityEngine.TransformHandle (Unity 6+). Reflection-based + /// serialization of a handle enumerates its direct children, which throws + /// NullReferenceException the moment the underlying object is destroyed (a common race + /// when reading component data during play mode while scripts Destroy() objects), and on + /// live objects would walk entire child hierarchies of handles. A handle carries no + /// serializable identity of its own, so it is written as null. The type is matched by + /// name because it does not exist on all Unity versions this package supports. + /// Intentionally internal for the same converter-scanner reason as + /// UnityEngineObjectConverter below (see issue #1138). + /// + internal class TransformHandleConverter : JsonConverter + { + public override bool CanRead => false; + + public override bool CanConvert(Type objectType) + => objectType?.FullName == "UnityEngine.TransformHandle"; + + public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer) + => writer.WriteNull(); + + public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer) + => throw new NotSupportedException("TransformHandleConverter is write-only."); + } + // Converter for UnityEngine.Object references (GameObjects, Components, Materials, Textures, etc.) // Intentionally internal: this converter is meant for MCP-internal serialization only. // Leaving it public lets third-party Newtonsoft converter scanners (e.g. jillejr's diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/TransformHandleConverterTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/TransformHandleConverterTests.cs new file mode 100644 index 000000000..092e1f5a6 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/TransformHandleConverterTests.cs @@ -0,0 +1,106 @@ +using System; +using System.Linq; +using System.Reflection; +using MCPForUnity.Runtime.Serialization; +using Newtonsoft.Json; +using NUnit.Framework; +using UnityEngine; + +namespace MCPForUnityTests.Editor.Helpers +{ + /// + /// Tests for TransformHandleConverter, which writes UnityEngine.TransformHandle + /// (Unity 6+) as null instead of letting reflection-based serialization enumerate + /// the handle's children - that enumeration throws NullReferenceException once the + /// underlying object is destroyed (a live race when component data is read during + /// play mode while scripts Destroy() objects). + /// The type is resolved by reflection so this file compiles on every Unity version + /// the package supports; handle-specific tests self-ignore where the type is absent. + /// + public class TransformHandleConverterTests + { + private JsonSerializerSettings _settings; + + [SetUp] + public void SetUp() + { + _settings = new JsonSerializerSettings + { + Converters = { new TransformHandleConverter() } + }; + } + + private static Type TransformHandleType => + typeof(Transform).Assembly.GetType("UnityEngine.TransformHandle"); + + // Finds a public instance property on Transform that returns a TransformHandle, + // which is how the GameObjectSerializer's reflection walk encounters one. + private static object GetHandleFor(Transform transform) + { + Type handleType = TransformHandleType; + if (handleType == null) return null; + + PropertyInfo handleProperty = typeof(Transform) + .GetProperties(BindingFlags.Public | BindingFlags.Instance) + .FirstOrDefault(p => p.PropertyType == handleType); + return handleProperty?.GetValue(transform); + } + + [Test] + public void CanConvert_RejectsUnrelatedTypes() + { + var converter = new TransformHandleConverter(); + + Assert.That(converter.CanConvert(typeof(Vector3)), Is.False); + Assert.That(converter.CanConvert(typeof(Transform)), Is.False); + Assert.That(converter.CanConvert(typeof(Matrix4x4)), Is.False); + } + + [Test] + public void CanConvert_MatchesTransformHandleByName() + { + Type handleType = TransformHandleType; + if (handleType == null) + Assert.Ignore("UnityEngine.TransformHandle does not exist on this Unity version."); + + Assert.That(new TransformHandleConverter().CanConvert(handleType), Is.True); + } + + [Test] + public void Serialize_LiveHandle_WritesNull() + { + var go = new GameObject("TransformHandleConverterTests_Live"); + try + { + object handle = GetHandleFor(go.transform); + if (handle == null) + Assert.Ignore("No TransformHandle-returning property on this Unity version."); + + string json = JsonConvert.SerializeObject(handle, _settings); + Assert.That(json, Is.EqualTo("null")); + } + finally + { + UnityEngine.Object.DestroyImmediate(go); + } + } + + [Test] + public void Serialize_HandleOfDestroyedObject_DoesNotThrow() + { + // The regression: a handle obtained before its object is destroyed. Without + // the converter, serializing it enumerates its children and throws + // NullReferenceException from TransformHandle.AssertHandleIsValid. + var go = new GameObject("TransformHandleConverterTests_Destroyed"); + object handle = GetHandleFor(go.transform); + UnityEngine.Object.DestroyImmediate(go); + + if (handle == null) + Assert.Ignore("No TransformHandle-returning property on this Unity version."); + + string json = null; + Assert.DoesNotThrow(() => json = JsonConvert.SerializeObject(handle, _settings)); + Assert.That(json, Is.EqualTo("null")); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/TransformHandleConverterTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/TransformHandleConverterTests.cs.meta new file mode 100644 index 000000000..177aca79a --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/TransformHandleConverterTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: be5e4cb07a0b45b5a39740c520e258b2 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: