Ensure Process.MainWindowHandle is refreshed

[fernandreu]

As discussed in #32690, this will set _haveMainWindow to true only if _mainWindowHandle is non-zero. It also forces the latter to be reset during a Process.Refresh() call, hence allowing the re-evaluation of both.

As suggested, the test I added is marked as manual due to needing to launch a UI (Notepad) as part of it. I did it this using Fact(Skip = "Manual test")] based on what I saw in ProcessStartInfoTests.StartInfo_WebPage.

[dnfclas]

All CLA requirements met.

[fernandreu]

Just to mention that, even though all the checks passed, there are a couple of things that could be improved:

• The _haveMainWindow = false line is in RefreshCore(), in Process.Windows.cs, but the field itself is defined in Process.Win32.cs. It might be cleaner if I create some sort of RefreshInternal method in Process.Win32.cs that resets that field instead, and then call it from RefreshCore(). Either that, or move the RefreshCore() method to ProcessWin32.cs. I am guessing there is no other Process.X.cs file that I would need to take care of, as otherwise the build would have failed as it currently stands.
• It seems that the .NET Framework implementation also has a call to EnsureState(State.HaveProcessInfo) to throw if the process has already exited. Not fully sure if this is also needed for .NET Core.

[stephentoub]

It seems that the .NET Framework implementation also has a call to EnsureState(State.HaveProcessInfo) to throw if the process has already exited. Not fully sure if this is also needed for .NET Core.

I was wondering the same thing, but I think it's ok as is. We always return 0 on Unix, even if the process has exited, and since we haven't been throwing for this condition in core until now, I don't see enough value in introducing the exception / breaking change.

Process.X.cs

This is a holdover from when the Windows code was split to support UWP. What you have is fine, and we should be able to subsequently recombine these Windows files.

[stephentoub]

We don't add tests like this; it'll never get run and end up just being dead code.

What prevents writing an automated test that's not permanently disabled?

[fernandreu]

The only way I can think of to test the full extend of this (i.e. whether MainWindowHandle eventually becomes non-zero) is to spawn a process with a UI. Based on this comment from @danmosemsft, I was assuming that this is undesirable for the pipeline, but it was fine to have a test like that if marked as manual.

However, there is an alternative: rather than checking if MainWindowHandle became non-zero, I could set it to a non-zero value, then check if it was refreshed back to zero. Using reflection, the test could look like this:

[Fact]
[PlatformSpecific(TestPlatforms.Windows)]
public void MainWindowHandle_ShouldRefresh_Windows()
{
    FieldInfo handleField = typeof(Process).GetField("_mainWindowHandle", BindingFlags.Instance | BindingFlags.NonPublic);
    FieldInfo flagField = typeof(Process).GetField("_haveMainWindow", BindingFlags.Instance | BindingFlags.NonPublic);

    CreateDefaultProcess();

    Assert.Equal(IntPtr.Zero, _process.MainWindowHandle);

    // MainWindowHandle should not update if _haveMainWindow is true until the process is refreshed
    IntPtr fakeHandle = (IntPtr)1234;
    handleField.SetValue(_process, fakeHandle);
    flagField.SetValue(_process, true);
    Assert.Equal(fakeHandle, _process.MainWindowHandle);
    _process.Refresh();
    Assert.Equal(IntPtr.Zero, _process.MainWindowHandle);
}

Instead of using reflection, I could also make use of InternalsVisibleToAttribute (as I see that other libraries do that already), and define the _haveMainWindow and _mainWindowHandle fields as internal. In both cases, the test might be getting too deep into the implementation details. As a slight variant, I could also add an internal setter to MainWindowHandle and an internal property HaveMainWindow, or perhaps an internal method to set those two fields.

Let me know which approach would you prefer and I will update the PR accordingly.

[stephentoub]

I'd be fine if the test is attributed as both [Outerloop] and [Trait(XunitConstants.Category, XunitConstants.IgnoreForCI)]. Then it'll only affect a dev working locally and running outerloop tests, in which case I think popping UI here and there is fair game... we already do it in the Process tests IIRC.

[danmoseley]

+1

[fernandreu]

No problem, I did the changes as requested. The job "Libraries Test Run release mono OSX x64 Debug" failed, but I would think it is unrelated to the changes, as they should only affect Windows in any case. Is it this same issue perhaps?

[stephentoub]

Thanks.

[stephentoub]

Nit: exePath => ExePath

[fernandreu]

Done

[stephentoub]

Thanks.

[firenero]

Hey @stephentoub, are there any estimates when this fix is planned to be released?

[stephentoub]

It's checked into master, so barring unforeseen circumstances it'll be part of .NET 5.
https://github.com/dotnet/core/blob/master/roadmap.md#upcoming-ship-dates

[danmoseley]

@firenero meantime, you can test bits from https://github.com/dotnet/core-sdk (should be there in a few days if not already).

We will start releasing official previews of .NET 5 soon. It might not be in the first, but I'd expect it to be in the second.

[firenero]

Thanks!

[fernandreu]

Also, you could replace any process.Refresh() call with process = Process.GetProcessById(process.Id) to achieve a similar effect in the meantime. It is what I am doing for now.

[Jandini]

@fernandreu, Thank you for the workaround. Works like a charm in core 3.1