Fix invalid ComVisible attributes

[hughbe]

Proposed Changes

• Fix invalid ComVisible attributes

Fixes #1878

Microsoft Reviewers: Open in CodeFlow

[hughbe]

See the definition in https://docs.microsoft.com/en-us/windows/win32/api/uiautomationcore/nf-uiautomationcore-irawelementproviderfragment-navigate

HRESULT Navigate(
  NavigateDirection direction ,
  IRawElementProviderFragment **pRetVal
);

Was the existing implementation incorrect? I think possibly technically because we'd return IUnknown rather than IRawElementProviderFragment. But I don't know enough interop to be able to say
/cc @JeremyKuhne @weltkante

[weltkante]

Yes I think this can be a bug if the IUnknown is not the one of IRawElementProviderFragment (which can happen if there are multiple interface vtables in play). The method may never have been used actively or you may have been lucky the CLR picks the right IUnknown. I don't think theres a guarantee it will pick the right one though if you don't specify it.

[hughbe]

I'll open an issue. This is why we should have stuff like the interop PR merged so we can yeet these bugs away

[JeremyKuhne]

you may have been lucky the CLR picks the right IUnknown

How is that possible? It isn't querying for IUnknown- any COM pointer is IUnknown by definition. You'd just be getting the same pointer. I suppose if you then did the query interface and it gave you a different IRawElementProviderFragment than itself, but that would be fundamentally broken, no?

There is value in not creating the derived RCW from a perf perspective. If we don't ever cast the object to a given interface we can avoid a lot of overhead. This is particularly true if we can keep things as IntPtr and avoid even the IUnknown wrapper.

[hughbe]

So I'm thinking of the following example:

IRawElementProviderFragment *pFragment = ...;
IRawElementProviderFragment* navigateResult;
HRESULT hr = pFragment->Navigate(NavigateDirection_Parent, &navigateResult);
assert(hr == S_OK);

UiaRect boundingRect;
hr = result->get_BoundingRect(&boundingRect);
assert(hr == S_OK);

// Do something with boundingRect

In this case, we expect navigateResult to be of type IRawElementProviderFragment. However, we actually returned a value of type IUnknown from the managed code. Isn't this undefined behaviour?

[weltkante]

How is that possible?

I don't know how the framework implements IUnknown, its perfectly fine to reuse the vtable of one of the interfaces (as long as you always pick the same one). IRawElementProviderFragment inherits from IUnknown so its perfectly fine to return the pointer to that instead of creating two separate vtables.

If the framework doesn't return IRawElementProviderFragment and instead returns an unrelated subclass of IUnknown then you get either an access violation or call a random function in the same position of the vtable, but with wrong arguments.

There is value in not creating the derived RCW from a perf perspective. If we don't ever cast the object to a given interface we can avoid a lot of overhead.

This is about creating a CCW though, the COM vtable of an interface, you are returning a .NET object to native code.

There is also a lot of value in only ever creating one vtable if you already know beforehand that the class implements only one interface. If the interface doesn't end up being marshaled or anything else complicated then you might even be able to only ever create that single interface (happens often in native COM implementations). Not saying the framework does this particular optimization, just saying its a possibility.

Isn't this undefined behaviour?

Depends. Native code does that all the time, getting an IUnknown back from a call and directly casting it to a subclass interface, assuming knowledge about what the implementation really returns.

If the framework is consistent in picking the IUnknown implementation to be, say, the first interface (which happens to be IRawElementProviderFragment), then it might just work out. I'm not optimistic it does, just saying its a possibility things have worked out so far.

There is no reason to not fix that though.

[JeremyKuhne]

If the framework is consistent in picking the IUnknown implementation to be, say, the first interface

Alright, I see what you're getting at. I was looking at the roundtripping of the RCW, which unwraps back to the native object. Passing AccessibleObject is going to give you a CCW for IUnknown itself I would presume.

Note that CCW that come back are also unwrapped to their managed object.