1284314 - Investigate stability of tear-off interfaces with remote COM objects

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

I wrote a test program for traversing our a11y interfaces. It crashes when CoUninitialize tries to disconnect its proxies. After reducing this down to a test case, it looks like this only happens when resolving two separate IServiceProvider objects for the same underlying Accessible.

Note that this is more complicated than just calling QueryInterface twice on the same IAccessible object; COM proxies cache QI results so it will just return the same IServiceProvider for both calls.

Where this gets messy is if I do something like:

root->QueryInterface(IID_IServiceProvider, sp1)
root->getChild(0)
child0->getParent()
parent->QueryInterface(IServiceProvider, sp2);

In this case, the COM proxy manager doesn't realize that parent and root are actually the same objects, so sp1 and sp2 are remoted as distinct interfaces.

I am not 100% certain as to the *why* of this crash, other than the fact that in my "Essential COM" book, tear-offs are considered "risky" when they are remoted. I probably need to dive deeper into the details of reference counting of remote objects to determine why this messes up the client.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Here's the client call stack at the crash site:

00 combase!CStdMarshal::DisconnectProxyFromChannel+0x2f
01 combase!CStdMarshal::DisconnectCliIPIDs+0x16f
02 combase!CStdMarshal::Disconnect+0x184
03 combase!DisconnectSwitch+0x21
04 combase!CStdMarshal::DisconnectAndRelease+0x2a
05 combase!COIDTable::ThreadCleanup+0xed
06 combase!FinishShutdown::__l5::<lambda_59bccf517150cc90d93e23ea9348175e>::operator()+0x5
07 combase!ObjectMethodExceptionHandlingAction<<lambda_59bccf517150cc90d93e23ea9348175e> >+0x15
08 combase!FinishShutdown+0x37
09 combase!ApartmentUninitialize+0x68
0a combase!wCoUninitialize+0xd0
0b combase!CoUninitialize+0x7e
0c a11ytest!mozilla::COMApartmentRegion<2>::~COMApartmentRegion<2>+0x15
0d a11ytest!main+0x426
0e a11ytest!invoke_main+0x1d
0f a11ytest!__scrt_common_main_seh+0xff
10 KERNEL32!BaseThreadInitThunk+0x24
11 ntdll!__RtlUserThreadStart+0x2f
12 ntdll!_RtlUserThreadStart+0x1b

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Jamie, have you ever encountered anything like this?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

(Also, I should point out that this very well could be a bug elsewhere in my code, but the circumstances surrounding its test case make me suspicious)

Comment 4

[James Teh [:Jamie]]

We've never seen anything like this. :(

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Note that we're now hitting similar crashes in Marionette. I temporarily disable the affected test (see https://hg.mozilla.org/integration/mozilla-inbound/rev/4d00141b4cb9), but we're going to need to fix this before that can be reenabled.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/baf4430c62c0e6bd5c9ca45ed7454dc311dd99b2
Bug 1284314: Disable a11y jsat mochitests on Windows e10s; r=bustage a=philor CLOSED TREE

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/748308e48c83a96aa8e18dad709e082669eaf5b3
Bug 1284314: Disable a11y jsat mochitests on Windows; r=bustage CLOSED TREE

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/baf4430c62c0
https://hg.mozilla.org/mozilla-central/rev/748308e48c83

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

It appears that our tearoff for IEnumVARIANT is implementing its own QI entry for IUnknown, which breaks the rules of COM because it messes up object identity. IUnknown *must* unconditionally query back to the original interface from which the tearoff was spawned.

I suspect that this is the problem, but will need to verify with testing.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Good news: the client crashes that I was seeing were a result of a bug in my test code.

Bad news: I do see a tearoff QI that could mess up object identity.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Make sure that ChildrenEnumVariant always forwards to mAnchorAcc

Tearoffs need their IUnknown to resolve to the IUnknown that belonged to the original object that created them. Otherwise it violates the rules of COM and may impair the COM runtime's ability to properly manage object lifetime.

I think this should be okay given that we have checks elsewhere for IsDefunct().

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to Aaron Klotz [:aklotz] from comment #11)
> Created attachment 8789095 [details] [diff] [review]
> Make sure that ChildrenEnumVariant always forwards to mAnchorAcc
> 
> Tearoffs need their IUnknown to resolve to the IUnknown that belonged to the
> original object that created them. Otherwise it violates the rules of COM
> and may impair the COM runtime's ability to properly manage object lifetime.

Implementing COM is so much fun as usual.

> I think this should be okay given that we have checks elsewhere for
> IsDefunct().

yeah, and we don't ever reset mAnchorAcc that I can see.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16a3dac277fffccef2c6c8ba68a96921e4d7ade0
Bug 1284314: Fix EnumVariant's QI so that it always resolves the original object's IUnknown; r=tbsaunde

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16a3dac277ff