Make webview2 disposal safer #14888
Make webview2 disposal safer #14888
Conversation
…into test_crash_logs
UI Smoke TestsTest: success. 2 passed, 0 failed. |
| @@ -40,15 +40,15 @@ jobs: | |||
| Write-Error "DynamoCLI.exe was not found!" | |||
| } | |||
| - name: Install dependencies for linux runtime | |||
| run: dotnet restore ${{ github.workspace }}\Dynamo\src\DynamoCore.sln -p:Platform=NET60_Linux --runtime=linux-x64 | |||
| run: dotnet restore ${{ github.workspace }}\Dynamo\src\DynamoCore.sln -p:Platform=NET_Linux --runtime=linux-x64 | |||
There was a problem hiding this comment.
why does this job that is called dynamo core windows built for platform linux?
There was a problem hiding this comment.
I think it tries to build DynamoCore for Linux on a windows machine. Maybe we thought it was a valuable scenario (build on windows but target linux)
|
|
||
| initState = AsyncMethodState.Done; | ||
| } | ||
| catch(ObjectDisposedException ex) |
There was a problem hiding this comment.
should we be catching other exceptions?
There was a problem hiding this comment.
ObjectDisposedException is the only exception type that we should expect to happen, and mostly in tests. It should not trigger crashes and Dynamo should be able to go on running.
For other types of exceptions, we should let Dynamo handle it (crash or not...) Not sure yet though...
| /// <returns></returns> | ||
| internal async Task Initialize(Action<string> logFn = null) | ||
| { | ||
| logger ??= logFn; |
There was a problem hiding this comment.
Oops. Should be logger = logFn ?? logger
Was trying to use logFn if it is valid.
Reason: This is to handle cases when we do not have access to the Dynamo Logger (we should try to make this a static so we have access to it everywhere).
logger should point to logFn (a DynamoLogger function call - when it is available) or Console.Writeline
| initTask = EnsureCoreWebView2Async(); | ||
| await initTask; | ||
|
|
||
| ObjectDisposedException.ThrowIf(diposeCalled, this); |
There was a problem hiding this comment.
so this is helpful to trigger early failure?
There was a problem hiding this comment.
Yes, it forces all callers to handle the exception.
Ex
await webview2.Initialize(); \\ - will throw Disposed exception if it was disposed before execution resumes
\\ - webview2.Dispose could be called before this function resumes execution
webview2.DoStuff(); \\ - Do not execute any more code if Initialize throws an exception
| webBrowserComponent.CoreWebView2.Settings.IsZoomControlEnabled = false; | ||
| webBrowserComponent.CoreWebView2.Settings.IsPinchZoomEnabled = false; | ||
| } | ||
| catch (ObjectDisposedException ex) |
There was a problem hiding this comment.
How can this exception be thrown now that we are waiting for initialize to happen?
There was a problem hiding this comment.
It can be thrown here https://github.com/DynamoDS/Dynamo/pull/14888/files#diff-405bba4d4ce51bef98ee2ea6e37e396b6834800587417bd4ccfa855590099ef3R43
we throw that exception so that callers of Initialize() can short-circuit their initialization code in case the object was disposed before Initialize finished
There was a problem hiding this comment.
I guess I'm trying to understand what will be the case when it's disposed while it's being initialized at the same time. Are you saying these things can happen concurrently in different threads?
There was a problem hiding this comment.
I can happen on the same thread, but asynchronously
For example a test:
using (webview2View = new Webview2View){
webview2View.Initialize();// async method might take a couple of seconds
} // might exit block before webview2View.Initialize finishes (dispose will be called here)
There was a problem hiding this comment.
Ah ok, so these are additional guardrails for sloppy testing code.
| // Dispose can be called from the Finalizer (which can run on a non UI thread) | ||
| if (Dispatcher != null) | ||
|
|
||
| if (initTask != null && !initTask.IsCompleted) |
There was a problem hiding this comment.
In line 41, you're awaiting initTask, so what will be the case where initTask is not complete?
There was a problem hiding this comment.
await initTask means that the execution of the function is put on hold until it finishes some internal awaits. Meanwhile the thread will continue to execute other function calls.
ex
DoStuff() {
webview2 = new DynamoWebvView2();
webview2.Initialize();// call but do not wait for it to finish
webview2.Dispose();
}
``` This is pretty much what happens in some tests
net6 cleanup
MLPipeline cleanup
DynamoWebView2 disposal now waits for Async Initialize to finish
InitializeAsync throws Disposed exception if webview is disposed before initialize is finished.