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

fix more memory leaks when starting and closing DynamoRevit #14366

Merged
merged 3 commits into from
Sep 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -109,24 +109,20 @@ protected virtual void Dispose(bool disposing)
// Cleanup
this.viewModel.LinkChanged -= NavigateToPage;
this.documentationBrowser.NavigationStarting -= ShouldAllowNavigation;
this.documentationBrowser.DpiChanged -= DocumentationBrowser_DpiChanged;

if (this.documentationBrowser.CoreWebView2 != null)
{
this.documentationBrowser.CoreWebView2.WebMessageReceived -= CoreWebView2OnWebMessageReceived;
}

// Note to test writers
// Disposing the document browser will cause future tests
// that uses the Browser component to crash
if (!Models.DynamoModel.IsTestMode)
{
this.documentationBrowser.Dispose();
}
this.documentationBrowser.DpiChanged -= DocumentationBrowser_DpiChanged;
try
{
if (this.documentationBrowser.CoreWebView2 != null)
this.documentationBrowser.CoreWebView2.WebMessageReceived -= CoreWebView2OnWebMessageReceived;

}
catch (Exception)
{
return;
}
}

async void InitializeAsync()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,8 @@ protected virtual void Dispose(bool disposing)
{
this.pmExtension.PackageLoader.PackgeLoaded -= OnPackageLoaded;
}

PackageDocumentationManager.Instance.MessageLogged -= OnMessageLogged;
PackageDocumentationManager.Instance.Dispose();
}

Expand Down
15 changes: 12 additions & 3 deletions src/DynamoCore/Models/DynamoModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -873,8 +873,8 @@ protected DynamoModel(IStartConfiguration config)
if (!IsServiceMode)
{
SearchModel = new NodeSearchModel(Logger);
SearchModel.ItemProduced +=
node => ExecuteCommand(new CreateNodeCommand(node, 0, 0, true, true));
SearchModel.ItemProduced += SearchModel_ItemProduced;

}

NodeFactory = new NodeFactory();
Expand Down Expand Up @@ -1021,6 +1021,11 @@ protected DynamoModel(IStartConfiguration config)
DynamoReady(new ReadyParams(this));
}

private void SearchModel_ItemProduced(NodeModel node)
{
ExecuteCommand(new CreateNodeCommand(node, 0, 0, true, true));
}

/// <summary>
/// It returns a PathManager instance based on the mode in order to reuse it's Singleton instance or create a new one
/// </summary>
Expand Down Expand Up @@ -1476,6 +1481,10 @@ public void Dispose()
FeatureFlags.MessageLogged -= LogMessageWrapper;
}
DynamoUtilities.DynamoFeatureFlagsManager.FlagsRetrieved -= CheckFeatureFlagTest;
if (!IsServiceMode)
{
SearchModel.ItemProduced -= SearchModel_ItemProduced;
}
}

