Ensure shutdown order of WebView2 and unsubscribe from publishers - #3318
Open
ColinM9991 wants to merge 3 commits into
Open
Ensure shutdown order of WebView2 and unsubscribe from publishers#3318ColinM9991 wants to merge 3 commits into
ColinM9991 wants to merge 3 commits into
Conversation
|
Build for this pull request: |
… Upgrade WebView2 package
|
Build for this pull request: |
|
Build for this pull request: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce startup/shutdown exceptions around WebView2 and message publishing by improving shutdown sequencing, adding disposal/unsubscribe paths for publishers and WebView callbacks, and enriching logging with caller metadata and exception details.
Changes:
- Refactors
FrontendPanelinitialization/shutdown to register/unregister WebView2 callbacks and publishers, and triggers shutdown earlier inMainFormclosing. - Makes
IMessagePublisherdisposable and adds/extendsDispose()implementations (composite + concrete publishers), plus adds unit tests for log formatting and composite disposal behavior. - Updates logging to use caller attributes (file/member/line) and include exception details; bumps WebView2 package version.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MobiFlightUnitTests/UI/MainFormTests.cs | Updates test publisher stub to satisfy new IDisposable publisher contract. |
| tests/MobiFlightUnitTests/MobiFlight/BrowserMessages/Publisher/CompositePublisherTests.cs | Adds coverage for composite disposal stopping further publishing. |
| tests/MobiFlightUnitTests/Base/LogTests.cs | New tests validating caller metadata formatting and exception logging. |
| src/MobiFlightConnector/UI/Panels/FrontendPanel.Designer.cs | Calls Shutdown() during disposal to ensure WebView-related teardown happens. |
| src/MobiFlightConnector/UI/Panels/FrontendPanel.cs | Moves WebView2 init to OnLoad, tracks registered handlers for cleanup, and adds shutdown logic/publisher disposal. |
| src/MobiFlightConnector/UI/MainForm.cs | Adjusts shutdown order to stop publishing (frontend) before stopping/shutting down execution manager. |
| src/MobiFlightConnector/MobiFlightConnector.csproj | Removes <UseWPF>true</UseWPF> (project appears WinForms-only). |
| src/MobiFlightConnector/MobiFlight/WebView/ThreadSafeWebView2.cs | Refactors thread-safe calls and adds disposal/handle checks (but contains a compile issue in ExecuteScriptAsync). |
| src/MobiFlightConnector/MobiFlight/WebView/StaticPageWebResourceRequestHandler.cs | Adds unregistration method for WebResourceRequested filter/event. |
| src/MobiFlightConnector/MobiFlight/WebView/AddCloseButtonHandlerOnNavigationCompleted.cs | Adds Unregister() to detach navigation-completed handler. |
| src/MobiFlightConnector/MobiFlight/BrowserMessages/Publisher/WebsocketPublisher.cs | Adds Dispose() for socket cleanup (needs more robust shutdown behavior). |
| src/MobiFlightConnector/MobiFlight/BrowserMessages/Publisher/PostMessagePublisher.cs | Makes publisher disposable and unsubscribes from WebView message event on dispose. |
| src/MobiFlightConnector/MobiFlight/BrowserMessages/Publisher/CompositePublisher.cs | Adds disposal state, disposes child publishers, and logs publish exceptions with stack traces. |
| src/MobiFlightConnector/MobiFlight/BrowserMessages/IMessagePublisher.cs | Makes publishers IDisposable via interface inheritance. |
| src/MobiFlightConnector/Base/Log.cs | Replaces stack-trace caller detection with caller attributes; adds exception overload and removes textbox appender. |
| Directory.Packages.props | Updates Microsoft.Web.WebView2 package version. |
Files not reviewed (1)
- src/MobiFlightConnector/UI/Panels/FrontendPanel.Designer.cs: Generated file
Suppressed comments (2)
src/MobiFlightConnector/UI/Panels/FrontendPanel.cs:82
- Even if handler/publisher registration succeeds,
Shutdown()may run beforeMessageExchange.Instance.SetPublisher(...). In that case a disposed_compositePublishercan get set back as the active publisher after shutdown.
_compositePublisher.AddPublisher("frontend", frontendPublisher);
_compositePublisher.AddPublisher("auth", authPublisher);
MessageExchange.Instance.SetPublisher(_compositePublisher);
}
src/MobiFlightConnector/UI/Panels/FrontendPanel.cs:35
- Target-typed
new()is also used for these lists. Consider using explicit constructors to avoid depending on newer C# language versions and to stay consistent with the surrounding codebase style.
private readonly List<(ThreadSafeWebView2 WebView, StaticPageWebResourceRequestHandler Handler)> _webResourceHandlers = new();
private readonly List<AddCloseButtonHandlerOnNavigationCompleted> _navigationHandlers = new();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
36
to
44
| async Task<string> IWebView2Adapter.ExecuteScriptAsync(string script) | ||
| { | ||
| if (CoreWebView2 == null) return null; | ||
| if (this.InvokeRequired) | ||
|
|
||
| if (InvokeRequired) | ||
| { | ||
| return await (Task<string>)this.Invoke(new Func<Task<string>>(async () => | ||
| await CoreWebView2.ExecuteScriptAsync(script))); | ||
| return await Invoke(async () => | ||
| await CoreWebView2.ExecuteScriptAsync(script)); | ||
| } |
Comment on lines
2494
to
2498
| // only cancel closing if not saved before | ||
| // which is indicated by empty CurrentFilename | ||
| e.Cancel = (execManager.Project.FilePath == null); | ||
| e.Cancel = (execManager?.Project.FilePath == null); | ||
| saveToolStripButton_Click(this, new EventArgs()); | ||
| } |
Comment on lines
+65
to
+68
| public void Dispose() | ||
| { | ||
| _webSocket?.Dispose(); | ||
| } |
Comment on lines
+12
to
15
| if (InvokeRequired) | ||
| { | ||
| this.Invoke(new Action(() => CoreWebView2?.PostWebMessageAsJson(jsonMessage))); | ||
| Invoke(InternalPostWebMessageAsJsonThreadSafe); | ||
| } |
| public partial class FrontendPanel : UserControl | ||
| { | ||
| CompositePublisher compositePublisher = new CompositePublisher(); | ||
| private readonly CompositePublisher _compositePublisher = new(); |
Comment on lines
+69
to
+73
| await FrontendWebView.EnsureCoreWebView2Async(null); | ||
| await UserAuthenticationWebView.EnsureCoreWebView2Async(null); | ||
|
|
||
| InitializeWebView(FrontendWebView, "/start"); | ||
| InitializeWebView(UserAuthenticationWebView); |
Comment on lines
+1
to
+2
| using MobiFlight.BrowserMessages; | ||
| using MobiFlight.WebView; |
Comment on lines
+567
to
+569
| public void Dispose() | ||
| { | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GetCallingMethodwithCallerMemberNameandCallerFilePath.CallerLineNumberis also in there now.ThreadSafeWebView2to introduce reusable local functionThreadSafeWebView2events if the underlying object has been disposed.StopPublishingfunction onFrontendPanelto remove the publishersStopPublishingupon main form shutdownAcceptance criteria
- [ ] Developer docs (e.g., readme.md)Related issues
https://discord.com/channels/608690978081210392/1542458812689162250