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

Add new api's to IObjectReference to safely use the underlying pointer. #1474

Draft
wants to merge 6 commits into
base: staging/AOT
Choose a base branch
from

Conversation

jlaanstra
Copy link
Collaborator

Add a pattern similar to SafeHandle to safely access the pointer of an ObjectReference.

Depends on #1458

@jlaanstra jlaanstra marked this pull request as draft January 26, 2024 19:08
if (__return_value__ != IntPtr.Zero)
{
(*(global::WinRT.Interop.IUnknownVftbl**)__return_value__)->Release(__return_value__);
MarshalInspectable<object>.DisposeAbi(__return_value__);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why MarshalInspectable<object> rather than just the Release call like before? It's more efficient.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty much how this is done everywhere else. I can revert but consistency is nice.

@@ -303,13 +314,13 @@ internal abstract class State : IDisposable
public System.Delegate del;
public System.Delegate eventInvoke;
private bool disposedValue;
private readonly IntPtr obj;
private readonly IObjectReference obj;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially a semantics change, as the event source state wasn't previously keeping the RCW instance alive (unless indirectly captured by the wrapped delegate), but only the pointer to the native object. Are we sure changing this is not going to introduce some unwanted side effects and/or potentially any leaks anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that this case isn't right, but without it how can we guarantee that the IntPtr is valid? I'll check it once the other changes are done.

Copy link
Member

@Sergio0694 Sergio0694 Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to. Looking at the state type, the pointer is only being used as object identity (as in, as a key in the internal cache). But it's never actually used to access the native COM object and interact with it. So even if the object did go away, this code would still be fine. There's some delicate GC stuff going on between the event source and the event state and I'm inclined to think the object reference wasn't being rooted by the state on purpose 😅

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