1263224 - Hand off a11y requests from RPC thread to main thread

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Remote RPCs destined for COM objects within the MTA are serviced by the RPC thread pool. We need to be able to take those requests and pass them to the main thread for processing and we need that mechanism to be as lightweight as possible.

Trevor suggested that we make this as generic as possible so that we don't need to implement every single interface that could possibly be supported.

I think that the best way to make this happen is to use CoGetInterceptor to generate handlers that send calls for various interfaces to a common event sink whose job it is to enqueue that request to the main thread as a Task.

Care must be taken to ensure that any out-params that are interfaces are also wrapped with interceptors before they are returned to the COM runtime as output. Failure to do so would result in future calls from those interfaces bypassing the handoff mechanism and attempting to execute directly on the RPC threads.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: WIP

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1263224: Smart pointers for passing COM interface pointers around in other apartments;

Review commit: https://reviewboard.mozilla.org/r/65418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65418/

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1263224: Adds MainThreadInvoker class for posting Asynchronous Procedure Calls to the main thread;

Review commit: https://reviewboard.mozilla.org/r/65420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65420/

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1263224: Add support for thread-safe weak references for COM-based objects;

Review commit: https://reviewboard.mozilla.org/r/65422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65422/

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1263224: Adds COM interception that allows us to proxy COM invocations;

Review commit: https://reviewboard.mozilla.org/r/65424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65424/

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1263224: Add ability to wrap a COM method invocation and hand it off to the main thread for execution;

Review commit: https://reviewboard.mozilla.org/r/65426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65426/

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8772672 [details]
Bug 1263224: Smart pointers for passing COM interface pointers around in other apartments;

https://reviewboard.mozilla.org/r/65418/#review63160

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8772673 [details]
Bug 1263224: Adds MainThreadInvoker class for posting Asynchronous Procedure Calls to the main thread;

https://reviewboard.mozilla.org/r/65420/#review63166

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8772674 [details]
Bug 1263224: Add support for thread-safe weak references for COM-based objects;

https://reviewboard.mozilla.org/r/65422/#review63168

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8772675 [details]
Bug 1263224: Adds COM interception that allows us to proxy COM invocations;

https://reviewboard.mozilla.org/r/65424/#review63176

