Cherry-picks https://github.com/o3de/o3de/pull/11992 (#12043)

From stabilization to development.

Fixes several memory/heap corruption bugs

These would eventually cause random crashes.  They were detected
by turning AzCore SystemAllocator to use the OS malloc and free
(That change is not included in this change), and then using
the Microsoft Debug CRT allocators with heap corruption detection
features enabled.

The specific errors are as follows:
 - Python files:  Deleting an event handler while inside the event
   handler invalidates the 'this' pointer which is then subsequently
   used.
 - Settings Registry:  Mutating the settings registry while iterating
   over it in a visitor pattern.  The iterators are invalidated
   as the document changes out from onder them and will cause random
   crashes depending on memory reuse.  Added automatic detection
   for this problem and fixed the instances where found.
 - Viewport UI Manager:  invalidating an iterator by calling erase on
   it, but then still using it afterwards.
 - PreviewRenderer.cpp - the null RHI cannot take screenshot captures
   so the threads that are responsible for doing this cannot terminate
   and wait forever.   This changes it so they fail immediately,
   in the null renderer, which allows the app to shut down gracefully
   without corrupting the heap.
 - RayTracingResourceList - using an iterator after invalidating it.

Note that these fix the kind of use-after-free errors that will cause
random, unexpected failures at any point after any of them occur as
they may modify or read from invalid memory.

Signed-off-by: lawsonamzn <[email protected]>

* As per PR comments.

* Added symmetric Lock For Reading / Writing functions
* Lock for writing will check that nobody is currently iterating
* Updated all locations that previously locked the mutex directly
  to either lock for reading or writing, based on whether they make
  changes to the registry or not (regardless of what kind of changes).

Signed-off-by: lawsonamzn <[email protected]>

* Updated, as per comments on pull request.

Tested manually (opening levels and poking around).
Tested by deleting cache and rebuilding all assets.
Tested by running python automated Tests.
Tested by running AzCore unit tests.

Signed-off-by: lawsonamzn <[email protected]>

Signed-off-by: lawsonamzn <[email protected]>

Signed-off-by: lawsonamzn <[email protected]>

AutomatedTesting/Assets/TestAnim/scene_export_actor.py

@@ -205,10 +205,12 @@ def on_update_manifest(args):
         log_exception_traceback()
     except:
         log_exception_traceback()
-
-    global sceneJobHandler
-    sceneJobHandler.disconnect()
-    sceneJobHandler = None
+    finally:
+        global sceneJobHandler
+        # do not delete or set sceneJobHandler to None, just disconnect from it.
+        # this call is occuring while the scene Job Handler itself is in the callstack, so deleting it here
+        # would cause a crash.
+        sceneJobHandler.disconnect()
 
 # try to create SceneAPI handler for processing
 try:

AutomatedTesting/Assets/TestAnim/scene_export_motion.py

@@ -50,10 +50,12 @@ def on_update_manifest(args):
         scene_export_utils.log_exception_traceback()
     except:
         scene_export_utils.log_exception_traceback()
-
-    global sceneJobHandler
-    sceneJobHandler.disconnect()
-    sceneJobHandler = None
+    finally:
+        global sceneJobHandler
+        # do not delete or set sceneJobHandler to None, just disconnect from it.
+        # this call is occuring while the scene Job Handler itself is in the callstack, so deleting it here
+        # would cause a crash.
+        sceneJobHandler.disconnect()
 
 # try to create SceneAPI handler for processing
 try:

AutomatedTesting/Assets/ap_test_assets/script_reinsert.py

@@ -10,8 +10,10 @@ sceneJobHandler = None
 
 def clear_sceneJobHandler():
     global sceneJobHandler
+    # do not delete or set sceneJobHandler to None, just disconnect from it.
+    # this call is occuring while the scene Job Handler itself is in the callstack, so deleting it here
+    # would cause a crash.
     sceneJobHandler.disconnect()
-    sceneJobHandler = None
 
 def on_prepare_for_export(args): 
     print (f'on_prepare_for_export')
@@ -68,11 +70,10 @@ def on_update_manifest(args):
 try:
     import azlmbr.scene as sceneApi
     
-    if sceneJobHandler is None:
-        sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
-        sceneJobHandler.connect()
-        sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
-        sceneJobHandler.add_callback('OnPrepareForExport', on_prepare_for_export)
+    sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
+    sceneJobHandler.connect()
+    sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
+    sceneJobHandler.add_callback('OnPrepareForExport', on_prepare_for_export)
 
 except:
     sceneJobHandler = None

AutomatedTesting/Editor/Scripts/auto_lod.py

@@ -109,16 +109,18 @@ def on_update_manifest(args):
         log_exception_traceback()
     except:
         log_exception_traceback()
-
-    global sceneJobHandler
-    sceneJobHandler = None
+    finally:
+        global sceneJobHandler
+        # do not delete or set sceneJobHandler to None, just disconnect from it.
+        # this call is occuring while the scene Job Handler itself is in the callstack, so deleting it here
+        # would cause a crash.
+        sceneJobHandler.disconnect()
 
 # try to create SceneAPI handler for processing
 try:
     import azlmbr.scene as sceneApi
-    if (sceneJobHandler == None):
-        sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
-        sceneJobHandler.connect()
-        sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
+    sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
+    sceneJobHandler.connect()
+    sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
 except:
     sceneJobHandler = None

AutomatedTesting/Editor/Scripts/scene_mesh_to_prefab.py

@@ -235,18 +235,19 @@ def on_update_manifest(args):
         log_exception_traceback()
 
     global sceneJobHandler
+    # do not delete or set sceneJobHandler to None, just disconnect from it.
+    # this call is occuring while the scene Job Handler itself is in the callstack, so deleting it here
+    # would cause a crash.
     sceneJobHandler.disconnect()
-    sceneJobHandler = None
     return data
 
 
 # try to create SceneAPI handler for processing
 try:
     import azlmbr.scene as sceneApi
-
-    if sceneJobHandler is None:
-        sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
-        sceneJobHandler.connect()
-        sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
+    
+    sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
+    sceneJobHandler.connect()
+    sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
 except:
     sceneJobHandler = None

AutomatedTesting/Gem/PythonTests/PythonAssetBuilder/export_chunks_builder.py

@@ -63,8 +63,10 @@ def on_update_manifest(args):
     scene = args[0]
     result = update_manifest(scene)
     global mySceneJobHandler
+    # do not delete or set sceneJobHandler to None, just disconnect from it.
+    # this call is occuring while the scene Job Handler itself is in the callstack, so deleting it here
+    # would cause a crash.
     mySceneJobHandler.disconnect()
-    mySceneJobHandler = None
     return result
 
 def main():

AutomatedTesting/Gem/PythonTests/assetpipeline/fbx_tests/assets/TwoSceneFiles_OneWithPythonOneWithout_PythonOnlyRunsOnFirstScene/python_builder.py

@@ -31,8 +31,10 @@ def on_update_manifest(args):
     scene = args[0]
     result = output_test_data(scene)
     global mySceneJobHandler
+    # do not delete or set sceneJobHandler to None, just disconnect from it.
+    # this call is occuring while the scene Job Handler itself is in the callstack, so deleting it here
+    # would cause a crash.
     mySceneJobHandler.disconnect()
-    mySceneJobHandler = None
     return result
 
 def main():

AutomatedTesting/Gem/PythonTests/assetpipeline/fbx_tests/assets/script_basics/base_example.py

@@ -10,8 +10,10 @@ sceneJobHandler = None
 
 def clear_sceneJobHandler():
     global sceneJobHandler
+    # do not delete or set sceneJobHandler to None, just disconnect from it.
+    # this call is occuring while the scene Job Handler itself is in the callstack, so deleting it here
+    # would cause a crash.
     sceneJobHandler.disconnect()
-    sceneJobHandler = None
 
 def on_prepare_for_export(args): 
     print (f'on_prepare_for_export')
@@ -68,11 +70,10 @@ def on_update_manifest(args):
 try:
     import azlmbr.scene as sceneApi
     
-    if sceneJobHandler is None:
-        sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
-        sceneJobHandler.connect()
-        sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
-        sceneJobHandler.add_callback('OnPrepareForExport', on_prepare_for_export)
+    sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
+    sceneJobHandler.connect()
+    sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
+    sceneJobHandler.add_callback('OnPrepareForExport', on_prepare_for_export)
 
 except:
     sceneJobHandler = None

AutomatedTesting/TestAssets/test_chunks_builder.py

@@ -43,15 +43,17 @@ def on_update_manifest(args):
     scene = args[0]
     result = update_manifest(scene)
     global mySceneJobHandler
+    # do not delete or set sceneJobHandler to None, just disconnect from it.
+    # this call is occuring while the scene Job Handler itself is in the callstack, so deleting it here
+    # would cause a crash.
     mySceneJobHandler.disconnect()
-    mySceneJobHandler = None
     return result
 
 def main():
     global mySceneJobHandler
     mySceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
-    mySceneJobHandler.connect()
     mySceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
+    mySceneJobHandler.connect()
 
 if __name__ == "__main__":
     main()

Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.cpp

