Skip to content

Override marshalling methods for Com objects #4

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nerzhulart
Copy link
Contributor

Override marshalling methods for Com objects to properly make AddRef/Release

SharpGenTools/SharpGenTools#140 should be merged first to apply this PR (also we need to update package dependency)

@nerzhulart
Copy link
Contributor Author

The general idea of this (and referenced PR in the main repo) is following:
When you receive a pointer to COM object you have to make AddRef, and when the pointer is collected - you have to make Release, but there is an exclusion: when the object is returned via 'Obj**' parameter. In this case native code makes an extra AddRef for newly created object, and callsite must call one more Release() to balance references.

So I patched the generator in main repo that it emits TransformObjectFromUnmanaged call on every constructed SharpGen object, and this call has a flag isOutParam that allows callsite to differ these cases

@jkoritzinsky
Copy link
Member

I've finally had some time to take a look at this PR.

I think this adds a lot of complexity for minimal gain. The primary way to get access to a COM object is via an out parameter, which is already AddRef'd before you get it. In a well-designed ref-counting system, this is always the case. SharpGenTools doesn't do any extra logic on ref-counting lifetime since it's generally trying to be a thin wrapper.

Additionally, the implementation of this seems to be messy with requiring the well-known-type override. Also, that override won't flow to all users, so every user of SharpGen.Runtime.COM would have to add that element to their project file.

@nerzhulart
Copy link
Contributor Author

I've finally had some time to take a look at this PR.

I think this adds a lot of complexity for minimal gain. The primary way to get access to a COM object is via an out parameter, which is already AddRef'd before you get it. In a well-designed ref-counting system, this is always the case. SharpGenTools doesn't do any extra logic on ref-counting lifetime since it's generally trying to be a thin wrapper.

Additionally, the implementation of this seems to be messy with requiring the well-known-type override. Also, that override won't flow to all users, so every user of SharpGen.Runtime.COM would have to add that element to their project file.

Sorry, didn't see your answer in time.
Seems you didn't understand the situation deeply. I meant that if you're working with COM you have strict rules of ref counting which I've described above, and we have 2 different cases of ref-balancing, which explicitly depends whether you receive your object via single (*) or double (**) pointer. And in the current SharpGen approach you don't have an ability to balance refs on your objects properly because SharpGen doesn't allow to differ these cases when it creates managed instances of COM objects. So you get either a memory leak when doing extra AddRef always or early object death when you never do extra AddRef.
I understand that all the users of SharpGen.Runtime.COM have to add the property to override the helper, but it's obviously, because otherwise they will face with the memory management issues which I've mentioned.

I can say that SharpGen with this patch already works in Rider debugger (we use it for ICorDebug) since this summer and everything is Ok. I was needed to add these changes because initially when we moved to SharpGen all our COM objects became invalid too early, we even can't run simple tests. Then I've tried to add simple AddRef in all calls and we got a huge memory leak. That's why I had to debug and create this patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants