Fix null-reference exception on failed mod upgrade

[mwerle]

Fixes #2162

THIS IS A HACK! I can't work out why the SortedDictionary "module_version" reverses it's sort-order after a module update has failed; someone more familiar with the codebase may be able to work out the actual bug.

[HebaruSan]

AvailableModule.Latest is called for every mod in Registry.Available and Registry.LatestAvailableWithProvides, which are in turn called in many other places such as the relationship resolver. Sorting the version list in Latest could cause a severe performance degradation across the board, as the registry would start re-sorting tons of lists at various places in miscellaneous operations throughout the code.

I don't think this is safe to merge as-is.

[mwerle]

As I said it's a hack; having said that, IMHO it's better than a NullRef. On my (admittedly fast) PC I didn't notice a performance impact; overall I think the the dataset is still too small for it to realistically matter at this stage

Also I used the same sorter as the SortedDictionary uses. Again, I have no idea why the dictionary sort order ends up reversing itself; according to the docs it shouldn't be possible.

Anyway, I just put this out there as I can't get any further with this and it might give someone more familiar with the codebase and C# a starting point.

[mwerle]

Ok, found where the dictionaries get their orders reversed; it's when the FileExistsKraken gets re-thrown in ModuleInstaller.cs:419, and re-caught in MainInstall.cs:259.

Before the throw it's in the order it should be, after the throw it's reversed. How or why the throw affects the sort-order of the dictionaries in the RegistryManager is anybodies guess; this is under Windows/DotNet, btw, haven't tested under Mono.

[HebaruSan]

Can we start with simply checking whether the check box cell reference is null before using it?

_MarkModForUpdate currently makes a hard assumption that the identifier it receives will be upgradeable, which we've seen is not a justified assumption, and it throws an NRE if it's not. Even if we consider this a serious enough error condition to warrant an exception (I don't think I do), we should be detecting it and throwing a more meaningful exception instead (something with a name like ModNotUpgradeableKraken, perhaps).

Let's keep your rewrite of that function to better resemble _MarkModForInstall 👍. Then in SetUpgradeChecked, if update_cell is null, just return. And remove the sort call. That should prevent the crash without adversely affecting any other code flows.

I haven't seen your investigation results, so I have to admit I'm skeptical that there's a problem with list sorting. How did you come to that conclusion?

[mwerle]

Then in SetUpgradeChecked, if update_cell is null, just return. And remove the sort call. That should prevent the crash without adversely affecting any other code flows.

If we just return from SetUpgradeChecked on error then following the failed upgrade you won't be able to upgrade any mod as none will be marked as upgradeable. It was my initial attempt to fix this.

So not really a fix, but better than a crash.

I haven't seen your investigation results, so I have to admit I'm skeptical that there's a problem with list sorting. How did you come to that conclusion?

By looking at the list in question in a debugger after every step through the code. Following the "throw", the list is sorted in increasing version order instead of decreasing; this is true for all mods in the "available_mods" registry, not just the mod I was trying to upgrade.

There's some more details in the bug report #2162.

[HebaruSan]

What's interesting about AvailableModule is that the sorting convention is set only by a parameter to the constructor of the default value of module_version, not the type itself, and that object isn't really protected in any way. So if module_version was replaced by a SortedDictionary from outside, say by a JSON deserializer (it is a [JsonProperty]!), it would fall back to the default sort order built in to the Version type, which is ascending, opposite to what AvailableModule requires and assumes.

If we eliminate RecentVersionComparer and make AvailableModule work with an ascending-order SortedDictionary, that would eliminate the risk of that happening. I'll add that to #2184...

[mwerle]

What's interesting about AvailableModule is that the sorting convention is set only by a parameter to the constructor of the default value of module_version, not the type itself, and that object isn't really protected in any way. So if module_version was replaced by a SortedDictionary from outside, say by a JSON deserializer (it is a [JsonProperty]!),

Interestingly enough when the registry is loaded from the JSON, it's loaded into the correct (ie, descending) order, despite the values in the JSON being in ascending order. It's only when the exception gets thrown that the order gets changed (well, perhaps the underlying object gets replaced; I didn't take note of the memory addresses..)

We still (I think?) need the "RecentVersionComparer" though as it correctly sorts numerical values rather than by ASCII-order, regardless of whether we keep the dictionary in ascending or descending order.

Anyway, I didn't see your new binary this morning - only saw the notification email after I had already gone out, so I'll have a look at your suggested changes and test them out.

Looks like we might be honing in on a "proper" fix :)

[HebaruSan]

The documentation for SortedDictionary says

If type TKey implements the System.IComparable<T> generic interface, the default comparer uses that implementation.

... and our key type Version does implement IComparable<Version>, so I think we still get the sophisticated sort without RecentVersionComparer.

[mwerle]

Ok, just tested your pull request, and it mostly works (I added review comments to the bits which don't).

Overall a much better fix than my hack :)

[HebaruSan]

Cool, I'll be glad to address your review comments. I don't see them yet, however; is there a "share your comments" button that you can click?

[mwerle]

Heh, forgot to click on the "Submit Review" button (I've used GitHub some, but am by no means an expert).

I also just worked out it's possible to send a pull request against your pull request, so I (think) I've just done that too.

But I guess for all intents and purposes, this pull request can be closed now.. (apart from the clean-up commit, my "fix" is obsolete). We can continue any discussion in PR #2184