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

Clean COM wrappers leak: Round 2 #4491

Merged
merged 10 commits into from
Nov 18, 2018
13 changes: 7 additions & 6 deletions Rubberduck.API/VBA/Parser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,11 @@
using System.Linq;
using System.Runtime.InteropServices;
using System.Threading;
using Rubberduck.Common;
using Rubberduck.Parsing.ComReflection;
using Rubberduck.Parsing.PreProcessing;
using Rubberduck.Parsing.Rewriter;
using Rubberduck.Parsing.Symbols.DeclarationLoaders;
using Rubberduck.Parsing.VBA;
using Rubberduck.Parsing.Symbols;
using Rubberduck.Parsing.UIContext;
using Rubberduck.Parsing.VBA.ComReferenceLoading;
using Rubberduck.Parsing.VBA.DeclarationCaching;
Expand Down Expand Up @@ -69,10 +67,10 @@ public interface IParserEvents
]
public sealed class Parser : IParser, IDisposable
{
private RubberduckParserState _state;
private SynchronousParseCoordinator _parser;
private IVBE _vbe;
private IVBEEvents _vbeEvents;
private readonly RubberduckParserState _state;
private readonly SynchronousParseCoordinator _parser;
private readonly IVBE _vbe;
private readonly IVBEEvents _vbeEvents;
private readonly IUiDispatcher _dispatcher;
private readonly CancellationTokenSource _tokenSource;

Expand Down Expand Up @@ -235,6 +233,9 @@ public void Dispose()
}

_disposed = true;
_parser?.Dispose();
_vbe?.Dispose();
_tokenSource.Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

why are these after the flicking the _disposed switch? That feels a little wrong

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 thought that was what we were supposed to -- if we throw during the dispose, the pooch is totally screwed anyway? At least subsequent calls would not throw again (but we're still screwed).

}
}
}
2 changes: 1 addition & 1 deletion Rubberduck.CodeAnalysis/QuickFixes/IgnoreOnceQuickFix.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public override void Fix(IInspectionResult result)

int annotationLine;
string codeLine;
var component = _state.ProjectsProvider.Component(result.QualifiedSelection.QualifiedName);
using (var component = _state.ProjectsProvider.Component(result.QualifiedSelection.QualifiedName))
using (var module = component.CodeModule)
{
annotationLine = result.QualifiedSelection.Selection.StartLine;
Expand Down
395 changes: 201 additions & 194 deletions Rubberduck.Core/Common/ExportFormatter.cs

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion Rubberduck.Core/Common/WindowsOperatingSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ public void ShowFolder(string folderPath)
{
Directory.CreateDirectory(folderPath);
}
Process.Start(folderPath);

using (Process.Start(folderPath)) { }
}
}
}
13 changes: 8 additions & 5 deletions Rubberduck.Core/UI/CodeExplorer/Commands/AddMDIFormCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,17 @@ private bool EvaluateCanExecuteCore(IVBProject project, CodeExplorerItemViewMode
return false;
}

foreach (var component in project.VBComponents)
using (var components = project.VBComponents)
{
using (component)
foreach (var component in components)
{
if (component.Type == ComponentType.MDIForm)
using (component)
{
// Only one MDI Form allowed per project
return false;
if (component.Type == ComponentType.MDIForm)
{
// Only one MDI Form allowed per project
return false;
}
}
}
}
Expand Down
26 changes: 14 additions & 12 deletions Rubberduck.Core/UI/CodeExplorer/Commands/CopyResultsCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,29 +22,31 @@ public CopyResultsCommand(RubberduckParserState state) : base(LogManager.GetCurr

protected override void OnExecute(object parameter)
{
const string XML_SPREADSHEET_DATA_FORMAT = "XML Spreadsheet";
const string XML_SPREADSHEET_DATA_FORMAT = "XML Spreadsheet";

ColumnInfo[] ColumnInfos = { new ColumnInfo("Project"), new ColumnInfo("Folder"), new ColumnInfo("Component"), new ColumnInfo("Declaration Type"), new ColumnInfo("Scope"),
new ColumnInfo("Name"), new ColumnInfo("Return Type") };
ColumnInfo[] ColumnInfos = { new ColumnInfo("Project"), new ColumnInfo("Folder"), new ColumnInfo("Component"), new ColumnInfo("Declaration Type"), new ColumnInfo("Scope"),
new ColumnInfo("Name"), new ColumnInfo("Return Type") };

// this.ProjectName, this.CustomFolder, this.ComponentName, this.DeclarationType.ToString(), this.Scope
var aDeclarations = _state.AllUserDeclarations.Select(declaration => declaration.ToArray()).ToArray();
// this.ProjectName, this.CustomFolder, this.ComponentName, this.DeclarationType.ToString(), this.Scope
var aDeclarations = _state.AllUserDeclarations.Select(declaration => declaration.ToArray()).ToArray();

const string resource = "Rubberduck User Declarations - {0}";
var title = string.Format(resource, DateTime.Now.ToString(CultureInfo.InvariantCulture));
var csvResults = ExportFormatter.Csv(aDeclarations, title, ColumnInfos);
var htmlResults = ExportFormatter.HtmlClipboardFragment(aDeclarations, title, ColumnInfos);
var rtfResults = ExportFormatter.RTF(aDeclarations, title);
const string resource = "Rubberduck User Declarations - {0}";
var title = string.Format(resource, DateTime.Now.ToString(CultureInfo.InvariantCulture));

var csvResults = ExportFormatter.Csv(aDeclarations, title, ColumnInfos);
var htmlResults = ExportFormatter.HtmlClipboardFragment(aDeclarations, title, ColumnInfos);
var rtfResults = ExportFormatter.RTF(aDeclarations, title);

var strm1 = ExportFormatter.XmlSpreadsheetNew(aDeclarations, title, ColumnInfos);
using (var strm1 = ExportFormatter.XmlSpreadsheetNew(aDeclarations, title, ColumnInfos))
{
//Add the formats from richest formatting to least formatting
_clipboard.AppendStream(DataFormats.GetDataFormat(XML_SPREADSHEET_DATA_FORMAT).Name, strm1);
_clipboard.AppendString(DataFormats.Rtf, rtfResults);
_clipboard.AppendString(DataFormats.Html, htmlResults);
_clipboard.AppendString(DataFormats.CommaSeparatedValue, csvResults);

_clipboard.Flush();
}
}
}
}
24 changes: 14 additions & 10 deletions Rubberduck.Core/UI/CodeExplorer/Commands/PrintCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,23 @@ protected override void OnExecute(object parameter)
{
while (index < text.Count)
{
var font = new Font(new FontFamily("Consolas"), 10, FontStyle.Regular);
printPageArgs.Graphics.DrawString(text[index++], font, Brushes.Black, 0, offsetY, new StringFormat());
using (var font = new Font(new FontFamily("Consolas"), 10, FontStyle.Regular))
bclothier marked this conversation as resolved.
Show resolved Hide resolved
using (var stringFormat = new StringFormat())
{
printPageArgs.Graphics.DrawString(text[index++], font, Brushes.Black, 0, offsetY,
stringFormat);

offsetY += font.Height;
offsetY += font.Height;

if (offsetY >= pageHeight)
{
printPageArgs.HasMorePages = true;
offsetY = 0;
return;
}
if (offsetY >= pageHeight)
{
printPageArgs.HasMorePages = true;
offsetY = 0;
return;
}

printPageArgs.HasMorePages = false;
printPageArgs.HasMorePages = false;
}
}
};

Expand Down
25 changes: 20 additions & 5 deletions Rubberduck.Core/UI/Command/ExportAllCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,27 @@ private bool Evaluate(IVBProject project)

