Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm][debugger] Remove JToken properties that are for internal use only #75958

Merged
merged 10 commits into from
Oct 19, 2022

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Sep 21, 2022

Suggested here: #75904 (comment).
Enhancement of Debugger Proxy communication. We used to have additional json properties that start with __ for internal use. We don't need to expose them outside though, so we can choose an option to clean them now. This does not affect tests.

RootHidden test fix is not connected with the removal, they were just failing randomly (when the failing test was run separately it was passing but when in a bunch it was failing). The reason is that sometimes Generic collection member __items has additional empty members added in the end of the set. Now we are making sure we take into consideration only first N elements, where N is the size of the collection.

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-Debugger-mono labels Sep 21, 2022
@ilonatommy ilonatommy added this to the 7.0.0 milestone Sep 21, 2022
@ilonatommy ilonatommy self-assigned this Sep 21, 2022
@ghost
Copy link

ghost commented Sep 21, 2022

Tagging subscribers to this area: @thaystg
See info in area-owners.md if you want to be subscribed.

Issue Details

Suggested here: #75904 (comment).
Enhancement of Debugger Proxy communication. We used to have additional json properties that start with __ for internal use. We don't need to expose them outside though, so we can choose an option to clean them now. This does not affect tests.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-Debugger-mono

Milestone: 7.0.0

@ghost
Copy link

ghost commented Sep 21, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Suggested here: #75904 (comment).
Enhancement of Debugger Proxy communication. We used to have additional json properties that start with __ for internal use. We don't need to expose them outside though, so we can choose an option to clean them now. This does not affect tests.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-Debugger-mono

Milestone: 7.0.0

@ilonatommy ilonatommy marked this pull request as draft September 29, 2022 12:38
@ilonatommy
Copy link
Member Author

ilonatommy commented Sep 30, 2022

The CI failure might be caused by #75392 - we have Thread.Sleep(1000); there as well.

@ilonatommy ilonatommy marked this pull request as ready for review September 30, 2022 13:06
Comment on lines 156 to 157
// sometimes items have dummy empty elements added after real items
maxSize = members.FirstOrDefault(m => m["name"]?.Value<string>() == "_size")?["value"]?["value"]?.Value<int>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this doing? And who adds _size, and _items?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so these are private fields for List<T>. We shouldn't depend on a type's private members like this. What do the "dummy empty elements" look like?

The error condition can be recreated by a = new List<int>(capacity: 20); a.Add(1); -> now the internal array _items will be of size 20, but it will have only 1 real value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does VS show all the elements in _items?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, you can use list.Capacity to know the size of _items.

Copy link
Member Author

@ilonatommy ilonatommy Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 2 behaviors in DebuggerTests.EvaluateOnCallFrameTests.EvaluateBrowsableRootHidden. The data:

[InlineData("EvaluateBrowsableClass", "TestEvaluateFieldsRootHidden", "testFieldsRootHidden", 10)]
[InlineData("EvaluateBrowsableClass", "TestEvaluatePropertiesRootHidden", "testPropertiesRootHidden", 10)]
[InlineData("EvaluateBrowsableStruct", "TestEvaluatePropertiesRootHidden", "testPropertiesRootHidden", 10)]
[InlineData("EvaluateBrowsableClassStatic", "TestEvaluateFieldsRootHidden", "testFieldsRootHidden", 10)]
[InlineData("EvaluateBrowsableClassStatic", "TestEvaluatePropertiesRootHidden", "testPropertiesRootHidden", 10)]
[InlineData("EvaluateBrowsableStructStatic", "TestEvaluateFieldsRootHidden", "testFieldsRootHidden", 10)]
[InlineData("EvaluateBrowsableStructStatic", "TestEvaluatePropertiesRootHidden", "testPropertiesRootHidden", 10)]
[InlineData("EvaluateBrowsableNonAutoPropertiesClass", "TestEvaluatePropertiesRootHidden", "testPropertiesRootHidden", 5)]
[InlineData("EvaluateBrowsableNonAutoPropertiesClassStatic", "TestEvaluatePropertiesRootHidden", "testPropertiesRootHidden", 5)]

result in producing the information you are talking about. On calling GetObjectMemberValues for listRootHidden in GetRootHiddenChildren we get a publicly available object that looks like this:

  "value": {
    "type": "object",
    "value": null,
    "description": "int[2]",
    "className": "int[]",
    "objectId": "dotnet:array:22",
    "subtype": "array"
  },
  "writable": false,
  "name": "Items",
  "isOwn": true,
  "__section__": "result",
  "__state__": null,
  "__parentTypeId__": 2,
  "__isNewSlot__": false
}

