Cleanup VARIANT marshalling and convert to managed #102498
Conversation
dotnet-policy-service
bot
added
the
community-contribution
Co-authored-by: Aaron Robinson <[email protected]>
| case VarEnum.VT_RECORD: | ||
| MarshalHelperConvertObjectToVariant(pValue, out v); | ||
| if (v.VarType != VarEnum.VT_RECORD) | ||
| throw new InvalidCastException(SR.InvalidCast_CannotCoerceByRefVariant); | ||
| break; |
There was a problem hiding this comment.
I'm trying to restore the behavior here, and relying on the location to be cleaned up by RAII at native side.
Is there any better option for this?
There was a problem hiding this comment.
What is the previous behavior in this case? I'm not following the issue being hit here.
There was a problem hiding this comment.
I don't see any destructors in MarshalOleRefVariantForObject. Is that where you are talking about?
There was a problem hiding this comment.
Previously in MarshalOleRefVariantForObject, if SafeVariantChangeType fails, would the data (potentially BSTR or something else) in vtmp be leaked? SafeVariantClear won't be called in throwing path.
I've concluded that SafeVariantChangeType is unnecessary and will always fail. The content of the VARIANT should be cleared at managed side in failing path.
|
@AaronRobinsonMSFT Can you answer remaining questions in this PR? The concern is about cleanup in exceptional cases with partial result. When taking a ref at managed side, RAII at native side may take care of the cleanup. |
|
Is there anything remaining in this? I think we should aim to merge this to avoid more conflicts with other refactoring. |
|
Thanks @huoyaoyuan. Appreciate the work here. I would ask that you please avoid converting more of the legacy COM interop code. The interop team is slowing making out way through it and it is both a learning exercise for us to understand what we support and how best to integrate it into the current code base. It is doubly costly for us when there is inevtiably a missed compat case and we need to track it down. It is best left for the interop team to port over. I appreciate the added tests, but many of the questions you've asked are only answerable as we slowly detangle/understand the existing code. Again, we do appreciate this PR, but we would prefer porting more commonly used code paths that have less impact to legacy and fragile .NET code bases.
The best way to address these issue is through additional testing. These code paths are very old and subtle in many ways. I will try and do one more pass soon so we can get this specific PR in. |
|
No problem. I was assuming that you have enough knowledge to answer all the questions raised. But apparently you may need to spend similar effort than me to understand the scenarios, or even more effort to be responsible for every corner cases. My interest in this was originally raised up when learning the types evolved with interop. Currently I have no extra work beyond this. |
* Handle simple cases of MarshalComVariantForOleVariant * Pseudo code for MarshalOleVariantForComVariant * Complete and setup managed call for MarshalObjectForOleVariant * Setup MarshalHelperConvertObjectToVariant except record * Setup MarshalHelperCastVariant * Cleanup Variant struct and FCall * Handle record case * Cleanup VariantOleToCom/VariantComToOle routines * Remove VariantData definition at unmanaged side * Eliminate a dead branch for ref * Remove CVTypes in native * Share GetComIPFromObjectRef * Add tests for managed to native side * Add test for native to managed side * Add test for byref returning to BYREF * Fix BYREF handling * Fix array marshalling * Fix record marshalling * Fix VT_VARIANT case * Cleanup and fix IUnknown case * Add more test for VT_UNKNOWN * Cleanup for VT_VARIANT * Cleanup SR and comment * Fix contract in GetTypeHandleForVarType * Move VariantChangeTypeEx to coreclr * Add unmanaged fast path for (U)Int64 * Make ComVariant immutable * Add GCPROTECT in ConvertSystemColorToOleColor * Fix variant init * Fix BYREF|EMPTY * Initialize ComVariant * Move GCPROTECT to the QCall * Pass UnknownWrapper through SetFieldsObject * Apply suggestions from code review Co-authored-by: Aaron Robinson <[email protected]> * Suggestions at native side * Rename ComIP * Add comments for ConvertWrappedObject * Try convert to RECORD for all cases * Dispose Variant on failure * Update formatting * Fix NO_MAPPING definition --------- Co-authored-by: Aaron Robinson <[email protected]>
Cleans up the managed Variant struct used as an intermediate value.
Basically, folds
MarshalHelperConvertObjectToVariantwithMarshalOleVariantForComVariant,MarshalHelperConvertVariantToObjectandMarshalHelperCastVariantwithMarshalComVariantForOleVariant.Tests were first written and tested on main.
Array and record marshalling are still kept native. VT_ARRAY marshalling looks sharing many logic with non-variant marshalling.