Critical bug in managed deserialization of asynchronously loaded subscenes

[eternal venture]

Been running into an issue with Managed deserialization when loading subscenes asynchronously (turning off parallel loading stops issue happening)

Parameter name: index
System.Array.InternalArray__set_Item[T] (System.Int32 index, T item) (at <314938d17f3848e8ac683e11b27f62ee>:0)
(wrapper managed-to-managed) Interactions.Client.Data.IndicatorRuntimeModifiers.GameObjectStateModifierRuntime[].System.Collections.Generic.IList`1.set_Item(int,Interactions.Client.Data.IndicatorRuntimeModifiers.GameObjectStateModifierRuntime)
Unity.Properties.IndexedCollectionPropertyBag`2+ListElementProperty[TList,TElement].SetValue (TList& container, TElement value) (at <1f2827b3bbf948c595ac00ad59b25baf>:0)
Unity.Entities.ManagedObjectRemap.Unity.Properties.IPropertyVisitor.Visit[TContainer,TValue] (Unity.Properties.Property`2[TContainer,TValue] property, TContainer& container) (at Library/PackageCache/com.unity.entities@bce0b9d843e1/Unity.Entities/Properties/ManagedObjectRemap.cs:174)```

I've done a bunch of debugging on this and I'm pretty sure it's a threading issue with Properties package and IndexedCollectionPropertyBag - which is either not designed to be used multithreaded like Entities is doing OR it's just a bug in the properties module.

Unfortunately the properties package is now a module inside UnityEditor so my ability to modify and debug it better is limited but here are my results.

The error is caused because for example, it's trying to write to index 4 in an array of length 3

Assuming I didn't bust this test, I'm pretty sure the problem is the same instance of the IndexedCollectionPropertyBag is being used across multiple threads at the same time

• first number is the managed thread index
• last number is the instance of the Property<TContainer, TValue> property

Diving into the source code, which again i'm limited because its in the unityengine so it's hard for me to definitively prove this specifically is the issue - but I think the problem is in IndexedCollectionPropertyBagEnumerator

struct IndexedCollectionPropertyBagEnumerator<TContainer> : IEnumerator<IProperty<TContainer>>
    {
        readonly IIndexedCollectionPropertyBagEnumerator<TContainer> m_Impl;
        readonly IndexedCollectionSharedPropertyState m_Previous;

        TContainer m_Container;
        int m_Position;

        internal IndexedCollectionPropertyBagEnumerator(IIndexedCollectionPropertyBagEnumerator<TContainer> impl, TContainer container)
        {
            m_Impl = impl;
            m_Container = container;
            m_Previous = impl.GetSharedPropertyState();
            m_Position = -1;
        }

        /// <inheritdoc/>
        public IProperty<TContainer> Current => m_Impl.GetSharedProperty();

        /// <inheritdoc/>
        object IEnumerator.Current => Current;

        /// <inheritdoc/>
        public bool MoveNext()
        {
            m_Position++;

            if (m_Position < m_Impl.GetCount(ref m_Container))
            {
                m_Impl.SetSharedPropertyState(new IndexedCollectionSharedPropertyState { Index = m_Position, IsReadOnly = false });
                return true;
            }

            m_Impl.SetSharedPropertyState(m_Previous);
            return false;
        }

        /// <inheritdoc/>
        public void Reset()
        {
            m_Position = -1;
            m_Impl.SetSharedPropertyState(m_Previous);
        }

        /// <inheritdoc/>
        public void Dispose()
        {
        }
    }```

In particular the MoveNext method which sets the
m_Impl.SetSharedPropertyState(new IndexedCollectionSharedPropertyState { Index = m_Position, IsReadOnly = false });
Shared Property of the Instance value (m_Impl), which as above is being shared between threads

__so if 2 threads are both iterating the same Type of Array at the same time to deserialize, their indices get mixed up __
A lot of the time it's just going to write to the wrong element - until 1 array is a different length and then its now writing out of range and an exception is thrown as above

The type of component that is causing a lot of issues.

public class InteractableIndicatorVisualModifiers : IComponentData
{
    public int sourceGuid;

    public GameObjectStateModifierRuntime[] States;
    public ParticleRendererMaterialModifiersRuntime[] ParticleMaterials;
    public ParticleStartColorModifiersRuntime[] ParticleStartColors;
    public ParticleStartSizeModifiersRuntime[] ParticleStartSizes;
    public TransformModifiersRuntime[] Transforms;```

I haven't created a bug report yet as they'll just request a repro and I don't have a small one.
On work project the repro rate is 90%+ for me atm which is destroying me as it's breaking a critical subscene loading, but for other developers it's like 1/20.
While large, if required, I can probably upload full work project.

@sudden pine Sorry for the ping; I try to avoid pinging you directly. I'm hoping I'm wrong here with my conclusion, but this seems like a fundamentally broken issue with subscene deserialization that I can't fix myself, so it'd be really good if someone can look at this. It's destroying my productivity as I can barely enter play mode in work project.

bit more code follow up, you can see in second screenshot the ListElementProperty is the same across threads and this is what is getting written to by the IndexedCollectionPropertyBagEnumerator

        /// <summary>
        /// Shared instance of a list element property. We re-use the same instance to avoid allocations.
        /// </summary>
        readonly ListElementProperty m_Property = new ListElementProperty();
        
        void IIndexedCollectionPropertyBagEnumerator<TList>.SetSharedPropertyState(IndexedCollectionSharedPropertyState state)
        {
            m_Property.m_Index = state.Index;
            m_Property.m_IsReadOnly = state.IsReadOnly;
        }

"re-use it" but it's also being re-used across threads

[velvet frigate]

(we're looking, and sorry for the delay; it seems legit to me but also thorny to fix)

[velvet frigate]

say, what version is this on? the bce0b9d843e1 from com.unity.entities@bce0b9d843e1 doesn't seem to be from our repo, and we don't seem to have a ManagedObjectRemap.cs:174

[eternal venture]

This was on work project, can't check for an hour but I'm pretty sure 1.4.0-exp.2
Sorry the hash won't match as we have a tiny fork on entities

[velvet frigate]

np. seems we have an idea of what is going on at least