Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make webview2 disposal safer #14888

Merged
merged 45 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from 44 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
c45a3da
update
pinzart Dec 7, 2023
135de06
Update NodeModelAssemblyLoader.cs
pinzart Dec 7, 2023
32aaa28
update
pinzart Dec 19, 2023
5129a93
Merge commit 'c45a3dafce611cfe68b25bbaad1c6f5a8c8c9b42' into test_cra…
pinzart Dec 19, 2023
8c2c694
Merge commit '135de06fd3b7c939b4b692b6bd0319b7a888a65c' into test_cra…
pinzart Dec 19, 2023
cf3c37d
Merge branch 'master' into test_crash_logs
pinzart90 Dec 19, 2023
b0ec268
update
pinzart Dec 20, 2023
4cc32b2
Merge branch 'master' into test_crash_logs
pinzart Dec 20, 2023
7cf5cbd
update
pinzart Dec 20, 2023
6c3add2
update
pinzart Dec 20, 2023
5e90045
Merge branch 'master' into test_crash_logs
pinzart Dec 21, 2023
6d062dd
Update AnalyticsTests.cs
pinzart Dec 21, 2023
b58be3d
Update SerializationTests.cs
pinzart Dec 21, 2023
aee8814
Merge branch 'master' into test_crash_logs
pinzart90 Dec 21, 2023
697b33d
Update Setup.cs
pinzart Dec 21, 2023
e047f1e
Merge branch 'test_crash_logs' of https://github.com/DynamoDS/Dynamo …
pinzart Dec 21, 2023
a3a9cd7
update
pinzart90 Jan 8, 2024
f85d5f4
Merge branch 'master' into test_crash_logs
pinzart90 Jan 8, 2024
f6a9aad
update
pinzart90 Jan 8, 2024
516b73d
Update SplashScreen.xaml.cs
pinzart90 Jan 8, 2024
241a7da
Merge branch 'master' into test_crash_logs
pinzart90 Jan 9, 2024
30cc0fd
Update PublishPackageViewModelTests.cs
pinzart90 Jan 9, 2024
84a280b
Update PublishPackageViewModelTests.cs
pinzart90 Jan 9, 2024
f4ee178
update
pinzart90 Jan 9, 2024
3d31d55
Update AssemblyInfo.cs
pinzart90 Jan 9, 2024
b27a3c4
Update TestUtilities.cs
pinzart90 Jan 9, 2024
2c34283
update
pinzart90 Jan 10, 2024
d25719f
upate
pinzart90 Jan 10, 2024
984cc8d
update
pinzart90 Jan 10, 2024
6d35115
update
pinzart90 Jan 10, 2024
95ace8c
update
pinzart90 Jan 10, 2024
b9846f7
Update AnalyticsTests.cs
pinzart90 Jan 10, 2024
e4dcf57
Update WebView2Utilities.cs
pinzart90 Jan 10, 2024
9ebd4b0
update
pinzart90 Jan 10, 2024
eaebcca
update
pinzart90 Jan 10, 2024
6867e6c
Update Setup.cs
pinzart90 Jan 10, 2024
3882214
update
pinzart90 Jan 19, 2024
4103d57
update
pinzart90 Jan 19, 2024
b88d49c
Update PeriodicEvaluationTests.cs
pinzart90 Jan 22, 2024
7216377
update
pinzart90 Jan 26, 2024
7ae0cb0
Merge branch 'master' into test_crash_logs
pinzart90 Jan 26, 2024
befb456
update
pinzart90 Feb 6, 2024
f3cd594
Merge branch 'master' into test_crash_logs
pinzart90 Feb 6, 2024
d5ff0b0
update
pinzart90 Feb 6, 2024
f7db55d
update - review comments
pinzart90 Feb 7, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ jobs:
uses: microsoft/setup-msbuild@v1.3
- name: Install dependencies for windows runtime
run: |
dotnet restore ${{ github.workspace }}\Dynamo\src\Dynamo.All.sln /p:Configuration=Release --runtime=win-x64 -p:DotNet=net8.0
dotnet restore ${{ github.workspace }}\Dynamo\src\Dynamo.All.sln /p:Configuration=Release --runtime=win-x64
- name: Build Dynamo with MSBuild for Windows
run: |
Write-Output "***Continue with the build, Good luck developer!***"
msbuild ${{ github.workspace }}\Dynamo\src\Dynamo.All.sln /p:Configuration=Release /p:DotNet=net8.0
msbuild ${{ github.workspace }}\Dynamo\src\Dynamo.All.sln /p:Configuration=Release
- name: Look for DynamoCLI.exe
run: |
Write-Output "***Locating DynamoCLI.exe!***"
Expand Down
41 changes: 0 additions & 41 deletions .github/workflows/build_dynamo_all_net6.0.yml

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Build DynamoCore.sln with .NET 6.0 on linux
name: Build DynamoCore.sln net6.0 linux
# Build DynamoCore.sln with .NET 8.0 on linux
name: Build DynamoCore.sln net8.0 linux

