Add new api's to IObjectReference to safely use the underlying pointer.#1474
Add new api's to IObjectReference to safely use the underlying pointer.#1474jlaanstra wants to merge 6 commits intomicrosoft:staging/AOTfrom
Conversation
jlaanstra
force-pushed
the
user/jlaans/dangerousThisPtr
branch
from
07d3a0b to
0a35e8d
Compare
jlaanstra
force-pushed
the
user/jlaans/dangerousThisPtr
branch
from
0a35e8d to
66876af
Compare
jlaanstra
force-pushed
the
user/jlaans/dangerousThisPtr
branch
from
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.
Why MarshalInspectable<object> rather than just the Release call like before? It's more efficient.
There was a problem hiding this comment.
This is pretty much how this is done everywhere else. I can revert but consistency is nice.
| public System.Delegate eventInvoke; | ||
| private bool disposedValue; | ||
| private readonly IntPtr obj; | ||
| private readonly IObjectReference obj; |
There was a problem hiding this comment.
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.
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.
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 😅
|
Closing this, we'll do the fix in 3.0 which is built with proper ref tracking from the start. |
2 participants
Add a pattern similar to SafeHandle to safely access the pointer of an ObjectReference.
Depends on #1458