Fix GUIMod crash when module doesn't have a compatible game version

[HebaruSan]

Problems

As of #2437, if you have an installed module that has since been completely de-indexed (e.g., MiniAVC: KSP-CKAN/CKAN-meta#561, KSP-CKAN/CKAN-meta@6cd6294), this exception happens at GUI startup:

System.NullReferenceException: Object reference not set to an instance of an object
  at CKAN.GUIMod..ctor (CKAN.CkanModule mod, CKAN.IRegistryQuerier registry, CKAN.Versioning.KspVersionCriteria current_ksp_version, System.Boolean incompatible) [0x003d6] in <60e037f139274e47b1d6c0206eefe140>:0 
  at CKAN.Main+<>c__DisplayClass229_0.<_UpdateModsList>b__2 (CKAN.InstalledModule m) [0x00006] in <60e037f139274e47b1d6c0206eefe140>:0 
  at System.Linq.Enumerable+SelectEnumerableIterator`2[TSource,TResult].MoveNext () [0x00048] in <cc3b329d40bd4675ae0e985e302972af>:0 
  at System.Linq.Enumerable+WhereEnumerableIterator`1[TSource].MoveNext () [0x0004e] in <cc3b329d40bd4675ae0e985e302972af>:0 
  at CKAN.Main._UpdateModsList (System.Boolean repo_updated, System.Collections.Generic.List`1[T] mc) [0x000fe] in <60e037f139274e47b1d6c0206eefe140>:0 
  at CKAN.Main+<>c__DisplayClass228_0.<UpdateModsList>b__0 () [0x0001b] in <60e037f139274e47b1d6c0206eefe140>:0 
  at CKAN.Util.Invoke[T] (T obj, System.Action action) [0x0002d] in <60e037f139274e47b1d6c0206eefe140>:0 
  at CKAN.Main.UpdateModsList (System.Boolean repo_updated, System.Collections.Generic.List`1[T] mc) [0x0001c] in <60e037f139274e47b1d6c0206eefe140>:0 
  at CKAN.Main.CurrentInstanceUpdated () [0x00062] in <60e037f139274e47b1d6c0206eefe140>:0 
  at CKAN.Main.OnLoad (System.EventArgs e) [0x002c2] in <60e037f139274e47b1d6c0206eefe140>:0 
  at System.Windows.Forms.Form.OnLoadInternal (System.EventArgs e) [0x00023] in <beba0b11547e4333a33d4d8cdf7b7c51>:0 
Waiting on threads to park on joinable thread list timed out.

Unrelatedly, if you had the old standalone MiniAVC module installed, it was not shown in the mod list.

Causes

This happens when one of your installed modules is totally de-indexed. I only understood this after @cculianu shared his registry.json, so this PR's description previously said the cause was unknown:

Old obsolete investigation notes

The exact cause is a bit mysterious. Only @cculianu has reported seeing this so far. A testing component revealed that the exception happens while the GUI is attempting to generate a mod list row for MiniAVC 1.0.3.0:

System.Exception: Failed to construct a GUIMod for MiniAVC 1.0.3.0 (Object reference not set to an instance of an object)

The same testing component established that the exception is thrown inside this block from #2437:

CKAN/GUI/GUIMod.cs

Lines 135 to 173 in ab714c2

	if (!VersionMaxWasGenerated)
	{
	VersionMaxWasGenerated = true;
	List<KspVersion> versions = new KspBuildMap(new Win32Registry()).KnownVersions; // should be sorted
	VersionsMax = new Dictionary<string, KspVersion>();
	VersionsMax[""] = versions.Last();
	foreach (var v in versions)
	{
	VersionsMax[v.Major.ToString()] = v; // add or replace
	VersionsMax[v.Major + "." + v.Minor] = v;
	}
	}
	const int Undefined = -1;
	KspVersion ksp_ver = registry.LatestCompatibleKSP(mod.identifier);
	string ver = ksp_ver?.ToString();
	int major = ksp_ver.Major, minor = ksp_ver.Minor, patch = ksp_ver.Patch;
	KspVersion value;
	if ( major == Undefined
	//|| (major >= UptoNines(VersionsMax[""].Major)) // 9.99.99
	|| (major > VersionsMax[""].Major) // 2.0.0
	|| (major == VersionsMax[""].Major && VersionsMax.TryGetValue(major.ToString(), out value) && minor >= UptoNines(value.Minor)) // 1.99.99 ?
	)
	KSPCompatibility = "any";
	else if (minor != Undefined
	&& VersionsMax.TryGetValue(major + "." + minor, out value)
	&& (patch == Undefined || patch >= UptoNines(value.Patch))
	)
	KSPCompatibility = major + "." + minor + "." + UptoNines(value.Patch);
	else
	KSPCompatibility = ver;
	// KSPCompatibility += " | " + major + "." + minor + "." + patch; // for testing

