From f5c756c93533023c24dbcaa3c6d423c522cdb0e7 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 27 Mar 2019 19:42:23 -0700 Subject: [PATCH] Fix dotnet/extensions#8612 JSInterop throws truncated call stack The fix for this is to use more ExceptionDispatchInfo! Basically everwhere that we handle an exception is now an EDI. It's easy to pass these around and they do the right thing as far as perserving the stack in. I de-factored this code a little bit to make all of this work, but it's now pretty unambiguously correct upon inspection. I started out wanting to get rid of the continue-with because we've found that pretty error prone overall. However making this method async-void started to ask more questions than it answered. What happens if serialization of the exception fails? \n\nCommit migrated from https://github.com/dotnet/extensions/commit/0a27fd3ad6604ca1f1278db5e01224a29fcab00d --- .../src/DotNetDispatcher.cs | 69 ++++++++----------- .../Microsoft.JSInterop/src/JSRuntimeBase.cs | 5 ++ .../test/DotNetDispatcherTest.cs | 64 +++++++++++++++++ 3 files changed, 97 insertions(+), 41 deletions(-) diff --git a/src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs b/src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs index 0f346bbfdd..ac936e670a 100644 --- a/src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs +++ b/src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs @@ -7,6 +7,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Reflection; +using System.Runtime.ExceptionServices; using System.Threading.Tasks; namespace Microsoft.JSInterop @@ -72,8 +73,10 @@ namespace Microsoft.JSInterop ? null : jsRuntimeBaseInstance.ArgSerializerStrategy.FindDotNetObject(dotNetObjectId); + // Using ExceptionDispatchInfo here throughout because we want to always preserve + // original stack traces. object syncResult = null; - Exception syncException = null; + ExceptionDispatchInfo syncException = null; try { @@ -81,45 +84,38 @@ namespace Microsoft.JSInterop } catch (Exception ex) { - syncException = ex; + syncException = ExceptionDispatchInfo.Capture(ex); } // If there was no callId, the caller does not want to be notified about the result - if (callId != null) + if (callId == null) { - // Invoke and coerce the result to a Task so the caller can use the same async API - // for both synchronous and asynchronous methods - var task = CoerceToTask(syncResult, syncException); - - task.ContinueWith(completedTask => + return; + } + else if (syncException != null) + { + // Threw synchronously, let's respond. + jsRuntimeBaseInstance.EndInvokeDotNet(callId, false, syncException); + } + else if (syncResult is Task task) + { + // Returned a task - we need to continue that task and then report an exception + // or return the value. + task.ContinueWith(t => { - try + if (t.Exception != null) { - var result = TaskGenericsUtil.GetTaskResult(completedTask); - jsRuntimeBaseInstance.EndInvokeDotNet(callId, true, result); + var exception = t.Exception.GetBaseException(); + jsRuntimeBaseInstance.EndInvokeDotNet(callId, false, ExceptionDispatchInfo.Capture(exception)); } - catch (Exception ex) - { - ex = UnwrapException(ex); - jsRuntimeBaseInstance.EndInvokeDotNet(callId, false, ex); - } - }); - } - } - private static Task CoerceToTask(object syncResult, Exception syncException) - { - if (syncException != null) - { - return Task.FromException(syncException); - } - else if (syncResult is Task syncResultTask) - { - return syncResultTask; + var result = TaskGenericsUtil.GetTaskResult(task); + jsRuntimeBaseInstance.EndInvokeDotNet(callId, true, result); + }, TaskScheduler.Current); } else { - return Task.FromResult(syncResult); + jsRuntimeBaseInstance.EndInvokeDotNet(callId, true, syncResult); } } @@ -175,9 +171,10 @@ namespace Microsoft.JSInterop { return methodInfo.Invoke(targetInstance, suppliedArgs); } - catch (Exception ex) + catch (TargetInvocationException tie) when (tie.InnerException != null) { - throw UnwrapException(ex); + ExceptionDispatchInfo.Capture(tie.InnerException).Throw(); + throw null; // unreachable } } @@ -285,15 +282,5 @@ namespace Microsoft.JSInterop return loadedAssemblies.FirstOrDefault(a => a.GetName().Name.Equals(assemblyName, StringComparison.Ordinal)) ?? throw new ArgumentException($"There is no loaded assembly with the name '{assemblyName}'."); } - - private static Exception UnwrapException(Exception ex) - { - while ((ex is AggregateException || ex is TargetInvocationException) && ex.InnerException != null) - { - ex = ex.InnerException; - } - - return ex; - } } } diff --git a/src/JSInterop/Microsoft.JSInterop/src/JSRuntimeBase.cs b/src/JSInterop/Microsoft.JSInterop/src/JSRuntimeBase.cs index 09379396cf..d18bc7f4fe 100644 --- a/src/JSInterop/Microsoft.JSInterop/src/JSRuntimeBase.cs +++ b/src/JSInterop/Microsoft.JSInterop/src/JSRuntimeBase.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Concurrent; +using System.Runtime.ExceptionServices; using System.Threading; using System.Threading.Tasks; @@ -80,6 +81,10 @@ namespace Microsoft.JSInterop { resultOrException = resultOrException.ToString(); } + else if (!success && resultOrException is ExceptionDispatchInfo edi) + { + resultOrException = edi.SourceException.ToString(); + } // We pass 0 as the async handle because we don't want the JS-side code to // send back any notification (we're just providing a result for an existing async call) diff --git a/src/JSInterop/Microsoft.JSInterop/test/DotNetDispatcherTest.cs b/src/JSInterop/Microsoft.JSInterop/test/DotNetDispatcherTest.cs index 93ef9a2498..e3f91a6fd0 100644 --- a/src/JSInterop/Microsoft.JSInterop/test/DotNetDispatcherTest.cs +++ b/src/JSInterop/Microsoft.JSInterop/test/DotNetDispatcherTest.cs @@ -285,6 +285,54 @@ namespace Microsoft.JSInterop.Tests Assert.Equal(2468, resultDto2.IntVal); }); + + [Fact] + public Task CanInvokeSyncThrowingMethod() => WithJSRuntime(async jsRuntime => + { + // Arrange + + // Act + var callId = "123"; + var resultTask = jsRuntime.NextInvocationTask; + DotNetDispatcher.BeginInvoke(callId, thisAssemblyName, nameof(ThrowingClass.ThrowingMethod), default, default); + + await resultTask; // This won't throw, it sets properties on the jsRuntime. + + // Assert + var result = Json.Deserialize(jsRuntime.LastInvocationArgsJson); + Assert.Equal(callId, result[0]); + Assert.False((bool)result[1]); // Fails + + // Make sure the method that threw the exception shows up in the call stack + // https://github.com/aspnet/AspNetCore/issues/8612 + var exception = (string)result[2]; + Assert.Contains(nameof(ThrowingClass.ThrowingMethod), exception); + }); + + [Fact] + public Task CanInvokeAsyncThrowingMethod() => WithJSRuntime(async jsRuntime => + { + // Arrange + + // Act + var callId = "123"; + var resultTask = jsRuntime.NextInvocationTask; + DotNetDispatcher.BeginInvoke(callId, thisAssemblyName, nameof(ThrowingClass.AsyncThrowingMethod), default, default); + + await resultTask; // This won't throw, it sets properties on the jsRuntime. + + // Assert + var result = Json.Deserialize(jsRuntime.LastInvocationArgsJson); + Assert.Equal(callId, result[0]); + Assert.False((bool)result[1]); // Fails + + // Make sure the method that threw the exception shows up in the call stack + // https://github.com/aspnet/AspNetCore/issues/8612 + var exception = (string)result[2]; + Assert.Contains(nameof(ThrowingClass.AsyncThrowingMethod), exception); + }); + + Task WithJSRuntime(Action testCode) { return WithJSRuntime(jsRuntime => @@ -413,6 +461,22 @@ namespace Microsoft.JSInterop.Tests public int IntVal { get; set; } } + public class ThrowingClass + { + [JSInvokable] + public static string ThrowingMethod() + { + throw new InvalidTimeZoneException(); + } + + [JSInvokable] + public static async Task AsyncThrowingMethod() + { + await Task.Yield(); + throw new InvalidTimeZoneException(); + } + } + public class TestJSRuntime : JSInProcessRuntimeBase { private TaskCompletionSource _nextInvocationTcs = new TaskCompletionSource();