The first cut of this isn't going to be pretty, but is necessary to be able to build a version 0.1 of this code.

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

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

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

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

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

Comment on attachment 8773947 [details] Bug 1268544: Refactor ProxyAccessible and dependencies; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66574/diff/1-2/

Comment on attachment 8773948 [details] Bug 1268544: Remove proxy-specific code paths from IAccessible and IA2 wrappers; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66576/diff/1-2/

Comment on attachment 8773949 [details] Bug 1268544: Disable proxy code paths in accessible/xpcom pending resolution of bug 1288839; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66578/diff/1-2/

Comment on attachment 8773950 [details] Bug 1268544: Integrate remote COM objects into a11y code; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66580/diff/1-2/

Comment on attachment 8773950 [details] Bug 1268544: Integrate remote COM objects into a11y code; +++ b/accessible/generic/Accessible.cpp Thanks for fixing up my whitespace sillyness, though it would have been even nicer in its own patch. +if CONFIG['OS_ARCH'] == 'WINNT': + LOCAL_INCLUDES += [ + '/accessible/ipc/win', + ] +else: + LOCAL_INCLUDES += [ + '/accessible/ipc/other', its kind of unfortunate how here "other" means !windows where in accessible/ it means "platform without an accessibility API" +DocAccessibleChildBase::HideEvent(const uint64_t& aRootID, const bool& aFromUser) +{ + SendHideEvent(aRootID, aFromUser); +} is this going to do more in the future? could you add it then? + const IAccessibleHolder& proxyStream = newChild.COMProxy(); + RefPtr<IAccessible> comPtr(proxyStream.Get()); + if (!comPtr) { + NS_ERROR("Could not obtain remote IAccessible interface"); + return 0; + } + ProxyAccessible* newProxy = personally I'd prefer a blank line after the } return doc->GetXPCAccessible(aProxy); } + +#if defined(XP_WIN) +bool +DocAccessibleParent::RecvCOMProxy(const IAccessibleHolder& aCOMProxy, + IAccessibleHolder* aParentCOMProxy) What exactly is this about? it seems to recieve one proxy and return a different one? +{ + RefPtr<IAccessible> ptr(aCOMProxy.Get()); + SetCOMInterface(ptr); + + Accessible* xpAccessible = OuterDocOfRemoteBrowser(); that's a weird name, maybe use outerDoc? + IAccessible* rawNative = nullptr; + if (xpAccessible) { + xpAccessible->GetNativeInterface((void**) &rawNative); Is there a reason to use the function? I'd kind of prefer it went away, and static casting should be fine. + } + IAccessibleHolder::COMPtrType native(rawNative); same thing as earlier about whitespace after } + aParentCOMProxy->Set(Move(native)); using the extra local seems kind of silly, but whatever +namespace mozilla { +namespace a11y { + +IAccessibleHolder +CreateHolderFromAccessible(Accessible* aAccToWrap) +{ + MOZ_ASSERT(aAccToWrap && NS_IsMainThread()); + if (!aAccToWrap) { + return nullptr; + } + + IAccessible* rawNative = nullptr; + aAccToWrap->GetNativeInterface((void**)&rawNative); + MOZ_ASSERT(rawNative); same comment on GetNativeInterface() I guess you do want to Addref, but it seems like we could create a function to do that without needing a virtual call, that also has better type safety. GetNativeInterface AddRefing is actually suprising to me because it doesn't on other platforms. + if (!rawNative) { + return nullptr; + } + STAUniquePtr<IAccessible> iaToProxy(rawNative); + + IAccessible* rawIntercepted = nullptr; + HRESULT hr = MainThreadHandoff::WrapInterface(iaToProxy, &rawIntercepted); + MOZ_ASSERT(SUCCEEDED(hr)); seems like a signal that function should just return a pointer or null, but anyway. + if (FAILED(hr)) { + return nullptr; + } + IAccessibleHolder::COMPtrType iaIntercepted(rawIntercepted); + return IAccessibleHolder(Move(iaIntercepted)); Seems like there should be a simpler way of doing that, and actually why do we need the different types in the first place? +DocAccessibleChild::SendCOMProxy(const IAccessibleHolder& aProxy) +{ + IAccessibleHolder parentProxy; + PDocAccessibleChild::SendCOMProxy(aProxy, &parentProxy); + mParentProxy.reset(parentProxy.Release()); that can just be mParentProxy = parentProxy right? + // For now we'll add the command to send the proxy here. This might move to + // PDocAccessible constructor in PBrowser. + sync COMProxy(IAccessibleHolder aDocCOMProxy) + returns(IAccessibleHolder aParentCOMProxy); huh, the use of this will proably deserve some consideration since the child will be blocked. + EXPORTS.mozilla.a11y += [ + 'COMPtrTypes.h', + 'DocAccessibleChild.h', + ] + + SOURCES += [ UNIFIED? or does the windows headers break that?

r=me with the second question answered. - if (acc->IsProxy()) { - // XXX evaluate performance of collecting all relation targets. - nsTArray<RelationType> types; - nsTArray<nsTArray<ProxyAccessible*>> targetSets; - acc->Proxy()->Relations(&types, &targetSets); - *aNRelations = types.Length(); - return S_OK; - } + MOZ_ASSERT(!acc->IsProxy()); I'd really like to see us stop creating these accessibles where IsProxy() is true, and then remove that machinary. @@ -1023,6 +923,22 @@ AccessibleWrap::accNavigate( if (accessible->IsDefunct()) return CO_E_OBJNOTCONNECTED; + // Make sure that varStart != CHILDID_SELF so we don't infinitely recurse + if (accessible->IsProxy() && varStart.vt == VT_I4 && + varStart.lVal != CHILDID_SELF) { + // Now that we have the starting object, delegate this request to that + // object as a self-relative request... + RefPtr<IAccessible> comProxy; + accessible->GetNativeInterface((void**) getter_AddRefs(comProxy)); + if (!comProxy) { + return E_UNEXPECTED; + } + VARIANT selfChildId; + selfChildId.vt = VT_I4; + selfChildId.lVal = CHILDID_SELF; + return comProxy->accNavigate(navDir, selfChildId, pvarEndUpAt); + } I'd like to understand how this ends up getting called on accessibles where IsProxy() is true, and why it isn't an assert like the other places.

+bool +ProxyAccessible::GetCOMInterface(void** aOutAccessible) const +{ + if (!aOutAccessible) { + return false; + } + RefPtr<IAccessible> addRefed = mCOMProxy; + addRefed.forget(aOutAccessible); + return !!mCOMProxy; is there a reason we can't make this IAccessible* GetCOMInterface() ? or wrap that in already_AddRefed? +ProxyAccessible::State() const +{ + VARIANT id; + id.vt = VT_I4; + id.lVal = CHILDID_SELF; these lines are kind of repeditive, and I think in future we could have a tiny VARIANT wrapper that handles them. + VARIANT varState; + DebugOnly<HRESULT> hr = acc->get_accState(id, &varState); + MOZ_ASSERT(SUCCEEDED(hr)); why is this one an assert and the others treat failure as an expected case? + // The remaining code should essentially be the inverse of the + // ia2Accessible::get_locale conversion to IA2Locale. I'll just believe it is, after all this will basically only be used for tests right? +ConvertBSTRAttributesToArray(const nsAString& aStr, + nsTArray<Attribute>& aAttrs) personally I'd prefer a pointer if its mutable. + ProxyAccessible(uint64_t aID, ProxyAccessible* aParent, + DocAccessibleParent* aDoc, role aRole, uint32_t aInterfaces, + RefPtr<IAccessible>& aIAccessible) I'd rather that not be a mutable reference. + void SetCOMInterface(RefPtr<IAccessible>& aIAccessible) same here

+++ b/accessible/base/NotificationController.cpp @@ -428,6 +428,10 @@ NotificationController::WillRefresh(mozilla::TimeStamp aTime) if (tabChild) { static_cast<TabChild*>(tabChild.get())-> SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc, id); +#if defined(XP_WIN) + IAccessibleHolder holder(CreateHolderFromAccessible(childDoc)); + ipcDoc->SendCOMProxy(holder); +#endif I'm not sure I understand what this method does, but this seems wrong. As I understand it the purpose is to connect the subtree in the content process to the tree in the parent. However all the docs here should be the child of another document in the content process, not an object in the parent. +static const mozilla::mscom::ArrayData sPlatformChildArrayData[] = { Some comments would help if you aren't planning to make this better soon which sounds rather hard. + {IID_IEnumVARIANT, 3, 1, VT_DISPATCH, IID_IDispatch, 2}, + {IID_IAccessible2, 30, 1, VT_UNKNOWN | VT_BYREF, IID_IAccessibleRelation, 2}, + {IID_IAccessibleRelation, 7, 1, VT_UNKNOWN | VT_BYREF, IID_IUnknown, 2} I think there's more at least on IAccessible2_2 and IAccessibleTable[2] +PlatformChild::~PlatformChild() +{ +} Why are we defining it if it does nothing? +class PlatformChild I'm not sure I understand the name of this class, and really it seems kind of silly and we could just have 3 StaticAutoPtrs in DocAccessibleParent.cpp but whatever. +DocAccessibleWrap::get_accParent( + /* [retval][out] */ IDispatch __RPC_FAR *__RPC_FAR *ppdispParent) +{ + if (!IPCDoc()) { + return DocAccessible::get_accParent(ppdispParent); + } + + IAccessible* dispParent = IPCDoc()->GetParentIAccessible(); + MOZ_ASSERT(dispParent); + if (!dispParent) { + return E_UNEXPECTED; This might well be correct for documents that are children of other documents in the content process, but it would probably be faster to only special case this when Parent() returns null. + if (BrowserTabsRemoteAutostart()) { + mscom::InterceptorLog::Init(); + UniquePtr<RegisteredProxy> regProxy( + mscom::RegisterProxy(L"ia2marshal.dll")); + gRegProxy = regProxy.release(); + UniquePtr<RegisteredProxy> regAccTlb( + mscom::RegisterTypelib(L"oleacc.dll", + RegistrationFlags::eUseSystemDirectory)); + gRegAccTlb = regAccTlb.release(); + UniquePtr<RegisteredProxy> regMiscTlb( + mscom::RegisterTypelib(L"Accessible.tlb")); + gRegMiscTlb = regMiscTlb.release(); Its kind of weird so would be nice to document somehow how in the child we need to use EnsureMTA, but don't here.

Comment on attachment 8773949 [details] Bug 1268544: Disable proxy code paths in accessible/xpcom pending resolution of bug 1288839; an easier patch to read would be just not creating xpcom objects in ToXPC(), but whatever. However you should be aware there's work going on to add more proxy paths to these interface implementations.

(In reply to Trevor Saunders (:tbsaunde) from comment #10) > Comment on attachment 8773950 [details] > Bug 1268544: Integrate remote COM objects into a11y code; > > +++ b/accessible/generic/Accessible.cpp > > Thanks for fixing up my whitespace sillyness, though it would have been even > nicer in its own patch. > > +if CONFIG['OS_ARCH'] == 'WINNT': > + LOCAL_INCLUDES += [ > + '/accessible/ipc/win', > + ] > +else: > + LOCAL_INCLUDES += [ > + '/accessible/ipc/other', > > its kind of unfortunate how here "other" means !windows where in accessible/ > it means "platform without an accessibility API" Is there anything actionable that you'd like to see out of this? > > +DocAccessibleChildBase::HideEvent(const uint64_t& aRootID, const bool& > aFromUser) > +{ > + SendHideEvent(aRootID, aFromUser); > +} > > is this going to do more in the future? could you add it then? Not really, maybe was more for consistency with ShowEvent? This could be a vestigial change that I ended up not needing. I can just remove it if you like. > > + const IAccessibleHolder& proxyStream = newChild.COMProxy(); > + RefPtr<IAccessible> comPtr(proxyStream.Get()); > + if (!comPtr) { > + NS_ERROR("Could not obtain remote IAccessible interface"); > + return 0; > + } > + ProxyAccessible* newProxy = > > personally I'd prefer a blank line after the } Agreed, fixed. > > return doc->GetXPCAccessible(aProxy); > } > + > +#if defined(XP_WIN) > +bool > +DocAccessibleParent::RecvCOMProxy(const IAccessibleHolder& aCOMProxy, > + IAccessibleHolder* aParentCOMProxy) > > What exactly is this about? it seems to recieve one proxy and return a > different one? The parent returns a COM proxy to the content process. The content process uses that proxy as the result when the AT does a content to chrome traversal. > > +{ > + RefPtr<IAccessible> ptr(aCOMProxy.Get()); > + SetCOMInterface(ptr); > + > + Accessible* xpAccessible = OuterDocOfRemoteBrowser(); > > that's a weird name, maybe use outerDoc? Sure, fixed. > > + IAccessible* rawNative = nullptr; > + if (xpAccessible) { > + xpAccessible->GetNativeInterface((void**) &rawNative); > > Is there a reason to use the function? I'd kind of prefer it went away, and > static casting should be fine. Yeah, we can do that, though of course we also need to AddRef(). It was more for consistency's sake. Do you mind if we address that in bug 1289852 along with the other cleanup work? > > + } > + IAccessibleHolder::COMPtrType native(rawNative); > > same thing as earlier about whitespace after } OK. > > + aParentCOMProxy->Set(Move(native)); > > using the extra local seems kind of silly, but whatever Cleaned up. > > +namespace mozilla { > +namespace a11y { > + > +IAccessibleHolder > +CreateHolderFromAccessible(Accessible* aAccToWrap) > +{ > + MOZ_ASSERT(aAccToWrap && NS_IsMainThread()); > + if (!aAccToWrap) { > + return nullptr; > + } > + > + IAccessible* rawNative = nullptr; > + aAccToWrap->GetNativeInterface((void**)&rawNative); > + MOZ_ASSERT(rawNative); > > same comment on GetNativeInterface() I guess you do want to Addref, but it > seems like we could create a function to do that without needing a virtual > call, that also has better type safety. GetNativeInterface AddRefing is > actually suprising to me because it doesn't on other platforms. Do you mind if we also take care of this one in bug 1289852? I think it would probably be safest to take a holistic view of the entire GetNativeInterface situation without worrying about how it affects all of the other patches that are pending. > > + if (!rawNative) { > + return nullptr; > + } > + STAUniquePtr<IAccessible> iaToProxy(rawNative); > + > + IAccessible* rawIntercepted = nullptr; > + HRESULT hr = MainThreadHandoff::WrapInterface(iaToProxy, &rawIntercepted); > + MOZ_ASSERT(SUCCEEDED(hr)); > > seems like a signal that function should just return a pointer or null, but > anyway. > > + if (FAILED(hr)) { > + return nullptr; > + } > + IAccessibleHolder::COMPtrType iaIntercepted(rawIntercepted); > + return IAccessibleHolder(Move(iaIntercepted)); > > Seems like there should be a simpler way of doing that, and actually why do > we need the different types in the first place? This is mainly an artifact of IAccessibleHolder::COMPtrType being UniquePtr, which doesn't support a getter_AddRefs()-style assignment, so we need to get the raw pointer first, then create the smart pointer, then construct the IAccessibleHolder. I'll add a note to bug 1287167 to clean this up. > > +DocAccessibleChild::SendCOMProxy(const IAccessibleHolder& aProxy) > +{ > + IAccessibleHolder parentProxy; > + PDocAccessibleChild::SendCOMProxy(aProxy, &parentProxy); > + mParentProxy.reset(parentProxy.Release()); > > that can just be mParentProxy = parentProxy right? Unfortunately no, since mParentProxy is a ProxyUniquePtr but parentProxy is a COMPtrHolder. I have made a note in bug 1287167 to make this nicer as well. > > + // For now we'll add the command to send the proxy here. This might move > to > + // PDocAccessible constructor in PBrowser. > + sync COMProxy(IAccessibleHolder aDocCOMProxy) > + returns(IAccessibleHolder aParentCOMProxy); > > huh, the use of this will proably deserve some consideration since the child > will be blocked. Bug 1273635 takes care of that :-) > > + EXPORTS.mozilla.a11y += [ > + 'COMPtrTypes.h', > + 'DocAccessibleChild.h', > + ] > + > + SOURCES += [ > > UNIFIED? or does the windows headers break that? There's a bunch of COM stuff that messes up UNIFIED.

(In reply to Trevor Saunders (:tbsaunde) from comment #11) > r=me with the second question answered. > > - if (acc->IsProxy()) { > - // XXX evaluate performance of collecting all relation targets. > - nsTArray<RelationType> types; > - nsTArray<nsTArray<ProxyAccessible*>> targetSets; > - acc->Proxy()->Relations(&types, &targetSets); > - *aNRelations = types.Length(); > - return S_OK; > - } > + MOZ_ASSERT(!acc->IsProxy()); > > I'd really like to see us stop creating these accessibles where IsProxy() is > true, and then remove that machinary. As discussed at last week's meeting, shall we land this as-is and follow up on that point in bug 1289852? > > @@ -1023,6 +923,22 @@ AccessibleWrap::accNavigate( > if (accessible->IsDefunct()) > return CO_E_OBJNOTCONNECTED; > > + // Make sure that varStart != CHILDID_SELF so we don't infinitely recurse > + if (accessible->IsProxy() && varStart.vt == VT_I4 && > + varStart.lVal != CHILDID_SELF) { > + // Now that we have the starting object, delegate this request to that > + // object as a self-relative request... > + RefPtr<IAccessible> comProxy; > + accessible->GetNativeInterface((void**) getter_AddRefs(comProxy)); > + if (!comProxy) { > + return E_UNEXPECTED; > + } > + VARIANT selfChildId; > + selfChildId.vt = VT_I4; > + selfChildId.lVal = CHILDID_SELF; > + return comProxy->accNavigate(navDir, selfChildId, pvarEndUpAt); > + } > > I'd like to understand how this ends up getting called on accessibles where > IsProxy() is true, and why it isn't an assert like the other places. When IsProxy() is true, the actual GetNativeInterface called is one of the overrides in ProxyWrappers.h. We're actually operating on a COM proxy to the content IAccessible in this case.

(In reply to Trevor Saunders (:tbsaunde) from comment #12) > +bool > +ProxyAccessible::GetCOMInterface(void** aOutAccessible) const > +{ > + if (!aOutAccessible) { > + return false; > + } > + RefPtr<IAccessible> addRefed = mCOMProxy; > + addRefed.forget(aOutAccessible); > + return !!mCOMProxy; > > is there a reason we can't make this IAccessible* GetCOMInterface() ? or > wrap that in already_AddRefed? No reason other than to be consistent with the style used by AccessibleWrap::GetNativeInterface. > > +ProxyAccessible::State() const > +{ > + VARIANT id; > + id.vt = VT_I4; > + id.lVal = CHILDID_SELF; > > these lines are kind of repeditive, and I think in future we could have a > tiny VARIANT wrapper that handles them. > Added to bug 1287167 > + VARIANT varState; > + DebugOnly<HRESULT> hr = acc->get_accState(id, &varState); > + MOZ_ASSERT(SUCCEEDED(hr)); > > why is this one an assert and the others treat failure as an expected case? That is vestigial and I can remove it. I added those to ensure that get_AccState was working as I expected. > > + // The remaining code should essentially be the inverse of the > + // ia2Accessible::get_locale conversion to IA2Locale. > > I'll just believe it is, after all this will basically only be used for > tests right? Correct. > > +ConvertBSTRAttributesToArray(const nsAString& aStr, > + nsTArray<Attribute>& aAttrs) > > personally I'd prefer a pointer if its mutable. Fixed. > > + ProxyAccessible(uint64_t aID, ProxyAccessible* aParent, > + DocAccessibleParent* aDoc, role aRole, uint32_t > aInterfaces, > + RefPtr<IAccessible>& aIAccessible) > > I'd rather that not be a mutable reference. Fixed. > > + void SetCOMInterface(RefPtr<IAccessible>& aIAccessible) > > same here Fixed.

(In reply to Trevor Saunders (:tbsaunde) from comment #13) > +++ b/accessible/base/NotificationController.cpp > @@ -428,6 +428,10 @@ NotificationController::WillRefresh(mozilla::TimeStamp > aTime) > if (tabChild) { > static_cast<TabChild*>(tabChild.get())-> > SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc, id); > +#if defined(XP_WIN) > + IAccessibleHolder holder(CreateHolderFromAccessible(childDoc)); > + ipcDoc->SendCOMProxy(holder); > +#endif > > I'm not sure I understand what this method does, but this seems wrong. As I > understand it the purpose is to connect the subtree in the content process to > the tree in the parent. However all the docs here should be the child of > another document in the content process, not an object in the parent. I added it out of concern that we're constructing a PDocAccessible, which seemed to me like it shouldn't exist in the parent without having a valid COM proxy. Is there somebody we need to needinfo about this, or should I just revert my change? > > +static const mozilla::mscom::ArrayData sPlatformChildArrayData[] = { > > Some comments would help if you aren't planning to make this better soon > which sounds rather hard. Sure. > > + {IID_IEnumVARIANT, 3, 1, VT_DISPATCH, IID_IDispatch, 2}, > + {IID_IAccessible2, 30, 1, VT_UNKNOWN | VT_BYREF, IID_IAccessibleRelation, > 2}, > + {IID_IAccessibleRelation, 7, 1, VT_UNKNOWN | VT_BYREF, IID_IUnknown, 2} > > I think there's more at least on IAccessible2_2 and IAccessibleTable[2] Yeah, there are a few more that use size_is but not length_is because they are out params instead of inout. I filed bug 1292452 to add these... my initial effort to add them is being foiled by some strange behaviors in the interceptor. > > +PlatformChild::~PlatformChild() > +{ > +} > > Why are we defining it if it does nothing? Fair enough. I think at one point I had stuff in there but now it is vestigial. > > +class PlatformChild > > I'm not sure I understand the name of this class, and really it seems kind of > silly and we could just have 3 StaticAutoPtrs in DocAccessibleParent.cpp but > whatever. Name was supposed to be the content process analog to accessible/base/Platform > > +DocAccessibleWrap::get_accParent( > + /* [retval][out] */ IDispatch __RPC_FAR *__RPC_FAR *ppdispParent) > +{ > + if (!IPCDoc()) { > + return DocAccessible::get_accParent(ppdispParent); > + } > + > + IAccessible* dispParent = IPCDoc()->GetParentIAccessible(); > + MOZ_ASSERT(dispParent); > + if (!dispParent) { > + return E_UNEXPECTED; > > This might well be correct for documents that are children of other documents > in the content process, but it would probably be faster to only special case > this when Parent() returns null. Just to be clear, you're saying that on the line above the GetParentIAccessible call I should add something like: if (!Parent()) { *ppdispParent = nullptr; return S_FALSE; } > > > + if (BrowserTabsRemoteAutostart()) { > + mscom::InterceptorLog::Init(); > + UniquePtr<RegisteredProxy> regProxy( > + mscom::RegisterProxy(L"ia2marshal.dll")); > + gRegProxy = regProxy.release(); > + UniquePtr<RegisteredProxy> regAccTlb( > + mscom::RegisterTypelib(L"oleacc.dll", > + RegistrationFlags::eUseSystemDirectory)); > + gRegAccTlb = regAccTlb.release(); > + UniquePtr<RegisteredProxy> regMiscTlb( > + mscom::RegisterTypelib(L"Accessible.tlb")); > + gRegMiscTlb = regMiscTlb.release(); > > Its kind of weird so would be nice to document somehow how in the child we > need > to use EnsureMTA, but don't here. Sure. Will comment.

(In reply to Aaron Klotz [:aklotz] from comment #18) > (In reply to Trevor Saunders (:tbsaunde) from comment #13) > > > > +DocAccessibleWrap::get_accParent( > > + /* [retval][out] */ IDispatch __RPC_FAR *__RPC_FAR *ppdispParent) > > +{ > > + if (!IPCDoc()) { > > + return DocAccessible::get_accParent(ppdispParent); > > + } > > + > > + IAccessible* dispParent = IPCDoc()->GetParentIAccessible(); > > + MOZ_ASSERT(dispParent); > > + if (!dispParent) { > > + return E_UNEXPECTED; > > > > This might well be correct for documents that are children of other documents > > in the content process, but it would probably be faster to only special case > > this when Parent() returns null. > > Just to be clear, you're saying that on the line above the > GetParentIAccessible call I should add something like: > if (!Parent()) { > *ppdispParent = nullptr; > return S_FALSE; > } Or I suppose I should just put it right at the top of get_accParent()...

Comment on attachment 8773946 [details] Bug 1268544: Refactor PDocAccessible and its dependencies, and add code to integrate remote COM objects; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66572/diff/1-2/

Comment on attachment 8773947 [details] Bug 1268544: Refactor ProxyAccessible and dependencies; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66574/diff/2-3/

Comment on attachment 8773948 [details] Bug 1268544: Remove proxy-specific code paths from IAccessible and IA2 wrappers; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66576/diff/2-3/

Comment on attachment 8773949 [details] Bug 1268544: Disable proxy code paths in accessible/xpcom pending resolution of bug 1288839; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66578/diff/2-3/

Comment on attachment 8773950 [details] Bug 1268544: Integrate remote COM objects into a11y code; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66580/diff/2-3/

> > + LOCAL_INCLUDES += [ > > + '/accessible/ipc/win', > > + ] > > +else: > > + LOCAL_INCLUDES += [ > > + '/accessible/ipc/other', > > > > its kind of unfortunate how here "other" means !windows where in accessible/ > > it means "platform without an accessibility API" > > Is there anything actionable that you'd like to see out of this? I don't think so. > > > > +DocAccessibleChildBase::HideEvent(const uint64_t& aRootID, const bool& > > aFromUser) > > +{ > > + SendHideEvent(aRootID, aFromUser); > > +} > > > > is this going to do more in the future? could you add it then? > > Not really, maybe was more for consistency with ShowEvent? This could be a > vestigial change that I ended up not needing. I can just remove it if you > like. that seems a little less confusing / less to track through. > > return doc->GetXPCAccessible(aProxy); > > } > > + > > +#if defined(XP_WIN) > > +bool > > +DocAccessibleParent::RecvCOMProxy(const IAccessibleHolder& aCOMProxy, > > + IAccessibleHolder* aParentCOMProxy) > > > > What exactly is this about? it seems to recieve one proxy and return a > > different one? > > The parent returns a COM proxy to the content process. The content process > uses that proxy as the result when the AT does a content to chrome traversal. can we at least document that the argument is the proxy for the document, and the return value is a proxy for the outer doc accessible that is the parent of the document? It'd be nice to have that in the method name too, but I'm not sure what a good name is. > > > > + IAccessible* rawNative = nullptr; > > + if (xpAccessible) { > > + xpAccessible->GetNativeInterface((void**) &rawNative); > > > > Is there a reason to use the function? I'd kind of prefer it went away, and > > static casting should be fine. > > Yeah, we can do that, though of course we also need to AddRef(). It was more > for consistency's sake. Do you mind if we address that in bug 1289852 along > with the other cleanup work? sure, it seems to fit in with cleaning up the accessibles where IsProxy() is true. > > > > +namespace mozilla { > > +namespace a11y { > > + > > +IAccessibleHolder > > +CreateHolderFromAccessible(Accessible* aAccToWrap) > > +{ > > + MOZ_ASSERT(aAccToWrap && NS_IsMainThread()); > > + if (!aAccToWrap) { > > + return nullptr; > > + } > > + > > + IAccessible* rawNative = nullptr; > > + aAccToWrap->GetNativeInterface((void**)&rawNative); > > + MOZ_ASSERT(rawNative); > > > > same comment on GetNativeInterface() I guess you do want to Addref, but it > > seems like we could create a function to do that without needing a virtual > > call, that also has better type safety. GetNativeInterface AddRefing is > > actually suprising to me because it doesn't on other platforms. > > Do you mind if we also take care of this one in bug 1289852? I think it > would probably be safest to take a holistic view of the entire > GetNativeInterface situation without worrying about how it affects all of > the other patches that are pending. yeah, that situation is complicated now, and I'll try to think about what's going on some for the next round of reviews. (In reply to Aaron Klotz [:aklotz] from comment #16) > (In reply to Trevor Saunders (:tbsaunde) from comment #11) > > r=me with the second question answered. > > > > - if (acc->IsProxy()) { > > - // XXX evaluate performance of collecting all relation targets. > > - nsTArray<RelationType> types; > > - nsTArray<nsTArray<ProxyAccessible*>> targetSets; > > - acc->Proxy()->Relations(&types, &targetSets); > > - *aNRelations = types.Length(); > > - return S_OK; > > - } > > + MOZ_ASSERT(!acc->IsProxy()); > > > > I'd really like to see us stop creating these accessibles where IsProxy() is > > true, and then remove that machinary. > > As discussed at last week's meeting, shall we land this as-is and follow up > on that point in bug 1289852? I think it might actually be simpler reviews the other way, but I'll live. > > @@ -1023,6 +923,22 @@ AccessibleWrap::accNavigate( > > if (accessible->IsDefunct()) > > return CO_E_OBJNOTCONNECTED; > > > > + // Make sure that varStart != CHILDID_SELF so we don't infinitely recurse > > + if (accessible->IsProxy() && varStart.vt == VT_I4 && > > + varStart.lVal != CHILDID_SELF) { > > + // Now that we have the starting object, delegate this request to that > > + // object as a self-relative request... > > + RefPtr<IAccessible> comProxy; > > + accessible->GetNativeInterface((void**) getter_AddRefs(comProxy)); > > + if (!comProxy) { > > + return E_UNEXPECTED; > > + } > > + VARIANT selfChildId; > > + selfChildId.vt = VT_I4; > > + selfChildId.lVal = CHILDID_SELF; > > + return comProxy->accNavigate(navDir, selfChildId, pvarEndUpAt); > > + } > > > > I'd like to understand how this ends up getting called on accessibles where > > IsProxy() is true, and why it isn't an assert like the other places. > > When IsProxy() is true, the actual GetNativeInterface called is one of the > overrides in ProxyWrappers.h. We're actually operating on a COM proxy to the > content IAccessible in this case. So, I still don't understand how that means accNavigate can be called on a non com proxy accessible where IsProxy() is true. Those are only used within gecko right and should never escape right? (In reply to Aaron Klotz [:aklotz] from comment #17) > (In reply to Trevor Saunders (:tbsaunde) from comment #12) > > +bool > > +ProxyAccessible::GetCOMInterface(void** aOutAccessible) const > > +{ > > + if (!aOutAccessible) { > > + return false; > > + } > > + RefPtr<IAccessible> addRefed = mCOMProxy; > > + addRefed.forget(aOutAccessible); > > + return !!mCOMProxy; > > > > is there a reason we can't make this IAccessible* GetCOMInterface() ? or > > wrap that in already_AddRefed? > > No reason other than to be consistent with the style used by > AccessibleWrap::GetNativeInterface. I'd rather not add more gross things to be similar to old gross things, but I'll live for now ;-) > > + VARIANT varState; > > + DebugOnly<HRESULT> hr = acc->get_accState(id, &varState); > > + MOZ_ASSERT(SUCCEEDED(hr)); > > > > why is this one an assert and the others treat failure as an expected case? > > That is vestigial and I can remove it. I added those to ensure that > get_AccState was working as I expected. ah ok (In reply to Aaron Klotz [:aklotz] from comment #18) > (In reply to Trevor Saunders (:tbsaunde) from comment #13) > > +++ b/accessible/base/NotificationController.cpp > > @@ -428,6 +428,10 @@ NotificationController::WillRefresh(mozilla::TimeStamp > > aTime) > > if (tabChild) { > > static_cast<TabChild*>(tabChild.get())-> > > SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc, id); > > +#if defined(XP_WIN) > > + IAccessibleHolder holder(CreateHolderFromAccessible(childDoc)); > > + ipcDoc->SendCOMProxy(holder); > > +#endif > > > > I'm not sure I understand what this method does, but this seems wrong. As I > > understand it the purpose is to connect the subtree in the content process to > > the tree in the parent. However all the docs here should be the child of > > another document in the content process, not an object in the parent. > > I added it out of concern that we're constructing a PDocAccessible, which > seemed to me like it shouldn't exist in the parent without having a valid > COM proxy. Is there somebody we need to needinfo about this, or should I > just revert my change? so, it might be correct, but just kind of pointless. The argument is this, the outer doc accessible that is the parent of any PDocAccessible here is in the content process. Therefore calling any methods on it is calling methods on a proxy that proxies to this content process. Since we proxy into this content process the normal GetChildAt / GetChildCount work on the outer doc accessible. A similar argument should hold for examining the outer doc accessible from this document. I think the people to ask are you and me, so since I'm not convinced it breaks something I'm fine with removing it or commenting that its probably not needed. > > +class PlatformChild > > > > I'm not sure I understand the name of this class, and really it seems kind of > > silly and we could just have 3 StaticAutoPtrs in DocAccessibleParent.cpp but > > whatever. > > Name was supposed to be the content process analog to > accessible/base/Platform Well, the idea of that header is that its a set of functions a platform accessibility layer implements to interact with the platform independent code, I'm not really sure how this class is similar. > > +DocAccessibleWrap::get_accParent( > > + /* [retval][out] */ IDispatch __RPC_FAR *__RPC_FAR *ppdispParent) > > +{ > > + if (!IPCDoc()) { > > + return DocAccessible::get_accParent(ppdispParent); > > + } > > + > > + IAccessible* dispParent = IPCDoc()->GetParentIAccessible(); > > + MOZ_ASSERT(dispParent); > > + if (!dispParent) { > > + return E_UNEXPECTED; > > > > This might well be correct for documents that are children of other documents > > in the content process, but it would probably be faster to only special case > > this when Parent() returns null. > > Just to be clear, you're saying that on the line above the > GetParentIAccessible call I should add something like: > if (!Parent()) { > *ppdispParent = nullptr; > return S_FALSE; > } I think I'd write it like this, though on second thought I'm not sure its faster. hr = DocAccessible::GetParent(aParent); if (!*aParent) { return hr; } // we might be a top level document in a content process. IAccessible* outerDoc = IPCDoc()->GetCOMInterface(); (In reply to Aaron Klotz [:aklotz] from comment #19) > (In reply to Aaron Klotz [:aklotz] from comment #18) > > (In reply to Trevor Saunders (:tbsaunde) from comment #13) > > > > > > +DocAccessibleWrap::get_accParent( > > > + /* [retval][out] */ IDispatch __RPC_FAR *__RPC_FAR *ppdispParent) > > > +{ > > > + if (!IPCDoc()) { > > > + return DocAccessible::get_accParent(ppdispParent); > > > + } > > > + > > > + IAccessible* dispParent = IPCDoc()->GetParentIAccessible(); > > > + MOZ_ASSERT(dispParent); > > > + if (!dispParent) { > > > + return E_UNEXPECTED; > > > > > > This might well be correct for documents that are children of other documents > > > in the content process, but it would probably be faster to only special case > > > this when Parent() returns null. > > > > Just to be clear, you're saying that on the line above the > > GetParentIAccessible call I should add something like: > > if (!Parent()) { > > *ppdispParent = nullptr; > > return S_FALSE; > > } > > Or I suppose I should just put it right at the top of get_accParent()... or yeah, you could just special case there.

(In reply to Trevor Saunders (:tbsaunde) from comment #25) > > > @@ -1023,6 +923,22 @@ AccessibleWrap::accNavigate( > > > if (accessible->IsDefunct()) > > > return CO_E_OBJNOTCONNECTED; > > > > > > + // Make sure that varStart != CHILDID_SELF so we don't infinitely recurse > > > + if (accessible->IsProxy() && varStart.vt == VT_I4 && > > > + varStart.lVal != CHILDID_SELF) { > > > + // Now that we have the starting object, delegate this request to that > > > + // object as a self-relative request... > > > + RefPtr<IAccessible> comProxy; > > > + accessible->GetNativeInterface((void**) getter_AddRefs(comProxy)); > > > + if (!comProxy) { > > > + return E_UNEXPECTED; > > > + } > > > + VARIANT selfChildId; > > > + selfChildId.vt = VT_I4; > > > + selfChildId.lVal = CHILDID_SELF; > > > + return comProxy->accNavigate(navDir, selfChildId, pvarEndUpAt); > > > + } > > > > > > I'd like to understand how this ends up getting called on accessibles where > > > IsProxy() is true, and why it isn't an assert like the other places. > > > > When IsProxy() is true, the actual GetNativeInterface called is one of the > > overrides in ProxyWrappers.h. We're actually operating on a COM proxy to the > > content IAccessible in this case. > > So, I still don't understand how that means accNavigate can be called on a > non com proxy accessible where IsProxy() is true. Those are only used > within gecko right and should never escape right? I think the problem there is that sometime accNavigate is being called with a child ID that refers to a proxy. Much like the ChildIDThunk in bug 1272146, some clients are invoking accNavigate on a chrome-based IAccessible but specifying a child ID that resolves to a content IAccessible.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b3a5e1756d36d9a498345547db054eca1c792c Bug 1268544: Refactor PDocAccessible and its dependencies, and add code to integrate remote COM objects; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/2bdfb9514317109acb9efb053717691126d4346a Bug 1268544: Refactor ProxyAccessible and dependencies; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/99f0ea19b8f535197d1feb35e2b6e4b591b0db13 Bug 1268544: Remove proxy-specific code paths from IAccessible and IA2 wrappers; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/ffc8ee715fdbed67d7ad01aacc2e3ed6d7b2c309 Bug 1268544: Disable proxy code paths in accessible/xpcom pending resolution of bug 1288839; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/e76f58f328d46cb17079b3439a44f7a245b1d98a Bug 1268544: Integrate remote COM objects into a11y code; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0085eb5ffe746352ea4cf4f357344443e8ddf90 Bug 1268544: Followup for atk missing header build error; r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/d83cba382cfe97ad55afa86ecba18185f5473850 Bug 1268544: Followup: fix missing include in DocAccessibleChildBase; r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/119a4b187938f44613da91a7ef1778fd63869d81 Bug 1268544: Add missing FINAL_LIBRARY entry to accessible/ipc/other/moz.build; r=bustage a=kwierso CLOSED TREE

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4a39e456f8945518602fc3ae5b3ff2aaeb47f29 Bug 1268544: Disable test_accessibility.py on Windows e10s; r=bustage a=kwierso CLOSED TREE

Backed out with also bug 1288841 and bug 1288843: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec53f8c2d48

(In reply to Aaron Klotz [:aklotz] from comment #29) > (In reply to Trevor Saunders (:tbsaunde) from comment #25) > > > > @@ -1023,6 +923,22 @@ AccessibleWrap::accNavigate( > > > > if (accessible->IsDefunct()) > > > > return CO_E_OBJNOTCONNECTED; > > > > > > > > + // Make sure that varStart != CHILDID_SELF so we don't infinitely recurse > > > > + if (accessible->IsProxy() && varStart.vt == VT_I4 && > > > > + varStart.lVal != CHILDID_SELF) { > > > > + // Now that we have the starting object, delegate this request to that > > > > + // object as a self-relative request... > > > > + RefPtr<IAccessible> comProxy; > > > > + accessible->GetNativeInterface((void**) getter_AddRefs(comProxy)); > > > > + if (!comProxy) { > > > > + return E_UNEXPECTED; > > > > + } > > > > + VARIANT selfChildId; > > > > + selfChildId.vt = VT_I4; > > > > + selfChildId.lVal = CHILDID_SELF; > > > > + return comProxy->accNavigate(navDir, selfChildId, pvarEndUpAt); > > > > + } > > > > > > > > I'd like to understand how this ends up getting called on accessibles where > > > > IsProxy() is true, and why it isn't an assert like the other places. > > > > > > When IsProxy() is true, the actual GetNativeInterface called is one of the > > > overrides in ProxyWrappers.h. We're actually operating on a COM proxy to the > > > content IAccessible in this case. > > > > So, I still don't understand how that means accNavigate can be called on a > > non com proxy accessible where IsProxy() is true. Those are only used > > within gecko right and should never escape right? > > I think the problem there is that sometime accNavigate is being called with > a child ID that refers to a proxy. Much like the ChildIDThunk in bug > 1272146, some clients are invoking accNavigate on a chrome-based IAccessible > but specifying a child ID that resolves to a content IAccessible. yeah, I think I commented on accNavigate being left out of the data for the ChildIDThunk, is there a reason not to just add it there and drop this?

https://hg.mozilla.org/mozilla-central/rev/e4b3a5e1756d https://hg.mozilla.org/mozilla-central/rev/2bdfb9514317 https://hg.mozilla.org/mozilla-central/rev/99f0ea19b8f5 https://hg.mozilla.org/mozilla-central/rev/ffc8ee715fdb https://hg.mozilla.org/mozilla-central/rev/e76f58f328d4 https://hg.mozilla.org/mozilla-central/rev/a0085eb5ffe7 https://hg.mozilla.org/mozilla-central/rev/d83cba382cfe https://hg.mozilla.org/mozilla-central/rev/119a4b187938

https://hg.mozilla.org/integration/mozilla-inbound/rev/12aa15cf58791f26f5fd77af31d97eb66f4d4125 Bug 1268544: Refactor PDocAccessible and its dependencies, and add code to integrate remote COM objects; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3a5a770cae858dc7c7d19a03560173978758ab Bug 1268544: Refactor ProxyAccessible and dependencies; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/4887b4164dd90bd6d43754d96558f005a1b27487 Bug 1268544: Remove proxy-specific code paths from IAccessible and IA2 wrappers; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/d898178a0809c9542e27696281c42fc0dca8853f Bug 1268544: Disable proxy code paths in accessible/xpcom pending resolution of bug 1288839; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d43e83e0700fa19565f54e55356caf49f488ba Bug 1268544: Integrate remote COM objects into a11y code; r=tbsaunde

Backed those out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea96bf70664 for incomprehensibly causing Win7 debug Marionette e10s to fail, https://treeherder.mozilla.org/logviewer.html#?job_id=34078720&repo=mozilla-inbound

And a more direct failure, asserting in an a11y test on Windows, https://treeherder.mozilla.org/logviewer.html#?job_id=34088656&repo=mozilla-inbound

this caused a large talos regression: == Change summary for alert #2520 (as of August 17 2016 13:31 UTC) == Summary of tests that regressed: a11yr summary windows7-32 opt e10s: 782.83 -> 3541.03 (352.34% worse) For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=2520 this seems to be e10s only, but we never had talos a11yr test validated for e10s (since e10s + a11y was not a v.48 item).

(In reply to Joel Maher ( :jmaher) from comment #41) > this caused a large talos regression: > == Change summary for alert #2520 (as of August 17 2016 13:31 UTC) == > > Summary of tests that regressed: > > a11yr summary windows7-32 opt e10s: 782.83 -> 3541.03 (352.34% worse) > > For up to date results, see: > https://treeherder.mozilla.org/perf.html#/alerts?id=2520 > > > this seems to be e10s only, but we never had talos a11yr test validated for > e10s (since e10s + a11y was not a v.48 item). Yeah, this is to be expected. We plan to land the fundamental changes first and then improve performance once we've been able to get Nightlies into the hands of a11y client vendors. We will have to work with the a11y team to figure out what are baseline should be for Talos.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b189275d7a4eba94b110f1ec464eab3d513bfc Bug 1268544: Refactor PDocAccessible and its dependencies, and add code to integrate remote COM objects; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4c3a792b14cd3fc33e41c5edee613ec8a0fde2 Bug 1268544: Refactor ProxyAccessible and dependencies; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/9912f206b678a5e75f4cb6d91b3a3e1e7124d688 Bug 1268544: Remove proxy-specific code paths from IAccessible and IA2 wrappers; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/8c550a7e9c2adb445010ff404613cc5979bc6fc6 Bug 1268544: Disable proxy code paths in accessible/xpcom pending resolution of bug 1288839; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/ff58e1a5f48351b9a19c5cb7be11594447c91545 Bug 1268544: Integrate remote COM objects into a11y code; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d00141b4cb968548740b75e1dc21d5536c66888 Bug 1268544: Temporarily disable test_about_pages.py on Windows e10s due to bustage; r=bustage

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf746c67d0e9ecb8fd06b30ef9247585070ae5b0 Bug 1268544: Remove extra MOZ_COUNT_CTOR that should have been removed; r=bustage

Backed this out for asserting in Windows M(oth) jobs: https://hg.mozilla.org/integration/mozilla-inbound/rev/dd022a0f3e42e38eb02018b71417f81ebbc7a696 https://hg.mozilla.org/integration/mozilla-inbound/rev/7567af62a4d20ed03d70b36e0f86b579329add32 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa54b521fc7239ac46339029ee809d89e16369bc https://hg.mozilla.org/integration/mozilla-inbound/rev/d4df5492322828c76d08c3e374b3f45325b9a75e https://hg.mozilla.org/integration/mozilla-inbound/rev/f8a46216e208847116c6372a7203581d4de36336 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a36f19239872c2e86d84e0dacb5715c488c239 https://hg.mozilla.org/integration/mozilla-inbound/rev/24f8fbe46850d572c36f8f6dbfa10e962c38a7d7 Also had to back out other a11y bugs, else there were failing hunks on the backout: bug 1288841: https://hg.mozilla.org/integration/mozilla-inbound/rev/ac95bd5946812806300a6c9a4beed824e46f2c81 bug 1272146: https://hg.mozilla.org/integration/mozilla-inbound/rev/a832f738b606f90c9afd10f61728bddbec605674 bug 1292452: https://hg.mozilla.org/integration/mozilla-inbound/rev/aea13a7c810545c72e4e6de6b71143505dbd8472 https://hg.mozilla.org/integration/mozilla-inbound/rev/aef7e275cc6dd3246481a6be6cdbd700f24abc64 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d84b4dd26a403675c8059e5d073ebc65d5fc07eb Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=34135611&repo=mozilla-inbound 20:13:47 INFO - [AccessFu] INFO content-script.js chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/doc_content_integration.html 20:13:47 INFO - [AccessFu] DEBUG AccessFu:Start 20:13:47 INFO - [AccessFu] DEBUG EventManager.start 20:13:47 INFO - [AccessFu] DEBUG AccessibilityEventObserver.addListener. Total: 1 20:13:48 INFO - Assertion failure: aInstanceSize == 0 || entry->GetClassSize() == aInstanceSize (Mismatched sizes were recorded in the memory leak logging table. The usual cause of this is having a templated class that uses MOZ_COUNT_{C,D}TOR in the constructor or destructor, respectively. As a workaround, the MOZ_COUNT_{C,D}TOR calls can be moved to a non-templated base class.), at c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/xpcom/base/nsTraceRefcnt.cpp:456 20:14:00 INFO - #01: NS_LogCtor [xpcom/base/nsTraceRefcnt.cpp:1169] 20:14:00 INFO - #02: mozilla::a11y::ProxyAccessible::ProxyAccessible(mozilla::a11y::DocAccessibleParent *) [obj-firefox/dist/include/mozilla/a11y/ProxyAccessible.h:76] 20:14:00 INFO - #03: mozilla::a11y::DocAccessibleParent::DocAccessibleParent() [obj-firefox/dist/include/mozilla/a11y/DocAccessibleParent.h:33] 20:14:00 INFO - #04: mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const &) [obj-firefox/ipc/ipdl/PContentParent.cpp:4363] 20:14:00 INFO - #05: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const &) [ipc/glue/MessageChannel.cpp:1663] 20:14:00 INFO - #06: mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message &&) [ipc/glue/MessageChannel.cpp:1602] 20:14:00 INFO - #07: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() [ipc/glue/MessageChannel.cpp:1569] 20:14:00 INFO - #08: mozilla::detail::RunnableMethodImpl<bool ( mozilla::ipc::MessageChannel::*)(void),0,1>::Run() [obj-firefox/dist/include/nsThreadUtils.h:766] 20:14:00 INFO - #09: mozilla::ipc::MessageChannel::DequeueTask::Run() [obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:566] 20:14:00 INFO - #10: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1058] 20:14:00 INFO - #11: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/glue/nsThreadUtils.cpp:290] 20:14:00 INFO - #12: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:124] 20:14:00 INFO - #13: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:232] 20:14:00 INFO - #14: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:226] 20:14:00 INFO - #15: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:206] 20:14:00 INFO - #16: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:158] 20:14:00 INFO - #17: nsAppShell::Run() [widget/windows/nsAppShell.cpp:264] 20:14:00 INFO - #18: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:285] 20:14:00 INFO - #19: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4302] 20:14:00 INFO - #20: XREMain::XRE_main(int,char * * const,nsXREAppData const *) [toolkit/xre/nsAppRunner.cpp:4429] 20:14:00 INFO - #21: XRE_main [toolkit/xre/nsAppRunner.cpp:4520] 20:14:00 INFO - #22: do_main [browser/app/nsBrowserApp.cpp:259] 20:14:00 INFO - #23: NS_internal_main(int,char * *,char * *) [browser/app/nsBrowserApp.cpp:392] 20:14:00 INFO - #24: wmain [toolkit/xre/nsWindowsWMain.cpp:118] 20:14:00 INFO - #25: __scrt_common_main_seh [f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:255] 20:14:00 INFO - #26: kernel32.dll + 0x17067 20:14:00 INFO - [NPAPI 328] ###!!! ABORT: Aborting on channel error.: file c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 2056 20:14:00 INFO - nsStringStats 20:14:00 INFO - => mAllocCount: 233 20:14:00 INFO - => mReallocCount: 1 20:14:00 INFO - => mFreeCount: 225 -- LEAKED 8 !!! 20:14:00 INFO - => mShareCount: 494 20:14:00 INFO - => mAdoptCount: 0 20:14:00 INFO - => mAdoptFreeCount: 0 20:14:00 INFO - => Process ID: 328, Thread ID: 3760 20:14:00 INFO - TEST-INFO | Main app process: exit 1 20:14:00 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_content_integration.html | application terminated with exit code 1

https://hg.mozilla.org/integration/mozilla-inbound/rev/6204f84101b2c141b9e7c6faa15575e9d70f2e9e Bug 1268544: Refactor PDocAccessible and its dependencies, and add code to integrate remote COM objects; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/23a7221a90d574597d33532c9a32fc2461de7705 Bug 1268544: Refactor ProxyAccessible and dependencies; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/4fae537cf252ca929e7d9c580ef1c00f151d0acc Bug 1268544: Remove proxy-specific code paths from IAccessible and IA2 wrappers; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c54547e3b274ca8aa4da6f9abde0f822b1289c Bug 1268544: Disable proxy code paths in accessible/xpcom pending resolution of bug 1288839; r=tbsaunde https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2a728e5832c61494588723c5b37c12a6d3a800 Bug 1268544: Integrate remote COM objects into a11y code; r=tbsaunde

https://hg.mozilla.org/mozilla-central/rev/6204f84101b2 https://hg.mozilla.org/mozilla-central/rev/23a7221a90d5 https://hg.mozilla.org/mozilla-central/rev/4fae537cf252 https://hg.mozilla.org/mozilla-central/rev/b1c54547e3b2 https://hg.mozilla.org/mozilla-central/rev/ad2a728e5832

I think we want this to ride the trains.