::: ipc/mscom/Interceptor.cpp:99
(Diff revision 1)
> +Interceptor::CreateInterceptor(REFIID aIid, IUnknown* aOuter, IUnknown** aOutput)
> +{
> +  // In order to aggregate, we *must* request IID_IUnknown as the initial
> +  // interface for the interceptor, as that IUnknown is non-delegating.
> +  HRESULT hr = ::CoGetInterceptor(aIid, aOuter, IID_IUnknown, (void**)aOutput);
> +  if (hr != kFileNotFound) {

What does this result mean? MSDN doesn't mention this on the main page for CoGetInterceptor. Lets  add a comment where we declare this explaining what this means.

::: ipc/mscom/Interceptor.cpp:105
(Diff revision 1)
> +    return hr;
> +  }
> +
> +  // CoGetInterceptor requires information from a typelib to be able to
> +  // generate its emulated vtable. If a typelib is unavailable,
> +  // CoGetInterceptor returns 0x80070002. In that case, we can try to explicitly

ah, ok, please move this up higher near where we use kFileNotFound.

::: ipc/mscom/Interceptor.cpp:252
(Diff revision 1)
> +  HRESULT hr;
> +  auto runOnMainThread = [&]() -> void {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    hr = mTarget->QueryInterface(aIid, aOutput);
> +  };
> +  if (!invoker.Invoke(NS_NewRunnableFunction(runOnMainThread))) {

This is sync right? maybe add a comment noting this.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8772676 [details]
Bug 1263224: Add ability to wrap a COM method invocation and hand it off to the main thread for execution;

https://reviewboard.mozilla.org/r/65426/#review63188

whew!

::: ipc/mscom/MainThreadHandoff.cpp:188
(Diff revision 1)
> +
> +  // (6) Unfortunately ICallFrame::WalkFrame does not correctly handle array
> +  // outparams. Instead, we find out whether anybody has called
> +  // mscom::RegisterArrayData to supply array parameter information and use it
> +  // if available. This is a terrible hack, but it works for the short term. In
> +  // the longer term we want to be able to use COM proxy/stub metadata to

how serious is this 'hack'? Like for example, what could break it? multiple a11y clients?

Please file a follow up on this.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #10)
> Comment on attachment 8772675 [details]
> Bug 1263224: Adds COM interception that allows us to proxy COM invocations;
> 
> https://reviewboard.mozilla.org/r/65424/#review63176
> 
> ::: ipc/mscom/Interceptor.cpp:99
> (Diff revision 1)
> > +Interceptor::CreateInterceptor(REFIID aIid, IUnknown* aOuter, IUnknown** aOutput)
> > +{
> > +  // In order to aggregate, we *must* request IID_IUnknown as the initial
> > +  // interface for the interceptor, as that IUnknown is non-delegating.
> > +  HRESULT hr = ::CoGetInterceptor(aIid, aOuter, IID_IUnknown, (void**)aOutput);
> > +  if (hr != kFileNotFound) {
> 
> What does this result mean? MSDN doesn't mention this on the main page for
> CoGetInterceptor. Lets  add a comment where we declare this explaining what
> this means.

One of the fundamental rules for creating an aggregated object in COM is that you must initially request IUnknown, and then QI off of that. Agreed that we should explicitly point that out, will clarify.

> 
> ::: ipc/mscom/Interceptor.cpp:105
> (Diff revision 1)
> > +    return hr;
> > +  }
> > +
> > +  // CoGetInterceptor requires information from a typelib to be able to
> > +  // generate its emulated vtable. If a typelib is unavailable,
> > +  // CoGetInterceptor returns 0x80070002. In that case, we can try to explicitly
> 
> ah, ok, please move this up higher near where we use kFileNotFound.

Sure.

> 
> ::: ipc/mscom/Interceptor.cpp:252
> (Diff revision 1)
> > +  HRESULT hr;
> > +  auto runOnMainThread = [&]() -> void {
> > +    MOZ_ASSERT(NS_IsMainThread());
> > +    hr = mTarget->QueryInterface(aIid, aOutput);
> > +  };
> > +  if (!invoker.Invoke(NS_NewRunnableFunction(runOnMainThread))) {
> 
> This is sync right? maybe add a comment noting this.

Yes, done.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #11)
> Comment on attachment 8772676 [details]
> Bug 1263224: Add ability to wrap a COM method invocation and hand it off to
> the main thread for execution;
> 
> https://reviewboard.mozilla.org/r/65426/#review63188
> 
> whew!
> 
> ::: ipc/mscom/MainThreadHandoff.cpp:188
> (Diff revision 1)
> > +
> > +  // (6) Unfortunately ICallFrame::WalkFrame does not correctly handle array
> > +  // outparams. Instead, we find out whether anybody has called
> > +  // mscom::RegisterArrayData to supply array parameter information and use it
> > +  // if available. This is a terrible hack, but it works for the short term. In
> > +  // the longer term we want to be able to use COM proxy/stub metadata to
> 
> how serious is this 'hack'? Like for example, what could break it? multiple
> a11y clients?

Nothing really breaks it, it's just not future-proof. Any time a new interface enters the picture, we'd need to watch for array parameter annotations in the IDL and add that data to our code.

> 
> Please file a follow up on this.

Filed bug 1289850.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8772672 [details]
Bug 1263224: Smart pointers for passing COM interface pointers around in other apartments;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65418/diff/1-2/

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8772673 [details]
Bug 1263224: Adds MainThreadInvoker class for posting Asynchronous Procedure Calls to the main thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65420/diff/1-2/

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8772674 [details]
Bug 1263224: Add support for thread-safe weak references for COM-based objects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65422/diff/1-2/

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8772675 [details]
Bug 1263224: Adds COM interception that allows us to proxy COM invocations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65424/diff/1-2/

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8772676 [details]
Bug 1263224: Add ability to wrap a COM method invocation and hand it off to the main thread for execution;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65426/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c16f3564621a
Smart pointers for passing COM interface pointers around in other apartments; r=jimm
https://hg.mozilla.org/integration/autoland/rev/5b6b7d841692
Adds MainThreadInvoker class for posting Asynchronous Procedure Calls to the main thread; r=jimm
https://hg.mozilla.org/integration/autoland/rev/e463fd04a669
Add support for thread-safe weak references for COM-based objects; r=jimm
https://hg.mozilla.org/integration/autoland/rev/7d617b3945a2
Adds COM interception that allows us to proxy COM invocations; r=jimm
https://hg.mozilla.org/integration/autoland/rev/3039b674a5a4
Add ability to wrap a COM method invocation and hand it off to the main thread for execution; r=jimm

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c16f3564621a
https://hg.mozilla.org/mozilla-central/rev/5b6b7d841692
https://hg.mozilla.org/mozilla-central/rev/e463fd04a669
https://hg.mozilla.org/mozilla-central/rev/7d617b3945a2
https://hg.mozilla.org/mozilla-central/rev/3039b674a5a4

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/366f6b46e1a5
cross-compilation fixup.

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/366f6b46e1a5