protected override void OnExecute(object parameter)
{
var projectNode = parameter as CodeExplorerProjectViewModel;

var vbproject = parameter as IVBProject;
switch (parameter)
{
case CodeExplorerProjectViewModel projectNode when projectNode.Declaration.Project != null:
Export(projectNode.Declaration.Project);
break;
case IVBProject vbproject:
Export(vbproject);
break;
default:
{
using (var project = _vbe.ActiveVBProject)
bclothier marked this conversation as resolved.
Show resolved Hide resolved
{
Export(project);
}
break;
bclothier marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

var project = projectNode?.Declaration.Project ?? vbproject ?? _vbe.ActiveVBProject;
private void Export(IVBProject project)
{
var desc = string.Format(RubberduckUI.ExportAllCommand_SaveAsDialog_Title, project.Name);

// If .GetDirectoryName is passed an empty string for a RootFolder,
Expand Down
51 changes: 33 additions & 18 deletions Rubberduck.Core/UI/Command/FindAllReferencesCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -224,32 +224,47 @@ private Declaration FindFormDesignerTarget(QualifiedModuleName? qualifiedModuleN
{
projectId = activeProject.ProjectId;
}
var component = _vbe.SelectedVBComponent;

if (component?.HasDesigner ?? false)
using (var component = _vbe.SelectedVBComponent)
{
DeclarationType selectedType;
string selectedName;
using (var selectedControls = component.SelectedControls)
if (component?.HasDesigner ?? false)
{
var selectedCount = selectedControls.Count;
if (selectedCount > 1)
DeclarationType selectedType;
string selectedName;
using (var selectedControls = component.SelectedControls)
{
return null;
var selectedCount = selectedControls.Count;
if (selectedCount > 1)
{
return null;
}

(selectedType, selectedName) = GetSelectedName(component, selectedControls, selectedCount);
}

// Cannot use DeclarationType.UserForm, parser only assigns UserForms the ClassModule flag
(selectedType, selectedName) = selectedCount == 0
? (DeclarationType.ClassModule, component.Name)
: (DeclarationType.Control, selectedControls[0].Name);
return _state.DeclarationFinder
.MatchName(selectedName)
.SingleOrDefault(m => m.ProjectId == projectId
&& m.DeclarationType.HasFlag(selectedType)
&& m.ComponentName == component.Name);
}
return _state.DeclarationFinder
.MatchName(selectedName)
.SingleOrDefault(m => m.ProjectId == projectId
&& m.DeclarationType.HasFlag(selectedType)
&& m.ComponentName == component.Name);

return null;
}
}

private static (DeclarationType, string Name) GetSelectedName(IVBComponent component, IControls selectedControls, int selectedCount)
{
// Cannot use DeclarationType.UserForm, parser only assigns UserForms the ClassModule flag
if (selectedCount == 0)
{
return (DeclarationType.ClassModule, component.Name);
}

using (var firstSelectedControl = selectedControls[0])
{
return (DeclarationType.Control, firstSelectedControl.Name);
}
return null;
}

private Declaration FindFormDesignerTarget(QualifiedModuleName qualifiedModuleName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,27 @@ public void EvaluateCanExecute(RubberduckParserState state)
}
}

public ICommandBars Parent { get; set; }
public ICommandBar Item { get; private set; }
private ICommandBars _parent;
public ICommandBars Parent
{
get => _parent;
set
{
_parent?.Dispose();
_parent = value;
}
}

private ICommandBar _item;
public ICommandBar Item
{
get => _item;
private set
{
_item?.Dispose();
_item = value;
}
}

public void RemoveCommandBar()
{
Expand All @@ -181,9 +200,7 @@ public void RemoveCommandBar()
Logger.Debug("Removing commandbar.");
RemoveChildren();
Item.Delete();
Item.Dispose();
Item = null;
Parent?.Dispose();
Parent = null;
Copy link
Member

Choose a reason for hiding this comment

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

Could we get a comment here that nulling them out also disposes them? That helps me not remove these lines

}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,27 @@ protected ParentMenuItemBase(string key, IEnumerable<IMenuItem> items, int? befo
_items = items.ToDictionary(item => item, item => null as ICommandBarControl);
}

public ICommandBarControls Parent { get; set; }
public ICommandBarPopup Item { get; private set; }
private ICommandBarControls _parent;
public ICommandBarControls Parent
{
get => _parent;
set
{
_parent?.Dispose();
_parent = value;
}
}

private ICommandBarPopup _item;
public ICommandBarPopup Item
{
get => _item;
private set
{
_item?.Dispose();
_item = value;
}
}

public string Key => Item?.Tag;

Expand Down Expand Up @@ -91,7 +110,6 @@ public void RemoveMenu()
Logger.Debug($"Removing menu {_key}.");
RemoveChildren();
Item?.Delete();
Item?.Dispose();
Item = null;
Copy link
Member

Choose a reason for hiding this comment

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

same as above re: the comment. Why is the Parent not disposed / nulled here?

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ protected override void OnExecute(object parameter)
}
else
{
target = _state.FindSelectedDeclaration(Vbe.ActiveCodePane);
target = _state.FindSelectedDeclaration(activePane);
}
}

Expand Down
Loading