However...

• MiniAVC isn't indexed in CKAN! It's only distributed as a bundled DLL inside other mods.
• If CKAN installed MiniAVC.dll as part of another mod, then it would not be added to the mods list but rather counted as an installed file and ignored, so it must have gotten there some other way; yet the issue report says no manual changes were made to GameData.
• 1.0.3.0 is a valid MiniAVC release version (from 2014), but there is no known pathway by which CKAN could have gotten that version string without metadata for it in the registry!

Hitting a dead end in trying to make this crash happen on my PC, I turned to auditing the code.

The crash happens here; since MiniAVC isn't in the registry, the LatestCompatibleKSP call returns null, and shortly after that the code attempts to access properties of that null reference:

CKAN/GUI/GUIMod.cs

Lines 152 to 154 in ab714c2

	KspVersion ksp_ver = registry.LatestCompatibleKSP(mod.identifier);
	string ver = ksp_ver?.ToString();
	int major = ksp_ver.Major, minor = ksp_ver.Minor, patch = ksp_ver.Patch;

Changes

• "any" and "x.y.max" in the KSP Max column (GUI) #2437's version formatting logic is moved from being embedded in GUIMod's constructor to KspVersion.ToYalovString. This eliminates the possibility of the version object being null inside the function. Instead, null is now checked before the function is called, and we fall back to a version string of "Unknown" in that case. It also allows us to use this logic in other places as well as test it (not included in this PR).
• The VersionsMax dictionary is now static and only generated once rather than being redundantly re-generated for every module.
• A bunch of commented code is removed.
• VersionMaxWasGenerated is removed and the generation code it governed is moved to a static constructor which accomplishes the same thing more reliably with less overhead.
• GUIMod's comparison operations now check the Identifier rather than the Name. This allows the old MiniAVC module to appear in the mod list, where it previously was hidden because its Name was the same as KSP-AVC's.

Fixes #2481.

[cculianu]

@HebaruSan -- I tried the provided ckan from the latest .zip you posted and.. it WORKS! Hurray!

I am not sure why MiniAVC is there. I have a pretty ancient KSP install from a long time ago. All my mods I installed with CKAN, though. No manually installed mods.

Here is what my mod list screen looks like:

I do seem to have a MiniAVC folder with MiniAVC in there from September 2016.. Should I delete that?

[HebaruSan]

@cculianu, that's great news! Thanks for trying that out.

What if you change the filter from "Installed" to "All" and type "MiniAVC" in "Filter by mod name"? Does MiniAVC show up in the list in CKAN? Maybe it was indexed a long time ago and you have metadata from back then...?

[cculianu]

Nope. Still not there.

Here is my entire CKAN folder (sans downloads/ folder). Maybe this will help you forensically? :

CKAN.zip

[HebaruSan]

Wow, it does think you have MiniAVC installed as a full module! And the install_time tells us that you installed it in September 2016, so it must have been indexed back then:

	"installed_modules": {
		"MiniAVC": {
			"install_time": "2016-09-26T17:41:31.496489+03:00",
			"source_module": {
				"abstract": "This plugin will notify you when updates are available for certain mods",
				"author": [
					"cybutek"
				],
				"download": "http://ksp.cybutek.net/miniavc/MiniAVC-1.0.3.0.zip",
				"download_size": 34408,
				"download_hash": {
					"sha1": "8B956D21495ACD0911EE4C181E049A0C664D3BB4",
					"sha256": "092AAFEB0FBC8EB002366136FE3AED4F496BADD7940861A8C42028D4F2822DDA"
				},
				"identifier": "MiniAVC",
				"ksp_version_strict": false,
				"license": [
					"GPL-3.0"
				],
				"name": "KSP AVC",
				"release_status": "stable",
				"resources": {
					"repository": "https://github.com/CYBUTEK/KSPAddonVersionChecker",
					"homepage": "http://forum.kerbalspaceprogram.com/threads/79745"
				},
				"version": "1.0.3.0",
				"spec_version": "1"
			},
			"installed_files": {
				"GameData/MiniAVC": {},
				"GameData/MiniAVC/MiniAVC.dll": {
					"sha1_sum": "F6-5D-DB-C2-41-CF-AE-83-3D-CC-39-F2-EA-FC-E3-EE-51-B1-EE-2F"
				}
			}
		},

It should be possible to find this in the metadata repositories, I'll take another look...

[HebaruSan]

OK, KSP-CKAN/NetKAN#1455 has the background info. There was an effort to support MiniAVC as a standalone mod so other mods could recommend it and users could choose whether to install it. But the architecture of MiniAVC doesn't work well for this: it checks the versions of mods in its DLL's folder and subfolders, so if we put the DLL in its own folder it won't check anything, but if we put it in GameData it will check too much. (RoverDude also thought that de-bundling MiniAVC was too much interference with mod authors' wishes.)

KSP-CKAN/CKAN-meta#561 added a .ckan file as a stopgap while they worked on how a .netkan would work.

Here's the bot updating it somehow: KSP-CKAN/CKAN-meta@f26d34f

A year later KSP-CKAN/NetKAN#3934 removed the filtering out of bundled copies of MiniAVC.

Then a year after that the .ckan file was deleted: KSP-CKAN/CKAN-meta@6cd6294

Long story short, the symptom we're looking for is: Install a mod, then it gets de-indexed.

[cculianu]

I'm actually a computer programmer by trade. I am not familiar enough with NetKAN or any of what even MiniAVC does (version control/checker?).

I understood about 95% of that. But one question remains: Should I delete that MiniAVC? What happens when a module is ancient? Does it still get loaded? Will it do Bad Things? :)