and it does not cause any problems: array has only two elements, it's public and always named Items, so after calling
JArray resultValue = await sdbHelper.GetArrayValues(rootObjectId.Value, token); on it, we will get the items we should expose.
However for these cases:

[InlineData("EvaluateBrowsableStruct", "TestEvaluateFieldsRootHidden", "testFieldsRootHidden", 10)]
[InlineData("EvaluateBrowsableNonAutoPropertiesStruct", "TestEvaluatePropertiesRootHidden", "testPropertiesRootHidden", 5)]
[InlineData("EvaluateBrowsableNonAutoPropertiesStructStatic", "TestEvaluatePropertiesRootHidden", "testPropertiesRootHidden", 5)]

we are getting only following private objects:

{
  "value": {
    "type": "object",
    "value": null,
    "description": "int[4]",
    "className": "int[]",
    "objectId": "dotnet:array:46",
    "subtype": "array"
  },
  "writable": false,
  "isOwn": true,
  "name": "_items",
  "__state__": null,
  "__section__": "private"
}
{
  "value": {
    "type": "number",
    "value": 2,
    "description": "2"
  },
  "writable": true,
  "isOwn": true,
  "name": "_size",
  "__state__": null,
  "__section__": "private"
}
{
  "value": {
    "type": "number",
    "value": 2,
    "description": "2"
  },
  "writable": true,
  "isOwn": true,
  "name": "_version",
  "__state__": null,
  "__section__": "private"
}
{
  "value": {
    "type": "object",
    "value": null,
    "description": "int[0]",
    "className": "int[]",
    "objectId": "dotnet:array:66",
    "subtype": "array"
  },
  "writable": false,
  "isOwn": true,
  "name": "s_emptyArray",
  "__state__": null,
  "__section__": "private",
  "__isStatic__": true
}

The only way to get access to children of the hidden root is to call GetArrayValues on private _items object. As you can see, it has 4 elements even though the size of the list == 2, they are like this (empty elements always in the end):

{
  "value": {
    "type": "number",
    "value": 1,
    "description": "1"
  },
  "writable": true,
  "name": "0"
}
{
  "value": {
    "type": "number",
    "value": 2,
    "description": "2"
  },
  "writable": true,
  "name": "1"
}
{
  "value": {
    "type": "number",
    "value": 0,
    "description": "0"
  },
  "writable": true,
  "name": "2"
}
{
  "value": {
    "type": "number",
    "value": 0,
    "description": "0"
  },
  "writable": true,
  "name": "3"
}

In Console Application we see the same data when we expand private members.
image

Copy link
Member Author

@ilonatommy ilonatommy Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, you can use list.Capacity to know the size of _items.

In the screenshot above we can se it would not work, capacity is 4 even though our array has only 2 elements. Count is fine but we don't have it in available members.

Copy link
Member Author

@ilonatommy ilonatommy Oct 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToDo: check what is failing without this change and why it was not failing before clearing the __ fields.

Edit: answer is - EvaluateBrowsableRootHidden is failing on main:

  Failed DebuggerTests.EvaluateOnCallFrameTests.EvaluateBrowsableRootHidden(outerClassName: "EvaluateBrowsableClassStatic", className: "TestEvaluatePropertiesRootHidden", localVarName: "testPropertiesRootHidden", breakLine: 10) [993 ms]
  Error Message:
   Assert.Equal() Failure
Expected: 10
Actual:   8
Failed:     1, Passed:    11, Skipped:     0, Total:    12

Reason:
refListElementsProp has 4 elements while it should have only 2 (it has 2 empty reserved places in memory as discussed before and they are being returned along with legible list members).

What now:
I will make a separate PR for it and revert the changes here. We will merge the other PR first.

@radical
Copy link
Member

radical commented Oct 3, 2022

And check for such properties in the tests, where we get results from GetProperties, EvaluateOnCallFrame etc. It could be a simple endswith(internal_), or whatever suffix is being used.

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the comments, the PR looks good 👍 Thanks for the effort, and your patience ;)

@ilonatommy ilonatommy merged commit acb4564 into dotnet:main Oct 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Debugger-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants