Fix disposal race condition

[lofcz]

BlazoredModal implements IAsyncDisposable. There is no public API to know whether our IJsObjectReference is currently being disposed or not. This occasionally results in an unhandled throw. As we don't really care whether our calls succeeded or not in this case, I've just wrapped them all in try, catch.

[lofcz]

Fixes #481
Feel free to try switching dependency to BlazorModalExperimental (nuget) to test the fix.

[SSchulze1989]

Since it is a bad practise to catch all exceptions and we pretty much know which exception to expect at this point it would be better to only catch the Microsoft.JSInterop.JSException specifically. Also there is no need to store the exception in variable e when it is never used again:

try
{
   /// code ...
}
catch (Microsoft.JSInterop.JSException) {}

[lofcz]

I don't see a reason to catch only a specific exception when future Blazor implementation may change, this way we are future proof.

The other comment I consider fair but NIT as the compiler emits the exact same IL and optimizes the unused var away, see Sharplab diff.

[lofcz]

@SSchulze1989 would you mind testing the branch (as simple as switching the dependency), to check whether this covers #481 completely?

[SSchulze1989]

1. Exception handling should always be as specific as possible. Otherwise you run into the danger of masking an actual issue with the implementation when another error occurs that you did not account for. Granted that is probably unlikely to happen in this case but on many compiler configurations this will produce a warning that should not be ignored.

2. The same reason for the unused variable is that this as well produces an unnecessary warning. I prefer to keep my code clear of any warnings as far as possible to 0 - so that when a warning for an actual issue appears I do not overlook it.

Granted these are personal preferences you may as well ignore them - but they are in line with common best practices in most programming languages.

[SSchulze1989]

@lofcz i did some preliminary testing and did not see the issue reoccur.

[lofcz]

It actually seems like we can implement the pattern mentioned in the top answer here: https://stackoverflow.com/questions/74928714/blazor-component-removed-from-dom-before-disposal-causes-js-interop-to-fail
to never raise the error in the first place, .net7+ only.

I'm not 100% positive whether that is the way though as we would introduce a synchronous finalizer while currently, we are working with IAsyncDisposable - so we would block on disposing and not implement the asynchronous dispose at all.

[lofcz]

@chrissainty how would you prefer this to be addressed? Upstream has to support .NET 6 so I could hack in the approach mentioned above with conditional compiling but that would mess up the otherwise clean code.

[chrissainty]

@lofcz I haven't had a chance to look into this myself yet, only do a bit of quick research.

Currently, I'm not convinced that the proposed solution is correct. In the linked SO post, there is a link to a GitHub issue (dotnet/aspnetcore#45777) where MutationObserver is mention as the correct way to handle disposing of certain JS related code. I need to look into this a bit more.

Also in the original issue (#481) it mentions JS calls to Focus Trap, we don't use JS for the focus trap. Also the clear body styles function mentioned doesn't exist either in our JS. So there are a lot of questions for me right now that need answering before picking a solution.

[chrissainty]

I've decided to close this PR. I'm not sure it's the right solution and it seems to have some other stuff in it not related to the issue it's fixing.