Skip to content

Conversation

@stecrain
Copy link
Contributor

@stecrain stecrain commented Jun 21, 2019

I found this with windbg and !idallocs provided by the UIExtensions.

Internal to Microsoft you should be able to open windbg, attach to a running React Native Windows App and use the command !idallocs, if it does not get the ui extensions automatically for you, then you probably need to update your debuggers.

Before this fix you could see the number of Windows_UI_Xaml!CPanel objects increasing as you visit more pages.
image

Using the !xamlTree debugger extensions you can see that some of the panels have no children, and their parent is a border. This made the leak pretty easy to pinpoint.
image

After this fix I am able to Go back and forth between pages (starting and ending on the same page), and see that the number of Windows_UI_Xaml!CPanel objects in the heap remains constant.
image

Microsoft Reviewers: Open in CodeFlow

@stecrain stecrain requested a review from a team as a code owner June 21, 2019 15:52
@ghost ghost added the vnext label Jun 21, 2019
@ahimberg ahimberg requested a review from randy-flynn June 21, 2019 17:02

winrt::com_ptr<ViewPanel> GetViewPanel()
winrt::com_ptr<ViewPanel> GetViewPanel(bool removeAllChildren = false)
{
Copy link
Contributor

@randy-flynn randy-flynn Jun 21, 2019

Choose a reason for hiding this comment

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

This feels like an odd way to induce removal of the children. Why this instead of just embedding this in removeAllChildren()? Just the logic for getting the container? #Closed

Copy link
Contributor Author

@stecrain stecrain Jun 21, 2019

Choose a reason for hiding this comment

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

I didn't want to duplicate the logic to check for the control and the border.
But now that you mention it, including a param removeAllChildren on GetViewPanel does seem strange. I'll take another crack at this. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

It also might work to include more helper functions so you don't duplicate too much logic.


In reply to: 296325388 [](ancestors = 296325388)

Copy link
Contributor

@randy-flynn randy-flynn left a comment

Choose a reason for hiding this comment

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

🕐

@ghost ghost added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Jun 21, 2019
@ghost ghost removed the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Jun 21, 2019
Copy link
Contributor

@randy-flynn randy-flynn left a comment

Choose a reason for hiding this comment

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

:shipit:

@stecrain stecrain added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jun 21, 2019
@ghost
Copy link

ghost commented Jun 21, 2019

Hello @stecrain!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit e145135 into microsoft:master Jun 21, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants