Fixes VSTS Bug 958221: System.ArgumentNullException exception in

[mkrueger]

System.Linq.Enumerable.OfTypeDocumentControllerExtension

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/958221

[mrward]

If GetAllExtensions can never be null then I guess this code can be changed:

monodevelop/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.Gui.Documents/DocumentController.cs

Line 675 in d167b82

var allExtensions = (extensionChain.GetAllExtensions () ?? Array.Empty<DocumentControllerExtension>())

[slluis]

extensions can only be null if the extension chain has been disposed, which happens when the document controller is disposed. If we are trying to access a disposed object, the bug is somewhere else.

[slluis]

I'm wondering if MonoDevelopDocumentTrackingService is keeping a reference to a disposed document.

[Therzok]

@slluis there's some other traces, not just MonoDevelopDocumentTrackingService:

System.Linq.Enumerable.OfType[DocumentControllerExtension](IEnumerable)
MonoDevelop.Ide.Gui.Documents.DocumentController.<GetContents>d__XXX.MoveNext()
System.Linq.Enumerable.TryGetFirst[Object](IEnumerable`1,Boolean&)
System.Linq.Enumerable.FirstOrDefault[Object](IEnumerable`1)
MonoDevelop.Ide.Gui.Documents.DocumentController.GetContent(Type)
MonoDevelop.Ide.Gui.Document.<>c__XXX.<GetContentIncludingAllViews>b__XXX(DocumentController)
System.Linq.Enumerable.SelectEnumerableIterator`2[[MonoDevelop.Ide.Gui.Documents.DocumentController, MonoDevelop.Ide, Version=2.6.0.0, Culture=neutral, PublicKeyToken=3ead7498f347467b],[System.Object, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]].MoveNext()
System.Linq.Enumerable.TryGetFirst[Object](IEnumerable`1,Func`2,Boolean&)
System.Linq.Enumerable.FirstOrDefault[Object](IEnumerable`1,Func`2)
MonoDevelop.Ide.Gui.Document.GetContentIncludingAllViews(Type)
MonoDevelop.Ide.Gui.Document.GetContent(Boolean,Type)
MonoDevelop.Ide.Gui.Document.GetContent[TextEditor](Boolean)
MonoDevelop.Ide.Gui.Document.GetContent[TextEditor]()
MonoDevelop.Ide.Gui.Document.get_Editor()
<addin>.ActiveDocumentChanged(Object,EventArgs)
System.CoreExtensions.SafeInvoke[DocumentEventArgs](EventHandler`1,Object,DocumentEventArgs)

[slluis]

@Therzok yes, there may be more than one problem.

This is not right though:

monodevelop/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.RoslynServices/MonoDevelopDocumentTrackingService.cs

Lines 77 to 78 in 50fbe0a

	ActiveDocumentChanged?.Invoke (this, TryGetActiveDocument ());
	activeDocument = e.Document;

activeDocument should be assigned before rising the event.

In any case, caching the active document here is a potential problem. It may happen that TryGetActiveDocument is called before MonoDevelopDocumentTrackingService gets the ActiveDocumentChanged event, in which case it would return a stale document.

[slluis]

Maybe IdeApp.Workbench.ActiveDocument can return a disposed document when the document is closed? worth checking different scenarios.

[Therzok]

Looks like the code was changed compared to the initial caching commit. This commit changed it to be assigned after

[mkrueger]

/fixed the activeDocument assignment - that was wrong.

Y there are different scenarios where that can happen.

[mkrueger]

How about that ? Just giving cutting the GetContent pipe on disposed Documents ?

[sevoku]

@mkrueger tests are failing now

[slluis]

My question still stands. Why is GetContent() being called on a disposed document?

[Therzok]

Honestly, I think this is a fallout from asynchronizing all the document code. Given we can have a situation where:

var doc = await GetDocument();
await SomethingElse(); // User closes the document in the meantime
...
// This is now disposed
DoSomething (doc);

and the user closes the document in the meantime, it can

[slluis]

@Therzok we have an event that is raised when a document is closed, so that case can be avoided. I don't agree this is a problem of asynchronizing code.

[Therzok]

Okay, I think I understand now what's going on, at least for the roslyn stack.

1. The order of assignment did cause most of the bugs.
2. SolutionCrawler runs on a background thread, so it might race with the UI thread disposing the document.

[Therzok]

We also should probably remove the activeDocument from the list when it's closed.

[mkrueger]

No idea what you mean with remove active document from the list.
At that point it's no longer an IDE document at that point it's a roslyn document. And does it really make sense if a document gets closed that it's removed from the crawler ?

Just because the IDE document is closed it's not affecting it's contents. Interesting would just be closing a document with changes and discard the changes on close.

[Therzok]

@mkrueger We need to modify this test and ensure it passes:

monodevelop/main/tests/MonoDevelop.Core.Tests/MonoDevelop.Projects/ExtensionChainTests.cs

Line 137 in 6f21173

Assert.IsNull (chain.GetAllExtensions ());

[Therzok]

Can we split this PR? The roslyn active doc fix is good to merge. Stalling this too long is not really a good idea...

[mkrueger]

In general I don't think that this change is harmful. Even if there is a deeper problem it makes the code more solid.

[Therzok]

I still think Lluis is right, in that it hides a problem, not fixes it.