Don't crash in OOP with TS projects/documents #20435

Pilchie · 2017-06-23T22:37:48Z

Ask Mode

Customer scenario

Enable the option to perform analysis in an external process, and then either:

search in solution explorer with a JS or TS file open in the editor.
Put the caret on a C# identifier in a cshtml file that also contains a JS script block.

Bugs this fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/454792 and https://devdiv.visualstudio.com/DevDiv/_workitems/edit/455588
Workarounds, if any: Disable the option to run these features out of process, but we'd really like to get feedback on the option to enable it by default later.
Risk: Low - just some checks specifically to prevent these cases, in the OOP specific codepaths.
Performance impact: Low - just some if checks.
Is this a regression from a previous update? Yes, if the OOP option is enabled.
Root cause analysis: Once again, we need to do more testing with JS/TS and razor scenarios.
How was the bug found? Divisional automated tests immediately after enabling the option for internal users, and internal dogfooders.

Pilchie · 2017-06-23T23:23:49Z

I recommend reviewing with ?w=1 for this one.

heejaechang · 2017-06-23T23:27:50Z

src/Workspaces/Remote/ServiceHub/Services/CodeAnalysisService_DocumentHighlights.cs

@@ -16,7 +16,8 @@ internal partial class CodeAnalysisService : IRemoteDocumentHighlights
        {
            var solution = await GetSolutionAsync().ConfigureAwait(false);
            var document = solution.GetDocument(documentId);
-            var documentsToSearch = ImmutableHashSet.CreateRange(documentIdsToSearch.Select(solution.GetDocument));
+            var documentsToSearch = ImmutableHashSet.CreateRange(


removing this to let things crash might make bug more explicit. with this, we might not crash and didn't know that some document just silent failed to run HR. same for FAR.

Without this we definitely crash as soon as you edit a .cshtml file with js/ts code in it 😄

See my question in email about how to filter for highlight references.

Add comment explaining the scenario here.

heejaechang · 2017-06-23T23:28:01Z

👍

Pilchie · 2017-06-23T23:30:32Z

Actually - questions from people posted publicly here:

Highlight references – should we filter on the VS side, the remote side, do the whole search on the VS side, do the non-remotable stuff on the VS side and then merge with the remotable stuff from the OOP side if there is any?
Things that operate on the solution. We might get partial results, because only the projects that are supported OOP will return results. It looks like there is at least one “FindDeclarations” API that will fall into this bucket.

heejaechang · 2017-06-23T23:32:07Z

All depends.

Not all language support OOP. So if a feature needs to run on 1 specfic language, then doing filtering on client side seems right.

If a feature needs to run on whole solution regardless of language, then the feature should run both and merge them.

heejae

CyrusNajmabadi · 2017-06-24T04:27:55Z

src/Features/Core/Portable/AddImport/AbstractAddImportFeatureService.cs

@@ -67,7 +67,7 @@ protected AbstractAddImportFeatureService()

            using (session)
            {
-                if (session == null)
+                if (session == null || !RemoteSupportedLanguages.IsSupported(document.Project.Language))


How would this happen? AbstractAddImportFeatureService is only derived with C# and VB.

I would add a comment saying we're being paranoid here.

CyrusNajmabadi · 2017-06-24T04:28:13Z

src/Features/Core/Portable/NavigateTo/AbstractNavigateToSearchService.Remote.cs

@@ -31,6 +31,11 @@ internal abstract partial class AbstractNavigateToSearchService

        private static Task<RemoteHostClient.Session> GetRemoteHostSessionAsync(Project project, CancellationToken cancellationToken)
        {
+            if (!RemoteSupportedLanguages.IsSupported(project.Language))


How does this happen. AbstractNavitateToSearchService is only instantiated for C# and VB.

I would add a comment saying we are being paranoid here.

CyrusNajmabadi · 2017-06-24T04:29:44Z

I don't understand why this change is needed at all. Several of these services are C#/VB only. And so they should only be getting C#/VB documents. As such, i don't see why they'd have a problem with remoting things.

Pilchie · 2017-06-26T16:41:02Z

@CyrusNajmabadi It's quite possible not all of these are needed - I did a bunch of this via code inspection.

The two known instances so far are:

A crash in highlight references in a cshtml file that contains both TS and C# - documentsToSearch ends up with nulls in it because it can't re-hydrate the IDs for the TS documents on the OOP side.
A crash in solution explorer search in a solution that contains TS file - the projectId to search doesn't exist in the remote workspace.

Basically, I think we need to carefully audit, and hopefully provide some safety guarantees for the future about how to guard features that work OOP for C# and VB when they get mixed with languages that don't support OOP.

I'm definitely open to suggestions for better ways to do that.

CyrusNajmabadi · 2017-06-26T16:59:14Z

Interesting. My primary concern here is that we are actually breaking functionality. i mean, take:

A crash in solution explorer search in a solution that contains TS file - the projectId to search doesn't exist in the remote workspace.

How was this working before?

--

My ideal would be that the remote workspace has all the project/documents/sourcetexts. It might be that it doesn't have all the services. But we would have had to be resilient to that before. Else, we would have crashed. And if the services are local, and not on the remote, then crashing indicates a degredation in functionality as those services used to work before we pushed things remotely.

CyrusNajmabadi · 2017-06-26T17:23:19Z

So i'm ok with this. THough i would like the code to maybe be commented to explain things.

CyrusNajmabadi · 2017-06-26T17:24:43Z

src/Workspaces/Core/Portable/FindSymbols/Declarations/DeclarationFinder_AllDeclarations.cs

@@ -93,19 +93,22 @@ internal static partial class DeclarationFinder
        private static async Task<(bool, ImmutableArray<SymbolAndProjectId>)> TryFindAllDeclarationsWithNormalQueryInRemoteProcessAsync(
            Project project, SearchQuery query, SymbolFilter criteria, CancellationToken cancellationToken)
        {
-            using (var session = await SymbolFinder.TryGetRemoteSessionAsync(
-                project.Solution, cancellationToken).ConfigureAwait(false))
+            if (RemoteSupportedLanguages.IsSupported(project.Language))


this check seems potentially appropriate. we should comment that this can be called through something like SolutionExplorer and that if this is not C#/VB that the search really can't happen at all (either on the host or remote side).

Strangely, nothing seems to protect against non-C#/VB projects in the in-proc case here.

Looking further - this method seems to only be reachable by tests and public API. We don't use it anywhere, but if anyone else uses those public entry points with a TS/Xaml/F# project they will blow up :(

Going to debug again and figure out how this works when it's in-proc (since I verified that it did...)

Yuck - it only works in the local process because Project.ContainsSymbolsWithNameAsync return false for TS projects before we happen to get a Compilation. We should probably move these checks higher up, and make it clearer that they don't work for TS/F#/Xaml in the API docs.

Filed #20573 to follow up on this more.

CyrusNajmabadi · 2017-06-26T17:25:14Z

src/Workspaces/Remote/ServiceHub/Services/CodeAnalysisService_DocumentHighlights.cs

@@ -16,7 +16,8 @@ internal partial class CodeAnalysisService : IRemoteDocumentHighlights
        {
            var solution = await GetSolutionAsync().ConfigureAwait(false);
            var document = solution.GetDocument(documentId);
-            var documentsToSearch = ImmutableHashSet.CreateRange(documentIdsToSearch.Select(solution.GetDocument));
+            var documentsToSearch = ImmutableHashSet.CreateRange(
+                documentIdsToSearch.Select(solution.GetDocument).Where(d => d != null));


Use .WhereNotNull();

CyrusNajmabadi · 2017-06-26T17:25:20Z

src/Workspaces/Remote/ServiceHub/Services/CodeAnalysisService_SymbolFinder.cs

@@ -31,6 +31,7 @@ internal partial class CodeAnalysisService : IRemoteSymbolFinder
                }

                var documents = documentArgs?.Select(solution.GetDocument)
+                                             .Where(d => d != null)


WhereNotNull

I couldn't find WhereNotNull, but maybe it was due to the diagnostic issue Jason was looking at. I'll try again.

CyrusNajmabadi

LGTM with some code comments.

CyrusNajmabadi

Thanks for the comments!

Pilchie · 2017-06-30T17:45:03Z

retest windows_debug_vs-integration_prtest please

Pilchie · 2017-06-30T17:45:27Z

retest windows_release_vs-integration_prtest please

Pilchie · 2017-06-30T17:45:39Z

Both failed with Vsixexpinstaller error.

Pilchie · 2017-06-30T17:45:48Z

Tagging @MattGertz for approval.

dnfclas added the cla-already-signed label Jun 23, 2017

Pilchie force-pushed the VenusOOP branch from a35e01c to 7b4adf9 Compare June 23, 2017 23:20

Pilchie requested review from CyrusNajmabadi and heejaechang June 23, 2017 23:23

Pilchie added the Area-IDE label Jun 23, 2017

Pilchie added this to the 15.3 milestone Jun 23, 2017

heejaechang reviewed Jun 23, 2017

View reviewed changes

CyrusNajmabadi reviewed Jun 24, 2017

View reviewed changes

CyrusNajmabadi reviewed Jun 26, 2017

View reviewed changes

CyrusNajmabadi approved these changes Jun 26, 2017

View reviewed changes

Pilchie added 2 commits June 29, 2017 17:15

Don't crash if the remote side doesn't have a document

99c9434

Don't search remote process for unsupported languages

3ea96c5

Pilchie force-pushed the VenusOOP branch from 7b4adf9 to 3ea96c5 Compare June 30, 2017 00:16

Pilchie changed the base branch from master to dev15.3.x June 30, 2017 00:21

Add comments and use WhereNotNull

4383d35

CyrusNajmabadi approved these changes Jun 30, 2017

View reviewed changes

Pilchie mentioned this pull request Jun 30, 2017

DeclarationFinder APIs should be more explicit about languages that don't support compilations #20573

Closed

MattGertz added Pending director approval Approved to merge and removed Pending director approval labels Jun 30, 2017

Pilchie merged commit 66362df into dotnet:dev15.3.x Jun 30, 2017

Pilchie deleted the VenusOOP branch June 30, 2017 21:13

Don't crash in OOP with TS projects/documents #20435

Don't crash in OOP with TS projects/documents #20435

Conversation

Pilchie commented Jun 23, 2017 • edited Loading

Ask Mode

Pilchie commented Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Jun 23, 2017

Pilchie commented Jun 23, 2017

heejaechang commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jun 24, 2017

Pilchie commented Jun 26, 2017

CyrusNajmabadi commented Jun 26, 2017

CyrusNajmabadi commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 23, 2017 •

edited

Loading

Pilchie commented Jun 23, 2017 •

edited

Loading