[MAN-481] Fortify event firing mechanism against userland exceptions (#…

…170)
Laerdal · Jan 27, 2025 · 648951e · 648951e
2 parents a6b68cd + 0b45088
commit 648951e
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 15 deletions.
diff --git a/...tbed.InstallAsync.ShouldThrowFirmwareInstallationTimeoutException_GivenTooSmallTimeout.cs b/...tbed.InstallAsync.ShouldThrowFirmwareInstallationTimeoutException_GivenTooSmallTimeout.cs
@@ -26,7 +26,7 @@ public async Task InstallAsync_ShouldThrowFirmwareInstallationTimeoutException_G
                 hostDeviceModel: "foobar",
                 hostDeviceManufacturer: "acme corp.",
 
-                timeoutInMs: 100,
+                timeoutInMs: 500,
                 maxTriesCount: 1
             ));
 
@@ -88,7 +88,7 @@ public override EFirmwareInstallationVerdict BeginInstallation(
                     await Task.Delay(10);
                     StateChangedAdvertisement(oldState: EFirmwareInstallationState.Idle, newState: EFirmwareInstallationState.Uploading);
 
-                    await Task.Delay(1_000);
+                    await Task.Delay(2_000);
                     StateChangedAdvertisement(oldState: EFirmwareInstallationState.Uploading, newState: EFirmwareInstallationState.Complete);
                 });
 

diff --git a/.../Common/Helpers/EventHandlerExtensions.cs → ...d/Common/Helpers/EventFiringExtensions.cs b/.../Common/Helpers/EventHandlerExtensions.cs → ...d/Common/Helpers/EventFiringExtensions.cs
@@ -5,12 +5,34 @@
 
 namespace Laerdal.McuMgr.Common.Helpers
 {
-    static internal class EventHandlerExtensions
+    static internal class EventFiringExtensions
     {
+        static internal void InvokeAndIgnoreExceptions<TEventArgs>(this EventHandler<TEventArgs> eventHandlers, ILogEmittable sender, TEventArgs args) //00
+        {
+#if DEBUG
+            ArgumentNullException.ThrowIfNull(args); //better not check this in release mode for performance reasons
+            ArgumentNullException.ThrowIfNull(sender);
+#endif
+
+            try
+            {
+                eventHandlers?.Invoke(sender, args);
+            }
+            catch
+            {
+                //ignored
+            }
+
+            //00  the difference between .InvokeAndIgnoreExceptions() and .InvokeAllEventHandlersAndIgnoreExceptions()
+            //    is that the event-firing will halt if one of the event handlers throws an exception
+        }
+
         static internal void InvokeAllEventHandlersAndIgnoreExceptions<TEventArgs>(this EventHandler<TEventArgs> eventHandlers, ILogEmittable sender, TEventArgs args)
         {
-            ArgumentNullException.ThrowIfNull(args);
+#if DEBUG
+            ArgumentNullException.ThrowIfNull(args); //better not check this in release mode for performance reasons
             ArgumentNullException.ThrowIfNull(sender);
+#endif
 
             if (eventHandlers == null)
                 return;

diff --git a/Laerdal.McuMgr/Shared/DeviceResetter/DeviceResetter.cs b/Laerdal.McuMgr/Shared/DeviceResetter/DeviceResetter.cs
@@ -170,8 +170,8 @@ void DeviceResetter_FatalErrorOccurred_(object _, FatalErrorOccurredEventArgs ea
             //    from missing libraries and symbols because we dont want the raw native exceptions to bubble up to the managed code
         }
 
-        void ILogEmittable.OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
-        void IDeviceResetterEventEmittable.OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
+        void ILogEmittable.OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAndIgnoreExceptions(this, ea); // in the special case of log-emitted we prefer the .invoke() flavour for the sake of performance
+        void IDeviceResetterEventEmittable.OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAndIgnoreExceptions(this, ea);
         void IDeviceResetterEventEmittable.OnStateChanged(StateChangedEventArgs ea) => _stateChanged?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         void IDeviceResetterEventEmittable.OnFatalErrorOccurred(FatalErrorOccurredEventArgs ea) => _fatalErrorOccurred?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
     }

diff --git a/Laerdal.McuMgr/Shared/FileDownloader/FileDownloader.cs b/Laerdal.McuMgr/Shared/FileDownloader/FileDownloader.cs
@@ -446,8 +446,8 @@ void FileDownloader_FatalErrorOccurred_(object _, FatalErrorOccurredEventArgs ea
         void IFileDownloaderEventEmittable.OnFatalErrorOccurred(FatalErrorOccurredEventArgs ea) => OnFatalErrorOccurred(ea);
         void IFileDownloaderEventEmittable.OnFileDownloadProgressPercentageAndDataThroughputChanged(FileDownloadProgressPercentageAndDataThroughputChangedEventArgs ea) => OnFileDownloadProgressPercentageAndDataThroughputChanged(ea);
 
+        private void OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAndIgnoreExceptions(this, ea); // in the special case of log-emitted we prefer the .invoke() flavour for the sake of performance
         private void OnCancelled(CancelledEventArgs ea) => _cancelled?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
-        private void OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         private void OnBusyStateChanged(BusyStateChangedEventArgs ea) => _busyStateChanged?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         private void OnDownloadCompleted(DownloadCompletedEventArgs ea) => _downloadCompleted?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         private void OnFatalErrorOccurred(FatalErrorOccurredEventArgs ea) => _fatalErrorOccurred?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);

diff --git a/Laerdal.McuMgr/Shared/FileUploader/FileUploader.cs b/Laerdal.McuMgr/Shared/FileUploader/FileUploader.cs
@@ -528,9 +528,9 @@ void FileUploader_FatalErrorOccurred_(object _, FatalErrorOccurredEventArgs ea_)
         void IFileUploaderEventEmittable.OnFatalErrorOccurred(FatalErrorOccurredEventArgs ea) => OnFatalErrorOccurred(ea);
         void IFileUploaderEventEmittable.OnFileUploadProgressPercentageAndDataThroughputChanged(FileUploadProgressPercentageAndDataThroughputChangedEventArgs ea) => OnFileUploadProgressPercentageAndDataThroughputChanged(ea);
 
+        private void OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAndIgnoreExceptions(this, ea); // in the special case of log-emitted we prefer the .invoke() flavour for the sake of performance
         private void OnCancelled(CancelledEventArgs ea) => _cancelled?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         private void OnCancelling(CancellingEventArgs ea) => _cancelling?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
-        private void OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         private void OnFileUploaded(FileUploadedEventArgs ea) => _fileUploaded?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         private void OnStateChanged(StateChangedEventArgs ea) => _stateChanged?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         private void OnBusyStateChanged(BusyStateChangedEventArgs ea) => _busyStateChanged?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);

diff --git a/Laerdal.McuMgr/Shared/FirmwareEraser/FirmwareEraser.cs b/Laerdal.McuMgr/Shared/FirmwareEraser/FirmwareEraser.cs
@@ -184,9 +184,9 @@ void FirmwareEraser_FatalErrorOccurred_(object _, FatalErrorOccurredEventArgs ea
             //    from missing libraries and symbols because we dont want the raw native exceptions to bubble up to the managed code
         }
 
-        void ILogEmittable.OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
-        void IFirmwareEraserEventEmittable.OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea); //       we made these interface implementations
-        void IFirmwareEraserEventEmittable.OnStateChanged(StateChangedEventArgs ea) => _stateChanged?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea); // explicit to avoid making them public
+        void ILogEmittable.OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAndIgnoreExceptions(this, ea); // in the special case of log-emitted we prefer the .invoke() flavour for the sake of performance
+        void IFirmwareEraserEventEmittable.OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAndIgnoreExceptions(this, ea); //                       we are using explicit interface
+        void IFirmwareEraserEventEmittable.OnStateChanged(StateChangedEventArgs ea) => _stateChanged?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea); // implementations to avoid making these methods public
         void IFirmwareEraserEventEmittable.OnBusyStateChanged(BusyStateChangedEventArgs ea) => _busyStateChanged?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         void IFirmwareEraserEventEmittable.OnFatalErrorOccurred(FatalErrorOccurredEventArgs ea) => _fatalErrorOccurred?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
     }

diff --git a/Laerdal.McuMgr/Shared/FirmwareInstaller/FirmwareInstaller.cs b/Laerdal.McuMgr/Shared/FirmwareInstaller/FirmwareInstaller.cs
@@ -386,7 +386,7 @@ void FirmwareInstaller_FatalErrorOccurred_(object _, FatalErrorOccurredEventArgs
             //10  we dont want to wrap our own exceptions obviously   we only want to sanitize native exceptions from java and swift that stem
             //    from missing libraries and symbols because we dont want the raw native exceptions to bubble up to the managed code
         }
-        
+
         void ILogEmittable.OnLogEmitted(LogEmittedEventArgs ea) => OnLogEmitted(ea);
         void IFirmwareInstallerEventEmittable.OnCancelled(CancelledEventArgs ea) => OnCancelled(ea); //just to make the class unit-test friendly without making the methods public
         void IFirmwareInstallerEventEmittable.OnLogEmitted(LogEmittedEventArgs ea) => OnLogEmitted(ea);
@@ -396,9 +396,9 @@ void FirmwareInstaller_FatalErrorOccurred_(object _, FatalErrorOccurredEventArgs
         void IFirmwareInstallerEventEmittable.OnIdenticalFirmwareCachedOnTargetDeviceDetected(IdenticalFirmwareCachedOnTargetDeviceDetectedEventArgs ea) => OnIdenticalFirmwareCachedOnTargetDeviceDetected(ea);
         void IFirmwareInstallerEventEmittable.OnFirmwareUploadProgressPercentageAndDataThroughputChanged(FirmwareUploadProgressPercentageAndDataThroughputChangedEventArgs ea) => OnFirmwareUploadProgressPercentageAndDataThroughputChanged(ea);
 
-        private void OnCancelled(CancelledEventArgs ea) => _cancelled?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea); //           we suppress exceptions here because if we allow them to bubble towards the native code then the
-        private void OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea); //        native code can crash which can potentially cause very nasty problems in the firmware installation
-        private void OnBusyStateChanged(BusyStateChangedEventArgs ea) => _busyStateChanged?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
+        private void OnLogEmitted(LogEmittedEventArgs ea) => _logEmitted?.InvokeAndIgnoreExceptions(this, ea); // in the special case of log-emitted we prefer the .invoke() flavour for the sake of performance 
+        private void OnCancelled(CancelledEventArgs ea) => _cancelled?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea); //                        we suppress exceptions here because if we allow them to bubble towards the native code then the
+        private void OnBusyStateChanged(BusyStateChangedEventArgs ea) => _busyStateChanged?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea); //   native code can crash which can potentially cause very nasty problems in the firmware installation
         private void OnFatalErrorOccurred(FatalErrorOccurredEventArgs ea) => _fatalErrorOccurred?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);
         private void OnIdenticalFirmwareCachedOnTargetDeviceDetected(IdenticalFirmwareCachedOnTargetDeviceDetectedEventArgs ea) => _identicalFirmwareCachedOnTargetDeviceDetected?.InvokeAllEventHandlersAndIgnoreExceptions(this, ea);