Cherry-pick ASAN fixes and Outliner Crash from dev to point release 24.09.2 (#18669)

* Cherrypick from Dev: Fixes an assert that occurs if you undo/redo prefab focus (#18659)

Fixes issue #4864 - https://github.com/o3de/o3de/issues/4864

Turns out that the outliner was creating undo/redo on selection change,
which is fine, but not if the seleciton change is BECAUSE of an undo/redo.

Tested by undoing and redoing many slice edits and operations.

Signed-off-by: Nicholas Lawson <[email protected]>

* CherryPick from Dev:  ASAN fixes (from PR #18655)

* Fixes many issues discovered by the Address Sanitizer

With these changes, I was able to run all of the tests in the following folders
in debug, with Address Sanitizer enabled.
* Everything in Code/* - so framework, tools, AP, etc.
* Everything In Gems/Atom* - so all atom tests in there.

Note that you will not be able to reproduce this - the current version of GoogleMock
has ASAN issues inside it.  I will be following this up with a fix to GoogleMock
that upgrades it to the latest version, which has those ASAN issues fixed.

Signed-off-by: Nicholas Lawson <[email protected]>

* Also fixes an issue where if the build machine is in a locale not using period as the
decimal separator, it will fail the tests.

Signed-off-by: Nicholas Lawson <[email protected]>

---------

Signed-off-by: Nicholas Lawson <[email protected]>

Code/Editor/Lib/Tests/test_ModularViewportCameraController.cpp

@@ -299,7 +299,12 @@ namespace UnitTest
         RepeatDiagonalMouseMovements(
             [this]
             {
-                return GetParam();
+                // note that earlier versions of GoogleMock required 'this' capture in order to call GetParam
+                // GetParam is not a static member, but only works on static class data, which can cause some compilers to complain
+                // about 'this' being captured but not used, and other situations to complain about this NOT being captured but used,
+                // depending on which version of googlemock we actually use.  To try to satisfy all versions, we will explicitly capture 'this'
+                // AND explicily use it despite it not being syntactically necessary (ie, 'this->' being impiled by GetParam()).
+                return this->GetParam();
             });
 
         // Then

Code/Framework/AzCore/AzCore/DOM/DomPath.cpp

@@ -464,7 +464,7 @@ namespace AZ::Dom
         }
 
         size_t pathEntryCount = 0;
-        for (size_t i = 1; i <= pathString.size(); ++i)
+        for (size_t i = 1; i < pathString.size(); ++i)
         {
             if (pathString[i] == PathSeparator)
             {

Code/Framework/AzCore/AzCore/IO/Streamer/FileRequest.h

@@ -272,6 +272,21 @@ namespace AZ::IO
 {
     class Streamer_SchedulerTest_RequestSorting_Test;
 
+    // Note to API Callers
+    // The FileRequest class is used to create requests that are processed by the Streamer, 
+    // If you do use this class, note that the read requests are hierarchical, and in general, a "Parent" high level request is created to
+    // for example read the entire contents of a file into a buffer, and the system then internally creates child requests with that parent
+    // to split reads up and to queue them and sort them, with one parent potentially having several child requests pointed at the same file name
+    // but potentially different sizes and offsets.
+    // To avoid copying strings all over the place, the child requests usually take the file name by const FilePath& reference, and establish a reference
+    // to the given memory instead of a copy.
+    //
+    // If you write code that exercises the low level child request APIs directly
+    // (For example, by using CreateRead instead of CreateReadRequest), be aware that input parameters such as `const RequestPath& path` 
+    // expect the given memory to be valid until the child request is complete as it is usually owned by the parent request.
+    // Be especially careful if writing tests that the memory is not from a temporary object that is going to be destroyed before the streamer
+    // system is exercised.
+
     class FileRequest final
     {
         friend Streamer_SchedulerTest_RequestSorting_Test;
@@ -295,24 +310,35 @@ namespace AZ::IO
 
         void CreateRequestLink(FileRequestPtr&& request);
         void CreateRequestPathStore(FileRequest* parent, RequestPath path);
+
+        // Public-facing API - creates a root request and stores the path and desired offset and size to be processed by the streamer.
         void CreateReadRequest(RequestPath path, void* output, u64 outputSize, u64 offset, u64 size,
             AZStd::chrono::steady_clock::time_point deadline, IStreamerTypes::Priority priority);
         void CreateReadRequest(RequestPath path, IStreamerTypes::RequestMemoryAllocator* allocator, u64 offset, u64 size,
             AZStd::chrono::steady_clock::time_point deadline, IStreamerTypes::Priority priority);
+
+        // Internal API.   The above internally creates the below individual child requests.  See note at the top of this class.
         void CreateRead(FileRequest* parent, void* output, u64 outputSize, const RequestPath& path, u64 offset, u64 size, bool sharedRead = false);
-        void CreateCompressedRead(FileRequest* parent, const CompressionInfo& compressionInfo, void* output,
-            u64 readOffset, u64 readSize);
-        void CreateCompressedRead(FileRequest* parent, CompressionInfo&& compressionInfo, void* output,
-            u64 readOffset, u64 readSize);
+
+        // Ensure compressionInfo& outlives the request if you call this API. 
+        void CreateCompressedRead(FileRequest* parent, const CompressionInfo& compressionInfo, void* output, u64 readOffset, u64 readSize);
+        void CreateCompressedRead(FileRequest* parent, CompressionInfo&& compressionInfo, void* output, u64 readOffset, u64 readSize);
+
         void CreateWait(FileRequest* parent);
+
+        // See the note at the top of this class about const & references to RequestPath.
         void CreateFileExistsCheck(const RequestPath& path);
         void CreateFileMetaDataRetrieval(const RequestPath& path);
+
         void CreateCancel(FileRequestPtr target);
         void CreateReschedule(FileRequestPtr target, AZStd::chrono::steady_clock::time_point newDeadline, IStreamerTypes::Priority newPriority);
         void CreateFlush(RequestPath path);
         void CreateFlushAll();
+
+        // The following copy the FileRange, so it does not need to exist beyond the call to this function.
         void CreateDedicatedCacheCreation(RequestPath path, const FileRange& range = {}, FileRequest* parent = nullptr);
         void CreateDedicatedCacheDestruction(RequestPath path, const FileRange& range = {}, FileRequest* parent = nullptr);
+
         void CreateReport(AZStd::vector<AZ::IO::Statistic>& output, IStreamerTypes::ReportType reportType);
         void CreateCustom(AZStd::any data, bool failWhenUnhandled = true, FileRequest* parent = nullptr);
 

Code/Framework/AzCore/AzCore/IO/Streamer/Statistics.cpp

@@ -8,6 +8,7 @@
 
 #include <AzCore/IO/Streamer/Statistics.h>
 #include <AzCore/Debug/Profiler.h>
+#include <AzCore/std/string/conversions.h>
 
 namespace AZ::IO
 {
@@ -139,10 +140,17 @@ namespace AZ::IO
         [[maybe_unused]] AZStd::string_view name,
         [[maybe_unused]] double value)
     {
-        AZ_PROFILE_DATAPOINT(AzCore, value,
-            AZStd::wstring::format(L"Streamer/%.*s/%.*s (Raw)",
-            aznumeric_cast<int>(owner.size()), owner.data(),
-            aznumeric_cast<int>(name.size()), name.data()).data());
+        // AZ_PROFILE_DATAPOINT requires a wstring as input, but you can't feed non-wstring parameters to wstring::format.
+        const size_t MaxStatNameLength = 256;
+        wchar_t statBufferStack[MaxStatNameLength];
+        statBufferStack[MaxStatNameLength - 1] = 0; // make sure it is null terminated
+
+        AZStd::to_wstring(statBufferStack, MaxStatNameLength - 1,
+            AZStd::string::format("Streamer/%.*s/%.*s (Raw)",
+                aznumeric_cast<int>(owner.size()), owner.data(),
+                aznumeric_cast<int>(name.size()), name.data()));
+        
+        AZ_PROFILE_DATAPOINT(AzCore, value,statBufferStack);
     }
 
     AZStd::string_view Statistic::GetOwner() const

Code/Framework/AzCore/AzCore/IO/Streamer/Streamer.cpp

@@ -254,52 +254,78 @@ namespace AZ::IO
 
     void Streamer::RecordStatistics()
     {
+        // create a buffer to reuse for wstring conversions in this loop calling AZStd::to_wstring:
+        const size_t MaxStatNameLength = 256;
+        wchar_t statBufferStack[MaxStatNameLength];
+        statBufferStack[MaxStatNameLength - 1] = 0; // make sure it is null terminated
+
+        // note that to_wstring will stop writing to the buffer when it encounters a null terminator in the source string,
+        // or when it runs out of room in the destination string.
+        // In the former case, it will add a null terminator on the end of the destination at the current location.
+        // In the latter case, (buffer is full), it will not null terminate.  To avoid this becoming a problem we will
+        // add a null at the end of the buffer ourselves, and then invoke any to_wstring calls with one less than the buffer size
+        // so that it will never overwrite that null.
+
         AZStd::vector<Statistic> statistics;
         m_streamStack->CollectStatistics(statistics);
         for (Statistic& stat : statistics)
         {
-            auto visitor = [&stat](auto&& value)
+            auto visitor = [&stat, &statBufferStack](auto&& value)
             {
                 AZ_UNUSED(stat);
                 using Type = AZStd::decay_t<decltype(value)>;
                 if constexpr (AZStd::is_same_v<Type, bool>)
                 {
-                    AZ_PROFILE_DATAPOINT(
-                        AzCore, (value ? 1 : 0), AZStd::wstring::format(L"Streamer/%.*s/%.*s", aznumeric_cast<int>(stat.GetOwner().length()), stat.GetOwner().data(),
-                        aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data()).data());
+                    // it is illegal to invoke a format() function on a wide string, (which AZ_PROFILE_DATAPOINT requires), but
+                    // give it non-wide string parameters (which is what is in stat such as stat.GetOwner()) so we have to format it first
+                    // and then convert.  
+                    AZStd::to_wstring(statBufferStack, MaxStatNameLength-1, AZStd::string::format("Streamer/%.*s/%.*s",
+                        aznumeric_cast<int>(stat.GetOwner().length()), stat.GetOwner().data(),
+                        aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data()));
+
+                    AZ_PROFILE_DATAPOINT( AzCore, (value ? 1 : 0), statBufferStack);
                 }
                 else if constexpr (AZStd::is_same_v<Type, double> || AZStd::is_same_v<Type, s64>)
                 {
-                    AZ_PROFILE_DATAPOINT(
-                        AzCore, value, AZStd::wstring::format(L"Streamer/%.*s/%.*s", aznumeric_cast<int>(stat.GetOwner().length()),
-                        stat.GetOwner().data(), aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data()).data());
+                    AZStd::to_wstring(statBufferStack, MaxStatNameLength-1, AZStd::string::format("Streamer/%.*s/%.*s",
+                        aznumeric_cast<int>(stat.GetOwner().length()),  stat.GetOwner().data(),
+                        aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data()));
+
+                    AZ_PROFILE_DATAPOINT(AzCore, value, statBufferStack);
                 }
                 else if constexpr (
                     AZStd::is_same_v<Type, Statistic::FloatRange> || AZStd::is_same_v<Type, Statistic::IntegerRange> ||
                     AZStd::is_same_v<Type, Statistic::ByteSize> || AZStd::is_same_v<Type, Statistic::ByteSizeRange>)
                 {
-                    AZ_PROFILE_DATAPOINT(
-                        AzCore, value.m_value, AZStd::wstring::format(L"Streamer/%.*s/%.*s", aznumeric_cast<int>(stat.GetOwner().length()), stat.GetOwner().data(),
-                        aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data()).data());
+                    AZStd::to_wstring(statBufferStack, MaxStatNameLength-1, AZStd::string::format("Streamer/%.*s/%.*s",
+                        aznumeric_cast<int>(stat.GetOwner().length()), stat.GetOwner().data(),
+                        aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data()));
+
+                    AZ_PROFILE_DATAPOINT(AzCore, value.m_value, statBufferStack);
                 }
                 else if constexpr (AZStd::is_same_v<Type, Statistic::Time> || AZStd::is_same_v<Type, Statistic::TimeRange>)
                 {
-                    AZ_PROFILE_DATAPOINT_PERCENT(
-                        AzCore, value.m_value, "Streamer/%.*s/%.*s (us)", aznumeric_cast<int>(stat.GetOwner().length()),
-                        stat.GetOwner().data(), aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data());
+                    AZStd::to_wstring(statBufferStack, MaxStatNameLength-1, AZStd::string::format("Streamer/%.*s/%.*s (us)",
+                        aznumeric_cast<int>(stat.GetOwner().length()), stat.GetOwner().data(),
+                        aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data()));
+
+                    AZ_PROFILE_DATAPOINT_PERCENT(AzCore, value.m_value, statBufferStack);
                 }
                 else if constexpr (AZStd::is_same_v<Type, Statistic::Percentage> || AZStd::is_same_v<Type, Statistic::PercentageRange>)
                 {
-                    AZ_PROFILE_DATAPOINT_PERCENT(
-                        AzCore, value.m_value, "Streamer/%.*s/%.*s (percent)", aznumeric_cast<int>(stat.GetOwner().length()),
-                        stat.GetOwner().data(), aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data());
+                     AZStd::to_wstring(statBufferStack, MaxStatNameLength-1, AZStd::string::format("Streamer/%.*s/%.*s (percent)",
+                         aznumeric_cast<int>(stat.GetOwner().length()), stat.GetOwner().data(),
+                         aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data()));
+
+                    AZ_PROFILE_DATAPOINT_PERCENT(AzCore, value.m_value, statBufferStack);
                 }
                 else if constexpr (AZStd::is_same_v<Type, Statistic::BytesPerSecond>)
                 {
-                    AZ_PROFILE_DATAPOINT_PERCENT(
-                        AzCore, value.m_value * (1.0f / (1024.0f * 1024.0f)), "Streamer/%.*s/%.*s (mbps)",
-                        aznumeric_cast<int>(stat.GetOwner().length()), stat.GetOwner().data(), aznumeric_cast<int>(stat.GetName().length()),
-                        stat.GetName().data());
+                    AZStd::to_wstring(statBufferStack, MaxStatNameLength-1, AZStd::string::format("Streamer/%.*s/%.*s (mbps)",
+                        aznumeric_cast<int>(stat.GetOwner().length()), stat.GetOwner().data(),
+                        aznumeric_cast<int>(stat.GetName().length()), stat.GetName().data()));
+
+                    AZ_PROFILE_DATAPOINT_PERCENT(AzCore, value.m_value * (1.0f / (1024.0f * 1024.0f), statBufferStack));
                 }
                 // Strings are not supported.
             };

Code/Framework/AzCore/AzCore/JSON/rapidjson.h

@@ -105,30 +105,23 @@
 #pragma clang diagnostic ignored "-Wunknown-warning-option"
 #endif
 
-// Detect what is available in the compiler and enable those features in RapidJSON.
-// Note that RapidJSON will use the combination of any of these to determine its final
-// set of instructions to use, so its best to set all that are applicable:
-#if defined(__SSE4_2__)
-#define RAPIDJSON_SSE42
-#endif
-
-#if defined(__SSE2__)
-#define RAPIDJSON_SSE2
-#endif
-
-#if defined(__ARM_NEON__) || defined(__ARM_NEON) // older compilers define __ARM_NEON
-#define RAPIDJSON_NEON
-#endif
+// RapidJSON has a number of optimizations available such as the ability to use NEON and SSE2.
+// We do not enable these operations.  
+// If you enable these optimizations in the current version of RapidJSON,
+// the function that scans for whitespace in the buffer requires that all strings you feed it 
+// to parse, 
+// * must be aligned to 16 bytes 
+// * must be a multiple of 16 bytes long,
+// * cannot be malformed in terms of missing a closing quote or bracket.
+// We cannot guarantee this as we use it for user generated input data, strings from
+// document files, inline strings from string literals, etc.
+
+// See https://github.com/Tencent/rapidjson/pull/2213 for the active discussion
+// currently ongoing in the RapidJSON community about this.
 
 #if defined(AZ_COMPILER_MSVC)
     // windows defines may or may not be present for unity builds, so ensure StrCmp resolves to StrCmpW to avoid conflicts
     #define StrCmp StrCmpW
-
-    // MSVC compiler does not necessarily specify any of the above macros.
-    // if we're compiling for a X64 target we can target SSE2.x at the very least.
-    #if defined(_M_AMD64)
-        #define RAPIDJSON_SSE2
-    #endif
 #endif
 
 // Push all of rapidjson into a different namespace (rapidjson_ly for backward compatibility, as this is what

Code/Framework/AzCore/AzCore/Memory/SystemAllocator.cpp

@@ -18,6 +18,14 @@
 #include <AzCore/Debug/Profiler.h>
 #include <memory>
 
+// Please note that AZCORE_SYSTEM_ALLOCATOR
+// are ONLY considered #defined for AzCore static library itself.
+// Do not use them in a header file or any other file.
+// If you need to change the system allocator behavior based on this define,
+//  then override the function from IAllocator in the header (without depending on the define),
+//  and then implement the different behavior in this cpp or another cpp in AzCore.
+
+// HPHA uses the high performance heap allocator for system allocations
 #define AZCORE_SYSTEM_ALLOCATOR_HPHA 1
 #define AZCORE_SYSTEM_ALLOCATOR_MALLOC 2
 
@@ -36,8 +44,24 @@
 
 #include <AzCore/Memory/HphaAllocator.h>
 
+#if AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC
+#include <AzCore/std/parallel/atomic.h>
+#endif
+
+
 namespace AZ
 {
+#if AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC
+    namespace SystemAllocatorPrivate
+    {
+        // when using malloc, we track the number of allocated bytes directly instead of via a sub allocator.
+        // note that there should only be one instance, ever, of system allocator, and it is always accessed via the environment
+        // which ensures that the code below is always running in the same context (usually in o3dekernel shared library).
+        static AZStd::atomic<SystemAllocator::size_type> g_AllocatedBytes = {0};
+    };
+
+#endif
+
     //////////////////////////////////////////////////////////////////////////
     AZ_TYPE_INFO_WITH_NAME_IMPL(SystemAllocator, "SystemAllocator", "{607C9CDF-B81F-4C5F-B493-2AD9C49023B7}");
     AZ_RTTI_NO_TYPE_INFO_IMPL(SystemAllocator, AllocatorBase);
@@ -77,6 +101,16 @@ namespace AZ
             .ExcludeFromDebugging(false);
     }
 
+    SystemAllocator::size_type SystemAllocator::NumAllocatedBytes() const
+    {
+#if (AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC)
+        return SystemAllocatorPrivate::g_AllocatedBytes;
+#else
+        return m_subAllocator->NumAllocatedBytes();
+ #endif
+    }
+
+
     //=========================================================================
     // Allocate
     // [9/2/2009]
@@ -90,6 +124,13 @@ namespace AZ
         AZ_Assert(byteSize > 0, "You can not allocate 0 bytes!");
         AZ_Assert((alignment & (alignment - 1)) == 0, "Alignment must be power of 2!");
 
+#if (AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC)
+        AllocateAddress address(AZ_OS_MALLOC(byteSize, alignment), byteSize);
+        if (address)
+        {
+            SystemAllocatorPrivate::g_AllocatedBytes += AZ_OS_MSIZE(address.m_value, alignment);
+        }
+#else
         byteSize = MemorySizeAdjustedUp(byteSize);
         AllocateAddress address =
             m_subAllocator->allocate(byteSize, alignment);
@@ -123,10 +164,20 @@ namespace AZ
     //=========================================================================
     auto SystemAllocator::deallocate(pointer ptr, size_type byteSize, size_type alignment) -> size_type
     {
-        byteSize = MemorySizeAdjustedUp(byteSize);
-        AZ_PROFILE_MEMORY_FREE(MemoryReserved, ptr);
-        AZ_MEMORY_PROFILE(ProfileDeallocation(ptr, byteSize, alignment, nullptr));
-        return m_subAllocator->deallocate(ptr, byteSize, alignment);
+        #if (AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC)
+            AZ_PROFILE_MEMORY_FREE(MemoryReserved, ptr);
+            byteSize = byteSize == 0 ? AZ_OS_MSIZE(ptr, alignment) : byteSize;
+            AZ_MEMORY_PROFILE(ProfileDeallocation(ptr, byteSize, alignment, nullptr));
+            AZ_Assert(SystemAllocatorPrivate::g_AllocatedBytes >= byteSize, "SystemAllocator: Deallocating more memory than allocated!");
+            SystemAllocatorPrivate::g_AllocatedBytes -= byteSize;
+            AZ_OS_FREE(ptr);
+            return byteSize;
+        #else
+            byteSize = MemorySizeAdjustedUp(byteSize);
+            AZ_PROFILE_MEMORY_FREE(MemoryReserved, ptr);
+            AZ_MEMORY_PROFILE(ProfileDeallocation(ptr, byteSize, alignment, nullptr));
+            return m_subAllocator->deallocate(ptr, byteSize, alignment);
+        #endif
     }
 
     //=========================================================================
@@ -135,14 +186,23 @@ namespace AZ
     //=========================================================================
     AllocateAddress SystemAllocator::reallocate(pointer ptr, size_type newSize, size_type newAlignment)
     {
-        newSize = MemorySizeAdjustedUp(newSize);
-
-        AZ_PROFILE_MEMORY_FREE(MemoryReserved, ptr);
-
-        AllocateAddress newAddress = m_subAllocator->reallocate(ptr, newSize, newAlignment);
+        #if (AZCORE_SYSTEM_ALLOCATOR == AZCORE_SYSTEM_ALLOCATOR_MALLOC)
+            AZ_PROFILE_MEMORY_FREE(MemoryReserved, ptr);
+            AZ_Assert(SystemAllocatorPrivate::g_AllocatedBytes >= AZ_OS_MSIZE(ptr, newAlignment), "SystemAllocator: Deallocating more memory than allocated!");
+            SystemAllocatorPrivate::g_AllocatedBytes -= AZ_OS_MSIZE(ptr, newAlignment);
+            AllocateAddress newAddress(AZ_OS_REALLOC(ptr, newSize, newAlignment), newSize);
+            if (newAddress)
+            {
+                SystemAllocatorPrivate::g_AllocatedBytes += newSize;
+            }
+            [[maybe_unused]] const size_type allocatedSize = newSize;
+        #else
+            newSize = MemorySizeAdjustedUp(newSize);
+            AZ_PROFILE_MEMORY_FREE(MemoryReserved, ptr);
+            AllocateAddress newAddress = m_subAllocator->reallocate(ptr, newSize, newAlignment);
+            [[maybe_unused]] const size_type allocatedSize = get_allocated_size(newAddress, 1);
+        #endif
 
-#if defined(AZ_ENABLE_TRACING)
-        [[maybe_unused]] const size_type allocatedSize = get_allocated_size(newAddress, 1);
         AZ_PROFILE_MEMORY_ALLOC(MemoryReserved, newAddress, newSize, "SystemAllocator realloc");
         AZ_MEMORY_PROFILE(ProfileReallocation(ptr, newAddress, allocatedSize, newAlignment));
 #endif

Code/Framework/AzCore/AzCore/Memory/SystemAllocator.h

@@ -49,7 +49,7 @@ namespace AZ
         size_type get_allocated_size(pointer ptr, size_type alignment) const override;
         void            GarbageCollect() override                 { m_subAllocator->GarbageCollect(); }
 
-        size_type       NumAllocatedBytes() const override       { return m_subAllocator->NumAllocatedBytes(); }
+        size_type       NumAllocatedBytes() const override;
 
         //////////////////////////////////////////////////////////////////////////
 

Code/Framework/AzCore/AzCore/Metrics/EventLoggerReflectUtils.cpp

@@ -49,6 +49,39 @@ namespace AZ::Metrics
             // in order to use the BehaviorContext::ClassBuilder::Constructor function
             ScriptEventValue() = default;
 
+            // Special case - when the Script Event Value is being copied from another,
+            // the default constructor cannot be used, as it would do
+            // m_valueStorage = other.m_valueStorage;
+            // m_eventValueMirror = other.m_eventValueMirror;
+            // The problem is that value storage is a local array of data, as in
+            // AZStd::vector<EventValueStorageVariant> m_valueStorage;
+            // and the mirror is supposed to basically point at the front of the storage, the first
+            // value in it, for types that require storage.  Having it point at the `other` storage
+            // risks the `other` being destroyed while this object is still in use
+            // So here, we make sure that the mirror value points the local storage, instead of the other.
+            ScriptEventValue(const ScriptEventValue& other)
+            {
+                m_eventValueMirror = other.m_eventValueMirror;
+                if (!other.m_valueStorage.empty())
+                {
+                    m_valueStorage = other.m_valueStorage;
+                
+                    // we cannot just copy the other's mirror - that would make our mirror point at their internal storage.
+                    if (AZStd::holds_alternative<AZStd::string>(m_valueStorage.front()))
+                    {
+                        m_eventValueMirror = AZStd::get<AZStd::string>(m_valueStorage.front());
+                    }
+                    else if (AZStd::holds_alternative<AZStd::vector<EventValue>>(m_valueStorage.front()))
+                    {
+                        m_eventValueMirror = EventArray{ AZStd::get<AZStd::vector<EventValue>>(m_valueStorage.front()) };
+                    }
+                    else if (AZStd::holds_alternative<AZStd::vector<EventField>>(m_valueStorage.front()))
+                    {
+                        m_eventValueMirror = EventObject{ AZStd::get<AZStd::vector<EventField>>(m_valueStorage.front()) };
+                    }
+                }
+            }
+
             //! string instances need storage
             ScriptEventValue(AZStd::string value)
                 : m_valueStorage{ EventValueStorageVariant{AZStd::in_place_type<AZStd::string>, AZStd::move(value)} }

Code/Framework/AzCore/AzCore/std/string/conversions.h

@@ -26,6 +26,11 @@ namespace AZStd
 {
     namespace Internal
     {
+        inline bool IsValidUtf8(const char* str, size_t length)
+        {
+            return Utf8::is_valid(str, str + length);
+        }
+
         template<size_t Size = sizeof(wchar_t)>
         struct WCharTPlatformConverter
         {
@@ -82,28 +87,43 @@ namespace AZStd
             }
 
             template<class Allocator>
-            static inline void to_wstring(AZStd::basic_string<wstring::value_type, wstring::traits_type, Allocator>& dest, AZStd::string_view src)
+            static inline bool to_wstring(AZStd::basic_string<wstring::value_type, wstring::traits_type, Allocator>& dest, AZStd::string_view src)
             {
+                if (!IsValidUtf8(src.begin(), src.size()))
+                {
+                    return false;
+                }
+
                 if constexpr (Size == 2)
                 {
                     Utf8::Unchecked::utf8to16(src.begin(), src.end(), AZStd::back_inserter(dest), dest.max_size());
+                    return true;
                 }
                 else if constexpr (Size == 4)
                 {
                     Utf8::Unchecked::utf8to32(src.begin(), src.end(), AZStd::back_inserter(dest), dest.max_size());
+                    return true;
                 }
             }
 
+            // note that this template is only ever instantiated on size 2 and 4 so adding anything after the if statements will result in unreachable code.
             template<size_t MaxElementCount>
-            static inline void to_wstring(AZStd::basic_fixed_string<wstring::value_type, MaxElementCount, wstring::traits_type>& dest, AZStd::string_view src)
+            static inline bool to_wstring(AZStd::basic_fixed_string<wstring::value_type, MaxElementCount, wstring::traits_type>& dest, AZStd::string_view src)
             {
+                if (!IsValidUtf8(src.begin(), src.size()))
+                {
+                    return false;
+                }
+
                 if constexpr (Size == 2)
                 {
                     Utf8::Unchecked::utf8to16(src.begin(), src.end(), AZStd::back_inserter(dest), dest.max_size());
+                    return true;
                 }
                 else if constexpr (Size == 4)
                 {
                     Utf8::Unchecked::utf8to32(src.begin(), src.end(), AZStd::back_inserter(dest), dest.max_size());
+                    return true;
                 }
             }
 
@@ -426,26 +446,41 @@ namespace AZStd
     inline AZStd::wstring to_wstring(long double val)           { AZStd::wstring wstr; to_wstring(wstr, val); return wstr; }
 
     template<class Allocator>
-    void to_wstring(AZStd::basic_string<wstring::value_type, wstring::traits_type, Allocator>& dest, AZStd::string_view src)
+    bool to_wstring(AZStd::basic_string<wstring::value_type, wstring::traits_type, Allocator>& dest, AZStd::string_view src)
     {
         dest.clear();
+        if (!Internal::IsValidUtf8(src.begin(), src.size()))
+        {
+            return false;
+        }
         Internal::WCharTPlatformConverter<>::to_wstring(dest, src);
+        return true;
     }
 
     template<size_t MaxElementCount>
-    void to_wstring(AZStd::basic_fixed_string<wstring::value_type, MaxElementCount, wstring::traits_type>& dest, AZStd::string_view src)
+    bool to_wstring(AZStd::basic_fixed_string<wstring::value_type, MaxElementCount, wstring::traits_type>& dest, AZStd::string_view src)
     {
         dest.clear();
+        if (!Internal::IsValidUtf8(src.begin(), src.size()))
+        {
+            return false;
+        }
         Internal::WCharTPlatformConverter<>::to_wstring(dest, src);
+        return true;
     }
 
-    inline void to_wstring(wchar_t* dest, size_t destSize, AZStd::string_view src)
+    inline bool to_wstring(wchar_t* dest, size_t destSize, AZStd::string_view src)
     {
+        if (!Internal::IsValidUtf8(src.begin(), src.size()))
+        {
+            return false;
+        }
         wchar_t* endWStr = Internal::WCharTPlatformConverter<>::to_wstring(dest, destSize, src);
         if (endWStr < (dest + destSize))
         {
             *endWStr = '\0'; // null terminator
         }
+        return true;
     }
 
 

Code/Framework/AzCore/AzCore/std/string/utf8/unchecked.h

@@ -1,6 +1,8 @@
 // Modified from original
 // Copyright 2006 Nemanja Trifunovic
 
+// Original repository at https://github.com/nemtrif/utfcpp
+
 /*
 Permission is hereby granted, free of charge, to any person or organization
 obtaining a copy of the software and accompanying documentation covered by

Code/Framework/AzCore/Platform/Common/WinAPI/AzCore/IO/SystemFile_WinAPI.cpp

@@ -21,6 +21,25 @@ namespace AZ::IO
     using FixedMaxPathWString = AZStd::fixed_wstring<MaxPathLength>;
     namespace
     {
+        // You can use this to issue a warning and print out the BYTES (not characters) of an invalid UTF-8 string
+        // in the format [00][01][02]...[FF].
+        void WarnInvalidUtf8String([[maybe_unused]] const char* message, [[maybe_unused]] const char* str)
+        {
+#if defined(AZ_ENABLE_TRACING)
+            FixedMaxPathString stringBytes;
+            size_t pos = 0;
+            while (stringBytes.size() < stringBytes.max_size() - 5 && str[pos] != 0)
+            {
+                unsigned char currentByte = str[pos];
+                stringBytes.append(AZStd::string::format("[%02X]", currentByte).c_str());
+                ++pos;
+            }
+            
+            AZ_Warning("SystemFile", false, "%s: Invalid UTF-8 string: %s", message, stringBytes.c_str());
+#endif
+        }
+
+
         //=========================================================================
         // GetAttributes
         //  Internal utility to avoid code duplication. Returns result of win32
@@ -29,7 +48,11 @@ namespace AZ::IO
         DWORD GetAttributes(const char* fileName)
         {
             FixedMaxPathWString fileNameW;
-            AZStd::to_wstring(fileNameW, fileName);
+            if (!AZStd::to_wstring(fileNameW, fileName))
+            {
+                WarnInvalidUtf8String("Invalid filename given to SystemFile::GetAttributes", fileName);
+                return INVALID_FILE_ATTRIBUTES;
+            }
             return GetFileAttributesW(fileNameW.c_str());
         }
 
@@ -41,7 +64,11 @@ namespace AZ::IO
         BOOL SetAttributes(const char* fileName, DWORD fileAttributes)
         {
             FixedMaxPathWString fileNameW;
-            AZStd::to_wstring(fileNameW, fileName);
+            if (!AZStd::to_wstring(fileNameW, fileName))
+            {
+                WarnInvalidUtf8String("Invalid filename given to SystemFile::GetAttributes", fileName);
+                return 0;
+            }
             return SetFileAttributesW(fileNameW.c_str(), fileAttributes);
         }
 
@@ -137,7 +164,11 @@ namespace AZ::IO
         }
 
         AZ::IO::FixedMaxPathWString fileNameW;
-        AZStd::to_wstring(fileNameW, m_fileName);
+        if (!AZStd::to_wstring(fileNameW, m_fileName))
+        {
+            WarnInvalidUtf8String("Invalid UTF-8 encoded string passed to SystemFile::Open", m_fileName.c_str());
+            return false;
+        }
         m_handle = INVALID_HANDLE_VALUE;
         m_handle = CreateFileW(fileNameW.c_str(), dwDesiredAccess, dwShareMode, 0, dwCreationDisposition, dwFlagsAndAttributes, 0);
 
@@ -357,7 +388,12 @@ namespace AZ::IO::Platform
         HANDLE hFile;
 
         AZ::IO::FixedMaxPathWString filterW;
-        AZStd::to_wstring(filterW, filter);
+        if (!AZStd::to_wstring(filterW, filter))
+        {
+            // we'd print out the string - but its an invalid string and not safe to do so... since its invalid encoding!
+            WarnInvalidUtf8String("Invalid UTF-8 encoded string passed to SystemFile::FindFiles", filter);
+            return;
+        }
         hFile = INVALID_HANDLE_VALUE;
         hFile = FindFirstFileW(filterW.c_str(), &fd);
 
@@ -389,7 +425,11 @@ namespace AZ::IO::Platform
         HANDLE handle = nullptr;
 
         AZ::IO::FixedMaxPathWString fileNameW;
-        AZStd::to_wstring(fileNameW, fileName);
+        if (!AZStd::to_wstring(fileNameW, fileName))
+        {
+            WarnInvalidUtf8String("Invalid UTF-8 encoded string passed to SystemFile::ModificationTime, returning 0", fileName);
+            return 0;
+        }
         handle = CreateFileW(fileNameW.c_str(), 0, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, 0, nullptr);
 
         if (handle == INVALID_HANDLE_VALUE)
@@ -419,7 +459,11 @@ namespace AZ::IO::Platform
         BOOL result = FALSE;
 
         AZ::IO::FixedMaxPathWString fileNameW;
-        AZStd::to_wstring(fileNameW, fileName);
+        if (!AZStd::to_wstring(fileNameW, fileName))
+        {
+            WarnInvalidUtf8String("Invalid UTF-8 encoded string passed to SystemFile::Length, returning 0", fileName);
+            return 0;
+        }
         result = GetFileAttributesExW(fileNameW.c_str(), GetFileExInfoStandard, &data);
 
         if (result)
@@ -437,7 +481,12 @@ namespace AZ::IO::Platform
     bool Delete(const char* fileName)
     {
         AZ::IO::FixedMaxPathWString fileNameW;
-        AZStd::to_wstring(fileNameW, fileName);
+        if (!AZStd::to_wstring(fileNameW, fileName))
+        {
+            WarnInvalidUtf8String("Invalid UTF-8 encoded string passed to SystemFile::Delete", fileName);
+            return false;
+        }
+
         if (DeleteFileW(fileNameW.c_str()) == 0)
         {
             return false;
@@ -449,9 +498,19 @@ namespace AZ::IO::Platform
     bool Rename(const char* sourceFileName, const char* targetFileName, bool overwrite)
     {
         AZ::IO::FixedMaxPathWString sourceFileNameW;
-        AZStd::to_wstring(sourceFileNameW, sourceFileName);
+        if (!AZStd::to_wstring(sourceFileNameW, sourceFileName))
+        {
+            WarnInvalidUtf8String("Invalid UTF-8 encoded string sourceFileName in SystemFile::Rename", sourceFileName);
+            return false;
+        }
+
         AZ::IO::FixedMaxPathWString targetFileNameW;
-        AZStd::to_wstring(targetFileNameW, targetFileName);
+        if (!AZStd::to_wstring(targetFileNameW, targetFileName))
+        {
+            WarnInvalidUtf8String("Invalid UTF-8 encoded string targetFileName in SystemFile::Rename", targetFileName);
+            return false;
+        }
+
         if (MoveFileExW(sourceFileNameW.c_str(), targetFileNameW.c_str(), overwrite ? MOVEFILE_REPLACE_EXISTING : 0) == 0)
         {
             return false;
@@ -491,7 +550,11 @@ namespace AZ::IO::Platform
         if (dirName)
         {
             AZ::IO::FixedMaxPathWString dirNameW;
-            AZStd::to_wstring(dirNameW, dirName);
+            if (!AZStd::to_wstring(dirNameW, dirName))
+            {
+                WarnInvalidUtf8String("Invalid UTF-8 encoded string passed to SystemFile::CreateDir", dirName);
+                return false;
+            }
             bool success = CreateDirRecursive(dirNameW);
             return success;
         }
@@ -503,7 +566,11 @@ namespace AZ::IO::Platform
         if (dirName)
         {
             AZ::IO::FixedMaxPathWString dirNameW;
-            AZStd::to_wstring(dirNameW, dirName);
+            if (!AZStd::to_wstring(dirNameW, dirName))
+            {
+                WarnInvalidUtf8String("Invalid UTF-8 encoded string passed to SystemFile::DeleteDir", dirName);
+                return false;
+            }
             return RemoveDirectory(dirNameW.c_str()) != 0;
         }
 

Code/Framework/AzCore/Tests/AZTestShared/Math/MathTestHelpers.cpp

@@ -19,6 +19,8 @@
 #include <AzCore/Math/Transform.h>
 #include <AzCore/Math/Color.h>
 
+#include <iomanip> // for std::setw
+
 // make gtest/gmock aware of these types so when a failure occurs we get more useful output
 namespace AZ
 {

Code/Framework/AzCore/Tests/Name/NameJsonSerializerTests.cpp

@@ -9,6 +9,8 @@
 #include <AzCore/Casting/lossy_cast.h>
 #include <AzCore/Name/NameJsonSerializer.h>
 #include <AzCore/Name/NameDictionary.h>
+#include <AzCore/Serialization/Locale.h>
+
 #include <Tests/Serialization/Json/BaseJsonSerializerFixture.h>
 #include <Tests/Serialization/Json/JsonSerializerConformityTests.h>
 
@@ -182,6 +184,7 @@ namespace JsonSerializationTests
     TEST_F(NameJsonSerializerTests, Load_ParseFloatingPointValue_NumberReturnedAsString)
     {
         using namespace AZ::JsonSerializationResult;
+        AZ::Locale::ScopedSerializationLocale scopedLocale; // For the purposes of test stability, assume the culture-invariant locale.
 
         rapidjson::Value testValue;
         testValue.SetDouble(3.1415);

Code/Framework/AzCore/Tests/Serialization/Json/StringSerializerTests.cpp

@@ -8,6 +8,8 @@
 
 #include <AzCore/Casting/lossy_cast.h>
 #include <AzCore/Serialization/Json/StringSerializer.h>
+#include <AzCore/Serialization/Locale.h>
+
 #include <Tests/Serialization/Json/BaseJsonSerializerFixture.h>
 #include <Tests/Serialization/Json/JsonSerializerConformityTests.h>
 
@@ -182,6 +184,8 @@ namespace JsonSerializationTests
     {
         using namespace AZ::JsonSerializationResult;
 
+        AZ::Locale::ScopedSerializationLocale scopedLocale; // For the purposes of test stability, assume the culture-invariant locale.
+
         rapidjson::Value testValue;
         testValue.SetDouble(3.1415);
         typename SerializerInfo<TypeParam>::DataType convertedValue{};

Code/Framework/AzCore/Tests/Streamer/SchedulerTests.cpp

@@ -33,8 +33,9 @@ namespace AZ::IO
             using ::testing::AnyNumber;
 
             UnitTest::LeakDetectionFixture::SetUp();
-
-            m_mock = AZStd::make_shared<StreamStackEntryMock>();
+            // a regular mock warns every time functions are called without being expected
+            // a NiceMock only pays attention to calls you tell it to pay attention to.
+            m_mock = AZStd::make_shared<testing::NiceMock<StreamStackEntryMock>>();
             ON_CALL(*m_mock, PrepareRequest(_)).WillByDefault([this](FileRequest* request) { m_mock->ForwardPrepareRequest(request); });
             ON_CALL(*m_mock, QueueRequest(_)).WillByDefault([this](FileRequest* request)   { m_mock->ForwardQueueRequest(request); });
             ON_CALL(*m_mock, UpdateStatus(_)).WillByDefault([this](StreamStackEntry::Status& status)
@@ -158,7 +159,7 @@ namespace AZ::IO
         // is publicly exposed. Since Streamer is mostly the threaded front end for
         // the Scheduler, this is fine.
         Streamer* m_streamer{ nullptr };
-        AZStd::shared_ptr<StreamStackEntryMock> m_mock;
+        AZStd::shared_ptr<testing::NiceMock<StreamStackEntryMock>> m_mock;
         AZStd::atomic_bool m_isStackIdle = false;
     };
 
@@ -406,9 +407,10 @@ namespace AZ::IO
         //////////////////////////////////////////////////////////////
         // Test equal priority requests that are both reading the same file
         //////////////////////////////////////////////////////////////
+        RequestPath emptyPath;
         FileRequestPtr readRequest = m_streamer->Read("SameFile", fakeBuffer, sizeof(fakeBuffer), 8, panicDeadline);
         FileRequestPtr sameFileRequest = m_streamer->CreateRequest();
-        sameFileRequest->m_request.CreateRead(&sameFileRequest->m_request, fakeBuffer, 8, RequestPath(), 0, 8);
+        sameFileRequest->m_request.CreateRead(&sameFileRequest->m_request, fakeBuffer, 8, emptyPath, 0, 8);
         sameFileRequest->m_request.m_parent = &readRequest->m_request;
         sameFileRequest->m_request.m_dependencies = 0;
 
@@ -419,7 +421,7 @@ namespace AZ::IO
 
         FileRequestPtr readRequest2 = m_streamer->Read("SameFile2", fakeBuffer, sizeof(fakeBuffer), 8, panicDeadline);
         FileRequestPtr sameFileRequest2 = m_streamer->CreateRequest();
-        sameFileRequest2->m_request.CreateRead(&sameFileRequest2->m_request, fakeBuffer, 8, RequestPath(), 0, 8);
+        sameFileRequest2->m_request.CreateRead(&sameFileRequest2->m_request, fakeBuffer, 8, emptyPath, 0, 8);
         sameFileRequest2->m_request.m_parent = &readRequest2->m_request;
         sameFileRequest2->m_request.m_dependencies = 0;
 

Code/Framework/AzFramework/AzFramework/Entity/SliceEntityOwnershipService.cpp

@@ -71,6 +71,18 @@ namespace AzFramework
         }
     }
 
+    bool SliceEntityOwnershipService::CheckAndAssertRootComponentIsAvailable()
+    {
+        bool componentReady = m_rootAsset && m_rootAsset->GetComponent();
+        if (!componentReady)
+        {
+            AZ_Assert(false, "SliceEntityOwnershipService - Attempt to use the root asset component, but slice has not yet been created.");
+            return false;
+        }
+        return true;
+
+    }
+
     void SliceEntityOwnershipService::Reset()
     {
         if (m_rootAsset)
@@ -113,13 +125,21 @@ namespace AzFramework
 
     void SliceEntityOwnershipService::AddEntity(AZ::Entity* entity)
     {
-        AZ_Assert(m_rootAsset && m_rootAsset->GetComponent(), "Root slice has not been created.");
+        if (!CheckAndAssertRootComponentIsAvailable())
+        {
+            return;
+        }
         m_rootAsset->GetComponent()->AddEntity(entity);
         HandleEntitiesAdded(EntityList{ entity });
     }
 
     void SliceEntityOwnershipService::AddEntities(const EntityList& entities)
     {
+        if (!CheckAndAssertRootComponentIsAvailable())
+        {
+            return;
+        }
+
         for (AZ::Entity* entity : entities)
         {
             AZ_Assert(!AzFramework::SliceEntityRequestBus::MultiHandler::BusIsConnectedId(entity->GetId()),
@@ -134,19 +154,31 @@ namespace AzFramework
     {
         if (entity)
         {
-            AZ_Assert(m_rootAsset && m_rootAsset->GetComponent(), "Root slice has not been created.");
             SliceEntityRequestBus::MultiHandler::BusDisconnect(entity->GetId());
-            m_entitiesRemovedCallback({ entity->GetId() });
-            return m_rootAsset->GetComponent()->RemoveEntity(entity);
+            if (m_entitiesAddedCallback)
+            {
+                m_entitiesRemovedCallback({ entity->GetId() });
+            }
+            if (CheckAndAssertRootComponentIsAvailable())
+            {
+                return m_rootAsset->GetComponent()->RemoveEntity(entity);
+            }
         }
         return false;
     }
 
     bool SliceEntityOwnershipService::DestroyEntityById(AZ::EntityId entityId)
     {
-        AZ_Assert(m_rootAsset && m_rootAsset->GetComponent(), "Root slice has not been created.");
-        AZ_Assert(m_entitiesRemovedCallback, "Callback function for DestroyEntityById has not been set.");
-        m_entitiesRemovedCallback({ entityId });
+        if (!CheckAndAssertRootComponentIsAvailable())
+        {
+            return false;
+        }
+
+        AZ_Assert(m_entitiesRemovedCallback, "Callback function m_entitiesRemovedCallback for DestroyEntityById has not been set.");
+        if (m_entitiesRemovedCallback)
+        {
+            m_entitiesRemovedCallback({ entityId });
+        }
 
         // Note: This function should actually be destroying the entity, but some legacy slice code already
         // expects it to just detach the entity (such as RestoreSliceEntity_SliceEntityDeleted_SliceEntityRestored),
@@ -161,15 +193,24 @@ namespace AzFramework
     {
         AZ_PROFILE_FUNCTION(AzFramework);
 
-        AZ_Assert(m_rootAsset && m_rootAsset.Get(), "Root slice asset has not been created yet.");
-
+        if (!m_rootAsset || !m_rootAsset.Get())
+        {
+            AZ_Assert(false, "Root slice asset has not been created yet.");
+            return;
+        }
+        
         CreateRootSlice(m_rootAsset.Get());
     }
 
     void SliceEntityOwnershipService::CreateRootSlice(AZ::SliceAsset* rootSliceAsset)
     {
         AZ_PROFILE_FUNCTION(AzFramework);
-        AZ_Assert(m_rootAsset && m_rootAsset.Get(), "Root slice asset has not been created yet.");
+
+        if (!m_rootAsset || !m_rootAsset.Get())
+        {
+            AZ_Assert(false, "Root slice asset has not been created yet.");
+            return;
+        }
 
         AZ::Entity* rootEntity = new AZ::Entity();
         rootEntity->CreateComponent<AZ::SliceComponent>();
@@ -210,6 +251,8 @@ namespace AzFramework
     {
         EntityList entities;
 
+        CheckAndAssertRootComponentIsAvailable();
+
         const AZ::SliceComponent* rootSliceComponent = m_rootAsset->GetComponent();
 
         AZ_Assert(rootSliceComponent, "Root slice component has not been created.");
@@ -246,7 +289,11 @@ namespace AzFramework
     {
         AZ_PROFILE_FUNCTION(AzFramework);
 
-        AZ_Assert(m_rootAsset, "The entity ownership service has not been initialized.");
+        if (!m_rootAsset)
+        {
+            AZ_Assert(false, "The entity ownership service has not been initialized.");
+            return false;
+        }
 
         AZ::Entity* newRootEntity = AZ::Utils::LoadObjectFromStream<AZ::Entity>(stream, m_serializeContext, filterDesc);
 
@@ -391,7 +438,11 @@ namespace AzFramework
     {
         AZ_PROFILE_FUNCTION(AzFramework);
 
-        AZ_Assert(readyAsset.GetAs<AZ::SliceAsset>(), "Asset is not a slice!");
+        if (!readyAsset.GetAs<AZ::SliceAsset>())
+        {
+            AZ_Assert(readyAsset.GetAs<AZ::SliceAsset>(), "OnAssetReady : Asset %s (%s) is not a slice!", readyAsset.GetHint().c_str(), readyAsset.GetId().ToString<AZStd::string>().c_str());
+            return;
+        }
 
         if (readyAsset == m_rootAsset)
         {
@@ -552,7 +603,11 @@ namespace AzFramework
     {
         AZ_PROFILE_FUNCTION(AzFramework);
 
-        AZ_Assert(sourceInstance.IsValid(), "Source slice instance is invalid.");
+        if (!sourceInstance.IsValid())
+        {
+            AZ_Assert(false, "Source slice instance is invalid.");
+            return {};
+        }
 
         AZ::SliceComponent::SliceInstance* newInstance = sourceInstance.GetReference()->CloneInstance(sourceInstance.GetInstance(),
             sourceToCloneEntityIdMap);
@@ -568,7 +623,11 @@ namespace AzFramework
 
     AZ::SliceComponent::SliceInstanceAddress SliceEntityOwnershipService::GetOwningSlice(AZ::EntityId entityId)
     {
-        AZ_Assert(m_rootAsset && m_rootAsset->GetComponent(), "The entity ownership service has not been initialized.");
+        if (!CheckAndAssertRootComponentIsAvailable())
+        {
+            return {};
+        }
+
         return m_rootAsset->GetComponent()->FindSlice(entityId);
     }
 
@@ -667,13 +726,19 @@ namespace AzFramework
 
     void SliceEntityOwnershipService::HandleEntityBeingDestroyed(const AZ::EntityId& entityId)
     {
-        AZ_Assert(m_rootAsset && m_rootAsset->GetComponent(), "Root slice has not been created.");
         AZ_Assert(m_entitiesRemovedCallback, "Callback function for entity removal has not been set.");
-        m_entitiesRemovedCallback({ entityId });
+
+        if (m_entitiesAddedCallback)
+        {
+            m_entitiesRemovedCallback({ entityId });
+        }
 
         // Entities removed through the application (as in via manual 'delete'),
         // should be removed from the root slice, but not again deleted.
-        m_rootAsset->GetComponent()->RemoveEntity(entityId, false);
+        if (CheckAndAssertRootComponentIsAvailable())
+        {
+            m_rootAsset->GetComponent()->RemoveEntity(entityId, false);
+        }
     }
 
     AZ::Data::AssetId SliceEntityOwnershipService::CurrentlyInstantiatingSlice()

Code/Framework/AzFramework/AzFramework/Entity/SliceEntityOwnershipService.h

@@ -150,6 +150,9 @@ namespace AzFramework
     private:
         EntityList GetRootSliceEntities();
 
+        // Checks if the root and root component is available, asserts if its not, and returns true if things are OK to proceed, false otherwise.
+        bool CheckAndAssertRootComponentIsAvailable();
+
         /// Helper function to send OnSliceInstantiationFailed events.
         static void DispatchOnSliceInstantiationFailed(const SliceInstantiationTicket& ticket,
             const AZ::Data::AssetId& assetId, bool canceled);

Code/Framework/AzNetworking/AzNetworking/Serialization/DeltaSerializer.cpp

@@ -73,7 +73,13 @@ namespace AzNetworking
         // Delete any left over records that might be hanging around
         for (auto iter : m_records)
         {
-            delete iter;
+            if (iter)
+            {
+                // this will probably trigger ASAN, since we don't know what the type is.
+                // But it should also only happen in scenarios where we are crashing or have already asserted/warned
+                // about mismatched serialize/deserialize.
+                delete iter;  
+            }
         }
         m_records.clear();
     }
@@ -194,8 +200,19 @@ namespace AzNetworking
     template <typename T>
     bool DeltaSerializerCreate::SerializeHelper(T& value, uint32_t bufferCapacity, bool isString, uint32_t& outSize, const char* name)
     {
+        // The way this functions is that it expects its operation to be done in two phases:
+        // First, it gathers records into the m_records vector for each object being serialized, representing the prior state
+        // Then it expects to be called again, on objects in exactly the same order but with the new state.
+        // It will then compare the two states and generate a delta in m_delta for the differences.
+        // Once it has done this, the value stored in m_records is no longer required, and can be deleted while we still
+        // are in a templated function that knows the type of T and can thus invoke the correct delete operator.
+
+        // if this starts be a bottleneck or memory hotspot, consider using a static frame buffer for m_records, the kind of datastructure
+        // that runs on pre-allocated memory and resets it every frame instead of frees it, to avoid needing to delete.
+
         typedef AbstractValue::ValueT<T> ValueType;
 
+        uint32_t objectPos = m_objectCounter;
         AbstractValue::BaseValue* baseValue = m_records.size() > m_objectCounter ? m_records[m_objectCounter] : nullptr;
         ++m_objectCounter;
 
@@ -238,6 +255,10 @@ namespace AzNetworking
                     return false;
                 }
             }
+
+            // once we get here, we don't need the record anymore, so discard it while we know what the type is.
+            delete static_cast<ValueType*>(baseValue);
+            m_records[objectPos] = nullptr;
         }
 
         return true;

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceUpdateExecutor.h

@@ -37,6 +37,8 @@ namespace AzToolsFramework
 
             void AddInstanceToQueue(InstanceOptionalReference instance) override;
             void AddTemplateInstancesToQueue(TemplateId instanceTemplateId, InstanceOptionalConstReference instanceToExclude = AZStd::nullopt) override;
+
+            // Note, this function destroys and re-creates Entity* and Component*, do not assume your pointers are still good after this.
             bool UpdateTemplateInstancesInQueue() override;
             void RemoveTemplateInstanceFromQueue(Instance* instance) override;
             void QueueRootPrefabLoadedNotificationForNextPropagation() override;

Code/Framework/AzToolsFramework/AzToolsFramework/UI/DocumentPropertyEditor/DocumentPropertyEditor.cpp

@@ -1416,7 +1416,7 @@ namespace AzToolsFramework
     void DocumentPropertyEditor::SetSavedStateKey(AZ::u32 key, AZStd::string propertyEditorName)
     {
         // We need to append some alphabetical characters to the key or it will be treated as a very large json array index
-        AZStd::string_view keyStr = AZStd::string::format("uuid%s", AZStd::to_string(key).c_str());
+        AZStd::string keyStr = AZStd::string::format("uuid%s", AZStd::to_string(key).c_str());
         // Free the settings ptr before creating a new one. If the registry key is the same, we want
         // the in-memory settings to be saved to disk (in settings destructor) before they're loaded
         // from disk (in settings constructor)

Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerWidget.cpp

@@ -365,41 +365,47 @@ namespace AzToolsFramework
         EntityIdList newlyDeselected;
         ExtractEntityIdsFromSelection(deselected, newlyDeselected);
 
-        ScopedUndoBatch undo("Select Entity");
+        // This function could be called during undo/redo, in which case, we don't want to MAKE new undo/redo commands
+        // but we still want to update our own internal state.
+        AZStd::unique_ptr<ScopedUndoBatch> undo;
+        AZStd::unique_ptr<SelectionCommand> selectionCommand;
+        if (!m_isDuringUndoRedo)
+        {
+            // initialize the selection command here to store the current selection before
+            // new entities are selected or deselected below
+            // (SelectionCommand calls GetSelectedEntities in the constructor)
 
-        // initialize the selection command here to store the current selection before
-        // new entities are selected or deselected below
-        // (SelectionCommand calls GetSelectedEntities in the constructor)
-        auto selectionCommand =
-            AZStd::make_unique<SelectionCommand>(AZStd::vector<AZ::EntityId>{}, "");
+            undo = AZStd::make_unique<ScopedUndoBatch>("Select Entity");
+            selectionCommand = AZStd::make_unique<SelectionCommand>(AZStd::vector<AZ::EntityId>{}, "");
+        }
 
+    
         // Add the newly selected and deselected entities from the outliner to the appropriate selection buffer.
         for (const AZ::EntityId& entityId : newlySelected)
         {
             m_entitiesSelectedByOutliner.insert(entityId);
         }
 
-        ToolsApplicationRequestBus::Broadcast(
-            &ToolsApplicationRequests::MarkEntitiesSelected, newlySelected);
+        ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequests::MarkEntitiesSelected, newlySelected);
 
         for (const AZ::EntityId& entityId : newlyDeselected)
         {
             m_entitiesDeselectedByOutliner.insert(entityId);
         }
 
-        ToolsApplicationRequestBus::Broadcast(
-            &ToolsApplicationRequests::MarkEntitiesDeselected, newlyDeselected);
+        ToolsApplicationRequestBus::Broadcast(&ToolsApplicationRequests::MarkEntitiesDeselected, newlyDeselected);
 
         // call GetSelectedEntities again after all changes, and then update the selection
         // command  so the 'after' state is valid and up to date
         EntityIdList selectedEntities;
-        ToolsApplicationRequests::Bus::BroadcastResult(
-            selectedEntities, &ToolsApplicationRequests::Bus::Events::GetSelectedEntities);
-
-        selectionCommand->UpdateSelection(selectedEntities);
+        ToolsApplicationRequests::Bus::BroadcastResult(selectedEntities, &ToolsApplicationRequests::Bus::Events::GetSelectedEntities);
 
-        selectionCommand->SetParent(undo.GetUndoBatch());
-        selectionCommand.release();
+        if ((undo) && (selectionCommand))
+        {
+            selectionCommand->UpdateSelection(selectedEntities);
+            selectionCommand->SetParent(undo->GetUndoBatch());
+            selectionCommand.release(); // SetParent is an ownership transfer, the undo batch will now own deletion of this memory.
+        }
 
         m_entitiesDeselectedByOutliner.clear();
         m_entitiesSelectedByOutliner.clear();
@@ -1240,6 +1246,16 @@ namespace AzToolsFramework
         QueueScrollToNewContent(GetEntityIdFromIndex(firstSelectedEntityIndex));
     }
 
+    // ToolsApplicationEventBus handler
+    void EntityOutlinerWidget::BeforeUndoRedo()
+    {
+        m_isDuringUndoRedo = true;
+    }
+    void EntityOutlinerWidget::AfterUndoRedo()
+    {
+        m_isDuringUndoRedo = false;
+    }
+
 }
 
 #include <UI/Outliner/moc_EntityOutlinerWidget.cpp>

Code/Framework/AzToolsFramework/AzToolsFramework/UI/Outliner/EntityOutlinerWidget.hxx

@@ -136,6 +136,10 @@ namespace AzToolsFramework
         EntityOutlinerSortFilterProxyModel* m_proxyModel;
         AZStd::vector<AZ::EntityId> m_selectedEntityIds;
 
+        // ToolsApplicationEventBus handler
+        void BeforeUndoRedo() override;
+        void AfterUndoRedo() override;
+
         void PrepareSelection();
         void DoCreateEntity();
         void DoCreateEntityWithParent(const AZ::EntityId& parentId);
@@ -194,6 +198,8 @@ namespace AzToolsFramework
         QIcon m_emptyIcon;
         QIcon m_clearIcon;
 
+        bool m_isDuringUndoRedo = false;
+
         void QueueContentUpdateSort(const AZ::EntityId& entityId);
         void SortContent();
 

Code/Framework/AzToolsFramework/Tests/ActionManager/HotKeyManagerTests.cpp

@@ -127,6 +127,9 @@ namespace UnitTest
         QApplication::setActiveWindow(m_defaultParentWidget);
         childWidget->setFocus();
 
+        // setting Focus actually requires us to pump the event pump.
+        QCoreApplication::processEvents();
+
         // Trigger a shortcut event to our child widget, which should in turn only trigger our child action
         QShortcutEvent testEvent(QKeySequence("Ctrl+Z"), 0, true);
         QApplication::sendEvent(childWidget, &testEvent);

Code/Framework/AzToolsFramework/Tests/ComponentAddRemove.cpp

@@ -1414,7 +1414,9 @@ namespace UnitTest
         ASSERT_EQ(pendingComponents.size(), 1);
 
         // the boots component should be flagged as invalid, since our entity was not activated
-        EntityCompositionRequests::ScrubEntityResults& resultForTestEntity = scrubResults.GetValue()[entities[0]->GetId()];
+        // Note that this copies the invalidated / validated components array into resultForTestEntity, rather than references
+        // as the reference will later become invalid as we reset scrubResults.
+        EntityCompositionRequests::ScrubEntityResults resultForTestEntity = scrubResults.GetValue()[entities[0]->GetId()];
         ASSERT_EQ(resultForTestEntity.m_invalidatedComponents.size(), 1);
 
         // Don't actually want to keep the component in the pending set, so that we can validate the initial problem, so add it back onto the entity

Code/Framework/AzToolsFramework/Tests/EntityOwnershipService/SliceEntityOwnershipTests.cpp

@@ -55,8 +55,6 @@ namespace UnitTest
         AZStd::unique_ptr<AzFramework::SliceEntityOwnershipService> m_sliceEntityOwnershipService;
     };
 
-    using SliceEntityOwnershipDeathTests = SliceEntityOwnershipTests;
-    
     TEST_F(SliceEntityOwnershipTests, AddEntity_InitalizedCorrectly_EntityCreated)
     {
         AZ::Entity* testEntity = aznew AZ::Entity("testEntity");
@@ -392,19 +390,13 @@ namespace UnitTest
         EXPECT_FALSE(sliceInstanceAddress.IsValid());
     }
 
-#if GTEST_HAS_DEATH_TEST
-#if AZ_TRAIT_DISABLE_FAILED_DEATH_TESTS
-    TEST_F(SliceEntityOwnershipDeathTests, DISABLED_AddEntity_RootSliceAssetAbsent_EntityNotCreated)
-#else
-    TEST_F(SliceEntityOwnershipDeathTests, AddEntity_RootSliceAssetAbsent_EntityNotCreated)
-#endif // AZ_TRAIT_DISABLE_FAILED_DEATH_TESTS
+    TEST_F(SliceEntityOwnershipTests, AddEntity_RootSliceAssetAbsent_EntityNotCreated)
     {
         m_sliceEntityOwnershipService->Destroy();
         AZ::Entity testEntity = AZ::Entity("testEntity");
-        EXPECT_DEATH(
-            {
-                m_sliceEntityOwnershipService->AddEntity(&testEntity);
-            }, ".*");
+        AZ_TEST_START_ASSERTTEST;
+        m_sliceEntityOwnershipService->AddEntity(&testEntity);
+        AZ_TEST_STOP_ASSERTTEST(1); // we expect an assert here but we expect NO death or crash, just a clean return.
+
     }
-#endif // GTEST_HAS_DEATH_TEST
 }

Code/Framework/AzToolsFramework/Tests/Prefab/PrefabTestFixture.h

@@ -86,6 +86,8 @@ namespace UnitTest
         AZ::EntityId GetRootContainerEntityId();
 
         //! After prefab template is updated, we need to propagate the changes to all prefab instances.
+        //! Note that any instances involved inside this template may be destroyed and recreated, so do not hold
+        //! onto Entity* or Component* pointers into objects that may be affected by this call.  They may not be valid.
         void PropagateAllTemplateChanges();
 
         //! Helper function to compare two instances and asserts will be thrown if two instances are not identical.

Code/Framework/AzToolsFramework/Tests/Prefab/PrefabUndoEditEntityTests.cpp

@@ -61,6 +61,11 @@ namespace UnitTest
         undoNode.Redo();
         PropagateAllTemplateChanges();
 
+        // Note for the following code and all other code in this file, 
+        // PropagateAllTemplateChanges() may delete and re-create entities, it is not safe
+        // to hold onto entity* pointers across this call.  You must fetch them by ID again.
+
+        wheelEntity = AzToolsFramework::GetEntityById(wheelEntityId);
         ASSERT_FLOAT_EQ(10.0f, wheelEntity->GetTransform()->GetWorldX());
         ASSERT_TRUE(wheelEntity->FindComponent<PrefabTestComponent>());
 
@@ -68,6 +73,7 @@ namespace UnitTest
         undoNode.Undo();
         PropagateAllTemplateChanges();
 
+        wheelEntity = AzToolsFramework::GetEntityById(wheelEntityId);
         ASSERT_FLOAT_EQ(0.0f, wheelEntity->GetTransform()->GetWorldX());
         ASSERT_FALSE(wheelEntity->FindComponent<PrefabTestComponent>());
 
@@ -75,6 +81,7 @@ namespace UnitTest
         undoNode.Redo();
         PropagateAllTemplateChanges();
 
+        wheelEntity = AzToolsFramework::GetEntityById(wheelEntityId);
         ASSERT_FLOAT_EQ(10.0f, wheelEntity->GetTransform()->GetWorldX());
         ASSERT_TRUE(wheelEntity->FindComponent<PrefabTestComponent>());
     }
@@ -122,6 +129,11 @@ namespace UnitTest
         undoNode.CaptureAndRedo({ wheelEntity }, carInstance->get(), levelRootInstance->get());
         PropagateAllTemplateChanges();
 
+        // Note for the following code and all other code in this file, 
+        // PropagateAllTemplateChanges() may delete and re-create entities, it is not safe
+        // to hold onto entity* pointers across this call.  You must fetch them by ID again.
+
+        wheelEntity = AzToolsFramework::GetEntityById(wheelEntityId);
         ASSERT_TRUE(overrideInterface->AreOverridesPresent(wheelEntityId));
         ASSERT_FLOAT_EQ(10.0f, wheelEntity->GetTransform()->GetWorldX());
         ASSERT_TRUE(wheelEntity->FindComponent<PrefabTestComponent>());
@@ -130,6 +142,7 @@ namespace UnitTest
         undoNode.Undo();
         PropagateAllTemplateChanges();
 
+        wheelEntity = AzToolsFramework::GetEntityById(wheelEntityId);
         ASSERT_FALSE(overrideInterface->AreOverridesPresent(wheelEntityId));
         ASSERT_FLOAT_EQ(0.0f, wheelEntity->GetTransform()->GetWorldX());
         ASSERT_FALSE(wheelEntity->FindComponent<PrefabTestComponent>());
@@ -138,6 +151,7 @@ namespace UnitTest
         undoNode.Redo();
         PropagateAllTemplateChanges();
 
+        wheelEntity = AzToolsFramework::GetEntityById(wheelEntityId);
         ASSERT_TRUE(overrideInterface->AreOverridesPresent(wheelEntityId));
         ASSERT_FLOAT_EQ(10.0f, wheelEntity->GetTransform()->GetWorldX());
         ASSERT_TRUE(wheelEntity->FindComponent<PrefabTestComponent>());
@@ -188,6 +202,11 @@ namespace UnitTest
         undoNode.CaptureAndRedo({ addedEntity }, carInstance->get(), levelRootInstance->get());
         PropagateAllTemplateChanges();
 
+        // Note for the following code and all other code in this file, 
+        // PropagateAllTemplateChanges() may delete and re-create entities, it is not safe
+        // to hold onto entity* pointers across this call.  You must fetch them by ID again.
+
+        addedEntity = AzToolsFramework::GetEntityById(addedEntityId);
         ASSERT_TRUE(overrideInterface->AreOverridesPresent(addedEntityId));
         ASSERT_FLOAT_EQ(10.0f, addedEntity->GetTransform()->GetWorldX());
         ASSERT_TRUE(addedEntity->FindComponent<PrefabTestComponent>());
@@ -196,6 +215,7 @@ namespace UnitTest
         undoNode.Undo();
         PropagateAllTemplateChanges();
 
+        addedEntity = AzToolsFramework::GetEntityById(addedEntityId);
         ASSERT_TRUE(overrideInterface->AreOverridesPresent(addedEntityId)); // The added entity itself is an override edit
         ASSERT_FLOAT_EQ(0.0f, addedEntity->GetTransform()->GetWorldX());
         ASSERT_FALSE(addedEntity->FindComponent<PrefabTestComponent>());
@@ -204,6 +224,7 @@ namespace UnitTest
         undoNode.Redo();
         PropagateAllTemplateChanges();
 
+        addedEntity = AzToolsFramework::GetEntityById(addedEntityId);
         ASSERT_TRUE(overrideInterface->AreOverridesPresent(addedEntityId));
         ASSERT_FLOAT_EQ(10.0f, addedEntity->GetTransform()->GetWorldX());
         ASSERT_TRUE(addedEntity->FindComponent<PrefabTestComponent>());

Code/Framework/AzToolsFramework/Tests/Prefab/PrefabUpdateTemplateTests.cpp

@@ -165,12 +165,18 @@ namespace UnitTest
 
         // Add a component to Wheel instance and use it to update the wheel template.
         PrefabTestComponent* prefabTestComponent = aznew PrefabTestComponent(true);
+        // cache the RTTI name of the prefab test component, because the component will be deleted during the next few lines.
+        AZStd::string prefabTestComponentRTTIName = prefabTestComponent->RTTI_GetTypeName();
+
         wheelEntity->AddComponent(prefabTestComponent);
         auto expectedComponentId = prefabTestComponent->GetId();
         PrefabDom updatedWheelInstanceDom;
         ASSERT_TRUE(PrefabDomUtils::StoreInstanceInPrefabDom(*wheelIsolatedInstance, updatedWheelInstanceDom));
         m_prefabSystemComponent->UpdatePrefabTemplate(wheelTemplateId, updatedWheelInstanceDom);
         m_instanceUpdateExecutorInterface->UpdateTemplateInstancesInQueue();
+        // The above call destroys and recreates Entity and component instances, so pointers into 
+        // entities and components are no longer valid.
+        prefabTestComponent = nullptr;
 
         // Validate that the wheel entity does have a component under it.
         wheelEntityComponents = PrefabTestDomUtils::GetPrefabDomComponentsPath(entityAlias).Get(wheelTemplateDom);
@@ -178,8 +184,7 @@ namespace UnitTest
         EXPECT_EQ(wheelEntityComponents->MemberCount(), 1);
 
         // Extract the component id of the entity in wheel template and verify that it matches with the component id of the wheel instance.
-        PrefabTestDomUtils::ValidateComponentsDomHasId(
-            *wheelEntityComponents, prefabTestComponent->RTTI_GetTypeName(), expectedComponentId);
+        PrefabTestDomUtils::ValidateComponentsDomHasId(*wheelEntityComponents, prefabTestComponentRTTIName.c_str(), expectedComponentId);
 
         // Validate that the wheels under the axle have the same DOM as the wheel template.
         PrefabTestDomUtils::ValidatePrefabDomInstances(wheelInstanceAliasesUnderAxle, axleTemplateDom, wheelTemplateDom);

Code/Tools/AssetProcessor/native/unittests/UtilitiesUnitTests.cpp

@@ -553,7 +553,7 @@ TEST_F(UtilitiesUnitTests, GetFileHashFromStream_LargeFileForcedAnotherThreadWri
     AZ::IO::FileIOBase::GetInstance()->Open(fileName.toUtf8(), AZ::IO::OpenMode::ModeWrite | AZ::IO::OpenMode::ModeBinary, writeHandle);
 
     // The job will close the stream
-    FileWriteThrashTestJob* job = aznew FileWriteThrashTestJob(true, nullptr, writeHandle, buffer);
+    FileWriteThrashTestJob* job = aznew FileWriteThrashTestJob(true, nullptr, writeHandle, AZStd::string_view(buffer, AZ_ARRAY_SIZE(buffer)));
     job->m_writeLoopCount = 100; // compensate for artificial hashing delay below, keeping this test duration similar to others
     job->Start();
 

Code/Tools/SceneAPI/SceneCore/Containers/GraphObjectProxy.cpp

@@ -101,7 +101,7 @@ namespace AZ
                     bufferArg = *name;
                 }
 
-                AZStd::string_view type = FetchPythonType(*behaviorMethod.GetArgument(argIndex));
+                AZStd::string type = FetchPythonType(*behaviorMethod.GetArgument(argIndex));
                 if (!type.empty())
                 {
                     AzFramework::StringFunc::Append(bufferArg, ": ");
@@ -135,14 +135,14 @@ namespace AZ
             AzFramework::StringFunc::Append(buffer, "(");
             if (behaviorProperty.m_setter)
             {
-                AZStd::string_view type = FetchPythonType(*behaviorProperty.m_setter->GetArgument(1));
+                AZStd::string type = FetchPythonType(*behaviorProperty.m_setter->GetArgument(1));
                 AzFramework::StringFunc::Append(buffer, type.data());
             }
             AzFramework::StringFunc::Append(buffer, ")");
 
             if (behaviorProperty.m_getter)
             {
-                AZStd::string_view type = FetchPythonType(*behaviorProperty.m_getter->GetResult());
+                AZStd::string type = FetchPythonType(*behaviorProperty.m_getter->GetResult());
                 AzFramework::StringFunc::Append(buffer, "->");
                 AzFramework::StringFunc::Append(buffer, type.data());
             }
@@ -278,12 +278,41 @@ namespace AZ
 
                 // record the "this" pointer's meta data like its RTTI so that it can be
                 // down casted to a parent class type if needed to invoke a parent method
+
+                // When storing a parameter, if the behavior parameter indicates that it is a pointer, (TR_POINTER flag is set)
+                // it is expected that what is stored in that parameter is a pointer to the value, more specifically
+                // it will decode it on the other end by dereferencing it twice, like this:
+                //    if (m_traits & BehaviorParameter::TR_POINTER)
+                //    {
+                //       valueAddress = *reinterpret_cast<void**>(valueAddress); // pointer to a pointer
+                //    }
+                // Notice the it expects it to be a void** (double reference) and dereferences it to get the address of the object.
+                // 
+                // That means that when TR_POINTER is set, not only does the object pointed to have to survive until used
+                // but the memory storing that pointer also has to survive until used.
+                // for example, this would be a scope use-after-free bug:
+                // {
+                //    AZ::Entity entity;
+                //    AZ::BehaviorArgument arg;
+                //    arg.m_traits = BehaviorParameter::TR_POINTER;
+                // 
+                //    {
+                //       const void* objectPtr = &entity;   // this is a 64-bit object living in the stack, holding the addr of entity.
+                //       arg.m_value = const_cast<void*>(&objectPtr); // notice, we are referencing objectPtr again with & since its TR_POINTER
+                //    }
+                //
+                //    arg.GetValueAddress(); // double dereference causes memory error.
+                //
+                //  The above is a memory error because the objectPtr is a stack variable, and the ADDRESS of it is what's stored in m_value.
+
+                // to avoid this, we cache the self pointer to the graph object ahead of time.
+                const void* self = reinterpret_cast<const void*>(m_graphObject.get());
+
                 if (const AZ::BehaviorParameter* thisInfo = behaviorMethod->GetArgument(0); hasSelfPointer)
                 {
                     // avoiding the "Special handling for the generic object holder." since it assumes
                     // the BehaviorObject.m_value is a pointer; the reference version is already dereferenced
                     AZ::BehaviorArgument theThisPointer;
-                    const void* self = reinterpret_cast<const void*>(m_graphObject.get());
                     if ((thisInfo->m_traits & AZ::BehaviorParameter::TR_POINTER) == AZ::BehaviorParameter::TR_POINTER)
                     {
                         theThisPointer.m_value = &self;

Gems/Atom/RHI/Code/Tests/ThreadTester.cpp

@@ -37,11 +37,24 @@ namespace UnitTest
 
         bool timedOut = false;
 
-        // Used to detect a deadlock.  If we wait for more than 5 seconds, it's likely a deadlock has occurred
+        // Used to detect a deadlock.  The longer we wait for them to finish, the more likely it is
+        // that we have instead deadlocked.
         while (threadCount > 0 && !timedOut)
         {
             AZStd::unique_lock<AZStd::mutex> lock(mutex);
-            timedOut = (AZStd::cv_status::timeout == cv.wait_until(lock, AZStd::chrono::steady_clock::now() + AZStd::chrono::seconds(5)));
+            // This completes in about 2 seconds in profile.   It is tempting to wait up to 5 seconds to make sure.
+            // However, the user
+            // * Might be running in debug (10x or more slower)   - 2s could be as long as 20s
+            // * Might be running with ASAN enabled or some other deep memory checker (also 2-5x slower)  - 20s could end up being 100s
+            // * Might have a slow machine with fewer actual physical cores for threads (2-5x slower)     - 100s could be as long as 500s
+            // * Might have a busy machine doing updates or other tasks in the background.  (Unknown multiplier, could be huge)
+            // As such, I'm going to wait for 500 seconds instead of just 5.
+            // If the test passes, it will not actually wait for any longer than the base amount of time anyway
+            // since this timeout unblocks as soon as all the threads are done anyway, so the only time it waits for the full 500
+            // seconds is if it truly is deadlocked.
+            // So if this fails you know with a very very high confidence that it is in fact deadlocked and not just running on a potato.
+
+            timedOut = (AZStd::cv_status::timeout == cv.wait_until(lock, AZStd::chrono::steady_clock::now() + AZStd::chrono::seconds(500)));
         }
 
         EXPECT_TRUE(threadCount == 0);