-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Description
I have come across an issue with a third-party UI test library which I have finally tracked down to their internal use of Process.MainWindowHandle. Consider the following example:
bool TestProcess()
{
var process = Process.Start("notepad");
try
{
for (var attempt = 0; attempt < 20; ++attempt)
{
process?.Refresh();
if (process?.MainWindowHandle != IntPtr.Zero)
{
return true;
}
Thread.Sleep(100);
}
return false;
}
finally
{
process?.Kill();
}
}This function will return true in .NET Framework after a few refreshes, but false in .NET Core.
The reason is: once the getter of MainWindowHandle is called once, it will never be evaluated again. See Process.Win32.cs:
public IntPtr MainWindowHandle
{
get
{
if (!_haveMainWindow)
{
EnsureState(State.IsLocal | State.HaveId);
_mainWindowHandle = ProcessManager.GetMainWindowHandle(_processId);
_haveMainWindow = true;
}
return _mainWindowHandle;
}
}There is no other _haveMainWindow assignment anywhere else that I can see. Therefore, if the handle was not ready yet (i.e. _mainWindowHandle gets assigned IntPtr.Zero), the class would assume it was ready anyway, set _haveMainWindow to true, and never re-evaluate this.
Due to this, I would propose two changes:
- Change the line with the true assignment to:
_haveMainWindow = _mainWindowHandle != IntPtr.Zero; - In Process.Refresh(), add
_haveMainWindow = false;(it seems to me this information should also be discarded together with the rest)
If you guys are happy with these changes, I would like to take care of making the pull request. I can also include a test demonstrating the issue as it stands, which would be very similar to the example I put above. I have already done these changes locally and can confirm it solves my issue without breaking any other test. Just be aware that it is unavoidable to launch a GUI to test this (I think). Microsoft-hosted agents in Azure Pipelines should not have any issues dealing with this, but perhaps you guys have other concerns.