private void InitializeCustomNodeManager()
Expand Down Expand Up @@ -2886,7 +2895,7 @@ public void RemoveWorkspace(WorkspaceModel workspace)
{
OnWorkspaceRemoveStarted(workspace);
if (_workspaces.Remove(workspace))
{
{
if (workspace is HomeWorkspaceModel)
{
workspace.Dispose();
Expand Down
18 changes: 15 additions & 3 deletions src/DynamoCoreWpf/Controls/NodeAutoCompleteSearchControl.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ namespace Dynamo.UI.Controls
/// Notice this control shares a lot of logic with InCanvasSearchControl for now
/// But they will diverge eventually because of UI improvements to auto complete.
/// </summary>
public partial class NodeAutoCompleteSearchControl
public partial class NodeAutoCompleteSearchControl : IDisposable
{
ListBoxItem HighlightedItem;

Expand All @@ -45,7 +45,10 @@ public NodeAutoCompleteSearchControl()
if (Application.Current != null)
{
Application.Current.Deactivated += CurrentApplicationDeactivated;
Application.Current.MainWindow.Closing += NodeAutoCompleteSearchControl_Unloaded;
if (Application.Current.MainWindow != null)
{
Application.Current.MainWindow.Closing += NodeAutoCompleteSearchControl_Unloaded;
Copy link
Contributor

Choose a reason for hiding this comment

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

}
}
HomeWorkspaceModel.WorkspaceClosed += this.CloseAutoCompletion;
}
Expand All @@ -55,8 +58,12 @@ private void NodeAutoCompleteSearchControl_Unloaded(object sender, System.Compon
if (Application.Current != null)
{
Application.Current.Deactivated -= CurrentApplicationDeactivated;
Application.Current.MainWindow.Closing -= NodeAutoCompleteSearchControl_Unloaded;
if (Application.Current.MainWindow != null)
{
Application.Current.MainWindow.Closing -= NodeAutoCompleteSearchControl_Unloaded;
}
}
HomeWorkspaceModel.WorkspaceClosed -= this.CloseAutoCompletion;
}

private void CurrentApplicationDeactivated(object sender, EventArgs e)
Expand Down Expand Up @@ -388,5 +395,10 @@ private void OnSuggestion_Click(object sender, RoutedEventArgs e)
}
ViewModel.PopulateAutoCompleteCandidates();
}

public void Dispose()
{
NodeAutoCompleteSearchControl_Unloaded(this,null);
}
}
}
16 changes: 13 additions & 3 deletions src/DynamoCoreWpf/Extensions/ViewExtensionCommandExecutive.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using Dynamo.Extensions;
using Dynamo.Logging;
using Dynamo.Models;
Expand All @@ -7,14 +7,19 @@

namespace Dynamo.Wpf.Extensions
{
internal class ViewExtensionCommandExecutive : ICommandExecutive
internal class ViewExtensionCommandExecutive : ICommandExecutive,IDisposable
{
private readonly DynamoViewModel dynamoViewModel;

public ViewExtensionCommandExecutive(DynamoViewModel model)
{
dynamoViewModel = model;
MessageLogged += (message) => { dynamoViewModel.Model.Logger.Log(message); };
MessageLogged += ViewExtensionCommandExecutive_MessageLogged;
}

private void ViewExtensionCommandExecutive_MessageLogged(ILogMessage message)
{
dynamoViewModel.Model.Logger.Log(message);
}

public void ExecuteCommand(DynamoModel.RecordableCommand command, string uniqueId, string extensionName)
Expand Down Expand Up @@ -42,5 +47,10 @@ private void Log(ILogMessage obj)
var handler = MessageLogged;
if (handler != null) handler(obj);
}

public void Dispose()
{
MessageLogged -= ViewExtensionCommandExecutive_MessageLogged;
}
}
}
13 changes: 10 additions & 3 deletions src/DynamoCoreWpf/Extensions/ViewLoadedParams.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Specialized;
using System.ComponentModel;
using System.Linq;
Expand Down Expand Up @@ -120,7 +120,7 @@ public void AddExtensionMenuItem(MenuItem menuItem)
/// <returns></returns>
public void AddToExtensionsSideBar(IViewExtension viewExtension, ContentControl contentControl)
{
bool added = dynamoView.AddOrFocusExtensionControl(viewExtension, contentControl);
bool added = dynamoView.AddOrFocusExtensionControl(viewExtension, contentControl);

if (added)
{
Expand Down Expand Up @@ -223,7 +223,7 @@ public void OpenViewExtension(string extensionName)
{
dynamoViewModel.OnViewExtensionOpenRequest(extensionName);
}

/// <summary>
/// Event raised when a component inside Dynamo raises a request to open a view extension.
/// </summary>
Expand All @@ -247,6 +247,13 @@ public event Action<string, object> ViewExtensionOpenRequestWithParameter
remove => dynamoViewModel.ViewExtensionOpenWithParameterRequest -= value;
}

public new void Dispose()
{
if (CommandExecutive is IDisposable disposable)
{ disposable.Dispose(); }
base.Dispose();
}

}
/// <summary>
/// An enum that represents the different possible
Expand Down
4 changes: 4 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3421,6 +3421,10 @@ public bool PerformShutdownSequence(ShutdownParams shutdownParams)


