adventuregamestudio / ags

AGS editor and engine source code
Other
701 stars 160 forks source link

Script Editor: location dropdown list causing "Out Of Range" exception #1919

Open tarnos12 opened 1 year ago

tarnos12 commented 1 year ago

Describe the bug When left clicking at a line of code the error pops up. Perhaps it happens only for newly created functions before/right after saving the file.(just a hunch tho) It happens only for specific lines which seems to be related to a specific function hence the above statement. It could also be related to my layout: d5dfc238b64881be36cc1c5272dd1a53

Error: InvalidArgument=The value '4' is not a valid value for 'index'.
Parameter name: index
Version: AGS 3.99.101.0

System.ArgumentOutOfRangeException: InvalidArgument=The value '4' is not a valid value for 'index'.
Parameter name: index
    at System.Windows.Forms.ComboBox.ObjectCollection.get_Item(Int32 index)
    at AGS.Editor.ComboBoxCustom.<OnCreateControl>b__4_2(Object s, DrawItemEventArgs a)
    at System.Windows.Forms.DrawItemEventHandler.Invoke(Object sender, DrawItemEventArgs e)
    at System.Windows.Forms.ComboBox.OnDrawItem(DrawItemEventArgs e)
    at System.Windows.Forms.ComboBox.WmReflectDrawItem(Message&m)
    at System.Windows.Forms.ComboBox.WndProc(Message&m)
    at AGS.Editor.ComboBoxCustom.WndProc(Message&m)
    at System.Windows.Forms.Control.ControlNativeWindow.OnMessage(Message&m)
    at System.Windows.Forms.Control.ControlNativeWindow.WndProc(Message&m)
    at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)

AGS Version AGS 4.0 https://cirrus-ci.com/build/5180877416693760 [edit] 3.6.0 too: #2268

To Reproduce I can't reproduce it.

Since those 2 issues seemed to happen around the same time, they might be related: https://github.com/adventuregamestudio/ags/issues/1920

ivan-mogilko commented 1 year ago

I was not able to reproduce this myself, but judging by the callstack, this possibly refers to the location dropdown list; in which case this may be connected with #1924. If #1924 is fixed, this problem should be retested.

tarnos12 commented 1 year ago

To add to that I did have some issues with dropdowns, some scripts would have empty drop down list, even tho there were plenty of functions in it. Same for certain GUI, list would show to be empty, until you clicked on it.

https://user-images.githubusercontent.com/10388576/219783158-7326a605-fc95-4a55-90d9-ad6aff9b3460.mp4

No idea how to reproduce it, I just clicked on some guis inside a folder, then another gui outside the folder and went back into the folder and this happened.

ivan-mogilko commented 10 months ago

Also reproduced in 3.6.0:

2268

  • Typing partial OOP code and switching between ash and asc file to copy and paste the heading
  • Using #region areas

After the error appears. When I save, the editor switches tabs from the asc file to the ash file automatically. The workaround is to close and re-open the editor.

AlanDrake commented 10 months ago

I'm still not sure what's going on there, but I expect the problem is happening inside the DrawItem += (s, a) => event inside ComboBoxCustom.

That's where the index is being accessed for drawing the item. I wonder if the event is being added multiple times, or if it's a concurrency issue. Maybe the latter?

ericoporto commented 10 months ago

Reminder, to anyone trying to reproduce this, you need to be using a Theme to get this error!

Still, I can't figure any way to reproduce this in my machine... I am typing object oriented code and hitting Ctrl+m to alternate between header and script like crazy and nothing happens.

I wonder if the event is being added multiple times

Adding a breakpoint inside the draw, it does get hit twice.

if it's a concurrency issue

I noticed this lint warning in my VS on the lock:

S3998: Replace this lock on 'ComboBox' with a lock against an object that cannot be accessed across application domain boundaries. S2445: Do not lock on writable field 'cmbFunctions', use a readonly field instead.

image

It looks like an object just be used as lock should be used instead, and then anytime we manipulate or read the combobox we would need to lock, check the example here: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock

This specific lock code in the script editor is from CJ (from git-blame), so we haven't touched on it.

Typing partial OOP code and switching between ash and asc

I don't know if this causes any problem, but I noticed at this point that the recently typed function, using the :: notation, is added to the _script.AutoCompleteData.Functions and not yet in the functions in the _script.AutoCompleteData.Structs.

ericoporto commented 10 months ago

If we are ok, I think a simple bandage fix should do it

diff --git a/Editor/AGS.Editor/ColorThemes/Controls/ComboBoxCustom.cs b/Editor/AGS.Editor/ColorThemes/Controls/ComboBoxCustom.cs
index 52ca59069..4718c8e73 100644
--- a/Editor/AGS.Editor/ColorThemes/Controls/ComboBoxCustom.cs
+++ b/Editor/AGS.Editor/ColorThemes/Controls/ComboBoxCustom.cs
@@ -63,7 +63,7 @@ namespace AGS.Editor

             DrawItem += (s, a) =>
             {
-                if (a.Index >= 0)
+                if (a.Index >= 0 && a.Index < this.Items.Count)
                 {
                     a.DrawBackground();
ivan-mogilko commented 10 months ago

If you suspect this is a multithreading issue, then reproducing this by hand may be a bad idea. It's better to investigate how the conflict between two threads may occur in theory, and then make some kind of an automatic test that runs operations in a loop very fast, trying to get this conflict happen.