TylerTemp / SaintsField

A Unity Inspector extension tool focusing on script fields inspector enhancement
MIT License
151 stars 9 forks source link

UnityException during RichTextDrawerDispose - Mem Leak? #3

Closed MPierer closed 5 months ago

MPierer commented 6 months ago

Using Package 2.1.9 and Unity 2022.3 (same with 2021) I get an exception in the Example scene when selection changes, while debugging a project.

UnityEngine.UnityException: Destroy can only be called from the main thread.
Constructors and field initializers will be executed from the loading thread when loading a scene.
Don't use this function in the constructor or field initializers, instead move initialization code to the Awake or Start function.
  at (wrapper managed-to-native) UnityEngine.Object.Destroy(UnityEngine.Object,single)
  at UnityEngine.Object.Destroy (UnityEngine.Object obj) [0x00007] in <3b24cc7fa9794ed8ab04312c53e6dedd>:0 
  at SaintsField.Editor.Core.RichTextDrawer.Dispose () [0x0001e] in .\Packages\today.comes.saintsfield@2.1.9\Editor\Core\RichTextDrawer.cs:54 
  at SaintsField.Editor.Drawers.RichLabelAttributeDrawer.Finalize () [0x00002] in .\Packages\today.comes.saintsfield@2.1.9\Editor\Drawers\RichLabelAttributeDrawer.cs:35 

The SaintsPropertyDrawers get instantiated by Unity on the main thread, but get delected by the Garbage Collector on the GC Thread. So I guess when the exception is thrown there is also a mem leak following. This will happen for other Drawers as well, like the Rate drawer, which needs to dispose Texture2Ds. Not sure what might be the best solution: either scheduling these Dispose calls on the main thread (like https://github.com/StansAssets/com.stansassets.foundation/tree/master/Runtime/Async/MainThreadDispatcher ) or listening to Selection Changed event and call sth. like "Disable" on the property drawers (see: https://discussions.unity.com/t/onenable-disable-on-property-drawers/117700/5 )

TylerTemp commented 6 months ago

Thanks for reporting!

However, I can not reproduce it with the following setup on 2.1.9:

Attempt 1

  1. Unity 2022.3.0f1
  2. Windows, target platform Windows
  3. Open the example scene, click Rate, and ensure the stars are displayed on the inspector. Click ReadOnly to un-active Rate. The Console didn't give any message.
  4. Window - Saints - Disable UI Toolkit Support, wait, and do step 3 again. Still the same.

Attempt 2

  1. Unity 2021.3.24f1
  2. Windows, target platform Windows
  3. do the same step 3 of Attempt 1, and still the same result
  4. 2021 does not use UI Toolkit as the default inspector so skip step 4

Please give the following information of how I can re-produce the env as you do to check the issue:

  1. Your platform: Windows/Mac/Linux(?)
  2. Your Unity version (ends with f1 so it's the full version I can check, and if it's f1c1 that means it's Chinese special edition)

BTW, thanks for the information you provide that says the following:

Note: This is called by the GC; and not necessarily on inspector selection change.

So indeed there IS an issue when SaintsField:

// RichLabelAttributeDrawer.cs
~RichLabelAttributeDrawer()
{
    _richTextDrawer.Dispose();
}

// RichTextDrawer.cs
public void Dispose()
{
    foreach (Texture cacheValue in _textureCache.Values)
    {
        UnityEngine.Object.Destroy(cacheValue);  // Yep, this should be the problem if Unity disallow Texture2D release on a GC calling...
    }
    _textureCache.Clear();
}

I'll see if there is a better option for SaintsField to handle this.

MPierer commented 6 months ago

Thanks for your comments and sorry for not being very precise in my report. It's in Unity 2022.3.10f1, but happens in 2020.3.48f1 as well. The exception is only caught in Debugger (Rider or Visual Studio), it won't be displayed in the normal console. Make sure the Debugger is set to catch exceptions. When the exceptions is thrown the calls to Destroy the Unity Textures (in RichTextDrawer.Dispose for example) won't have any effect and memory will be leaked.