remove duplicate startup entries

[ahmadalli]

• Searched for similar pull requests
• Compiled the code with Visual Studio
• Require translation update
• Require document update (readme.md, wikipage, etc)

What is the purpose of your pull request?

• Bug fix
• Improvement
• New feature

---

Description of your pull request and other information

fixes #3011

[chenshaoju]

👍 Looks good to me, but someone else must approve.

[database64128]

The status of the AutoStartupItem checkbox is checked on app startup.

shadowsocks-windows/shadowsocks-csharp/View/MenuViewController.cs

Line 358 in 5f760a5

AutoStartupItem.Checked = AutoStartup.Check();

It's impossible to have duplicate registry entries, unless you manually add them after starting the app. Or the check process may have failed to cover some edge cases.

[ahmadalli]

as I've explained on #3011, I've experienced it after upgrade. I'm not sure from which version but I guess it was from 4.2.1 to 4.3.0. I was experiencing the issue on two different machines

[ahmadalli]

also there's an issue with the Check method code:

shadowsocks-windows/shadowsocks-csharp/Controller/System/AutoStartup.cs

Lines 74 to 88 in 73fa920

	string[] runList = runKey.GetValueNames();
	foreach (string item in runList)
	{
	if (item.Equals(Key, StringComparison.OrdinalIgnoreCase))
	return true;
	else if (item.Equals("Shadowsocks", StringComparison.OrdinalIgnoreCase)) // Compatibility with older versions
	{
	string value = Convert.ToString(runKey.GetValue(item));
	if (Program.ExecutablePath.Equals(value, StringComparison.OrdinalIgnoreCase))
	{
	runKey.DeleteValue(item);
	runKey.SetValue(Key, Program.ExecutablePath);
	return true;
	}
	}

if the loop reaches the key first, it won't go further and won't reach the Shadowsocks key therefore it won't delete it

[database64128]

Please take another look at AutoStartup.cs. AutoStartup.Check() does almost exactly what you added in your PR, except it doesn't check for or delete duplicate entries.

As for the issue, I doubt if it's caused by the update. Can you reproduce it and provide detailed steps?

[ahmadalli]

the Check method is the right place to do what I did in this PR. but it has a corner case:

lets assume runList array is this: ["key1", "key2", ..., "shadowsocks_xyz <this is the Key variable>", "key3", ..., "shadowsocks"]

whe the foreach loop reaches the shadowsocks_xyz, it'll return true and won't reach the shadowsocks key and won't get a chance to remove the key.

[database64128]

if the loop reaches the key first, it won't go further and won't reach the Shadowsocks key therefore it won't delete it

It's impossible to have duplicate registry entries in the first place, unless you manually add them after starting the app, which shouldn't be covered by the checker.

[ahmadalli]

It's impossible to have duplicate registry entries in the first place, unless you manually add them after starting the app, which shouldn't be covered by the checker.

the case check is covering is exactly fixing duplicate startup entries (different names, same path) which is caused by upgrading

[ahmadalli]

I've fixed the issue of check method on my latest commit. the only difference is that I don't just check for Shadowsocks on the registry, I check all the entries to see if they have the same path as current executable

[ahmadalli]

here's a detailed way to reproduce this issue:

1. install v4.1.9.2 (any version before commit ced49e2) on C:/Temp
2. open shadowsocks and click on Start on Boot to enable it
3. close shadowsocks, remove the contents of C:/Temp directory
4. install 4.2.0.1 (this is the version I used for testing. later versions should be fine) on C:/Temp
5. open shadowsocks
6. Start on Boot is inactive
7. check the contents of HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\CurrentVersion\Run
8. click on Start on Boot to enable it
9. check the contents of HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\CurrentVersion\Run again

and there will be two Shadowsocks_... entries on the registry with the same path

[ahmadalli]

and the current Check method doesn't remove the other Shdowsocks_... key because:

1. it gives up checking after it finds the Key variable on the registry
2. there's no check of other Shadowsocks_... entries. just checks if there's any Shadowsocks entry (which there isn't in this case)

[database64128]

Thank you for your contribution and detailed explanations! 😊

[ahmadalli]

yw ;)

if you're going to merge it now, just let me adjust one more thing

[ghost]

string.GetHashCode() are not guranteed same between different versions, it's the possible reason why there's two different entry.

[ahmadalli]

true, the other issue is that the source string (which we calculate hash of) has been changed from ced49e2. it used to be hash of Application.StartupPath but it changed to Program.ExecutablePath