Fix COM interop ELEMDESC #1254
Merged
Fix COM interop ELEMDESC #1254
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ELEMDESC doesn't contain a pointer. This fixes a regression introduced in dotnet#818. Fixes dotnet#896
AaronRobinsonMSFT
approved these changes
Jul 2, 2019
RussKie
added a commit
to RussKie/Test-ComInterop-ELEMDESC
that referenced
this pull request
Jul 2, 2019
Demo for repro dotnet/winforms#1254
|
Could you please help me with repro steps? I've created a test .NET Core 3.0 app https://github.com/RussKie/Test-ComInterop-ELEMDESC with the above code snippet. I'm guessing I'm not exercising the intended code path (since I don't fully understand the underlying issue). Thanks |
|
I'm not sure how to repro in a similar way with a WinForms app. The call stack for WPF is below. The native COM TypeDescriptor uses WinForms through reflection. Here is where it gets hooked up: I haven't done much of anything with TypeDescriptors or System.ComponentModel so I can't help you much more at the moment. I've got a lot to learn here as well. (@maryamariyan, @safern, take note) |
|
Do you a repro app available by a chance? |
|
Can someone comment on the interop/marshaling rules here? In #818 I was assuming that in Desktop Framework, being defined as a class it should already have been marshaled by reference instead of being inlined. What I can see calling Marshal.SizeOf is that in (64 bit) Desktop the tagELEMENT size is 32 byte (which is correct) and in Core it is just 24 byte leading to an AccessViolation. /cc @sharwell |
|
Approved for preview7 |
wtgodbe
pushed a commit
to wtgodbe/winforms
that referenced
this pull request
Jul 2, 2019
ELEMDESC doesn't contain a pointer. This fixes a regression introduced in dotnet#818. Fixes dotnet#896
|
Shouldn't a test be added for this? @AaronRobinsonMSFT can you help with that since it looks like @sharwell tried without luck to get coverage? I'd hate for this scenario to regress again. |
AdamYoblick
pushed a commit
that referenced
this pull request
Jul 2, 2019
We can add some unit tests for it. I've been speaking with @RussKie a bit about it and looking into .NET's dependency on this WinForms code. It might also be possible to add some light scenario tests if Office components happen to be registered. |
@weltkante I added an exhaustive rundown of properly defining On desktop this was defined as a class, which is really sub-optimal. You do get pass-by-ref semantics by default in interface definitions, but it requires allocating on the managed heap and a native allocation and two way copy by the marshaling layer. It is orders of magnitude faster to have a blittable struct definition and pass that by ref as the marshaling layer will simply pin the data on interop calls. When it isn't the right size, all of the pointer accesses are going to be pretty much random (hence the AV). |
We have two options here. The first is related to what @JeremyKuhne mentioned above and consume an existing COM class that exposes a property (e.g. consume an Office installed class). This is generally a bad idea because the assumes a specific machine configuration - just bad. There are probably some Windows classes that could be used, but again this depends on a machine configuration which is bad. An alternative approach would be to define/build/consume a COM server to be used in testing. This follows the coreclr repos approach, makes testing far more stable, and allows us to add additional scenarios without having to search for one that "kind of" works. .NET Core 3.0 SDK supports creating/consuming managed COM servers so this should be relatively easy. On top of the ability to author one, consumption can be done through RegFree COM which makes this scenario extremely reliable since the testing environment is self contained and won't pollute the build/developer's machine. I could imagine additional tricks to exercise this scenario, but honestly I would recommend the latter rather than the former or some cleverness that makes debugging or understand the test difficult. We on the interop team have strived to make COM as consistent with the official COM story as possible in .NET Core so the more we follow official COM idioms the more reliable these scenarios will be. |
|
@AaronRobinsonMSFT could you please add a test(s) or at least a demo app in which we could repo the original issue and then see it fixed (replacing binaries). So we can pass it on to our testers. |
|
@RussKie The description in the PR written by @JeremyKuhne was taken from the original offline report of the issue and is basically the entire app - minus project system boiler plate. In fact, @JeremyKuhne was on that thread if I recall and verified the app worked with his fix. As an aside, I would not rely on that app in any capacity. It exercises such a narrow code path relative to the COM scenario. My fear is this small app becomes the glorified sniff test for COM interaction with WPF and that is just not the case. Please do not simply pass it off to a team to use as the test, it is a helpful start but rather silly being considered any kind of actual validation of the COM scenario. |
|
I have created a WinForms app (#1254 (comment)) and was not able to see failures.
Could you please share this with us? |
|
@RussKie Oh sorry. I missed that. I will share this offline. |
| @@ -719,7 +719,7 @@ private static void ProcessFunctions(UnsafeNativeMethods.ITypeInfo typeInfo, IDi | |||
|
|
|||
| unsafe | |||
| { | |||
| typeDesc = *funcDesc.elemdescFunc.tdesc; | |||
| typeDesc = funcDesc.elemdescFunc.tdesc; | |||
There was a problem hiding this comment.
Are all the unsafe blocks still needed? (Also in the definition of the struct.)
There was a problem hiding this comment.
They aren't needed but I think this specific insertion was for preview 7 so I assumed less code churn was preferred. They definitely should be removed after preview 7 though.
There was a problem hiding this comment.
Yes, I was going for the minimal change for preview 7. :)
@RussKie Ugh, sorry. I shared it with you offline as well, but autocomplete sent it to someone else who didn't tell me. |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818.
Fixes #896
This particular bug is breaking Office interop on WPF and WinForms. Repro is to reference the following COM components:
In a WPF app, add the following to the main Window xaml:
And the following to the code behind for Window_Loaded:
This should be considered for preview 7.
Ultimately we should use the COM types defined in CoreFX to reduce risk and cut the assembly size for WinForms, but making that change is non-trivial for 3.0. #1253
cc: @sharwell, @AaronRobinsonMSFT