Fix BlazorWebView disposal #7349
Conversation
| protected override async ValueTask DisposeAsyncCore() | ||
| { | ||
| await base.DisposeAsyncCore(); | ||
|
|
||
| if (_nativeToJSPorts is not null) | ||
| { | ||
| foreach (var port in _nativeToJSPorts) | ||
| { | ||
| port?.Close(); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Turns out that these ports get closed automatically when the WebView gets unloaded. Now that this logic was running for the first time, an exception was being thrown because the ports were being closed twice. Hence the removal of this method.
There was a problem hiding this comment.
Thanks @MackinnonBuck.
Don't we need unit-tests for these now?
| // see: https://github.com/microsoft/microsoft-ui-xaml/issues/6872 | ||
| // See: https://github.com/dotnet/maui/issues/7277 | ||
| Handler?.DisconnectHandler(); |
There was a problem hiding this comment.
@PureWeen will this cause issues at all?
There was a problem hiding this comment.
@MackinnonBuck why is this specifically needed on Windows and not other platforms?
There was a problem hiding this comment.
@Eilon The original issue that this PR is solving was reported for Windows. On Android and iOS, the Window.Destroying event does not get invoked when the app gets closed (#6993), so it wouldn't make a difference anyway. That said, we could make this line of code platform-independent, but that might introduce a risk that it doesn't work as expected on Android and iOS if/when #6993 gets fixed. Do you think it's worth investigating another way to make this work on other platforms?
There was a problem hiding this comment.
Each platform has a version of Unloaded if you want to tie into those?
Android => ViewDetachedFromWindow
iOS => override WillMoveToWindow
It doesn't cover the window destroying case but it makes the behavior symmetrical for when a view is removed from the visual hierarchy.
There was a problem hiding this comment.
@PureWeen this PR already adds a handler for the Unloaded event to invoke DisconnectHandler(). Just so I understand, is your suggestion to use the platform-specific APIs instead?
There was a problem hiding this comment.
@PureWeen BlazorWebView retains hardly any state at all (it's managed by the underlying platform-specific WebView), so the user would basically be starting over from the beginning.
There was a problem hiding this comment.
the primary use case of BlazorWebView will be MAUI Blazor apps where the WebView is basically the entire app
That might be the most common pattern, but certainly not the only one. I think optimizing for the most common pattern can make sense, but we have to be careful to not cause significant harm to less-common scenarios (I'm not saying any suggestion here violates that, but I want to make sure we don't end up there). This stuff is very hard for me to think through 😄
There was a problem hiding this comment.
@Eilon, if we compare MAUI Blazor with Blazor WebAssembly for a minute, we don't call Dispose() for Blazor WebAssembly components when, for example, the browser window is closed. If someone needs some logic to run when the application window is closing, couldn't you argue that the resource being disposed exists in a context broader than the BlazorWebView, so therefore the disposal logic should be handled from that broader context (in MAUI lifecycle events, for example)? For example, @PureWeen brought up offline that the issue faced in #7277 could be fixed by running the disposal logic from within a MAUI context. This obviously has the downside that if you want to share Razor components between supported Blazor platforms, you would potentially need multiple versions of the component that manage state/disposal logic differently. But this comes back to what I said earlier about there not really being an equivalent case in Blazor WebAssembly, so maybe that concern isn't valid?
There was a problem hiding this comment.
So basically, leaving it as-is where components don't get disposed with the WebView getting unloaded would actually maintain some level of parity with Blazor WebAssembly, for example.
Just throwing some ideas out there, because there is definitely a real risk of us breaking certain scenarios as you pointed out.
There was a problem hiding this comment.
The conclusion we reached offline is that we're going to hold off on having BlazorWebView determine when to call DisconnectHandler() for now. The existing behavior is more acceptable than initially thought, so we feel comfortable revisiting this topic later when the general concept of disposal in MAUI is better understood.
|
Let's refrain from merging until this conversation is resolved. |
TanayParikh
added
blocked
|
We're holding off on manually invoking |
Description of Change
BlazorWebViewHandler.DisconnectHandler()has not been getting invoked when theBlazorWebViewgets unloaded. This was due to the incorrect assumption thatDisconnectHandler()gets invoked automatically by the framework (see #3604). The result is that none of the*WebViewManagerdisposal logic was running, including the logic that ultimately runsDispose()in Blazor components. This PR solves this problem in addition to a bug preventing disposal on Windows when the application closes.Fixes an issue where
DisconnectHandler()throws an exception on Android due to the double-closing of Android WebView message channel ports.Issues Fixed
Fixes 7277Repurposed to fix a potential future bug where
DisconnectHandler()throws an exception on Android.