Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix null reference in recommendations #2984

Merged
merged 1 commit into from
Feb 2, 2020

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Feb 2, 2020

Problem

When loading the recommendations screen when it would contain multiple options:

System.NullReferenceException: Object reference not set to an instance of an object.
   at CKAN.ChooseRecommendedMods.RecommendedModsListView_ItemChecked(Object sender, EventArgs e)
   at System.Windows.Forms.ListView.OnItemChecked(ItemCheckedEventArgs e)
   at System.Windows.Forms.ListView.WmReflectNotify(Message& m)
   at System.Windows.Forms.ListView.WndProc(Message& m)
   at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)

I have no idea how I didn't encounter this during development of #2981, since that inherently required loading this screen with multiple options to test. Maybe there's a race condition?

Cause

Apparently ListView.Items can contain null, specifically in the ItemChecked event while processing an AddRange call. Upon reflection this is less surprising than at first blush, since on Windows these are wrappers around C++ components, which probably:

  1. Expand the Items list with nulls, since it may be a std::vector that has to copy its contents every time it is expanded
  2. Assign them one by one, triggering ItemChecked for each in turn

(Whereas I would expect a C# component to approach this as foreach (...) { Items.Add(...); }, which would never insert a null value.)

The code from #2981 assumed ListView.Items would not contain nulls.

Changes

Now RecommendedModsListView_ItemChecked skips nulls.

@HebaruSan HebaruSan added Bug Something is not working as intended Easy This is easy to fix GUI Issues affecting the interactive GUI Pull request labels Feb 2, 2020
@HebaruSan HebaruSan requested a review from DasSkelett February 2, 2020 18:21
politas added a commit that referenced this pull request Feb 2, 2020
@politas politas merged commit 944818b into KSP-CKAN:master Feb 2, 2020
@politas politas removed the request for review from DasSkelett February 2, 2020 21:40
@HebaruSan HebaruSan deleted the fix/rec-nre branch February 2, 2020 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Easy This is easy to fix GUI Issues affecting the interactive GUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants