manually release ITypeInfo references

[weltkante]

Fixes #3305
Relates to #3271

Proposed changes

Release ITypeInfo RCW manually instead of letting the GC clean up

I'm doing this seperately from PR #3276 so it can be reviewed and merged before figuring out all the problems of the package updates.

Customer Impact

Tests should become more stable regardless how they are run

Regression?

Yes (after merging PR #3276)

Risk

no known risk

Before

Tests start failing after PR #3276

After

Tests should succeed before and after PR #3276

Test methodology

Locally running tests both with and without PR #3276

Test environment(s)

local

Microsoft Reviewers: Open in CodeFlow

[weltkante]

/cc @hughbe @RussKie @JeremyKuhne Instead of doing try/finally blocks I experimented with a disposable struct, tell me what you think. If you don't like it I can fall back to implementing this via try/finally blocks.

[hughbe]

Also, fixes #3271

[weltkante]

Also, fixes #3271

#3271 also happens in IPictureTests and I didn't touch those yet. Those tests aren't using ITypeInfo so I'm not sure if #3271 is the same issue. I'd leave it open for now.

(The error in #3271 is not coming from the ITypeInfo instance, but from the IDispatch instance. Adding manual release for the IDispatch objects isn't going to have the same impact as for ITypeInfo because the images themselves are not shared between tests, only their type info is.)

[hughbe]

Cheers thanks.

Just to confirm, is this specific to ITypeInfo? I think we should somehow have tests in dotnet/runtime verifying this behaviour.

Surely others will run into this somehow? Does this happen in netfx

[weltkante]

Is ImageCollection_AddStrip_InvokeWithHandle_Success a new/unknown flaky test? Can't seem to find previews mentions.

[hughbe]

Don’t think so. I have noticed some flakiness surrounding ImageList tests with handles in the past

[weltkante]

Just to confirm, is this specific to ITypeInfo?

Its specific to unmanaged COM objects shared across multiple STA threads (ITypeInfo happens to be shared on the native side according to your linked discussion)

Surely others will run into this somehow? Does this happen in netfx

You need to have mutliple STA threads starting up/shutting down, this is not a common scenario, and if you have it you run into a lot of other problems (for example SystemEvents not dealing gracefully with it, WinForms itself doesn't deal gracefully with it, WPF has a few issues posted). From what I can tell having multiple STA threads starting and terminating needs more work across the whole framework before it can work reliably. And I don't think the old COM interop is going to get any fixes in this area.

I think we should somehow have tests in dotnet/runtime verifying this behaviour.

The best you could do is verify that the bug still exists? I think the point is that the behavior is so complex and edge case that its not worth to fix. Writing a test to ensure something is not fixed is odd.

[hughbe]

Thanks, is the concern around the fix because it would break things or just that we don’t want to fix anything because it’s too much effort (relative to the benefit)? i can understand the former but the latter seems... well not really in the spirit of open source

[weltkante]

The problem is probably to fix only the bug without introducing regressions. You need to change the way COM objects are associated to apartments. I had commented an idea on the other thread:

I guess the right fix for this bug in .NET would be to drop the apartment association in the RCW cache if you see the same COM identity being used from multiple apartments without proxy

There may be existing code which relies on the "wrong" apartment association for some side effect.

Introducing/replacing functionality like this is always risky to get some edge case wrong. I have the impression they'd rather spend their time on creating the new COM interop system which will be more open to taking such fixes.

[JeremyKuhne]

Looks good.

[RussKie]

Hmm, now we have ImageCollection_AddStrip_InvokeWithHandle_Success test failing...

    System.Windows.Forms.Tests.ImageCollectionTests.ImageCollection_AddStrip_InvokeWithHandle_Success(transparentColor: Color [A=18, R=52, G=86, B=120], value: Bitmap { Flags = 2, FrameDimensionsList = [7462dc86-6180-4c7e-8e3f-ee7333a7a483], Height = 16, HorizontalResolution = 96, Palette = ColorPalette { Entries = [...], Flags = 0 }, ... }, expectedCount: 1) [FAIL]
      System.InvalidOperationException : Image cannot be added to the ImageList.
      Stack Trace:
        F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ImageList.cs(414,0): at System.Windows.Forms.ImageList.AddToHandle(Bitmap bitmap)
        F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ImageList.ImageCollection.cs(336,0): at System.Windows.Forms.ImageList.ImageCollection.Add(Original original, ImageInfo imageInfo)
        F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ImageList.ImageCollection.cs(427,0): at System.Windows.Forms.ImageList.ImageCollection.AddStrip(Image value)
        F:\workspace\_work\1\s\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\ImageList.ImageCollectionTests.cs(881,0): at System.Windows.Forms.Tests.ImageCollectionTests.ImageCollection_AddStrip_InvokeWithHandle_Success(Color transparentColor, Image value, Int32 expectedCount)

[weltkante]

Hmm, now we have ImageCollection_AddStrip_InvokeWithHandle_Success test failing...

Yeah I mentioned that above, seems its not know to be flaky, but it can't have anything to do with this PR. This is literally just cleaning up manually instead of letting the GC do it.

I think it only happened once or do we have multiple failures?

[RussKie]

Looks like the first timer... Passed on a rebuild.

[RussKie]

Thank you for the fix! 🚀