-
Notifications
You must be signed in to change notification settings - Fork 106
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
base: staging/AOT
Are you sure you want to change the base?
Add new api's to IObjectReference to safely use the underlying pointer. #1474
Conversation
07d3a0b
to
0a35e8d
Compare
0a35e8d
to
66876af
Compare
66876af
to
d7a0742
Compare
if (__return_value__ != IntPtr.Zero) | ||
{ | ||
(*(global::WinRT.Interop.IUnknownVftbl**)__return_value__)->Release(__return_value__); | ||
MarshalInspectable<object>.DisposeAbi(__return_value__); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
Add a pattern similar to SafeHandle to safely access the pointer of an ObjectReference.
Depends on #1458