From acb456435af158453aff1e8b485d6ba03afc975c Mon Sep 17 00:00:00 2001 From: Ilona Tomkowicz <32700855+ilonatommy@users.noreply.github.com> Date: Wed, 19 Oct 2022 18:04:08 +0200 Subject: [PATCH] [wasm][debugger] Remove `JToken` properties that are for internal use only (#75958) * Keep underscore removable properties as constants. * Clean up before exposing externally. * Value types need cleanup as well. * Fixed RootHidden tests. * Applied a variation of @radical's idea. * Fix tests. * Reverted to @radical's version. * Revert 14a2be2eca82e0848e73130d7a8405e0f99f62bd. * Applied radical's suggestions. --- .../Common/InternalUseFieldName.cs | 38 +++++++++ .../MemberObjectsExplorer.cs | 79 ++++++++++++------- .../MemberReferenceResolver.cs | 11 ++- .../debugger/BrowserDebugProxy/MonoProxy.cs | 24 +++--- .../BrowserDebugProxy/ValueTypeClass.cs | 8 +- .../DebuggerTestSuite/DebuggerTestBase.cs | 14 ++++ .../EvaluateOnCallFrameTests.cs | 4 +- 7 files changed, 129 insertions(+), 49 deletions(-) create mode 100644 src/mono/wasm/debugger/BrowserDebugProxy/Common/InternalUseFieldName.cs diff --git a/src/mono/wasm/debugger/BrowserDebugProxy/Common/InternalUseFieldName.cs b/src/mono/wasm/debugger/BrowserDebugProxy/Common/InternalUseFieldName.cs new file mode 100644 index 0000000000000..1a591bd16f32d --- /dev/null +++ b/src/mono/wasm/debugger/BrowserDebugProxy/Common/InternalUseFieldName.cs @@ -0,0 +1,38 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#nullable enable + +using System.Collections.Generic; + +namespace Microsoft.WebAssembly.Diagnostics; + +internal sealed class InternalUseFieldName +{ + public static InternalUseFieldName Hidden = new(nameof(Hidden)); + public static InternalUseFieldName State = new(nameof(State)); + public static InternalUseFieldName Section = new(nameof(Section)); + public static InternalUseFieldName Owner = new(nameof(Owner)); + public static InternalUseFieldName IsStatic = new(nameof(IsStatic)); + public static InternalUseFieldName IsNewSlot = new(nameof(IsNewSlot)); + public static InternalUseFieldName IsBackingField = new(nameof(IsBackingField)); + public static InternalUseFieldName ParentTypeId = new(nameof(ParentTypeId)); + + private static readonly HashSet s_names = new() + { + Hidden.Name, + State.Name, + Section.Name, + Owner.Name, + IsStatic.Name, + IsNewSlot.Name, + IsBackingField.Name, + ParentTypeId.Name + }; + + private InternalUseFieldName(string fieldName) => Name = $"__{fieldName}__"; + + public static int Count => s_names.Count; + public static bool IsKnown(string name) => !string.IsNullOrEmpty(name) && s_names.Contains(name); + public string Name { get; init; } +} diff --git a/src/mono/wasm/debugger/BrowserDebugProxy/MemberObjectsExplorer.cs b/src/mono/wasm/debugger/BrowserDebugProxy/MemberObjectsExplorer.cs index 0a854864306cb..f36c046fae4bf 100644 --- a/src/mono/wasm/debugger/BrowserDebugProxy/MemberObjectsExplorer.cs +++ b/src/mono/wasm/debugger/BrowserDebugProxy/MemberObjectsExplorer.cs @@ -63,17 +63,17 @@ private static async Task ReadFieldValue( // for backing fields, we are getting it from the properties typePropertiesBrowsableInfo.TryGetValue(field.Name, out state); } - fieldValue["__state"] = state?.ToString(); - fieldValue["__section"] = field.Attributes.HasFlag(FieldAttributes.Private) + fieldValue[InternalUseFieldName.State.Name] = state?.ToString(); + fieldValue[InternalUseFieldName.Section.Name] = field.Attributes.HasFlag(FieldAttributes.Private) ? "private" : "result"; if (field.IsBackingField) { - fieldValue["__isBackingField"] = true; - fieldValue["__parentTypeId"] = parentTypeId; + fieldValue[InternalUseFieldName.IsBackingField.Name] = true; + fieldValue[InternalUseFieldName.ParentTypeId.Name] = parentTypeId; } if (field.Attributes.HasFlag(FieldAttributes.Static)) - fieldValue["__isStatic"] = true; + fieldValue[InternalUseFieldName.IsStatic.Name] = true; if (getObjectOptions.HasFlag(GetObjectCommandOptions.WithSetter)) { @@ -236,7 +236,7 @@ public static async Task ExpandFieldValues( if (typeInfo.Info.IsNonUserCode && getCommandOptions.HasFlag(GetObjectCommandOptions.JustMyCode) && field.Attributes.HasFlag(FieldAttributes.Private)) continue; - if (!Enum.TryParse(fieldValue["__state"].Value(), out DebuggerBrowsableState fieldState) + if (!Enum.TryParse(fieldValue[InternalUseFieldName.State.Name].Value(), out DebuggerBrowsableState fieldState) || fieldState == DebuggerBrowsableState.Collapsed) { fieldValues.Add(fieldValue); @@ -296,11 +296,9 @@ public static async Task GetExpandedMemberValues( JArray GetHiddenElement() { - return new JArray(JObject.FromObject(new - { - name = namePrefix, - __hidden = true - })); + var emptyHidden = JObject.FromObject(new { name = namePrefix }); + emptyHidden.Add(InternalUseFieldName.Hidden.Name, true); + return new JArray(emptyHidden); } } @@ -361,14 +359,14 @@ public static async Task> ExpandPropertyValues( continue; } - bool isExistingMemberABackingField = existingMember["__isBackingField"]?.Value() == true; + bool isExistingMemberABackingField = existingMember[InternalUseFieldName.IsBackingField.Name]?.Value() == true; if (isOwn && !isExistingMemberABackingField) { // repeated propname on the same type! cannot happen throw new Exception($"Internal Error: should not happen. propName: {propName}. Existing all members: {string.Join(",", allMembers.Keys)}"); } - bool isExistingMemberABackingFieldOwnedByThisType = isExistingMemberABackingField && existingMember["__owner"]?.Value() == typeName; + bool isExistingMemberABackingFieldOwnedByThisType = isExistingMemberABackingField && existingMember[InternalUseFieldName.Owner.Name]?.Value() == typeName; if (isExistingMemberABackingField && (isOwn || isExistingMemberABackingFieldOwnedByThisType)) { // this is the property corresponding to the backing field in *this* type @@ -382,8 +380,8 @@ public static async Task> ExpandPropertyValues( { // this has `new` keyword if it is newSlot but direct child was not a newSlot: var child = allMembers.FirstOrDefault( - kvp => (kvp.Key == propName || kvp.Key.StartsWith($"{propName} (")) && kvp.Value["__parentTypeId"]?.Value() == typeId).Value; - bool wasOverriddenByDerivedType = child != null && child["__isNewSlot"]?.Value() != true; + kvp => (kvp.Key == propName || kvp.Key.StartsWith($"{propName} (")) && kvp.Value[InternalUseFieldName.ParentTypeId.Name]?.Value() == typeId).Value; + bool wasOverriddenByDerivedType = child != null && child[InternalUseFieldName.IsNewSlot.Name]?.Value() != true; if (wasOverriddenByDerivedType) { /* @@ -409,7 +407,7 @@ public static async Task> ExpandPropertyValues( */ JObject backingFieldForHiddenProp = allMembers.GetValueOrDefault(overriddenOrHiddenPropName); - if (backingFieldForHiddenProp is null || backingFieldForHiddenProp["__isBackingField"]?.Value() != true) + if (backingFieldForHiddenProp is null || backingFieldForHiddenProp[InternalUseFieldName.IsBackingField.Name]?.Value() != true) { // hiding with a non-auto property, so nothing to adjust // add the new property @@ -424,12 +422,12 @@ public static async Task> ExpandPropertyValues( async Task UpdateBackingFieldWithPropertyAttributes(JObject backingField, string autoPropName, MethodAttributes getterMemberAccessAttrs, DebuggerBrowsableState? state) { - backingField["__section"] = getterMemberAccessAttrs switch + backingField[InternalUseFieldName.Section.Name] = getterMemberAccessAttrs switch { MethodAttributes.Private => "private", _ => "result" }; - backingField["__state"] = state?.ToString(); + backingField[InternalUseFieldName.State.Name] = state?.ToString(); if (state is null) return; @@ -472,16 +470,16 @@ async Task AddProperty( } propRet["isOwn"] = isOwn; - propRet["__section"] = getterAttrs switch + propRet[InternalUseFieldName.Section.Name] = getterAttrs switch { MethodAttributes.Private => "private", _ => "result" }; - propRet["__state"] = state?.ToString(); + propRet[InternalUseFieldName.State.Name] = state?.ToString(); if (parentTypeId != -1) { - propRet["__parentTypeId"] = parentTypeId; - propRet["__isNewSlot"] = isNewSlot; + propRet[InternalUseFieldName.ParentTypeId.Name] = parentTypeId; + propRet[InternalUseFieldName.IsNewSlot.Name] = isNewSlot; } string namePrefix = GetNamePrefixForValues(propNameWithSufix, typeName, isOwn, state); @@ -586,7 +584,7 @@ public static async Task GetObjectMemberValues( if (getCommandType.HasFlag(GetObjectCommandOptions.AccessorPropertiesOnly)) { foreach (var f in allFields) - f["__hidden"] = true; + f[InternalUseFieldName.Hidden.Name] = true; } AddOnlyNewFieldValuesByNameTo(allFields, allMembers, typeName, isOwn); } @@ -632,8 +630,8 @@ static void AddOnlyNewFieldValuesByNameTo(JArray namedValues, IDictionary() == true) - item["__owner"] = typeName; + if (item[InternalUseFieldName.IsBackingField.Name]?.Value() == true) + item[InternalUseFieldName.Owner.Name] = typeName; continue; } @@ -676,6 +674,33 @@ public GetMembersResult(JArray value, bool sortByAccessLevel) PrivateMembers = t.PrivateMembers; } + public void CleanUp() + { + JProperty[] toRemoveInObject = new JProperty[InternalUseFieldName.Count]; + + CleanUpJArray(Result); + CleanUpJArray(PrivateMembers); + + void CleanUpJArray(JArray arr) + { + foreach (JToken item in arr) + { + if (item is not JObject jobj || jobj.Count == 0) + continue; + + int removeCount = 0; + foreach (JProperty jp in jobj.Properties()) + { + if (InternalUseFieldName.IsKnown(jp.Name)) + toRemoveInObject[removeCount++] = jp; + } + + for (int i = 0; i < removeCount; i++) + toRemoveInObject[i].Remove(); + } + } + } + public static GetMembersResult FromValues(IEnumerable values, bool splitMembersByAccessLevel = false) => FromValues(new JArray(values), splitMembersByAccessLevel); @@ -694,10 +719,10 @@ public static GetMembersResult FromValues(JArray values, bool splitMembersByAcce private void Split(JToken member) { - if (member["__hidden"]?.Value() == true) + if (member[InternalUseFieldName.Hidden.Name]?.Value() == true) return; - if (member["__section"]?.Value() is not string section) + if (member[InternalUseFieldName.Section.Name]?.Value() is not string section) { Result.Add(member); return; diff --git a/src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs b/src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs index 8cd27c6e27b93..b1f978419a9aa 100644 --- a/src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs +++ b/src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs @@ -263,9 +263,14 @@ async Task ResolveAsLocalOrThisMember(string name) { if (scopeCache.Locals.Count == 0 && !localsFetched) { - Result scope_res = await proxy.GetScopeProperties(sessionId, scopeId, token); - if (!scope_res.IsOk) - throw new ExpressionEvaluationFailedException($"BUG: Unable to get properties for scope: {scopeId}. {scope_res}"); + try + { + await proxy.GetScopeProperties(sessionId, scopeId, token); + } + catch (Exception ex) + { + throw new ExpressionEvaluationFailedException($"BUG: Unable to get properties for scope: {scopeId}. {ex}"); + } localsFetched = true; } diff --git a/src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs b/src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs index 5a19d42e2cad6..68f543232a221 100644 --- a/src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs +++ b/src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs @@ -742,15 +742,15 @@ internal async Task> RuntimeGetObjectMembers(Sess { switch (objectId.Scheme) { + // ToDo: fix Exception types here case "scope": - Result resScope = await GetScopeProperties(id, objectId.Value, token); - return resScope.IsOk - ? ValueOrError.WithValue( - new GetMembersResult((JArray)resScope.Value?["result"], sortByAccessLevel: false)) - : ValueOrError.WithError(resScope); + GetMembersResult resScope = await GetScopeProperties(id, objectId.Value, token); + resScope.CleanUp(); + return ValueOrError.WithValue(resScope); case "valuetype": var resValue = await MemberObjectsExplorer.GetValueTypeMemberValues( context.SdbAgent, objectId.Value, getObjectOptions, token, sortByAccessLevel, includeStatic: true); + resValue?.CleanUp(); return resValue switch { null => ValueOrError.WithError($"Could not get properties for {objectId}"), @@ -765,6 +765,7 @@ internal async Task> RuntimeGetObjectMembers(Sess case "object": var resObj = await MemberObjectsExplorer.GetObjectMemberValues( context.SdbAgent, objectId.Value, getObjectOptions, token, sortByAccessLevel, includeStatic: true); + resObj.CleanUp(); return ValueOrError.WithValue(resObj); case "pointer": var resPointer = new JArray { await context.SdbAgent.GetPointerContent(objectId.Value, token) }; @@ -1393,14 +1394,14 @@ private async Task OnEvaluateOnCallFrame(MessageId msg_id, int scopeId, st return true; } - internal async Task GetScopeProperties(SessionId msg_id, int scopeId, CancellationToken token) + internal async Task GetScopeProperties(SessionId msg_id, int scopeId, CancellationToken token) { try { ExecutionContext context = GetContext(msg_id); Frame scope = context.CallStack.FirstOrDefault(s => s.Id == scopeId); if (scope == null) - return Result.Err(JObject.FromObject(new { message = $"Could not find scope with id #{scopeId}" })); + throw new Exception($"Could not find scope with id #{scopeId}"); VarInfo[] varIds = scope.Method.Info.GetLiveVarsAt(scope.Location.IlLocation.Offset); @@ -1408,21 +1409,20 @@ internal async Task GetScopeProperties(SessionId msg_id, int scopeId, Ca if (values != null) { if (values == null || values.Count == 0) - return Result.OkFromObject(new { result = Array.Empty() }); + return new GetMembersResult(); PerScopeCache frameCache = context.GetCacheForScope(scopeId); foreach (JObject value in values) { frameCache.Locals[value["name"]?.Value()] = value; } - return Result.OkFromObject(new { result = values }); + return GetMembersResult.FromValues(values); } - return Result.OkFromObject(new { result = Array.Empty() }); + return new GetMembersResult(); } catch (Exception exception) { - Log("verbose", $"Error resolving scope properties {exception.Message}"); - return Result.Exception(exception); + throw new Exception($"Error resolving scope properties {exception.Message}"); } } diff --git a/src/mono/wasm/debugger/BrowserDebugProxy/ValueTypeClass.cs b/src/mono/wasm/debugger/BrowserDebugProxy/ValueTypeClass.cs index bcd34e9b7d0cc..b0977d4facd79 100644 --- a/src/mono/wasm/debugger/BrowserDebugProxy/ValueTypeClass.cs +++ b/src/mono/wasm/debugger/BrowserDebugProxy/ValueTypeClass.cs @@ -99,15 +99,15 @@ JObject GetFieldWithMetadata(FieldTypeClass field, JObject fieldValue, bool isSt if (isStatic) fieldValue["name"] = field.Name; FieldAttributes attr = field.Attributes & FieldAttributes.FieldAccessMask; - fieldValue["__section"] = attr == FieldAttributes.Private ? "private" : "result"; + fieldValue[InternalUseFieldName.Section.Name] = attr == FieldAttributes.Private ? "private" : "result"; if (field.IsBackingField) { - fieldValue["__isBackingField"] = true; + fieldValue[InternalUseFieldName.IsBackingField.Name] = true; return fieldValue; } typeFieldsBrowsableInfo.TryGetValue(field.Name, out DebuggerBrowsableState? state); - fieldValue["__state"] = state?.ToString(); + fieldValue[InternalUseFieldName.State.Name] = state?.ToString(); return fieldValue; } } @@ -253,7 +253,7 @@ public async Task ExpandedFieldValues(MonoSDBHelper sdbHelper, bool includeStati JArray visibleFields = new(); foreach (JObject field in fields) { - if (!Enum.TryParse(field["__state"]?.Value(), out DebuggerBrowsableState state)) + if (!Enum.TryParse(field[InternalUseFieldName.State.Name]?.Value(), out DebuggerBrowsableState state)) { visibleFields.Add(field); continue; diff --git a/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs b/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs index b533d39910ac1..906803936c160 100644 --- a/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs +++ b/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs @@ -925,6 +925,18 @@ internal async Task GetObjectOnLocals(JToken locals, string name) return await GetProperties(objectId); } + internal void AssertInternalUseFieldsAreRemoved(JToken item) + { + if (item is JObject jobj && jobj.Count != 0) + { + foreach (JProperty jp in jobj.Properties()) + { + Assert.False(InternalUseFieldName.IsKnown(jp.Name), + $"Property {jp.Name} of object: {jobj} is for internal proxy use and should not be exposed externally."); + } + } + } + /* @fn_args is for use with `Runtime.callFunctionOn` only */ internal virtual async Task GetProperties(string id, JToken fn_args = null, bool? own_properties = null, bool? accessors_only = null, bool expect_ok = true) { @@ -978,6 +990,7 @@ internal virtual async Task GetProperties(string id, JToken fn_args = nu { foreach (var p in locals) { + AssertInternalUseFieldsAreRemoved(p); if (p["name"]?.Value() == "length" && p["enumerable"]?.Value() != true) { p.Remove(); @@ -1035,6 +1048,7 @@ internal virtual async Task GetProperties(string id, JToken fn_args = nu { foreach (var p in locals) { + AssertInternalUseFieldsAreRemoved(p); if (p["name"]?.Value() == "length" && p["enumerable"]?.Value() != true) { p.Remove(); diff --git a/src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs b/src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs index 30d391700c9af..3e4e588bd082a 100644 --- a/src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs +++ b/src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs @@ -1180,9 +1180,7 @@ public async Task EvaluateBrowsableRootHidden( var (refList, _) = await EvaluateOnCallFrame(id, "testPropertiesNone.list"); var refListProp = await GetProperties(refList["objectId"]?.Value()); - var list = refListProp - .Where(v => v["name"]?.Value() == "Items" || v["name"]?.Value() == "_items") - .FirstOrDefault(); + var list = refListProp.First(v => v["name"]?.Value() is "Items" or "_items"); var refListElementsProp = await GetProperties(list["value"]["objectId"]?.Value()); var (refArray, _) = await EvaluateOnCallFrame(id, "testPropertiesNone.array");