on:
push:
Expand All @@ -17,22 +17,24 @@ jobs:
path: Dynamo
- name: Setup dotnet
uses: actions/setup-dotnet@v4
with:
dotnet-version: '8.0.x'
- name: Disable problem matcher
run: echo "::remove-matcher owner=csc::"
- 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
- name: Build Dynamo with MSBuild for Linux
run: |
echo "***Continue with the build, Good luck developer!***"
dotnet build ${{ github.workspace }}/Dynamo/src/DynamoCore.sln -c Release /p:Platform=NET60_Linux
dotnet build ${{ github.workspace }}/Dynamo/src/DynamoCore.sln -c Release /p:Platform=NET_Linux
- name: Look for DynamoCLI.exe
run: |
cd "${{ github.workspace }}/Dynamo/bin/NET60_Linux/Release"
cd "${{ github.workspace }}/Dynamo/bin/NET_Linux/Release"
echo "***Locating DynamoCLI for Linux!***"
test "./DynamoCLI.exe" && echo "DynamoCLI exists!"
- name: Run smoke tests
run: |
cd "${{ github.workspace }}/Dynamo/bin/NET60_Linux/Release"
cd "${{ github.workspace }}/Dynamo/bin/NET_Linux/Release"
echo "***Running Smoke tests on linux***"
#TODO unfortunately dotnet does not find any tests in this assembly.
#dotnet test DynamoCoreTests.dll --filter "TestCategory~UnitTest"
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:
- name: Setup dotnet
uses: actions/setup-dotnet@v4
with:
dotnet-version: '6.0.x'
dotnet-version: '8.0.x'
- name: Disable problem matcher
run: Write-Output "::remove-matcher owner=csc::"
- name: Setup msbuild
Expand All @@ -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
Copy link
Member

@mjkkirschner mjkkirschner Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this job that is called dynamo core windows built for platform linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

- name: Build Dynamo with MSBuild for Linux
run: |
Write-Output "***Continue with the build, Good luck developer!***"
msbuild ${{ github.workspace }}\Dynamo\src\DynamoCore.sln /p:Configuration=Release /p:Platform=NET60_Linux
msbuild ${{ github.workspace }}\Dynamo\src\DynamoCore.sln /p:Configuration=Release /p:Platform=NET_Linux
- name: Look for DynamoCLI
run: |
Write-Output "***Locating DynamoCLI for Linux!***"
if (Test-Path -Path "${{ github.workspace }}\Dynamo\bin\NET60_Linux\Release\DynamoCLI") {
if (Test-Path -Path "${{ github.workspace }}\Dynamo\bin\NET_Linux\Release\DynamoCLI") {
Write-Output "DynamoCLI exists!"
} else {
Write-Error "DynamoCLI was not found!"
Expand Down
2 changes: 1 addition & 1 deletion src/Config/CS_SDK.props
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project>
<PropertyGroup>
<Platforms>AnyCPU;NET60_Linux;</Platforms>
<Platforms>AnyCPU;NET_Linux;</Platforms>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<PlatformTarget >x64</PlatformTarget>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,22 +170,30 @@ async void InitializeAsync()
};
}

//Initialize the CoreWebView2 component otherwise we can't navigate to a web page
await documentationBrowser.EnsureCoreWebView2Async();

this.documentationBrowser.CoreWebView2.WebMessageReceived += CoreWebView2OnWebMessageReceived;
comScriptingObject = new ScriptingObject(this.viewModel);
//register the interop object into the browser.
this.documentationBrowser.CoreWebView2.AddHostObjectToScript("bridge", comScriptingObject);

this.documentationBrowser.CoreWebView2.Settings.IsZoomControlEnabled = true;
this.documentationBrowser.CoreWebView2.Settings.AreDevToolsEnabled = true;

initState = AsyncMethodState.Done;
try
{
//Initialize the CoreWebView2 component otherwise we can't navigate to a web page
await documentationBrowser.Initialize(Log);

this.documentationBrowser.CoreWebView2.WebMessageReceived += CoreWebView2OnWebMessageReceived;
comScriptingObject = new ScriptingObject(this.viewModel);
//register the interop object into the browser.
this.documentationBrowser.CoreWebView2.AddHostObjectToScript("bridge", comScriptingObject);

this.documentationBrowser.CoreWebView2.Settings.IsZoomControlEnabled = true;
this.documentationBrowser.CoreWebView2.Settings.AreDevToolsEnabled = true;

initState = AsyncMethodState.Done;
}
catch(ObjectDisposedException ex)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be catching other exceptions?

Copy link
Contributor Author

@pinzart90 pinzart90 Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

{
Log(ex.Message);
}

}
//if we make it this far, for example to do re-entry to to this method, while we're still
//initializing, don't do anything, just bail.
if(initState == AsyncMethodState.Done)
if (initState == AsyncMethodState.Done)
{
if (Directory.Exists(VirtualFolderPath))
{
Expand All @@ -211,11 +219,6 @@ private void CoreWebView2OnWebMessageReceived(object sender, CoreWebView2WebMess
/// </summary>
public void Dispose()
{
if (initState == AsyncMethodState.Started)
{
Log("DocumentationBrowserView is being disposed but async initialization is still not done");
}

Dispose(true);
GC.SuppressFinalize(this);
}
Expand Down
Loading
Loading