BackgroundPreviewViewModel.Dispose();
foreach (var wsvm in workspaces)
{
wsvm.Dispose();
}
MainGuideManager?.CloseRealTimeInfoWindow();

model.ShutDown(shutdownParams.ShutdownHost);
Expand Down
14 changes: 8 additions & 6 deletions src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ public override void Dispose()
{
cate.DisposeTree();
}
Model.EntryAdded += AddEntry;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be -=?

Model.EntryUpdated -= UpdateEntry;
Model.EntryRemoved -= RemoveEntry;

Expand All @@ -425,12 +426,7 @@ private void InitializeCore()
searchIconAlignment = System.Windows.HorizontalAlignment.Left;

// When Library changes, sync up
Model.EntryAdded += entry =>
{
InsertEntry(MakeNodeSearchElementVM(entry), entry.Categories);
RaisePropertyChanged("BrowserRootCategories");
};

Model.EntryAdded += AddEntry;
Model.EntryUpdated += UpdateEntry;
Model.EntryRemoved += RemoveEntry;

Expand All @@ -440,6 +436,12 @@ private void InitializeCore()
InsertClassesIntoTree(LibraryRootCategories);
}

private void AddEntry(NodeSearchElement entry)
{
InsertEntry(MakeNodeSearchElementVM(entry), entry.Categories);
RaisePropertyChanged("BrowserRootCategories");
}

private IEnumerable<RootNodeCategoryViewModel> CategorizeEntries(IEnumerable<NodeSearchElement> entries, bool expanded)
{
var tempRoot = entries.GroupByRecursive<NodeSearchElement, string, NodeCategoryViewModel>(
Expand Down
6 changes: 6 additions & 0 deletions src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2892,7 +2892,13 @@ public void Dispose()
dynamoViewModel.SideBarTabItems.CollectionChanged -= this.OnCollectionChanged;

if (fileTrustWarningPopup != null)
{
fileTrustWarningPopup.CleanPopup();
}
//TODO code smell.
var workspaceView = this.ChildOfType<WorkspaceView>();
workspaceView?.Dispose();
(workspaceView?.NodeAutoCompleteSearchBar?.Child as IDisposable)?.Dispose();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to dispose this specific control?

Copy link
Member Author

Choose a reason for hiding this comment

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

the NodeAutoCompleteSearchControl has a dispose method which does not seem to be called from anywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor point but how about disposing it inside the workspaceview's dispose method - would be cleaner.

}
}
}
7 changes: 6 additions & 1 deletion src/DynamoCoreWpf/Views/Core/WorkspaceView.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ namespace Dynamo.Views
/// <summary>
/// Interaction logic for WorkspaceView.xaml
/// </summary>
public partial class WorkspaceView
public partial class WorkspaceView: IDisposable
{
public enum CursorState
{
Expand Down Expand Up @@ -1134,5 +1134,10 @@ private void OnGeometryScaling_Click(object sender, RoutedEventArgs e)
}
GeoScalingPopup.IsOpen = true;
}

public void Dispose()
{
RemoveViewModelsubscriptions(ViewModel);
}
}
}
14 changes: 12 additions & 2 deletions src/DynamoCoreWpf/Views/SplashScreen/SplashScreen.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ public DynamoView DynamoView
set
{
dynamoView = value;
if(dynamoView == null)
{
return;
}
viewModel = value.DataContext as DynamoViewModel;
// When view model is closed, we need to close the splash screen if it is displayed.
viewModel.RequestClose += SplashScreenRequestClose;
Expand Down Expand Up @@ -484,8 +488,14 @@ internal void CloseWindow()
// RequestUpdateLoadBarStatus event needs to be unsubscribed when the splash screen window is closed, to avoid populating the info on splash screen.
else
{
this.Close();
viewModel?.Model.ShutDown(false);
Close();
if (viewModel != null)
{
viewModel.RequestClose -= SplashScreenRequestClose;
}

DynamoView?.Close();
DynamoView = null;
}
}

