Page 1 of 1

GC sometimes collects live objects?

PostPosted: Tue Jan 16, 2018 10:44 am
by phantombeta
While working on my mod, I stumbled upon a rather annoying bug: sometimes when changing pages in the shop menu, it'd crash GZDoom or the menu would stop working. No matter how many null checks I added, it wouldn't make the bug disappear.

Recently I got fed up with this bug, and put GZDoom through a debugger. This showed almost much nothing, aside from the fact that it was crashing due to apparently accesses to freed memory.
While trying to figure it out, I realized that maybe it was something with the garbage collection... And apparently it is.

Here's an example PK3.
There's two ways to trigger this bug, one is through the shop menu and one is through the weapon assignment menu.

Common steps:
  • Load the PK3.
  • Start the game.
  • Do "GC PAUSE 1", "GC STEPMUL 1000000" and "GC NOW" in the console. (In this order)

Shop menu:
  • Bind the "Buy Menu" command and activate it or do "S7_LuciusShop" in the console.
  • Click around on the items in the list on the left side of the screen.
Now either the menu will stop working, or the game will crash. If the former happens, you can click around the items list for a bit and it should crash.


Weapon assignment menu:
  • Give yourself all items
  • Bind the "Weap assign menu" command and activate it or do "S7_WeapMenu" in the console.
  • Click on an empty/unused, non-blocked spot in the binds grid. This may or may not crash GZDoom.
  • Click on a weapon in the list on the bottom of the screen.
GZDoom will then freeze for a bit and crash.

(Note: This also happens without messing around with the GC command. The only reason you're supposed to do it for this test PK3 is because it makes it much easier to reproduce the bug. Without doing that, it requires some luck to trigger the bug.)

Re: GC sometimes collects live objects?

PostPosted: Wed Jan 17, 2018 8:50 am
by _mental_
I checked the first menu only. Clicking on weapon leads to double free of three S7_ShopMenu_ListItem objects from S7_ShopMenu_List object. Certainly they came from
Code: Select allExpand view
Array<S7_ShopMenu_ListItem> items;

That's all I can figure out from debugging the given sample. Have no idea why they are removed twice.

Re: GC sometimes collects live objects?

PostPosted: Wed Jan 17, 2018 3:49 pm
by gwHero
It looks like this is related to a problem that occurs sometimes with the TC I'm working on. At rare occasions an entry in a dynamic array is empty, not because it was deleted (which is something that can occur too - sometimes a deleted item can result in an empty element instead of being really removed), but this is more serious: the item is empty after it had just been added to the array.

Since it was difficult to reproduce, I had not yet a small testwad ready for a bug report in which I had narrowed it down; but after reading this post I tried the GC commands (thank you phantombeta for that!) and - bingo - it occurred right away.

So I created a small testwad too; it's so small that it will not likely occur, but after issuing the GC commands, it will occur very quickly. (see attached pk3; I was in doubt to create a separate thread, but since I think it's related I thought it could be helpful here)

Re: GC sometimes collects live objects?

PostPosted: Thu Jan 18, 2018 9:21 am
by _mental_
I found the problem but have no complete solution yet.

There is GC::WriteBarrier() call missing for objects (instances of classes) added to dynamic arrays. Not sure about removal though.
Dynamic array's elements are handled internally as opaque pointers. In other words there is no guarantee that these objects are subject to GC.
However it seems to be impossible to add such non-GC objects (pointers to native struct's) from user's script.

In any case I need Graf's knowledge on this topic.

Re: GC sometimes collects live objects?

PostPosted: Thu Jan 18, 2018 9:46 am
by Graf Zahl
Do you mean by writing to an array index or by pushing an element onto the array? Everything that uses VM instructions to store a pointer should be fine, but it looks like this requires a different Push method than regular pointers.

(BTW, I'm not too fond of how the GC works - all the write barrier stuff is really annoying and cause of countless problems.)

Re: GC sometimes collects live objects?

PostPosted: Thu Jan 18, 2018 10:13 am
by _mental_
It's about pushing new entries to a dynamic array. Hacking in a write barrier there fixed collection of used objects.

Re: GC sometimes collects live objects?

PostPosted: Thu Jan 18, 2018 10:31 am
by Graf Zahl
Ok. Since you say 'hack', I guess it's not a truly viable solution, is it?

Re: GC sometimes collects live objects?

PostPosted: Thu Jan 18, 2018 11:18 am
by _mental_
To verify my hunch I added a cast to DObject* to Push() action function of FDynArray_Ptr and call GC::WriteBarrier() on it.
This solved the problem. At the same time void* is used here for some reason.

Re: GC sometimes collects live objects?

PostPosted: Thu Jan 18, 2018 11:52 am
by Graf Zahl
The reason being that you can create arrays of non-object type, e.g. sectors or lines. Trying to call WriteBarrier on those will pretty nicely crash.

Re: GC sometimes collects live objects?

PostPosted: Thu Jan 18, 2018 3:15 pm
by _mental_
Yes, that’s the problem. I found no way to distinguish a pointer to native struct from a pointer to managed object of class derived from DObject.
However it seems to be impossible to create the first case in user’s script.
All of them are defined as native fields like array<@type> and managed from C++.
So my hack most likely will work fine until such dynamic array is used from a core script.
This doesn’t make it acceptable solution though.

Re: GC sometimes collects live objects?

PostPosted: Thu Jan 18, 2018 3:56 pm
by Graf Zahl
You are mixing up plain struct arrays with pointer arrays here. @-syntax is a reference to a value type of a native struct, because normally you can only have pointers to them.
array<sector> should work.

Re: GC sometimes collects live objects?

PostPosted: Sun Jan 21, 2018 4:10 am
by gwHero
Thanks all for fixing this (did some tests with the latest devbuild). I think this is a very important fix, because without this patch dynamic arrays containing objects are unstable. This had troubled me for weeks and thanks to phantombeta's report I now know it was even nastier than I suspected. I'll do some more extensive testing myself.