WpfGfx: Mitigate RemoveChild crash

[etvorun]

Summary

Mitigates a crash in visual child removal by making CMilVisual::RemoveChild tolerant of already-detached children.

What changed

• Updated src/Microsoft.DotNet.Wpf/src/WpfGfx/core/resources/node.cpp in CMilVisual::RemoveChild.
• Added a guard for the case where m_rgpChildren.Remove(pChild) fails but pChild->GetParent() is NULL.
• Return S_OK for this detached-child case to make the operation idempotent.

Why

In practice, child removal can occasionally be requested for a child that is already detached. Treating this as a hard invalid-argument failure can crash the process. Returning success for the already-detached case preserves stability while keeping strict failure behavior for other invalid states.

Validation

• Manual validation only, per repository workflow.
• No local build or test commands were run.

Fixes #11464

Microsoft Reviewers: Open in CodeFlow

[Copilot]

Pull request overview

This PR mitigates a crash in WpfGfx's visual tree management by making CMilVisual::RemoveChild tolerant of already-detached children. The change addresses a rare timing issue where a remove command might be processed for a child that has already been detached from its parent.

Changes:

• Added a guard in RemoveChild to check if a child's parent is NULL when removal from the collection fails
• Returns S_OK (success) for the already-detached case to make the operation idempotent
• Preserves existing E_INVALIDARG behavior for other invalid states

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The logic here is incomplete and could cause silent failures. When m_rgpChildren.Remove(pChild) fails, there are three possible states:

1. pChild->GetParent() == NULL (already detached) - the code handles this
2. pChild->GetParent() == this (inconsistent state: child thinks it has this parent, but isn't in the collection) - this falls through to E_INVALIDARG correctly
3. pChild->GetParent() == someOtherParent (child belongs to a different parent) - this currently returns S_OK inappropriately

The check should verify that pChild->GetParent() is either NULL (already detached) or equal to this (inconsistent state to fail on). If the parent is a different visual, this is a programming error and should fail with E_INVALIDARG.

Suggested change

	// command. There are no known repros. For now we'll pretend we processed it.
	if (pChild->GetParent() == NULL)
	{
	return S_OK;
	}
	// command. There are no known repros. For now we'll pretend we processed it
	// only if the child no longer has a parent. Any other parent state is a
	// programming error and should fail.
	CMilVisual *pParent = pChild->GetParent();
	if (pParent == NULL)
	{
	return S_OK;
	}
	// If the child still has a parent at this point, then either:
	// - pParent == this: the child thinks it belongs to this visual but is
	// missing from m_rgpChildren, or
	// - pParent != this: the child belongs to a different visual.
	// Both represent inconsistent or invalid state and are treated as errors.
	if (pParent != this)
	{
	IFC(E_INVALIDARG);
	}

[Copilot]

The comment references "Watson dumps" which is Microsoft's internal crash reporting system. Consider whether this terminology should be more generic for the open-source version of WPF, or if it should reference the specific issue number (Issue #11464) instead for better traceability.

Suggested change

	// We've seen a handful of Watson dumps where incoming child is already detached
	// from parent and about to be deleted by the next MilCmdChannelDeleteResource
	// command. There are no known repros. For now we'll pretend we processed it.
	// We've seen a handful of crash reports (see Issue #11464) where the incoming
	// child is already detached from its parent and about to be deleted by the next
	// MilCmdChannelDeleteResource command. There are no known repros. For now we'll
	// pretend we processed it.