@@ -58,7 +58,7 @@ namespace AZ
     {
         {
             // Push the file to be merged under protection of the Settings Mutex
-            AZStd::scoped_lock lock(m_settingsRegistry.m_settingMutex);
+            AZStd::scoped_lock lock(m_settingsRegistry.LockForWriting());
             m_settingsRegistry.m_mergeFilePathStack.emplace(m_mergeEventArgs.m_mergeFilePath);
         }
         m_settingsRegistry.m_preMergeEvent.Signal(mergeEventArgs);
@@ -70,7 +70,7 @@ namespace AZ
 
         {
             // Pop the file that finished merging under protection of the Settings Mutex
-            AZStd::scoped_lock lock(m_settingsRegistry.m_settingMutex);
+            AZStd::scoped_lock lock(m_settingsRegistry.LockForWriting());
             m_settingsRegistry.m_mergeFilePathStack.pop();
         }
     }
@@ -130,6 +130,8 @@ namespace AZ
         rapidjson::Pointer pointer(path.data(), path.length());
         if (pointer.IsValid())
         {
+            AZStd::scoped_lock lock(LockForReading());
+
             const rapidjson::Value* value = pointer.Get(m_settings);
             if constexpr (AZStd::is_same_v<T, bool>)
             {
@@ -197,7 +199,7 @@ namespace AZ
 
     void SettingsRegistryImpl::SetContext(SerializeContext* context)
     {
-        AZStd::scoped_lock lock(m_settingMutex);
+        AZStd::scoped_lock lock(LockForWriting());
 
         m_serializationSettings.m_serializeContext = context;
         m_deserializationSettings.m_serializeContext = context;
@@ -205,7 +207,7 @@ namespace AZ
 
     void SettingsRegistryImpl::SetContext(JsonRegistrationContext* context)
     {
-        AZStd::scoped_lock lock(m_settingMutex);
+        AZStd::scoped_lock lock(LockForWriting());
 
         m_serializationSettings.m_registrationContext = context;
         m_deserializationSettings.m_registrationContext = context;
@@ -224,7 +226,10 @@ namespace AZ
         rapidjson::Pointer pointer(path.data(), path.length());
         if (pointer.IsValid())
         {
-            AZStd::scoped_lock lock(m_settingMutex);
+            // During GetValue and Visit, we are not about to mutate
+            // the values in the registry, so do NOT call LockForWriting, just
+            // lock the mutex.
+            AZStd::scoped_lock lock(LockForReading());
             const rapidjson::Value* value = pointer.Get(m_settings);
             if (value)
             {
@@ -294,7 +299,7 @@ namespace AZ
     {
         PreMergeEventHandler preMergeHandler{ AZStd::move(callback) };
         {
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForWriting());
             preMergeHandler.Connect(m_preMergeEvent);
         }
         return preMergeHandler;
@@ -302,7 +307,7 @@ namespace AZ
 
     auto SettingsRegistryImpl::RegisterPreMergeEvent(PreMergeEventHandler& preMergeHandler) -> void
     {
-        AZStd::scoped_lock lock(m_settingMutex);
+        AZStd::scoped_lock lock(LockForWriting());
         preMergeHandler.Connect(m_preMergeEvent);
     }
 
@@ -310,7 +315,7 @@ namespace AZ
     {
         PostMergeEventHandler postMergeHandler{ AZStd::move(callback) };
         {
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForWriting());
             postMergeHandler.Connect(m_postMergeEvent);
         }
         return postMergeHandler;
@@ -318,13 +323,13 @@ namespace AZ
 
     auto SettingsRegistryImpl::RegisterPostMergeEvent(PostMergeEventHandler& postMergeHandler) -> void
     {
-        AZStd::scoped_lock lock(m_settingMutex);
+        AZStd::scoped_lock lock(LockForWriting());
         postMergeHandler.Connect(m_postMergeEvent);
     }
 
     void SettingsRegistryImpl::ClearMergeEvents()
     {
-        AZStd::scoped_lock lock(m_settingMutex);
+        AZStd::scoped_lock lock(LockForWriting());
         m_preMergeEvent.DisconnectAllHandlers();
         m_postMergeEvent.DisconnectAllHandlers();
     }
@@ -399,7 +404,7 @@ namespace AZ
         rapidjson::Pointer pointer(path.data(), path.length());
         if (pointer.IsValid())
         {
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForReading());
             return GetTypeNoLock(path);
         }
         return SettingsType{};
@@ -438,37 +443,31 @@ namespace AZ
 
     bool SettingsRegistryImpl::Get(bool& result, AZStd::string_view path) const
     {
-        AZStd::scoped_lock lock(m_settingMutex);
         return GetValueInternal(result, path);
     }
 
     bool SettingsRegistryImpl::Get(s64& result, AZStd::string_view path) const
     {
-        AZStd::scoped_lock lock(m_settingMutex);
         return GetValueInternal(result, path);
     }
 
     bool SettingsRegistryImpl::Get(u64& result, AZStd::string_view path) const
     {
-        AZStd::scoped_lock lock(m_settingMutex);
         return GetValueInternal(result, path);
     }
 
     bool SettingsRegistryImpl::Get(double& result, AZStd::string_view path) const
     {
-        AZStd::scoped_lock lock(m_settingMutex);
         return GetValueInternal(result, path);
     }
 
     bool SettingsRegistryImpl::Get(AZStd::string& result, AZStd::string_view path) const
     {
-        AZStd::scoped_lock lock(m_settingMutex);
         return GetValueInternal(result, path);
     }
 
     bool SettingsRegistryImpl::Get(FixedValueString& result, AZStd::string_view path) const
     {
-        AZStd::scoped_lock lock(m_settingMutex);
         return GetValueInternal(result, path);
     }
 
@@ -485,7 +484,7 @@ namespace AZ
         rapidjson::Pointer pointer(path.data(), path.length());
         if (pointer.IsValid())
         {
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForReading());
             const rapidjson::Value* value = pointer.Get(m_settings);
             if (value)
             {
@@ -498,7 +497,7 @@ namespace AZ
 
     bool SettingsRegistryImpl::Set(AZStd::string_view path, bool value)
     {
-        if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
+        if (AZStd::scoped_lock lock(LockForWriting()); !SetValueInternal(path, value))
         {
             return false;
         }
@@ -508,7 +507,7 @@ namespace AZ
 
     bool SettingsRegistryImpl::Set(AZStd::string_view path, s64 value)
     {
-        if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
+        if (AZStd::scoped_lock lock(LockForWriting()); !SetValueInternal(path, value))
         {
             return false;
         }
@@ -518,7 +517,7 @@ namespace AZ
 
     bool SettingsRegistryImpl::Set(AZStd::string_view path, u64 value)
     {
-        if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
+        if (AZStd::scoped_lock lock(LockForWriting()); !SetValueInternal(path, value))
         {
             return false;
         }
@@ -528,7 +527,7 @@ namespace AZ
 
     bool SettingsRegistryImpl::Set(AZStd::string_view path, double value)
     {
-        if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
+        if (AZStd::scoped_lock lock(LockForWriting()); !SetValueInternal(path, value))
         {
             return false;
         }
@@ -538,7 +537,7 @@ namespace AZ
 
     bool SettingsRegistryImpl::Set(AZStd::string_view path, AZStd::string_view value)
     {
-        if (AZStd::scoped_lock lock(m_settingMutex); !SetValueInternal(path, value))
+        if (AZStd::scoped_lock lock(LockForWriting()); !SetValueInternal(path, value))
         {
             return false;
         }
@@ -571,7 +570,7 @@ namespace AZ
             {
                 SettingsType anchorType;
                 {
-                    AZStd::scoped_lock lock(m_settingMutex);
+                    AZStd::scoped_lock lock(LockForWriting());
                     rapidjson::Value& setting = pointer.Create(m_settings, m_settings.GetAllocator());
                     setting = AZStd::move(store);
                     anchorType = GetTypeNoLock(path);
@@ -598,7 +597,7 @@ namespace AZ
             return false;
         }
 
-        AZStd::scoped_lock lock(m_settingMutex);
+        AZStd::scoped_lock lock(LockForWriting());
         return pointerPath.Erase(m_settings);
     }
 
@@ -726,7 +725,7 @@ namespace AZ
             {
                 rapidjson::Pointer pointer(AZ_SETTINGS_REGISTRY_HISTORY_KEY "/-");
                 AZ_Error("Settings Registry", false, R"(Anchor path "%.*s" is invalid.)", AZ_STRING_ARG(anchorKey));
-                AZStd::scoped_lock lock(m_settingMutex);
+                AZStd::scoped_lock lock(LockForWriting());
                 pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
                     .AddMember(rapidjson::StringRef("Error"), rapidjson::StringRef("Invalid anchor key."), m_settings.GetAllocator())
                     .AddMember(rapidjson::StringRef("Path"),
@@ -772,7 +771,8 @@ namespace AZ
 
         SettingsType anchorType;
         {
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForWriting());
+
             rapidjson::Value& anchorRoot = anchorPath.IsValid() ? anchorPath.Create(m_settings, m_settings.GetAllocator())
                 : m_settings;
 
@@ -824,7 +824,7 @@ namespace AZ
                     static_cast<int>(path.length()), path.data());
                 Pointer pointer(AZ_SETTINGS_REGISTRY_HISTORY_KEY "/-");
 
-                AZStd::scoped_lock lock(m_settingMutex);
+                AZStd::scoped_lock lock(LockForWriting());
                 Value pathValue(path.data(), aznumeric_caster(path.length()), m_settings.GetAllocator());
                 pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
                     .AddMember(StringRef("Error"), StringRef("Unable to read registry file."), m_settings.GetAllocator())
@@ -870,7 +870,7 @@ namespace AZ
         {
             AZ_Error("Settings Registry", false, "Folder path for the Setting Registry is too long: %.*s",
                 static_cast<int>(path.size()), path.data());
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForWriting());
             pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
                 .AddMember(StringRef("Error"), StringRef("Folder path for the Setting Registry is too long."), m_settings.GetAllocator())
                 .AddMember(StringRef("Path"), Value(path.data(), aznumeric_caster(path.length()), m_settings.GetAllocator()), m_settings.GetAllocator());
@@ -906,7 +906,7 @@ namespace AZ
                     if (fileList.size() >= MaxRegistryFolderEntries)
                     {
                         AZ_Error("Settings Registry", false, "Too many files in registry folder.");
-                        AZStd::scoped_lock lock(m_settingMutex);
+                        AZStd::scoped_lock lock(LockForWriting());
                         pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
                             .AddMember(StringRef("Error"), StringRef("Too many files in registry folder."), m_settings.GetAllocator())
                             .AddMember(StringRef("Path"), Value(folderPath.c_str(), aznumeric_caster(folderPath.Native().size()), m_settings.GetAllocator()), m_settings.GetAllocator())
@@ -1009,6 +1009,7 @@ namespace AZ
     SettingsRegistryInterface::VisitResponse SettingsRegistryImpl::Visit(Visitor& visitor, StackedString& path, AZStd::string_view valueName,
         const rapidjson::Value& value) const
     {
+        ++m_visitDepth;
         VisitResponse result;
         VisitArgs visitArgs(*this);
         switch (value.GetType())
@@ -1052,6 +1053,7 @@ namespace AZ
                     path.Push(fieldName);
                     if (Visit(visitor, path, fieldName, member.value) == VisitResponse::Done)
                     {
+                        --m_visitDepth;
                         return VisitResponse::Done;
                     }
                     path.Pop();
@@ -1063,6 +1065,7 @@ namespace AZ
                 visitArgs.m_fieldName = valueName;
                 if (visitor.Traverse(visitArgs, VisitAction::End) == VisitResponse::Done)
                 {
+                    --m_visitDepth;
                     return VisitResponse::Done;
                 }
             }
@@ -1085,6 +1088,7 @@ namespace AZ
                     entryName.remove_prefix(endIndex + 1);
                     if (Visit(visitor, path, entryName, entry) == VisitResponse::Done)
                     {
+                        --m_visitDepth;
                         return VisitResponse::Done;
                     }
                     counter++;
@@ -1097,6 +1101,7 @@ namespace AZ
                 visitArgs.m_fieldName = valueName;
                 if (visitor.Traverse(visitArgs, VisitAction::End) == VisitResponse::Done)
                 {
+                    --m_visitDepth;
                     return VisitResponse::Done;
                 }
             }
@@ -1148,6 +1153,8 @@ namespace AZ
             AZ_Assert(false, "Unsupported RapidJSON type: %i.", aznumeric_cast<int>(value.GetType()));
             result = VisitResponse::Done;
         }
+
+        --m_visitDepth;
         return result;
     }
 
@@ -1202,7 +1209,7 @@ namespace AZ
         AZ_Error("Settings Registry", false, R"(Two registry files in "%.*s" point to the same specialization: "%s" and "%s")",
             AZ_STRING_ARG(folderPath), lhs.m_relativePath.c_str(), rhs.m_relativePath.c_str());
 
-        AZStd::scoped_lock lock(m_settingMutex);
+        AZStd::scoped_lock lock(LockForWriting());
         historyPointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
             .AddMember(StringRef("Error"), StringRef("Too many files in registry folder."), m_settings.GetAllocator())
             .AddMember(StringRef("Path"),
@@ -1363,7 +1370,7 @@ namespace AZ
                 }
             }
 
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForWriting());
             pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
                 .AddMember(StringRef("Error"), StringRef("Unable to parse registry file due to invalid json."), m_settings.GetAllocator())
                 .AddMember(StringRef("Path"), Value(path, m_settings.GetAllocator()), m_settings.GetAllocator())
@@ -1389,7 +1396,7 @@ namespace AZ
                     R"(To merge the supplied settings registry file, the settings within it must be placed within a JSON Object '{}')"
                     R"( in order to allow moving of its fields using the root-key as an anchor.)", path);
 
-                AZStd::scoped_lock lock(m_settingMutex);
+                AZStd::scoped_lock lock(LockForWriting());
                 pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
                     .AddMember(StringRef("Error"), StringRef("Cannot merge registry file with a root which is not a JSON Object,"
                         " an empty root key and a merge approach of JsonMergePatch. Otherwise the Settings Registry would be overridden."
@@ -1450,7 +1457,7 @@ namespace AZ
         SettingsType anchorType;
         if (rootKey.empty())
         {
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForWriting());
             mergeResult = JsonSerialization::ApplyPatch(m_settings, m_settings.GetAllocator(), jsonPatch, mergeApproach, applyPatchSettings);
             anchorType = GetTypeNoLock(rootKey);
         }
@@ -1459,7 +1466,7 @@ namespace AZ
             Pointer root(rootKey.data(), rootKey.length());
             if (root.IsValid())
             {
-                AZStd::scoped_lock lock(m_settingMutex);
+                AZStd::scoped_lock lock(LockForWriting());
                 Value& rootValue = root.Create(m_settings, m_settings.GetAllocator());
                 mergeResult = JsonSerialization::ApplyPatch(rootValue, m_settings.GetAllocator(), jsonPatch, mergeApproach, applyPatchSettings);
                 anchorType = GetTypeNoLock(rootKey);
@@ -1468,7 +1475,7 @@ namespace AZ
             {
                 AZ_Error("Settings Registry", false, R"(Failed to root path "%.*s" is invalid.)",
                     aznumeric_cast<int>(rootKey.length()), rootKey.data());
-                AZStd::scoped_lock lock(m_settingMutex);
+                AZStd::scoped_lock lock(LockForWriting());
                 pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
                     .AddMember(StringRef("Error"), StringRef("Invalid root key."), m_settings.GetAllocator())
                     .AddMember(StringRef("Path"), Value(path, m_settings.GetAllocator()), m_settings.GetAllocator());
@@ -1478,7 +1485,7 @@ namespace AZ
         if (mergeResult.GetProcessing() != JsonSerializationResult::Processing::Completed)
         {
             AZ_Error("Settings Registry", false, R"(Failed to fully merge registry file "%s".)", path);
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForWriting());
             pointer.Create(m_settings, m_settings.GetAllocator()).SetObject()
                 .AddMember(StringRef("Error"), StringRef("Failed to fully merge registry file."), m_settings.GetAllocator())
                 .AddMember(StringRef("Path"), Value(path, m_settings.GetAllocator()), m_settings.GetAllocator());
@@ -1486,7 +1493,7 @@ namespace AZ
         }
 
         {
-            AZStd::scoped_lock lock(m_settingMutex);
+            AZStd::scoped_lock lock(LockForWriting());
             pointer.Create(m_settings, m_settings.GetAllocator()).SetString(path, m_settings.GetAllocator());
         }
 
@@ -1514,4 +1521,18 @@ namespace AZ
     {
         m_useFileIo = useFileIo;
     }
+
+    AZStd::scoped_lock<AZStd::recursive_mutex> SettingsRegistryImpl::LockForWriting() const
+    {
+        // ensure that we aren't actively iterating over this data that is about to be
+        // invalid.
+        AZ_Assert(m_visitDepth == 0, "Attempt to mutate the Settings Registry while visiting, "
+            "this may invalidate visitor iterators and cause crashes.  Visit depth is %i", m_visitDepth);
+        return AZStd::scoped_lock(m_settingMutex);
+    }
+
+    AZStd::scoped_lock<AZStd::recursive_mutex> SettingsRegistryImpl::LockForReading() const
+    {
+        return AZStd::scoped_lock(m_settingMutex);
+    }
 } // namespace AZ

Code/Framework/AzCore/AzCore/Settings/SettingsRegistryImpl.h

@@ -17,6 +17,7 @@
 #include <AzCore/std/containers/fixed_vector.h>
 #include <AzCore/std/containers/vector.h>
 #include <AzCore/std/parallel/mutex.h>
+#include <AzCore/std/parallel/scoped_lock.h>
 
 // Using a define instead of a static string to avoid the need for temporary buffers to composite the full paths.
 #define AZ_SETTINGS_REGISTRY_HISTORY_KEY "/Amazon/AzCore/Runtime/Registry/FileHistory"
@@ -117,7 +118,15 @@ namespace AZ
 
         void SignalNotifier(AZStd::string_view jsonPath, SettingsType type);
 
-        
+        //! Locks the m_settingMutex but also checks to make sure that someone is not currently
+        //! visiting/iterating over the registry, which is invalid if you're about to modify it
+        AZStd::scoped_lock<AZStd::recursive_mutex> LockForWriting() const;
+
+        //! For symmetry with the above, locks with intent to only read data.  This can be done
+        //! even during iteration/visiting.
+        AZStd::scoped_lock<AZStd::recursive_mutex> LockForReading() const;
+
+        // only use the setting mutex via the above functions.
         mutable AZStd::recursive_mutex m_settingMutex;
         mutable AZStd::recursive_mutex m_notifierMutex;
         NotifyEvent m_notifiers;
@@ -159,5 +168,10 @@ namespace AZ
         //! Stack tracking the files currently being merged
         //! This is protected by m_settingsMutex
         AZStd::stack<AZ::IO::FixedMaxPath> m_mergeFilePathStack;
+
+        // if this is nonzero, we are in a visit operation.  It can be used to detect illegal modifications
+        // of the tree during visit.
+        mutable int m_visitDepth = 0; // mutable due to it being a debugging value used in const.
+
     };
 } // namespace AZ

Code/Framework/AzCore/AzCore/Settings/SettingsRegistryMergeUtils.cpp

@@ -796,20 +796,25 @@ namespace AZ::SettingsRegistryMergeUtils
 
     void MergeSettingsToRegistry_ManifestGemsPaths(SettingsRegistryInterface& registry)
     {
-        auto MergeGemPathToRegistry = [&registry](AZStd::string_view manifestKey,
-            AZStd::string_view gemName,
-            AZ::IO::PathView gemRootPath)
+        // cache a vector so that we don't mutate the registry while inside visitor iteration.
+        AZStd::vector<AZStd::pair<AZStd::string, AZ::IO::FixedMaxPath>> collectedGems;
+        auto CollectManifestGems =
+            [&collectedGems](AZStd::string_view manifestKey, AZStd::string_view gemName, AZ::IO::PathView gemRootPath)
         {
-            using FixedValueString = SettingsRegistryInterface::FixedValueString;
             if (manifestKey == GemNameKey)
             {
-                const auto manifestGemJsonPath = FixedValueString::format("%s/%.*s/Path",
-                    ManifestGemsRootKey, AZ_STRING_ARG(gemName));
-                registry.Set(manifestGemJsonPath, gemRootPath.LexicallyNormal().Native());
+                collectedGems.push_back(AZStd::make_pair(AZStd::string(gemName), AZ::IO::FixedMaxPath(gemRootPath)));
             }
         };
 
-        VisitAllManifestGems(registry, MergeGemPathToRegistry);
+        VisitAllManifestGems(registry, CollectManifestGems);
+
+        for (const auto& [gemName, gemRootPath] : collectedGems)
+        {
+            using FixedValueString = SettingsRegistryInterface::FixedValueString;
+            const auto manifestGemJsonPath = FixedValueString::format("%s/%.*s/Path", ManifestGemsRootKey, AZ_STRING_ARG(gemName));
+            registry.Set(manifestGemJsonPath, gemRootPath.LexicallyNormal().Native());
+        }
     }
 
     void MergeSettingsToRegistry_AddRuntimeFilePaths(SettingsRegistryInterface& registry)
@@ -994,13 +999,21 @@ namespace AZ::SettingsRegistryMergeUtils
     void MergeSettingsToRegistry_GemRegistries(SettingsRegistryInterface& registry, const AZStd::string_view platform,
         const SettingsRegistryInterface::Specializations& specializations, AZStd::vector<char>* scratchBuffer)
     {
-        auto MergeGemRootRegistryFolder = [&registry, &platform, &specializations, &scratchBuffer]
-        (AZStd::string_view, AZ::IO::FixedMaxPath gemPath)
+        // collect the paths first, then mutate the registry, so that we do not do any registry modifications while visiting it.
+        AZStd::vector<AZ::IO::FixedMaxPath> gemPaths;
+        auto CollectRegistryFolders = [&gemPaths]
+            (AZStd::string_view, AZ::IO::FixedMaxPath gemPath)
         {
-            registry.MergeSettingsFolder((gemPath / SettingsRegistryInterface::RegistryFolder).Native(),
-                specializations, platform, "", scratchBuffer);
+            gemPaths.push_back(gemPath);
         };
-        VisitActiveGems(registry, MergeGemRootRegistryFolder);
+        
+        VisitActiveGems(registry, CollectRegistryFolders);
+
+        for (const auto& gemPath : gemPaths)
+        {
+            registry.MergeSettingsFolder(
+                (gemPath / SettingsRegistryInterface::RegistryFolder).Native(), specializations, platform, "", scratchBuffer);
+        }
     }
 
     void MergeSettingsToRegistry_ProjectRegistry(SettingsRegistryInterface& registry, const AZStd::string_view platform,

Code/Framework/AzToolsFramework/AzToolsFramework/ViewportUi/ViewportUiManager.cpp

@@ -152,8 +152,8 @@ namespace AzToolsFramework::ViewportUi
     {
         if (auto clusterIt = m_clusterButtonGroups.find(clusterId); clusterIt != m_clusterButtonGroups.end())
         {
-            m_clusterButtonGroups.erase(clusterIt);
             m_viewportUi->RemoveViewportUiElement(clusterIt->second->GetViewportUiElementId());
+            m_clusterButtonGroups.erase(clusterIt);
         }
     }
 
@@ -161,8 +161,8 @@ namespace AzToolsFramework::ViewportUi
     {
         if (auto switcherIt = m_switcherButtonGroups.find(switcherId); switcherIt != m_switcherButtonGroups.end())
         {
-            m_switcherButtonGroups.erase(switcherIt);
             m_viewportUi->RemoveViewportUiElement(switcherIt->second->GetViewportUiElementId());
+            m_switcherButtonGroups.erase(switcherIt);
         }
     }
 
@@ -246,8 +246,8 @@ namespace AzToolsFramework::ViewportUi
     {
         if (auto textFieldIt = m_textFields.find(textFieldId); textFieldIt != m_textFields.end())
         {
-            m_textFields.erase(textFieldIt);
             m_viewportUi->RemoveViewportUiElement(textFieldIt->second->m_viewportId);
+            m_textFields.erase(textFieldIt);
         }
     }
 

Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingResourceList.h

@@ -144,6 +144,9 @@ namespace AZ
                     m_indirectionList.SetEntry(itLast->second.m_indirectionIndex, resourceIndex);                 
                 }
 
+                // cache the indirection index so that its okay to erase the iterator
+                uint32_t cachedIndirectionIndex = it->second.m_indirectionIndex;
+
                 // remove the last entry from the resource list
                 m_resources.pop_back();
 
@@ -151,7 +154,7 @@ namespace AZ
                 m_resourceMap.erase(it);
 
                 // remove the entry from the indirection list
-                m_indirectionList.RemoveEntry(it->second.m_indirectionIndex);
+                m_indirectionList.RemoveEntry(cachedIndirectionIndex);
             }
         }
 

Gems/Atom/Tools/AtomToolsFramework/Code/Source/PreviewRenderer/PreviewRenderer.cpp

@@ -130,8 +130,19 @@ namespace AtomToolsFramework
             m_currentCaptureRequest = m_captureRequestQueue.front();
             m_captureRequestQueue.pop();
 
-            m_state.reset();
-            m_state.reset(new PreviewRendererLoadState(this));
+            bool canCapture = false;
+            AZ::Render::FrameCaptureRequestBus::BroadcastResult(canCapture, &AZ::Render::FrameCaptureRequestBus::Events::CanCapture);
+
+            // if we're not on a device that can capture, immediately trigger the "failed" state.
+            if (!canCapture)
+            {
+                CancelCaptureRequest();
+            }
+            else
+            {
+                m_state.reset();
+                m_state.reset(new PreviewRendererLoadState(this));
+            }
         }
     }
 

Gems/Blast/Editor/Scripts/blast_chunk_processor.py

@@ -140,17 +140,18 @@ def on_update_manifest(args):
         scene = args[0]
         return update_manifest(scene)
     except:
-        global sceneJobHandler
-        sceneJobHandler = None
         log_exception_traceback()
+    finally:
+        global sceneJobHandler
+        sceneJobHandler.disconnect()
 
 # try to create SceneAPI handler for processing
 try:
     import azlmbr.scene as sceneApi
-    if (sceneJobHandler == None):
-        sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
-        sceneJobHandler.connect()
-        sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
-        sceneJobHandler.add_callback('OnPrepareForExport', on_prepare_for_export)
+    sceneJobHandler = sceneApi.ScriptBuildingNotificationBusHandler()
+    sceneJobHandler.add_callback('OnUpdateManifest', on_update_manifest)
+    sceneJobHandler.add_callback('OnPrepareForExport', on_prepare_for_export)
+    sceneJobHandler.connect()
+        
 except:
     sceneJobHandler = None