Fix OOP crashes due to inability to roundtrip error symbols.

[CyrusNajmabadi]

Fixes #14365
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1131811

Core issue here was that we were not serializing enough information to properly rehydrate error symbols between local/OOP. For example, say you had:

The PointCollection.Add(System.Windows.Point) method (where PointCollection is in PresentationCore.dll and Point is in WindowsBase.dll). However, you don't actually have a reference to WindowsBase.dll. The compiler represents this as an error-type for Point, with a containing 'missing namespace' hierarchy for System.Windows.

With OOP we need to be able to round-trip the symbols we're talking about on the client side with the OOP server so that it can then do the searching for it there. In this case we failed to do that because we were encoding System.Windows.Point only as Point, and thus failed to match to the corresponding method on the oop side when we rehydrated things there.

We now properly encode and rehydrate the containing namespaces of errors types ensuring that this works.

While this seems like it would be difficult for customers to hit, ti's actually not uncommon as as dlls get added/removed to roslyn during things like solution-load or restores. So if a OOP request is made during that time, this could trigger.

This was discovered by examining all our fault hits at PerfWatson and seeing that all of them contained the siganture for an error type for them. This allowed me to create a simple repro that immediately triggered the issue locally.

[CyrusNajmabadi]

have verified this test fails in OOP scenarios prior to the fix and passes post the fix.

[jasonmalinowski]

I'm not quite getting why this had to be changed here -- the symbol key written out for the namespace of the error symbol would still have been a valid symbol key, right? Would this also have been fixed if we just had the resolving of the namespace just call CreateErrorNamespaceSymbol if it can't find the original namespace?

[CyrusNajmabadi]

the symbol key written out for the namespace of the error symbol would still have been a valid symbol key, right? Would this also have been fixed if we just had the resolving of the namespace just call CreateErrorNamespaceSymbol if it can't find the original namespace?

1. i went down that path and felt very oogy about that. i.e. i didn't like the idea of just blindly resolving as an ErrorNamespace if we couldnt' find it again. I want that behavior to be explicit only if that's what went in.
2. i also went down a path of actually exposing an IsMissing/IsError bit on INamespaceSymbol so that we could just recurse to the parent and store that bit and then rehydrate the ErrorNamespace on the other end. Indeed, my early commits show that. However, it was much larger in scope, and as i thought about it, i realized it wasn't necessary, so i went with the simpelr appraoch.

[jasonmalinowski]

@CyrusNajmabadi Does this need to target something other than master? I don't quite know what our ship schedule is but if this is dogfood impacting we might have to go to a servicing branch? (I don't know how "bad" this actually is or honestly what our snap schedule is either.)

[CyrusNajmabadi]

@CyrusNajmabadi Does this need to target something other than master? I don't quite know what our ship schedule is but if this is dogfood impacting we might have to go to a servicing branch? (I don't know how "bad" this actually is or honestly what our snap schedule is either.)

Have pinged @jinujoseph about this.

[jasonmalinowski]

The code handling nested namespaces seems to be missing a .Reverse() somewhere. And even if I'm wrong, we may want a test ensuring that.

[jasonmalinowski]

Can we Debug.Assert() that this didn't really do anything, since it should always be null?

[CyrusNajmabadi]

good idea

[jasonmalinowski]

Looks good!

[ghost]

Auto-approval

[jinujoseph]

Let's keep this in master for 16.7.preview3