I'm really glad I was able to help you fix this and that you fixed it so fast!

It's possible other people in the wild that have been using KSP since the early days (and CKAN) somehow ended up with this bug and/or in this situation. So in my mind I think it's awesome you fixed it.

I did nothing outside the ordinary except use CKAN to install mods (apparently including MiniAVC)... so.. there ya go.

Now.. should I delete MiniAVC from GameData/?

[HebaruSan]

Indeed, thanks for your help!

At this point I would not delete it manually. You would end up with an installed module entry in your registry pointing to files that no longer exist, which may cause errors. The ideal endgame is that you would uninstall it from within CKAN itself, which would then update all the tracking data structures properly. But you can't do that currently because it doesn't show up in the mod list, so that's the next thing I plan to investigate.

[cculianu]

Ok, well.. until then. MiniAVC that hopefully does no evil will continue to live, like a virus, in my GameData folder.

Do let me know if I can help any further, ha ha.

Pretty awesome how fast you fixed it. Great project. I know C# well too.. maybe someday when my plate is freer I can also contribute to this project (although sometimes too many cooks spoil the pot).

Really good useful software, this CKAN. Thumbs up on all counts. 👍

[linuxgurugamer]

Note that CKAN is perfectly happy if you DELETE files. When it updates or deletes, it is fine with files being missing.

[HebaruSan]

Update, it's hiding MiniAVC because GUIMod's comparison operations rely on GUIMod.Name:

CKAN/GUI/GUIMod.cs

Lines 324 to 340 in 37c8477

	private bool Equals(GUIMod other)
	{
	return Equals(Name, other.Name);
	}
	public override bool Equals(object obj)
	{
	if (ReferenceEquals(null, obj)) return false;
	if (ReferenceEquals(this, obj)) return true;
	if (obj.GetType() != GetType()) return false;
	return Equals((GUIMod) obj);
	}
	public override int GetHashCode()
	{
	return (Name != null ? Name.GetHashCode() : 0);
	}

... and the old MiniAVC module's name property is "KSP AVC", which is shared with the actual KSP-AVC:

https://github.com/KSP-CKAN/NetKAN/blob/56fc0358d7ba4ad514c748299e51aafaee1a49b0/NetKAN/KSP-AVC.netkan#L6

So GUI thinks MiniAVC is already in the list when it's not. This should be identifier-based instead. This could be causing assorted other problems if there are other cases of mods inappropriately sharing the same name.

[cculianu]

I am on the reviewers for this despite my not being familiar with this codebase.

Using general programmer mojo heuristics I say this appears correct, assuming identifier is unique for a module across all versions and not a hash value or something version dependent.

So.. I’m hitting approve here. (Not qualified really to do otherwise, ha).

[HebaruSan]

I'm guessing LGG requested your review to confirm that the test build was working (good idea). Or I somehow misclicked it and don't remember. Either way, thanks!

[cculianu]

Hmm. Should I try building it and seeing if old defunct mini avc shows up now? Or did your other change make it not appear despite this change?

(Actually if you can post a .zip again that’s easier as I’m not set up to build on this Mac and booting up windows for this is slightly painful as my windows system needs updates and is crufty)

[HebaruSan]

Sure, here's a new test build:

• (Link removed, get the official release)

You should see a new row labeled "KSP AVC" (due to the shared name problem mentioned above), and unchecking its checkbox should allow you to uninstall. After that, the standard release of CKAN v1.25.2 should be able to start without the error.

[cculianu]

I love you! It's there! I am deleting it now! YOU ARE THE BEST! 💃

[politas]

Great work, @HebaruSan !

[cculianu]

Indeed! I've been running the demo build you sent me for weeks now without a single issue. I'm super happy!!