When we invoke NotifyWinEvent we specify OBJID_CLIENT as the object id and provide a child ID for the specific Accessible that we are notifying for. This requires MSAA to frequently use the RootAccessible for querying descendants, forcing a11y calls to go through COM objects hosted in the parent process even when resolving properties / methods that actually belong to content. According to https://msdn.microsoft.com/en-us/library/ms971310.aspx#actvaccess_topic4 this means that MSAA is treating those descendants as "simple elements" contained by the root IAccessible, instead of IAccessible objects in their own right. I think that what we need to be doing here is creating object IDs for each proxied Accessible. When we call NotifyWinEvent, we then use that object ID and CHILDID_SELF as the child ID. In our WM_GETOBJECT handler, we then need to resolve that object ID to its corresponding COM proxy. Fixing this bug is pretty much required for everything else in this a11y+e10s implementation to work. Otherwise MSAA will frequently try to go through the root COM object instead of talking directly to the COM objects belonging to the descendants.

The problem with this is that it breaks the de facto standard way of retrieving an IAccessible2 object given a previously obtained unique id. NVDA (and perhaps other AT) use this to pass IAccessible2 objects between processes in serialised data (e.g. from a virtual buffer). What we do now is fetch IAccessible2::uniqueID in one process, then (possibly at some later time) call AccessibleObjectFromEvent with this unique id passed as the dwChildID parameter. I just looked at the IA2 spec and it seems it still says uniqueID is not related to child id, but this is not true for all current implementations and there was definitely discussion about changing this (though I'm not sure why it never happened). For what it's worth, I have no idea why Mozilla (and other implementers) chose child id in the first place years ago. It always seemed like pointless indirection to me. If you're never going to use child ids other than CHILDID_SELF, we can just pass the unique id as the object id instead of the child id. However, this still means there's a compatibility break; older AT will simply stop working with Firefox once this is implemented. We also need some way of knowing whether a server is using child id or object id. This could be determined from the event, but that isn't enough if the AT wants to start processing without being triggered by an event (which can and does happen). It's worth noting that custom object ids need to be positive, whereas custom child ids need to be negative. IAccessible2::uniqueID currently returns a negative id. Perhaps we could say that if IAccessible2::uniqueID returns positive, we should use object id instead of child id. One question: > This requires MSAA to frequently use the RootAccessible for querying descendants, forcing a11y calls to go through COM objects hosted in the parent process even when resolving properties / methods that actually belong to content. WM_GETOBJECT still goes through the parent. As I understand it, the only extra call that would then go through the parent is IAccessible::accChild, since AccessibleObjectFromEvent always tries accChild. From that point, we should have the real child accessible, and according to what you've said in the past, that should bypass the parent process. Am I missing something here or are you concerned that those accChild calls alone are really going to cause problems?

(In reply to James Teh [:Jamie] from comment #1) > The problem with this is that it breaks the de facto standard way of > retrieving an IAccessible2 object given a previously obtained unique id. > NVDA (and perhaps other AT) use this to pass IAccessible2 objects between > processes in serialised data (e.g. from a virtual buffer). What we do now is > fetch IAccessible2::uniqueID in one process, then (possibly at some later > time) call AccessibleObjectFromEvent with this unique id passed as the > dwChildID parameter. I just looked at the IA2 spec and it seems it still > says uniqueID is not related to child id, but this is not true for all > current implementations and there was definitely discussion about changing > this (though I'm not sure why it never happened). > > For what it's worth, I have no idea why Mozilla (and other implementers) > chose child id in the first place years ago. It always seemed like pointless > indirection to me. > > If you're never going to use child ids other than CHILDID_SELF, we can just > pass the unique id as the object id instead of the child id. However, this > still means there's a compatibility break; older AT will simply stop working > with Firefox once this is implemented. We also need some way of knowing > whether a server is using child id or object id. This could be determined > from the event, but that isn't enough if the AT wants to start processing > without being triggered by an event (which can and does happen). Ugh, that's unfortunate that there is such a link between uniqueID and the MSAA childID. > > It's worth noting that custom object ids need to be positive, whereas custom > child ids need to be negative. IAccessible2::uniqueID currently returns a > negative id. Perhaps we could say that if IAccessible2::uniqueID returns > positive, we should use object id instead of child id. That's a brilliant hack! In the worst case, I think that could be a decent workaround. > > One question: > > > This requires MSAA to frequently use the RootAccessible for querying descendants, forcing a11y calls to go through COM objects hosted in the parent process even when resolving properties / methods that actually belong to content. > > WM_GETOBJECT still goes through the parent. As I understand it, the only > extra call that would then go through the parent is IAccessible::accChild, > since AccessibleObjectFromEvent always tries accChild. From that point, we > should have the real child accessible, and according to what you've said in > the past, that should bypass the parent process. Am I missing something here > or are you concerned that those accChild calls alone are really going to > cause problems? accChild is just fine; we're prepared to handle traversal calls like that. Unfortunately when I spin up a test build on my Windows 10 box, something is calling into us on the root with IAccessible::get_accRole and a child ID which points to a remote document. Not only do we have to do the traversal, but in this case we also have to actually query that remote document for its role. I can't explain why we get_accRole was invoked like this for any reason other than that it saw a NotifyWinEvent with a child ID and could not directly resolve an object. (If you have any other suggestions as to why this might be happening, I'm all ears!)

(In reply to Aaron Klotz [:aklotz] from comment #2) > Ugh, that's unfortunate that there is such a link between uniqueID and the > MSAA childID. Unfortunate perhaps, but necessary regardless. :) > accChild is just fine; we're prepared to handle traversal calls like that. > Unfortunately when I spin up a test build on my Windows 10 box, something is > calling into us on the root with IAccessible::get_accRole and a child ID > which points to a remote document. Hmm. Is the child id positive (child index) or negative (unique id)? It's worth noting that even if you get rid of negative child ids, I believe MSAA technically still allows clients to call like this with child indexes. > I can't explain why we get_accRole was invoked like this for any reason > other than that it saw a NotifyWinEvent with a child ID and could not > directly resolve an object. Are you getting a call to accChild with that same child id first? Is that call failing for some reason? If so, why? AccessibleObjectFromEvent should call accChild first. > (If you have any other suggestions as to why this might be happening, I'm > all ears!) I guess it's theoretically possible for a client to bypass AccessibleObjectFromEvent and just fetch CHILDID_SELF and then talk to children itself. However, this seems incredibly unlikely.

I've written a thunk to convert foo->get_accBar(childID) to child->get_accBar(CHILDID_SELF). That should mitigate this without any requirements to modify ATs. Morphing this bug to land that.

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

Some additional info about how this works: ChildIDThunk implements IInterceptorSink which is defined at ipc/mscom/Interceptor.h. It is derived from ICallFrameEvents which is documented at https://msdn.microsoft.com/en-us/library/windows/desktop/ms680109(v=vs.85).aspx The sink receives events that contain encapsulated COM method calls that are presented as ICallFrame objects, documented at https://msdn.microsoft.com/en-us/library/windows/desktop/ms683709(v=vs.85).aspx The thunk uses ICallFrame to manipulate the parameters of the stack frame. In particular, it does a lookup on the child ID, substitutes CHILDID_SELF in the stack frame, and forwards the call to the child interface pointer.

Let me know if you need any additional context.

Comment on attachment 8773952 [details] Bug 1272146: Add thunk for IAccessible property accesses that pass non-self child IDs; over all same comments as the other patches tons more comments, and clean up some stuff mostly use COM style a bit less ;) but given people really want this landed I guess its fine to fix those and we can clean it up next week. >+/* static */ HRESULT >+ChildIDThunk::Create(IInterceptorSink** aOutput) >+{ >+ MOZ_ASSERT(aOutput); >+ *aOutput = nullptr; >+ ChildIDThunk* thunk = new ChildIDThunk(); >+ HRESULT hr = thunk->QueryInterface(IID_IInterceptorSink, (void**)aOutput); >+ thunk->Release(); >+ return hr; >+} This function doesn't seem to do anything useful, is there a reason we can't just directly use the ctor? >+ >+ChildIDThunk::ChildIDThunk() >+ : mRefCnt(1) That works, but afaik its not the usually way and if you go with the previous thing doesn't work terribly well with assigning to a RefPtr. >+ChildIDThunk::~ChildIDThunk() >+{ >+} you could just use the default >+ChildIDThunk::QueryInterface(REFIID riid, void** ppv) >+{ >+ IUnknown* punk = nullptr; >+ if (!ppv) { >+ return E_INVALIDARG; >+ } >+ >+ if (riid == IID_IUnknown || riid == IID_ICallFrameEvents || >+ riid == IID_IInterceptorSink) { >+ punk = static_cast<IInterceptorSink*>(this); >+ } >+ >+ *ppv = punk; >+ if (!punk) { >+ return E_NOINTERFACE; >+ } >+ >+ punk->AddRef(); punc doesn't seem to be adding value over just using *ppv. >+enum IAccessibleVTableIndex hrm, I'm not sure this enum is more readable than putting the constants in the switch, but maybe you expect to need them elsewhere? Otherwise it would be a lot easier to check the correctness of this against the interface if the param index and the method index were next to each other. >+ eget_accDefaultAction = 20, >+ eaccLocation = 22, it looks like accSelect takes a variant for a child id? >+ eaccDoDefaultAction = 25, what about accNavigate? >+ChildIDThunk::IsMethodInteresting(const ULONG aMethodIdx, ULONG& aChildIDIdx) I'd rather not use references, I think the ideal thing would be return Maybe<uint32_t> or similar >+ChildIDThunk::IsChildIDSelf(ICallFrame* aFrame, const ULONG aChildIDIdx, >+ LONG& aOutChildID, VARTYPE& aOutVarType) similar dislike of mutable references >+{ >+ VARIANT paramValue; >+ HRESULT hr = aFrame->GetParam(aChildIDIdx, &paramValue); >+ if (FAILED(hr)) { should we assert here? >+ const bool isVariant = paramValue.vt & VT_VARIANT; >+ MOZ_ASSERT(isVariant); >+ if (!isVariant) { >+ return false; >+ } >+ >+ VARIANT& childID = isVariant ? *(paramValue.pvarVal) : paramValue; isVariant must be true >+ aOutChildID = childID.lVal; >+ aOutVarType = childID.vt; this always returns the same value right? >+ChildIDThunk::ValidateChildIDParamSize(const VARTYPE aVarType, >+ const ULONG aParamSize) >+{ >+ if (aVarType != VT_VARIANT) { >+ return false; >+ } >+ return aParamSize == sizeof(VARIANT); I'm not sure what the purpose of this function is, comment it? >+ * ICallFrame::SetParam always returns E_NOTIMPL, so we'll just have to work heh >+ChildIDThunk::SetChildIDParam(ICallFrame* aFrame, const ULONG aParamIdx, >+ const VARTYPE aVarType, const LONG aChildID) >+{ >+ PVOID stackBase = aFrame->GetStackLocation(); why not just use void*? >+ switch (aVarType) { don't we know that its VT_VARIANT and can just simplify to assert that? >+ChildIDThunk::ResolveTargetInterface(REFIID aIid, IUnknown** aOut) >+{ >+ MOZ_ASSERT(aOut); >+ if (!aOut) { >+ return E_INVALIDARG; >+ } We control all the callers right? why keep the null check? you could also use NonNull or whatever its called if you like. >+ChildIDThunk::OnCall(ICallFrame* aFrame) >+{ >+#if defined(MOZILLA_INTERNAL_API) >+ MOZ_ASSERT(NS_IsMainThread()); >+ MOZ_ASSERT(XRE_IsParentProcess()); >+#endif >+ >+ IID iid; >+ ULONG method; >+ HRESULT hr = aFrame->GetIIDAndMethod(&iid, &method); assert? >+ if (FAILED(hr)) { >+ return hr; >+ } >+ >+ RefPtr<IUnknown> target; >+ hr = ResolveTargetInterface(iid, getter_AddRefs(target)); >+ if (FAILED(hr)) { is the error code useful, or can we just have the function return already_AddRefed? >+ if (iid != IID_IAccessible || !IsMethodInteresting(method, childIDIdx) || >+ IsChildIDSelf(aFrame, childIDIdx, childID, vt)) { >+ // Just pass this call through to the root I'm not sure what root means, maybe say "only methods on IAccessible take a child id we need to mess with" or something? >+ // Otherwise we need to resolve the child node... >+ RefPtr<IAccessible> acc; >+ hr = target->QueryInterface(iid, (void**)getter_AddRefs(acc)); its pretty silly how we QI'd when resolving it, and now we do it again. >+ RefPtr<IDispatch> disp; >+ hr = acc->get_accChild(vChildID, getter_AddRefs(disp)); >+ if (FAILED(hr)) { >+ aFrame->SetReturnValue(hr); >+ return S_OK; >+ } >+ >+ RefPtr<IAccessible> directAccessible; >+ // Yes, given our implementation we could just downcast, but that's not very COMy I'm not really sure how much I care, this is pretty gross, but ok for now I guess. >+ // Now that we have the IAccessible for the child ID we need to replace it >+ // in the activation record with a self-referant child ID >+ hr = SetChildIDParam(aFrame, childIDIdx, vt, CHILDID_SELF); >+ MOZ_ASSERT(SUCCEEDED(hr)); >+ if (FAILED(hr)) { >+ return hr; is this error code useful? >+ChildIDThunk::SetInterceptor(IWeakReference* aInterceptor) >+{ >+ mInterceptor = aInterceptor; hrm, is the interceptor the thing owning this object? >+class ChildIDThunk : public mozilla::mscom::IInterceptorSink comment how it works? >+ RefPtr<mozilla::mscom::IWeakReference> mInterceptor; why do we need to use custom weak references? this is only used on the main thread of the parent process? and adding asserts about thread expectations would be good). >+RootAccessibleWrap::GetNativeInterface(void** aOutAccessible) >+{ >+ MOZ_ASSERT(aOutAccessible); >+ if (!aOutAccessible) { >+ return; >+ } is it ever called with null? >+ *aOutAccessible = nullptr; >+ >+ IAccessible* rawRootAccessible = nullptr; >+ RootAccessible::GetNativeInterface((void**)&rawRootAccessible); >+ >+ STAUniquePtr<IAccessible> rootAccessible(rawRootAccessible); you could move that down below the if, and this is always in the parent process STA right? so the type is kind of wierd. >+ if (!mozilla::BrowserTabsRemoteAutostart() || XRE_IsContentProcess()) { >+ // We only need to wrap this interface if our process is non-content e10s >+ if (aOutAccessible) { that should never be null right? >+ *aOutAccessible = rootAccessible.release(); >+ } >+ return; >+ } I think it would be easier to read if this part came before callling RootAccessible::GetNativeInterface() and you just static casted this since we know in that case its fine. >+ *aOutAccessible = interceptor.forget().take(); interceptor.forget(aOutAccessible) right?

(In reply to Trevor Saunders (:tbsaunde) from comment #9) > Comment on attachment 8773952 [details] > Bug 1272146: Add thunk for IAccessible property accesses that pass non-self > child IDs; > > over all same comments as the other patches tons more comments, and clean up > some stuff mostly use COM style a bit less ;) but given people really want > this landed I guess its fine to fix those and we can clean it up next week. > > >+/* static */ HRESULT > >+ChildIDThunk::Create(IInterceptorSink** aOutput) > >+{ > >+ MOZ_ASSERT(aOutput); > >+ *aOutput = nullptr; > >+ ChildIDThunk* thunk = new ChildIDThunk(); > >+ HRESULT hr = thunk->QueryInterface(IID_IInterceptorSink, (void**)aOutput); > >+ thunk->Release(); > >+ return hr; > >+} > > This function doesn't seem to do anything useful, is there a reason we can't > just directly use the ctor? > > >+ > >+ChildIDThunk::ChildIDThunk() > >+ : mRefCnt(1) > > That works, but afaik its not the usually way and if you go with the previous > thing doesn't work terribly well with assigning to a RefPtr. Replaced Create with direct use of constructor and adjusted mRefCnt initialization accordingly. > > >+ChildIDThunk::~ChildIDThunk() > >+{ > >+} > > you could just use the default Vestigial. Removed. > > >+ChildIDThunk::QueryInterface(REFIID riid, void** ppv) > >+{ > >+ IUnknown* punk = nullptr; > >+ if (!ppv) { > >+ return E_INVALIDARG; > >+ } > >+ > >+ if (riid == IID_IUnknown || riid == IID_ICallFrameEvents || > >+ riid == IID_IInterceptorSink) { > >+ punk = static_cast<IInterceptorSink*>(this); > >+ } > >+ > >+ *ppv = punk; > >+ if (!punk) { > >+ return E_NOINTERFACE; > >+ } > >+ > >+ punk->AddRef(); > > punc doesn't seem to be adding value over just using *ppv. I cleaned this up. > > >+enum IAccessibleVTableIndex > > hrm, I'm not sure this enum is more readable than putting the constants in > the > switch, but maybe you expect to need them elsewhere? Otherwise it would be a > lot easier to check the correctness of this against the interface if the > param > index and the method index were next to each other. > > >+ eget_accDefaultAction = 20, > >+ eaccLocation = 22, > > it looks like accSelect takes a variant for a child id? > > >+ eaccDoDefaultAction = 25, > > what about accNavigate? You're right, I missed those. Thanks. > > >+ChildIDThunk::IsMethodInteresting(const ULONG aMethodIdx, ULONG& aChildIDIdx) > > I'd rather not use references, I think the ideal thing would be return > Maybe<uint32_t> or similar Accepted, changed. > > >+ChildIDThunk::IsChildIDSelf(ICallFrame* aFrame, const ULONG aChildIDIdx, > >+ LONG& aOutChildID, VARTYPE& aOutVarType) > > similar dislike of mutable references > > >+{ > >+ VARIANT paramValue; > >+ HRESULT hr = aFrame->GetParam(aChildIDIdx, &paramValue); > >+ if (FAILED(hr)) { > > should we assert here? Done > > >+ const bool isVariant = paramValue.vt & VT_VARIANT; > >+ MOZ_ASSERT(isVariant); > >+ if (!isVariant) { > >+ return false; > >+ } > >+ > >+ VARIANT& childID = isVariant ? *(paramValue.pvarVal) : paramValue; > > isVariant must be true Vestigial. Fixed this and the propagation of the (constant) variant type. > > >+ aOutChildID = childID.lVal; > >+ aOutVarType = childID.vt; > > this always returns the same value right? Yep. Fixed. > > >+ChildIDThunk::ValidateChildIDParamSize(const VARTYPE aVarType, > >+ const ULONG aParamSize) > >+{ > >+ if (aVarType != VT_VARIANT) { > >+ return false; > >+ } > >+ return aParamSize == sizeof(VARIANT); > > I'm not sure what the purpose of this function is, comment it? Mostly vestigial. Replaced in favour of just comparing to sizeof(VARIANT) directly. > > >+ * ICallFrame::SetParam always returns E_NOTIMPL, so we'll just have to work > > heh > > >+ChildIDThunk::SetChildIDParam(ICallFrame* aFrame, const ULONG aParamIdx, > >+ const VARTYPE aVarType, const LONG aChildID) > >+{ > >+ PVOID stackBase = aFrame->GetStackLocation(); > > why not just use void*? Fixed. > > >+ switch (aVarType) { > > don't we know that its VT_VARIANT and can just simplify to assert that? > > >+ChildIDThunk::ResolveTargetInterface(REFIID aIid, IUnknown** aOut) > >+{ > >+ MOZ_ASSERT(aOut); > >+ if (!aOut) { > >+ return E_INVALIDARG; > >+ } > > We control all the callers right? why keep the null check? you could also use > NonNull or whatever its called if you like. Fixed. > > >+ChildIDThunk::OnCall(ICallFrame* aFrame) > >+{ > >+#if defined(MOZILLA_INTERNAL_API) > >+ MOZ_ASSERT(NS_IsMainThread()); > >+ MOZ_ASSERT(XRE_IsParentProcess()); > >+#endif > >+ > >+ IID iid; > >+ ULONG method; > >+ HRESULT hr = aFrame->GetIIDAndMethod(&iid, &method); > > assert? Fixed. > > >+ if (FAILED(hr)) { > >+ return hr; > >+ } > >+ > >+ RefPtr<IUnknown> target; > >+ hr = ResolveTargetInterface(iid, getter_AddRefs(target)); > >+ if (FAILED(hr)) { > > is the error code useful, or can we just have the function return > already_AddRefed? It is useful in the sense that it can be returned and propagated up into the AT, thus giving a more useful error code. > > >+ if (iid != IID_IAccessible || !IsMethodInteresting(method, childIDIdx) || > >+ IsChildIDSelf(aFrame, childIDIdx, childID, vt)) { > >+ // Just pass this call through to the root > > I'm not sure what root means, maybe say "only methods on IAccessible take a > child id we need to mess with" or something? Clarified. > > >+ // Otherwise we need to resolve the child node... > >+ RefPtr<IAccessible> acc; > >+ hr = target->QueryInterface(iid, (void**)getter_AddRefs(acc)); > > its pretty silly how we QI'd when resolving it, and now we do it again. We had to because it was initially QI'd to be an IInterceptor, so we still need to QI to IAccessible. > > >+ RefPtr<IDispatch> disp; > >+ hr = acc->get_accChild(vChildID, getter_AddRefs(disp)); > >+ if (FAILED(hr)) { > >+ aFrame->SetReturnValue(hr); > >+ return S_OK; > >+ } > >+ > >+ RefPtr<IAccessible> directAccessible; > >+ // Yes, given our implementation we could just downcast, but that's not very COMy > > I'm not really sure how much I care, this is pretty gross, but ok for now I > guess. > > >+ // Now that we have the IAccessible for the child ID we need to replace it > >+ // in the activation record with a self-referant child ID > >+ hr = SetChildIDParam(aFrame, childIDIdx, vt, CHILDID_SELF); > >+ MOZ_ASSERT(SUCCEEDED(hr)); > >+ if (FAILED(hr)) { > >+ return hr; > > is this error code useful? Same thing, the error code can be propagated to the client. > > >+ChildIDThunk::SetInterceptor(IWeakReference* aInterceptor) > >+{ > >+ mInterceptor = aInterceptor; > > hrm, is the interceptor the thing owning this object? Correct. > > >+class ChildIDThunk : public mozilla::mscom::IInterceptorSink > > comment how it works? > > >+ RefPtr<mozilla::mscom::IWeakReference> mInterceptor; > > why do we need to use custom weak references? this is only used on the main > thread of the parent process? and adding asserts about thread expectations > would > be good). We need ones that are thread-safe. Not for this context where everything is main thread, but this is the same code that is used in the content MTA where this _is_ an issue. > > >+RootAccessibleWrap::GetNativeInterface(void** aOutAccessible) > >+{ > >+ MOZ_ASSERT(aOutAccessible); > >+ if (!aOutAccessible) { > >+ return; > >+ } > > is it ever called with null? Nope, simplified. > > >+ *aOutAccessible = nullptr; > >+ > >+ IAccessible* rawRootAccessible = nullptr; > >+ RootAccessible::GetNativeInterface((void**)&rawRootAccessible); > >+ > >+ STAUniquePtr<IAccessible> rootAccessible(rawRootAccessible); > > you could move that down below the if, and this is always in the parent > process > STA right? so the type is kind of wierd. I got rid of this and just used RefPtr directly since it's all main thread as you pointed out. I'm not sure what I was thinking there. I was probably still in content process mode. > > >+ if (!mozilla::BrowserTabsRemoteAutostart() || XRE_IsContentProcess()) { > >+ // We only need to wrap this interface if our process is non-content e10s > >+ if (aOutAccessible) { > > that should never be null right? Yup. Simplified. > > >+ *aOutAccessible = rootAccessible.release(); > >+ } > >+ return; > >+ } > > I think it would be easier to read if this part came before callling > RootAccessible::GetNativeInterface() and you just static casted this since we > know in that case its fine. This should be much cleaner now because RefPtr. > > > >+ *aOutAccessible = interceptor.forget().take(); > > interceptor.forget(aOutAccessible) right? Fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d84b4dd26a403675c8059e5d073ebc65d5fc07eb Bug 1272146: Add thunk for IAccessible property accesses that pass non-self child IDs; r=tbsaunde

Had to back this out to successfully back bug 1268544 out: https://hg.mozilla.org/integration/mozilla-inbound/rev/a832f738b606f90c9afd10f61728bddbec605674

https://hg.mozilla.org/integration/mozilla-inbound/rev/42ac31b54b3797949ea2747898e66a7fd7f42678 Bug 1272146: Add thunk for IAccessible property accesses that pass non-self child IDs; r=tbsaunde

I guess we can let it ride the train, fill free to ping me if you disagre.