[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 14a2be2.

* Applied radical's suggestions.

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<string> 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; }
+}

src/mono/wasm/debugger/BrowserDebugProxy/MemberObjectsExplorer.cs

@@ -63,17 +63,17 @@ private static async Task<JObject> 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<JArray> ExpandFieldValues(
  if (typeInfo.Info.IsNonUserCode && getCommandOptions.HasFlag(GetObjectCommandOptions.JustMyCode) && field.Attributes.HasFlag(FieldAttributes.Private))
  continue;
 
- if (!Enum.TryParse(fieldValue["__state"].Value<string>(), out DebuggerBrowsableState fieldState)
+ if (!Enum.TryParse(fieldValue[InternalUseFieldName.State.Name].Value<string>(), out DebuggerBrowsableState fieldState)
  || fieldState == DebuggerBrowsableState.Collapsed)
  {
  fieldValues.Add(fieldValue);
@@ -296,11 +296,9 @@ public static async Task<JArray> 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<Dictionary<string, JObject>> ExpandPropertyValues(
  continue;
  }
 
- bool isExistingMemberABackingField = existingMember["__isBackingField"]?.Value<bool>() == true;
+ bool isExistingMemberABackingField = existingMember[InternalUseFieldName.IsBackingField.Name]?.Value<bool>() == 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<string>() == typeName;
+ bool isExistingMemberABackingFieldOwnedByThisType = isExistingMemberABackingField && existingMember[InternalUseFieldName.Owner.Name]?.Value<string>() == 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<Dictionary<string, JObject>> 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<int>() == typeId).Value;
- bool wasOverriddenByDerivedType = child != null && child["__isNewSlot"]?.Value<bool>() != true;
+ kvp => (kvp.Key == propName || kvp.Key.StartsWith($"{propName} (")) && kvp.Value[InternalUseFieldName.ParentTypeId.Name]?.Value<int>() == typeId).Value;
+ bool wasOverriddenByDerivedType = child != null && child[InternalUseFieldName.IsNewSlot.Name]?.Value<bool>() != true;
  if (wasOverriddenByDerivedType)
  {
  /*
@@ -409,7 +407,7 @@ public static async Task<Dictionary<string, JObject>> ExpandPropertyValues(
  */
 
  JObject backingFieldForHiddenProp = allMembers.GetValueOrDefault(overriddenOrHiddenPropName);
- if (backingFieldForHiddenProp is null || backingFieldForHiddenProp["__isBackingField"]?.Value<bool>() != true)
+ if (backingFieldForHiddenProp is null || backingFieldForHiddenProp[InternalUseFieldName.IsBackingField.Name]?.Value<bool>() != true)
  {
  // hiding with a non-auto property, so nothing to adjust
  // add the new property
@@ -424,12 +422,12 @@ public static async Task<Dictionary<string, JObject>> 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<GetMembersResult> 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<string
  if (valuesDict.TryAdd(name, item as JObject))
  {
  // new member
- if (item["__isBackingField"]?.Value<bool>() == true)
- item["__owner"] = typeName;
+ if (item[InternalUseFieldName.IsBackingField.Name]?.Value<bool>() == 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<JToken> 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<bool>() == true)
+ if (member[InternalUseFieldName.Hidden.Name]?.Value<bool>() == true)
  return;
 
- if (member["__section"]?.Value<string>() is not string section)
+ if (member[InternalUseFieldName.Section.Name]?.Value<string>() is not string section)
  {
  Result.Add(member);
  return;

src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs

@@ -263,9 +263,14 @@ async Task<JObject> 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;
  }
 

src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs

@@ -742,15 +742,15 @@ internal async Task<ValueOrError<GetMembersResult>> RuntimeGetObjectMembers(Sess
  {
  switch (objectId.Scheme)
  {
+ // ToDo: fix Exception types here
  case "scope":
- Result resScope = await GetScopeProperties(id, objectId.Value, token);
- return resScope.IsOk
- ? ValueOrError<GetMembersResult>.WithValue(
- new GetMembersResult((JArray)resScope.Value?["result"], sortByAccessLevel: false))
- : ValueOrError<GetMembersResult>.WithError(resScope);
+ GetMembersResult resScope = await GetScopeProperties(id, objectId.Value, token);
+ resScope.CleanUp();
+ return ValueOrError<GetMembersResult>.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<GetMembersResult>.WithError($"Could not get properties for {objectId}"),
@@ -765,6 +765,7 @@ internal async Task<ValueOrError<GetMembersResult>> RuntimeGetObjectMembers(Sess
  case "object":
  var resObj = await MemberObjectsExplorer.GetObjectMemberValues(
  context.SdbAgent, objectId.Value, getObjectOptions, token, sortByAccessLevel, includeStatic: true);
+ resObj.CleanUp();
  return ValueOrError<GetMembersResult>.WithValue(resObj);
  case "pointer":
  var resPointer = new JArray { await context.SdbAgent.GetPointerContent(objectId.Value, token) };
@@ -1393,36 +1394,35 @@ private async Task<bool> OnEvaluateOnCallFrame(MessageId msg_id, int scopeId, st
  return true;
  }
 
- internal async Task<Result> GetScopeProperties(SessionId msg_id, int scopeId, CancellationToken token)
+ internal async Task<GetMembersResult> 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);
 
  var values = await context.SdbAgent.StackFrameGetValues(scope.Method, context.ThreadId, scopeId, varIds, token);
  if (values != null)
  {
  if (values == null || values.Count == 0)
- return Result.OkFromObject(new { result = Array.Empty<object>() });
+ return new GetMembersResult();
 
  PerScopeCache frameCache = context.GetCacheForScope(scopeId);
  foreach (JObject value in values)
  {
  frameCache.Locals[value["name"]?.Value<string>()] = value;
  }
- return Result.OkFromObject(new { result = values });
+ return GetMembersResult.FromValues(values);
  }
- return Result.OkFromObject(new { result = Array.Empty<object>() });
+ 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}");
  }
  }
 

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<string>(), out DebuggerBrowsableState state))
+ if (!Enum.TryParse(field[InternalUseFieldName.State.Name]?.Value<string>(), out DebuggerBrowsableState state))
  {
  visibleFields.Add(field);
  continue;

src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs

@@ -925,6 +925,18 @@ internal async Task<JToken> 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<JToken> 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<JToken> GetProperties(string id, JToken fn_args = nu
  {
  foreach (var p in locals)
  {
+ AssertInternalUseFieldsAreRemoved(p);
  if (p["name"]?.Value<string>() == "length" && p["enumerable"]?.Value<bool>() != true)
  {
  p.Remove();
@@ -1035,6 +1048,7 @@ internal virtual async Task<JToken> GetProperties(string id, JToken fn_args = nu
  {
  foreach (var p in locals)
  {
+ AssertInternalUseFieldsAreRemoved(p);
  if (p["name"]?.Value<string>() == "length" && p["enumerable"]?.Value<bool>() != true)
  {
  p.Remove();