Consolidate IStream code

[hughbe]

Description:

• Chose to match corefx interop rather than ComTypes due to the poor performance of the System definition
• This PR adopts the definition of IStream found in corefx which has been optimised for performance and correctness, especially surrounding optional parameters.
• It also separates out the code for various Ole32 interfaces from the monolithic layout of UnsafeNativeMethods.cs. In this process, some bugs were found in the COM interop for these classes that were fixed.

Customer Impact:

Improved performance and no crashes if the user passes null parameters to IStream methods.

Regression:

None

Risk:

Possible incorrect definitions of interface/interop methods. These are tested with unit tests - any incorrectness would throw EntryPointNotFound exceptions and break tests.

Microsoft Reviewers: Open in CodeFlow

[hughbe]

This is all taken from https://github.com/dotnet/corefx/tree/master/src/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs which is basically a more performant version of the previous class

[codecov]

Codecov Report

Merging #1481 into master will increase coverage by 0.03265%.
The diff coverage is 20.99644%.

@@                 Coverage Diff                 @@
##              master       #1481         +/-   ##
===================================================
+ Coverage   25.79151%   25.82417%   +0.03266%     
===================================================
  Files            802         804          +2     
  Lines         268189      268148         -41     
  Branches       37965       37968          +3     
===================================================
+ Hits           69170       69247         +77     
+ Misses        194099      193980        -119     
- Partials        4920        4921          +1

Flag	Coverage Δ
#Debug	25.82417% <20.99644%> (+0.03265%)	⬆️
#production	25.82417% <20.99644%> (+0.03265%)	⬆️
#test	?

[JeremyKuhne]

Thanks for pulling my impl over! You beat me to it. :) It is wayyy faster than the current code.

We should clean up the interface definitions and do some basic cleanup and auditing of them as we bring them over into new files. I've left comments to the main things we should be looking for.

Pay particular attention to anything that takes a pointer. I've seen a number of cases where foo* is defined as int in the interfaces. :( Note that I find it much easier to audit these things by having an unmanaged project to search in the Solution Explorer for definitions. You can clone my Interop project for a simple one to do searches in. It can be a little easier to find the right definition by searching for IID, so IID_IStream for IStream. The interface def is typically right below it.

When we get into v5 I'll help fix some of these further and go into more detail where they're not being done efficiently. In general they're not in great shape, with a lot of things being defined incorrectly.

[nsivov]

It looks like this would fix behavior regarding optional output arguments, in Read/Write/CopyTo/Seek. Currently this causing problems with imagelist resource loading, when comctl32.dll:ImageList_Read() is used with managed IStream wrapper, see [1]. Looking forward for this getting merged.

[1] https://bugs.winehq.org/show_bug.cgi?id=47561

[hughbe]

Awesome - we love unintended good consequences! We should also do an audit of other interfaces to see if there's other problems

[merriemcgaw]

This needs to go through Tell-Mode process. Please fill out the template below:
**Description

Customer Impact

Regression?

Risk**

[merriemcgaw]

Let's add the Tell-Mode label when we're all approved and ready to merge so I can discuss in shiproom/tactics.

[JeremyKuhne]

There a couple of broken API defs, but mostly nits. The definitions that are broken have always been broken- @hughbe didn't introduce any problems. We were lucky that the existing definitions mostly worked or perhaps unlucky that we didn't fail hard all the time on the bad defs.

I think it would be better to yank the [MarshalAs] for bool to BOOL and just pass Interop.BOOL for clarity. Given that there is one bool defined wrong, it is probably worth making that change to the code here.

@hughbe, thanks a ton for cleaning the existing stuff up. There are a lot of problems in the existing COM imports, it's exciting to see them get fixed up!

[JeremyKuhne]

I did another line-by-line review of the imports and found a few other existing problems. Outside of that my comments are all nits.

[hughbe]

@JeremyKuhne how do we look now ! :D

[RussKie]

Thank you gentlemen