Skip to content

Commit

Permalink
Fixing memory leaks of Numeric/DomainUpDown elements accessible objec…
Browse files Browse the repository at this point in the history
…ts (#7330)

Fixes #7328
⚠️Doesn't fix memory leaks of TextPattern. It will be fixed as another fix for TextBox as well.

## Proposed changes

- Rework and refactor accessible objects implementation of UpDown controls. Removed `ItemList` and Item accessible objects, because they are unnecessary. Moved the implementation to the base class. [This comment](#4207 (comment)) describes why we don't keep ItemsList anymore. 
- Add a call of `UiaDisconnectProvider` for `Numeric/DomainUpDown` elements (its textbox and buttons) when the control is disposing.

## Customer Impact

- Less memory leaks

## Regression? 

- No

## Risk

- Minimal

## Screenshots <!-- Remove this section if PR does not change UI -->

### Before

- There are some left objects in memory due to `UpDownEdit` and `UpDownButtons`:
![image](https://user-images.githubusercontent.com/49272759/174846510-c6be894d-d85d-43f0-b439-f52f93ca8991.png)

### After
- Memory is clear after disposing:
![image](https://user-images.githubusercontent.com/49272759/174846839-57c3faa0-b76c-495f-8bd6-4a42d2d5bb38.png)



## Test methodology
- CTI
- Manual
- Unit tests

## Accessibility testing 
- Using Narrator/Inspect and WinDbg

## Test environment(s) 
- .NET 7.0 Preview 4
- Windows 11
  • Loading branch information
vladimir-krestov authored Aug 9, 2022
1 parent 4070954 commit eff830b
Show file tree
Hide file tree
Showing 18 changed files with 186 additions and 364 deletions.
41 changes: 41 additions & 0 deletions docs/list-of-diagnostics.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# List of Diagnostics Produced by Windows Forms .NET APIs

## Obsoletions

Per https://github.com/dotnet/designs/blob/master/accepted/2020/better-obsoletion/better-obsoletion.md and similar to https://github.com/dotnet/runtime/blob/main/docs/project/list-of-diagnostics.md, we now have a strategy for marking existing APIs as `[Obsolete]`. This takes advantage of the new diagnostic id and URL template mechanisms introduced to `ObsoleteAttribute` in .NET 5.

The diagnostic id values reserved for obsoletions are `WFDEV001` through `WFDEV999`. When obsoleting an API, claim the next three-digit identifier in the `WFDEV###` sequence and add it to the list below. The URL template for all obsoletions is `https://aka.ms/winforms-warnings/{0}`. The `{0}` placeholder is replaced by the compiler with the `WFDEV###` identifier.

The acceptance criteria for adding an obsoletion includes:

* Add the obsoletion to the table below, claiming the next diagnostic id
* Ensure the description is meaningful within the context of this table, and without requiring the context of the calling code
* Add new constants to `src\Common\src\Obsoletions.cs`, following the existing conventions
* A `...Message` const using the same description added to the table below
* A `...DiagnosticId` const for the `WFDEV###` id
* Annotate `src` files by referring to the constants defined from `Obsoletions.cs`
* Specify the `UrlFormat = Obsoletions.SharedUrlFormat`
* Example: `[Obsolete(Obsoletions.DomainUpDownAccessibleObjectMessage, DiagnosticId = Obsoletions.DomainUpDownAccessibleObjectDiagnosticId, UrlFormat = Obsoletions.SharedUrlFormat)]`
* If the `Obsoletions` type is not available in the project, link it into the project
* `<Compile Include="..\..\Common\src\Obsoletions.cs" Link="Common\Obsoletions.cs" />`
* Apply the `:book: documentation: breaking` label to the PR that introduces the obsoletion
* Follow up with the breaking change process to communicate and document the breaking change
* In the breaking-change issue filed in [dotnet/docs](https://github.com/dotnet/docs), specifically mention that this breaking change is an obsoletion with a `WFDEV` diagnostic id
* The documentation team will produce a PR that adds the obsoletion to the [WFDEV warnings](https://docs.microsoft.com/dotnet/core/compatibility/winforms-obsoletions) page
* That PR will also add a new URL specific to this diagnostic ID; e.g. [WFDEV001](https://docs.microsoft.com/dotnet/core/compatibility/winforms-warnings/WFDEV001)
* Connect with `@gewarren` or `@BillWagner` with any questions
* Register the `WFDEV###` URL in `aka.ms`
* The vanity name will be `winforms-warnings/WFDEV###`
* Ensure the link's group owner matches the group owner of `winforms-warnings/WFDEV001`
* Connect with `@igveliko` or `@gewarren` with any questions

### Obsoletion Diagnostics (`WFDEV001` - `WFDEV999`)

| Diagnostic ID | Description |
| :---------------- | :---------- |
| __`WFDEV001`__ | Casting to/from IntPtr is unsafe, use `WParamInternal`. |
| __`WFDEV001`__ | Casting to/from IntPtr is unsafe, use `LParamInternal`. |
| __`WFDEV001`__ | Casting to/from IntPtr is unsafe, use `ResultInternal`. |
| __`WFDEV002`__ | `DomainUpDown.DomainUpDownAccessibleObject` is no longer used to provide accessible support for `DomainUpDown` controls. |
| __`WFDEV003`__ | `DomainUpDown.DomainItemAccessibleObject` is no longer used to provide accessible support for `DomainUpDown` items. |

20 changes: 20 additions & 0 deletions src/Common/src/Obsoletions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System;

internal static class Obsoletions
{
internal const string SharedUrlFormat = "https://aka.ms/winforms-warnings/{0}";

// Please see docs\project\list-of-diagnostics.md for instructions on the steps required
// to introduce a new obsoletion, apply it to downlevel builds, claim a diagnostic id,
// and ensure the "aka.ms/dotnet-warnings/{0}" URL points to documentation for the obsoletion
// The diagnostic ids reserved for obsoletions are WFDEV### (WFDEV001 - WFDEV999).

internal const string DomainUpDownAccessibleObjectMessage = $"{nameof(System.Windows.Forms.DomainUpDown.DomainUpDownAccessibleObject)} is no longer used to provide accessible support for {nameof(System.Windows.Forms.DomainUpDown)} controls.";
internal const string DomainUpDownAccessibleObjectDiagnosticId = "WFDEV002";

internal const string DomainItemAccessibleObjectMessage = $"{nameof(System.Windows.Forms.DomainUpDown.DomainItemAccessibleObject)} is no longer used to provide accessible support for {nameof(System.Windows.Forms.DomainUpDown)} items.";
internal const string DomainItemAccessibleObjectDiagnosticId = "WFDEV003";
}
2 changes: 0 additions & 2 deletions src/System.Windows.Forms/src/PublicAPI.Shipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2816,7 +2816,6 @@ override System.Windows.Forms.DockingAttribute.IsDefaultAttribute() -> bool
override System.Windows.Forms.DomainUpDown.CreateAccessibilityInstance() -> System.Windows.Forms.AccessibleObject!
override System.Windows.Forms.DomainUpDown.DomainItemAccessibleObject.Name.get -> string?
override System.Windows.Forms.DomainUpDown.DomainItemAccessibleObject.Name.set -> void
override System.Windows.Forms.DomainUpDown.DomainItemAccessibleObject.Parent.get -> System.Windows.Forms.AccessibleObject!
override System.Windows.Forms.DomainUpDown.DomainItemAccessibleObject.Role.get -> System.Windows.Forms.AccessibleRole
override System.Windows.Forms.DomainUpDown.DomainItemAccessibleObject.State.get -> System.Windows.Forms.AccessibleStates
override System.Windows.Forms.DomainUpDown.DomainItemAccessibleObject.Value.get -> string?
Expand Down Expand Up @@ -3138,7 +3137,6 @@ override System.Windows.Forms.MonthCalendar.IsInputKey(System.Windows.Forms.Keys
override System.Windows.Forms.MonthCalendar.RescaleConstantsForDpi(int deviceDpiOld, int deviceDpiNew) -> void
override System.Windows.Forms.MonthCalendar.SetBoundsCore(int x, int y, int width, int height, System.Windows.Forms.BoundsSpecified specified) -> void
override System.Windows.Forms.MonthCalendar.WndProc(ref System.Windows.Forms.Message m) -> void
override System.Windows.Forms.NumericUpDown.CreateAccessibilityInstance() -> System.Windows.Forms.AccessibleObject!
override System.Windows.Forms.NumericUpDown.DownButton() -> void
override System.Windows.Forms.NumericUpDown.OnKeyDown(System.Windows.Forms.KeyEventArgs! e) -> void
override System.Windows.Forms.NumericUpDown.OnKeyUp(System.Windows.Forms.KeyEventArgs! e) -> void
Expand Down
2 changes: 2 additions & 0 deletions src/System.Windows.Forms/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ System.Windows.Forms.VisualStyles.VisualStyleRenderer.HitTestBackground(System.D
abstract System.Windows.Forms.CommonDialog.RunDialog(nint hwndOwner) -> bool
override System.Windows.Forms.ButtonBase.OnClick(System.EventArgs! e) -> void
override System.Windows.Forms.ColorDialog.RunDialog(nint hwndOwner) -> bool
override System.Windows.Forms.DomainUpDown.DomainItemAccessibleObject.Parent.get -> System.Windows.Forms.AccessibleObject?
override System.Windows.Forms.FontDialog.HookProc(nint hWnd, int msg, nint wparam, nint lparam) -> nint
override System.Windows.Forms.FontDialog.RunDialog(nint hWndOwner) -> bool
override System.Windows.Forms.ToolStripPanel.CreateAccessibilityInstance() -> System.Windows.Forms.AccessibleObject!
override System.Windows.Forms.UpDownBase.CreateAccessibilityInstance() -> System.Windows.Forms.AccessibleObject!
static System.Windows.Forms.Control.FromChildHandle(nint handle) -> System.Windows.Forms.Control?
static System.Windows.Forms.Control.FromHandle(nint handle) -> System.Windows.Forms.Control?
static System.Windows.Forms.Control.ReflectMessage(nint hWnd, ref System.Windows.Forms.Message m) -> bool
Expand Down
1 change: 1 addition & 0 deletions src/System.Windows.Forms/src/System.Windows.Forms.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
</ItemGroup>

<ItemGroup>
<Compile Include="..\..\Common\src\Obsoletions.cs" Link="Common\Obsoletions.cs" />
<Compile Include="..\..\Common\src\RTLAwareMessageBox.cs" Link="Common\RTLAwareMessageBox.cs" />
</ItemGroup>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,25 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.ComponentModel;

namespace System.Windows.Forms
{
public partial class DomainUpDown
{
[EditorBrowsable(EditorBrowsableState.Never)]
[Obsolete(
Obsoletions.DomainItemAccessibleObjectMessage,
error: false,
DiagnosticId = Obsoletions.DomainItemAccessibleObjectDiagnosticId,
UrlFormat = Obsoletions.SharedUrlFormat)]
public class DomainItemAccessibleObject : AccessibleObject
{
private string? _name;
private readonly DomainItemListAccessibleObject _parent;

public DomainItemAccessibleObject(string? name, AccessibleObject parent)
{
_name = name;
_parent = (DomainItemListAccessibleObject)parent.OrThrowIfNull();
}

public override string? Name
Expand All @@ -29,13 +35,7 @@ public override string? Name
}
}

public override AccessibleObject Parent
{
get
{
return _parent;
}
}
public override AccessibleObject? Parent => null;

public override AccessibleRole Role
{
Expand All @@ -61,7 +61,7 @@ public override string? Value
}
}

internal override int[] RuntimeId => new int[] { RuntimeIDFirstItem, Parent.GetHashCode(), GetHashCode() };
internal override int[] RuntimeId => new int[] { RuntimeIDFirstItem, GetHashCode() };
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,85 +2,34 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using static Interop;
using System.ComponentModel;

namespace System.Windows.Forms
{
public partial class DomainUpDown
{
[EditorBrowsable(EditorBrowsableState.Never)]
[Obsolete(
Obsoletions.DomainUpDownAccessibleObjectMessage,
error: false,
DiagnosticId = Obsoletions.DomainUpDownAccessibleObjectDiagnosticId,
UrlFormat = Obsoletions.SharedUrlFormat)]
public class DomainUpDownAccessibleObject : ControlAccessibleObject
{
private DomainItemListAccessibleObject? _domainItemList;
private readonly UpDownBase _owningDomainUpDown;
private readonly UpDownBaseAccessibleObject _upDownBaseAccessibleObject;

/// <summary>
/// </summary>
public DomainUpDownAccessibleObject(DomainUpDown owner) : base(owner)
{
_owningDomainUpDown = owner;
_upDownBaseAccessibleObject = new(owner);
}

private DomainItemListAccessibleObject ItemList
{
get
{
if (_domainItemList is null)
{
_domainItemList = new DomainItemListAccessibleObject(this);
}

return _domainItemList;
}
}

public override AccessibleRole Role
{
get
{
AccessibleRole role = Owner.AccessibleRole;

if (role != AccessibleRole.Default)
{
return role;
}
public override AccessibleRole Role => _upDownBaseAccessibleObject.Role;

return AccessibleRole.SpinButton;
}
}

/// <summary>
/// </summary>
public override AccessibleObject? GetChild(int index)
{
switch (index)
{
// TextBox child
case 0:
return _owningDomainUpDown.TextBox.AccessibilityObject.Parent;
// Up/down buttons
case 1:
return _owningDomainUpDown.UpDownButtonsInternal.AccessibilityObject.Parent;
case 2:
return ItemList;
default:
return null;
}
}
public override AccessibleObject? GetChild(int index) => _upDownBaseAccessibleObject.GetChild(index);

public override int GetChildCount()
{
return 3;
}
public override int GetChildCount() => _upDownBaseAccessibleObject.GetChildCount();

// We need to provide a unique ID. Others are implementing this in the same manner. First item is static - 0x2a (RuntimeIDFirstItem).
// Second item can be anything, but it's good to supply HWND.
internal override int[] RuntimeId
=> new int[]
{
RuntimeIDFirstItem,
PARAM.ToInt(_owningDomainUpDown.InternalHandle),
_owningDomainUpDown.GetHashCode()
};
internal override int[] RuntimeId => _upDownBaseAccessibleObject.RuntimeId;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,7 @@ public event EventHandler? SelectedItemChanged
/// should not call base.CreateAccessibilityObject.
/// </summary>
protected override AccessibleObject CreateAccessibilityInstance()
{
return new DomainUpDownAccessibleObject(this);
}
=> new DomainUpDownAccessibleObject(this);

/// <summary>
/// Displays the next item in the object collection.
Expand Down
Loading

0 comments on commit eff830b

Please sign in to comment.