Fix WindowsFormsApplicationBase.IsSingleInstance

[elachlan]

Add currentUserSID to GetApplicationInstanceID for use in TryCreatePipeServer to avoid system wide blocking. Used in conjunction with PipeOptions.CurrentUserOnly.

Fixes #3715
Fixes #11232
Fixes #11365

Microsoft Reviewers: Open in CodeFlow

[elachlan]

@weltkante had this suggestion and I ran with it. I think the concerns raised by @zanchey are technically valid, but block a pragmatic fix for most use cases. It might be mitigated by us using PipeOptions.CurrentUserOnly as well.

@Olina-Zhang could your team test this? I don't have a multi-user environment I can easily utilize for testing.

[codecov]

Codecov Report

Attention: Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.27371%. Comparing base (48a3d7c) to head (03230e4).
Report is 614 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11258         +/-   ##
===================================================
- Coverage   74.34305%   74.27371%   -0.06935%     
===================================================
  Files           3012        3025         +13     
  Lines         625885      626888       +1003     
  Branches       46553       46743        +190     
===================================================
+ Hits          465302      465613        +311     
- Misses        157190      157923        +733     
+ Partials        3393        3352         -41     

Flag	Coverage Δ
Debug	74.27371% <95.00000%> (-0.06935%)	⬇️
integration	17.99401% <0.00000%> (-0.34132%)	⬇️
production	47.00072% <87.50000%> (+0.01172%)	⬆️
test	96.99432% <100.00000%> (-0.04596%)	⬇️
unit	43.97653% <87.50000%> (+0.07251%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[paul1956]

Can you remove ByVal and make private function parameters start with lower case character. If touching code, it should conform to current VB Style otherwise it will conflict with other PR's trying to clean up VB Code Style.

[Olina-Zhang]

@elachlan tested this PR using following 2 apps(one is Winforms VB app, another is Winforms C#) in .Net 9.0 SDK build on a server with multi-users. Every logged in user can launch the app, issue is fixed. Thanks!
VB_SingleInstance_true.zip
SingleInstanceTrue_CSharp.zip

[elachlan]

Is MD5 okay here? really we are just taking a path and making a guid. I don't expect it to be an issue. Because its in the startup path I believe its quite important to have a fast hashing algorithm here as well.

[weltkante]

that depends on whether you are treating the input (the file path) as a secret and/or want to prevent colissions with forged inputs - md5 is fine against accidental colissions - personally I don't think there's a security issue here, and if it were, then it needs to be adressed at the permission level since the named objects are visible and accessible to non-dotnet code as well, malicious code could interact with the named objects directly instead of going through the file path hashing

[elachlan]

I have some concerns about using the filepath of the assembly because we are calling it using Assembly.GetCallingAssembly().

This needs to be tested with self-contained single file and AOT scenarios. As far as I know for self contained single file applications I've had to use Assembly.GetEntryAssembly() to get the correct location of where the exe is located. If Assembly.GetCallingAssembly() is used it might get the location of the assembly that is extracted to the users appdata.

I am unsure if this is still the case.

@Olina-Zhang maybe your team could test this scenario?

[Olina-Zhang]

@Olina-Zhang maybe your team could test this scenario?

Use your new updated code to test these issues again, right? Or publish app with self-contained to test?

[elachlan]

The main issue to test is #11365 using the updated code.

Then additionally try #11365 but on a single file published app.

[Olina-Zhang]

Hi @elachlan, tested the updated code, here is the result:

1. GH issue WindowsFormsApplicationBase.IsSingleInstance applications are global on a machine, not per user #3715 and GH issue Winforms VB application with Single Instance setting crashes for multiple users on terminal server #11232 - Fixed
2. For GH issue: Single instance app doesn't work when same app is in different folders #11365, the scenarios customer reported about launch exe from 2 different folders with one user - Fixed, we can see 2 app instances
3. For GH issue: Single instance app doesn't work when same app is in different folders #11365, copy it on a server machine in 2 different folders, then launch exe from 2 different folders with different users - Launch exe successfully with different users
4. For GH issue: Single instance app doesn't work when same app is in different folders #11365, published app with self-contained, put it in 2 different folders, then launch exe from different folders with one user - app can launch from one folder, and app cannot launch from another folder, get a message that "it is already installed from a different location" --- it is expected, same as the .Net framework's behavior
5. For GH issue: Single instance app doesn't work when same app is in different folders #11365, published app with self-contained, copy it on a server machine in one folder, then launch exe with different users --- first user can install and launch, second user can install but cannot launch, get following exception in EventViewer:

Application: WinFormsApp1.exe
CoreCLR Version: 9.0.24.25601
.NET Version: 9.0.0-preview.5.24256.1
Description: The process was terminated due to an unhandled exception.
Exception Info: Microsoft.VisualBasic.ApplicationServices.CantStartSingleInstanceException: This single-instance application could not connect to the original instance.
   at Microsoft.VisualBasic.ApplicationServices.WindowsFormsApplicationBase.Run(String[] commandLine)
   at WinFormsApp1.Program.Main()

[elachlan]

That last scenario with published self-contained app and two users is interesting and unexpected.

We will need to investigate further.

[elachlan]

@Olina-Zhang can you retest the published self-contained failure scenario with the latest? The event viewer will have a bit more detail hopefully.

[Olina-Zhang]

@Olina-Zhang can you retest the published self-contained failure scenario with the latest? The event viewer will have a bit more detail hopefully.

I didn't see any difference with updated code, same as Yesterday's:

[elachlan]

My change was to pass in the GetApplicationInstanceID value to the exception so we could see what it was. But it seems not to have worked.

[elachlan]

@lonitra I cannot proceed with this PR as I don't have the resources for testing. I suggest we have @LeafShi1 and team take over this work as they are better resourced. I'd also advocate for a backport to 9.0 as its blocking migrations.