Hand off a11y requests from RPC thread to main thread

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks 1 bug)

Trunk
mozilla50
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox50 fixed)

Details

(Whiteboard: aes+)

Attachments

(6 attachments)

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.
Depends on: 1267368
Posted patch WIPSplinter Review
Blocks: 1258839
Whiteboard: aes-win
Whiteboard: aes-win → aes+
Depends on: 1287002
Depends on: 1287875
Review commit: https://reviewboard.mozilla.org/r/65418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65418/
Attachment #8772672 - Flags: review?(jmathies)
Attachment #8772673 - Flags: review?(jmathies)
Attachment #8772674 - Flags: review?(jmathies)
Attachment #8772675 - Flags: review?(jmathies)
Attachment #8772676 - Flags: review?(jmathies)
Blocks: 1288199
Attachment #8772672 - Flags: review?(jmathies) → review+
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
Attachment #8772673 - Flags: review?(jmathies) → review+
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 on attachment 8772674 [details]
Bug 1263224: Add support for thread-safe weak references for COM-based objects;

https://reviewboard.mozilla.org/r/65422/#review63168
Attachment #8772674 - Flags: review?(jmathies) → review+
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.
Attachment #8772675 - Flags: review?(jmathies) → review+
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.
Attachment #8772676 - Flags: review?(jmathies) → review+
(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.
(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 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 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 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 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 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/
Pushed by aklotz@mozilla.com:
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
Depends on: 1372959
You need to log in before you can comment on or make changes to this bug.