Expand Down
9 changes: 8 additions & 1 deletion src/DynamoManipulation/DynamoManipulationExtension.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.ComponentModel;
Expand Down Expand Up @@ -174,6 +174,13 @@ private void UnregisterEventHandlers()
BackgroundPreviewViewModel.CanNavigateBackgroundPropertyChanged -= Watch3DViewModelNavigateBackgroundPropertyChanged;
BackgroundPreviewViewModel.ViewMouseDown -= Watch3DViewModelOnViewMouseDown;
}

var settings = GetRunSettings(WorkspaceModel);
if (settings != null)
{
settings.PropertyChanged -= OnRunSettingsPropertyChanged;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does need to be unsubscribed here?

Copy link
Member Author

@mjkkirschner mjkkirschner Sep 6, 2023

Choose a reason for hiding this comment

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

Because when Dynamo is shutdown the SetWorkspaceModel call above is not called. So we do it somewhere in the shutdown/dispose hook path of this extension.

In a context like Revit where Dynamo can be shutdown and started over and over, this leads to leaks. In Sandbox it's not an issue.

}

}

private void Watch3DViewModelOnViewMouseDown(object o, MouseButtonEventArgs mouseButtonEventArgs)
Expand Down
10 changes: 6 additions & 4 deletions src/GraphMetadataViewExtension/GraphMetadataViewExtension.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,12 @@ public override void Closed()

protected virtual void Dispose(bool disposing)
{
viewModel.Dispose();

this.graphMetadataMenuItem.Checked -= MenuItemCheckHandler;
this.graphMetadataMenuItem.Unchecked -= MenuItemUnCheckedHandler;
viewModel?.Dispose();
if (graphMetadataMenuItem != null)
{
graphMetadataMenuItem.Checked -= MenuItemCheckHandler;
graphMetadataMenuItem.Unchecked -= MenuItemUnCheckedHandler;
}
}

public override void Dispose()
Expand Down
3 changes: 2 additions & 1 deletion src/GraphMetadataViewExtension/GraphMetadataViewModel.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.ObjectModel;
using System.IO;
using System.Windows.Media.Imaging;
Expand Down Expand Up @@ -266,6 +266,7 @@ private void MarkCurrentWorkspaceModified()
public void Dispose()
{
this.viewLoadedParams.CurrentWorkspaceChanged -= OnCurrentWorkspaceChanged;
this.viewLoadedParams.CurrentWorkspaceCleared += OnCurrentWorkspaceChanged;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure it's not -=?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, it's probably a copy paste error, but I will double check and add it to #14379

if (linterManager != null)
{
linterManager.PropertyChanged -= OnLinterManagerPropertyChange;
Expand Down
5 changes: 4 additions & 1 deletion src/LibraryViewExtensionWebView2/LibraryViewController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,6 @@ public void Dispose()
protected void Dispose(bool disposing)
{
if (!disposing) return;

if (observer != null) observer.Dispose();
observer = null;
if (this.dynamoWindow != null)
Expand All @@ -636,11 +635,15 @@ protected void Dispose(bool disposing)
}
if (this.browser != null)
{
browser.CoreWebView2.RemoveHostObjectFromScript("bridgeTwoWay");
browser.SizeChanged -= Browser_SizeChanged;
browser.Loaded -= Browser_Loaded;
browser.Dispose();
browser = null;
}
twoWayScriptingObject.Dispose();
dynamoViewModel = null;
commandExecutive = null;
}

public static async Task<string> ExecuteScriptFunctionAsync(WebView2 webView2, string functionName, params object[] parameters)
Expand Down
Loading