Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ELEMDESC native interop definition #896

Closed
weltkante opened this issue Apr 28, 2019 · 3 comments · Fixed by #1254
Closed

Fix ELEMDESC native interop definition #896

weltkante opened this issue Apr 28, 2019 · 3 comments · Fixed by #1254
Labels
🪲 bug Product bug (most likely)

Comments

@weltkante
Copy link
Contributor

This is a follow-up issue to PR #818 where it was noted (link to comment) that the definition of ELEMDESC that WinForms uses seems to be wrong.

Currently the definition is embedding the TYPEDESC by reference (it was a class before, so the interop rules should already have been embedding it as a pointer before PR #818)

public unsafe struct tagELEMDESC {
    public NativeMethods.tagTYPEDESC* tdesc;
    public NativeMethods.tagPARAMDESC paramdesc;
}

while the definition of ELEMDESC and TYPEDESC on msdn is

typedef struct tagELEMDESC {
  TYPEDESC tdesc;
  PARAMDESC paramdesc;
} ELEMDESC, *LPELEMDESC;

typedef struct tagTYPEDESC {
  union {
    struct tagTYPEDESC  *lptdesc;
    struct tagARRAYDESC *lpadesc;
    HREFTYPE            hreftype;
  } DUMMYUNIONNAME;
  VARTYPE vt;
} TYPEDESC;

Notes taken from the previous discussion in the PR

  • @sharwell wants to have tests to make sure nothing breaks, because fixing this likely requires also correcting all usage of ELEMDESC in WinForms
  • @zsd4yr suggests taking a look into the old source control system to see if there's any history to the definition explaining the way it is
  • @JeremyKuhne suggests using the version in System.Runtime.InteropServices.ComTypes - note that this still requires reviewing and adjusting callers in WinForms which previously used the broken version
@merriemcgaw merriemcgaw added the 🪲 bug Product bug (most likely) label Apr 29, 2019
@merriemcgaw merriemcgaw added this to the Future milestone Apr 29, 2019
@zsd4yr
Copy link
Contributor

zsd4yr commented May 9, 2019

Who is taking point on this?

@sharwell
Copy link
Member

I was not able to construct a case where any of this code gets used. Is any of this COM stuff supported in .NET Core?

@weltkante
Copy link
Contributor Author

weltkante commented May 11, 2019

Looking through the Com2Interop namespace it looks like the only nontrivial user is AxHost, so it's probably all about hosting unmanaged ActiveX controls inside WinForms. Maybe its about the design time experience of ActiveX controls? I could find a reference to VS so there is/was some hidden connection here. Since the VS designer is still Desktop Framework you may not be able to test this code path in .NET Core yet, but moving forward I think VS will need a separate WinForms Core designer, at least once new features or .NET Core only 3rd party controls start to appear the Desktop Designer will no longer work.

Hosting 3rd party unmanaged ActiveX controls inside WinForms is something we have occasionally used, so I don't think it should be dropped entirely. However if my assumption is right and the Com2Interop namespace is only used at design time then VS is probably the only consumer and I'm all for moving it out of WinForms and into the project system. Unless you want to support 3rd party WinForms designers to host unmanaged ActiveX controls.

JeremyKuhne added a commit to JeremyKuhne/winforms that referenced this issue Jul 1, 2019
ELEMDESC doesn't contain a pointer. This fixes a regression introduced in dotnet#818.

Fixes dotnet#896
wtgodbe pushed a commit that referenced this issue Jul 2, 2019
ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818.

Fixes #896
wtgodbe pushed a commit to wtgodbe/winforms that referenced this issue Jul 2, 2019
ELEMDESC doesn't contain a pointer. This fixes a regression introduced in dotnet#818.

Fixes dotnet#896
AdamYoblick pushed a commit that referenced this issue Jul 2, 2019
ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818.

Fixes #896
RussKie pushed a commit that referenced this issue Jul 3, 2019
ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818.

Fixes #896

(cherry picked from commit 52be7e5)
@RussKie RussKie removed this from the Future milestone Jun 16, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🪲 bug Product bug (most likely)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants