Inspect.exe doesn't appear to correctly traverse past the chrome-to-content threshold without this patch.

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

https://reviewboard.mozilla.org/r/66592/#review63376 ::: accessible/generic/OuterDocAccessible.cpp:173 (Diff revision 1) > > +uint32_t > +OuterDocAccessible::ChildCount() const > +{ > + uint32_t result = mChildren.Length(); > + if (!result && !!RemoteChildDoc()) { this affects on the web a11y sciprting (XPCOM part now) too, they shouldn't be able to cross the process boundaries. Can you move that into platform layer?

(In reply to alexander :surkov from comment #3) > https://reviewboard.mozilla.org/r/66592/#review63376 > > ::: accessible/generic/OuterDocAccessible.cpp:173 > (Diff revision 1) > > > > +uint32_t > > +OuterDocAccessible::ChildCount() const > > +{ > > + uint32_t result = mChildren.Length(); > > + if (!result && !!RemoteChildDoc()) { > > this affects on the web a11y sciprting (XPCOM part now) too, they shouldn't > be able to cross the process boundaries. Can you move that into platform > layer? its not really web at all... so it seems fine, arguably even a correctness fix since it makes it the same as when you cross between documents in the same process.

r=me without the ChildAt() part. @@ -71,7 +71,9 @@ OuterDocAccessible::ChildAtPoint(int32_t aX, int32_t aY, // Always return the inner doc as direct child accessible unless bounds // outside of it. Accessible* child = GetChildAt(0); - NS_ENSURE_TRUE(child, nullptr); + if (NS_WARN_IF(!child)) { + return nullptr; + } personally I prefer NS_ENSURE_* +OuterDocAccessible::ChildCount() const +{ + uint32_t result = mChildren.Length(); + if (!result && !!RemoteChildDoc()) { I'd rather drop the !! but whatever +OuterDocAccessible::GetChildAt(uint32_t aIndex) const +{ + Accessible* result = AccessibleWrap::GetChildAt(aIndex); + if (result || aIndex) { + return result; + } + // If we are asking for child 0 and GetChildAt doesn't return anything, try + // to get the remote child doc and return that instead. + ProxyAccessible* remoteChild = RemoteChildDoc(); + if (!remoteChild) { + return nullptr; + } + return WrapperFor(remoteChild); WrapperFor is only a function on windows, and on !windows there isn't even an Accessible for you to return here.

(In reply to Trevor Saunders (:tbsaunde) from comment #4) > > > +OuterDocAccessible::ChildCount() const > > > +{ > > > + uint32_t result = mChildren.Length(); > > > + if (!result && !!RemoteChildDoc()) { > > > > this affects on the web a11y sciprting (XPCOM part now) too, they shouldn't > > be able to cross the process boundaries. Can you move that into platform > > layer? > > its not really web at all... one day XPCOM nsIAccessible will be replaced by web API. > so it seems fine, arguably even a correctness > fix since it makes it the same as when you cross between documents in the > same process. seamless multi-process accessible tree navigation goes at performance/memory cost, thus it should be available for those consumers only who needs that. AccessFu, which uses XPCOM API, for example, doesn't need it.

So what's the path forward here? Overrides for ChildCount and GetChildAt in AccessibleWrap that do the work there? What's the best way to determine if an accessible is an OuterDocAccessible? NativeRole() == roles::INTERNAL_FRAME?

(In reply to alexander :surkov from comment #6) > (In reply to Trevor Saunders (:tbsaunde) from comment #4) > > > > > +OuterDocAccessible::ChildCount() const > > > > +{ > > > > + uint32_t result = mChildren.Length(); > > > > + if (!result && !!RemoteChildDoc()) { > > > > > > this affects on the web a11y sciprting (XPCOM part now) too, they shouldn't > > > be able to cross the process boundaries. Can you move that into platform > > > layer? > > > > its not really web at all... > > one day XPCOM nsIAccessible will be replaced by web API. ... maybe, and even if that happens you'll need to do something about cross origin iframes, so you'd have to touch that code anyway. > > so it seems fine, arguably even a correctness > > fix since it makes it the same as when you cross between documents in the > > same process. > > seamless multi-process accessible tree navigation goes at performance/memory > cost, thus it should be available for those consumers only who needs that. that's basically unrelated to this patch. > AccessFu, which uses XPCOM API, for example, doesn't need it. sure, and that's not used anywhere where e10s is enabled.

(In reply to Aaron Klotz [:aklotz] from comment #8) > So what's the path forward here? Overrides for ChildCount and GetChildAt in > AccessibleWrap that do the work there? that's what I did for atk / mac a while back, though I'm not sure there was a good reason for that. I think you could just go forward with the ChildCount() part, but I admit its kind of odd to fix it, but not GetChildAt. > What's the best way to determine if an accessible is an OuterDocAccessible? > NativeRole() == roles::INTERNAL_FRAME? I think there's an IsOuterDoc method that gets used by atk / mac.

(In reply to Aaron Klotz [:aklotz] from comment #8) > So what's the path forward here? Overrides for ChildCount and GetChildAt in > AccessibleWrap that do the work there? that would be a bit better as it affects Windows platform only, but it if you are on Windows, then XPCOM is still affected. If you are really interested to change MSAA part only, then ChildrenEnumVariant and get_accChildCount/GetXPAccessibleFor should be fixed. Btw, you do not fix 'parent' part in your patch. Is it something that already works? > What's the best way to determine if an accessible is an OuterDocAccessible? > NativeRole() == roles::INTERNAL_FRAME? as Trev said, Accessible::IsOuterDoc()

(In reply to Trevor Saunders (:tbsaunde) from comment #9) > > > its not really web at all... > > > > one day XPCOM nsIAccessible will be replaced by web API. > > ... maybe, and even if that happens you'll need to do something about cross > origin iframes, so you'd have to touch that code anyway. maybe not, treating them as separate documents should work > > > so it seems fine, arguably even a correctness > > > fix since it makes it the same as when you cross between documents in the > > > same process. > > > > seamless multi-process accessible tree navigation goes at performance/memory > > cost, thus it should be available for those consumers only who needs that. > > that's basically unrelated to this patch. This patch enables cross-process navigation between documents, even if you just started DOMi. That's the whole point of the patch, no? > > AccessFu, which uses XPCOM API, for example, doesn't need it. > > sure, and that's not used anywhere where e10s is enabled. you mean a11y e10s? that's good then. Anyway I would appreciate if we spent some time to design a solution that will let a web API app and a Windows screen reader to be running simultaneously.

(In reply to alexander :surkov from comment #12) > (In reply to Trevor Saunders (:tbsaunde) from comment #9) > > > > > its not really web at all... > > > > > > one day XPCOM nsIAccessible will be replaced by web API. > > > > ... maybe, and even if that happens you'll need to do something about cross > > origin iframes, so you'd have to touch that code anyway. > > maybe not, treating them as separate documents should work At the cost of exposing a giant security whole sure you can let js poke at cross process iframes through some accessibility API. > > > > so it seems fine, arguably even a correctness > > > > fix since it makes it the same as when you cross between documents in the > > > > same process. > > > > > > seamless multi-process accessible tree navigation goes at performance/memory > > > cost, thus it should be available for those consumers only who needs that. > > > > that's basically unrelated to this patch. > > This patch enables cross-process navigation between documents, even if you > just started DOMi. That's the whole point of the patch, no? no, it should be obvious from the patch it doesn't start anything, and that's elsewhere. > > > AccessFu, which uses XPCOM API, for example, doesn't need it. > > > > sure, and that's not used anywhere where e10s is enabled. > > you mean a11y e10s? that's good then. Anyway I would appreciate if we spent > some time to design a solution that will let a web API app and a Windows > screen reader to be running simultaneously. Well, e10s isn't enabled at all for android. I'll care more about a theoretical web accessibility API when there's good reason to implement such a thing.

(In reply to Trevor Saunders (:tbsaunde) from comment #13) > > maybe not, treating them as separate documents should work > > At the cost of exposing a giant security whole sure you can let js poke at > cross process iframes through some accessibility API. that wasn't my suggestion actually > > This patch enables cross-process navigation between documents, even if you > > just started DOMi. That's the whole point of the patch, no? > > no, it should be obvious from the patch it doesn't start anything, and > that's elsewhere. not sure what you mean by that. Am I reading the patch wrong that outer document for a tab document claims it has a child now, but before the patch it was 0? and thus now you can have seamless navigation between documents belonging to different processes?

Aaron, could you address please comment #11? I'm not yet clear about scope of the bug, and whether your intent was to put a fix into cross-platform code.

(In reply to alexander :surkov from comment #11) > (In reply to Aaron Klotz [:aklotz] from comment #8) > > So what's the path forward here? Overrides for ChildCount and GetChildAt in > > AccessibleWrap that do the work there? > > that would be a bit better as it affects Windows platform only, but it if > you are on Windows, then XPCOM is still affected. If you are really > interested to change MSAA part only, then ChildrenEnumVariant and > get_accChildCount/GetXPAccessibleFor should be fixed. Based on our chat on IRC, I am going to rewrite this to be MSAA-specific. > > Btw, you do not fix 'parent' part in your patch. Is it something that > already works? > The content to parent issue is fixed in bug 1268544. We send a special COM proxy to content that we return when the parent is requested.

Comment on attachment 8780640 [details] [diff] [review] Ensure that MSAA child retrieval code is able to cross the chrome to content boundary longer version of what I said in irc. This doesn't like a good idea you now have the code to poke at children of outer docs in two places, and the Enumerator hunk is pretty gross. >- *pcountChildren = ChildCount(); >+ if (IsOuterDoc()) { >+ *pcountChildren = 1; that's not really correct, you should check if RemoteChildDoc() returns a ProxyAccessible* >+ if (mAnchorAcc->IsOuterDoc()) { >+ if (!aCount) { >+ return S_OK; >+ } >+ if (mCurIndex) { >+ return S_FALSE; >+ } >+ ProxyAccessible* remoteChildDoc = >+ mAnchorAcc->AsOuterDoc()->RemoteChildDoc(); >+ if (!remoteChildDoc) { >+ return S_FALSE; >+ } >+ VariantInit(aItems); >+ aItems->vt = VT_DISPATCH; >+ aItems->pdispVal = >+ AccessibleWrap::NativeAccessible(WrapperFor(remoteChildDoc)); >+ *aCountFetched = 1; >+ mCurIndex++; >+ return aCount == 1 ? S_OK : S_FALSE; is returning a value and S_FALSE expected?

Comment on attachment 8780670 [details] [diff] [review] Modify OuterDocAccessible on Windows so that child retrieval code is able to cross the chrome to content boundary ># HG changeset patch ># User Aaron Klotz <aklotz@mozilla.com> ># Date 1469216405 21600 ># Fri Jul 22 13:40:05 2016 -0600 ># Node ID a60e323bb5c4119ea9fc432234b12f664a1b683e ># Parent f4bce8d7d7f32a29c49591858d2f7c7d3060dc1e >Bug 1288843: Modify OuterDocAccessible so that ChildCount() and ChildAt(0) resolve RemoteChildDoc(); r?tbsaunde > >MozReview-Commit-ID: 38qOXftPFid > >diff --git a/accessible/generic/OuterDocAccessible.cpp b/accessible/generic/OuterDocAccessible.cpp >--- a/accessible/generic/OuterDocAccessible.cpp >+++ b/accessible/generic/OuterDocAccessible.cpp >@@ -168,16 +168,50 @@ OuterDocAccessible::RemoveChild(Accessib > bool > OuterDocAccessible::IsAcceptableChild(nsIContent* aEl) const > { > // outer document accessible doesn't not participate in ordinal tree > // mutations. > return false; > } > >+#if defined(XP_WIN) >+ >+// On Windows e10s, sicne we don't cache in the chrome process, these next two spelling "since" >+// functions must be implemented so that we properly cross the chrome-to-content >+// boundary when traversing. >+ >+uint32_t >+OuterDocAccessible::ChildCount() const no need to ifdef this one, and if you didn't we could clean up some atk code, but no big deal. does fixing ChildAt() make ChildAtPoint() just work because of the loop at the end of Accessible::ChildAtPoint()? that's pretty sweet.

Yeah, in fact fixing ChildAtPoint() was the original motivation for this bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eefa457c3680aec93c8f9971de843ed5ad8e49ee Bug 1288843: Modify OuterDocAccessible so that ChildCount() and ChildAt(0) resolve RemoteChildDoc(); r=tbsaunde

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

(In reply to Aaron Klotz [:aklotz] from comment #22) > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > eefa457c3680aec93c8f9971de843ed5ad8e49ee > Bug 1288843: Modify OuterDocAccessible so that ChildCount() and ChildAt(0) > resolve RemoteChildDoc(); r=tbsaunde This approach has some issue: 1) it doesn't address js-api problem on windows 2) makes cross-platform accessilbe trees differ depending on a platform 3) puts platform-specific code into cross-platorm part Aaron, would you please file a follow up bug to finish your MSAA patch and take it instead this one? Perhaps a more elegant way could be an introduction of OuterDocAccessibleWrap class that overrides those methods.

(In reply to alexander :surkov from comment #14) > (In reply to Trevor Saunders (:tbsaunde) from comment #13) > > > maybe not, treating them as separate documents should work > > > > At the cost of exposing a giant security whole sure you can let js poke at > > cross process iframes through some accessibility API. > > that wasn't my suggestion actually Well, if your not going to do that then clearly you need to modify how js gets at children to return null when trying to get a child that is a document in a different origin. That means this patch can't make it any harder to implement some web api. > > > This patch enables cross-process navigation between documents, even if you > > > just started DOMi. That's the whole point of the patch, no? > > > > no, it should be obvious from the patch it doesn't start anything, and > > that's elsewhere. > > not sure what you mean by that. Am I reading the patch wrong that outer > document for a tab document claims it has a child now, but before the patch > it was 0? and thus now you can have seamless navigation between documents > belonging to different processes? not really, before bug 1286544 I think the ia2 / msaa / xpcom ways of poking at children of outer docs containing top level content documents already worked.

> not really, before bug 1286544 I think the ia2 / msaa / xpcom ways of poking > at children of outer docs containing top level content documents already > worked. err, bug 1268544 of course. (In reply to alexander :surkov from comment #24) > (In reply to Aaron Klotz [:aklotz] from comment #22) > > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > > eefa457c3680aec93c8f9971de843ed5ad8e49ee > > Bug 1288843: Modify OuterDocAccessible so that ChildCount() and ChildAt(0) > > resolve RemoteChildDoc(); r=tbsaunde > > This approach has some issue: > 1) it doesn't address js-api problem on windows which isn't all that important in the first place, and should such an API be implemented something will need to be done about when children of outerdocs are available anyway. > 2) makes cross-platform accessilbe trees differ depending on a platform which 1 is only important to internal things and very few of them care, and 2 it would make more sense to refactor things such that the trees are merged on all platforms. > 3) puts platform-specific code into cross-platorm part > > Aaron, would you please file a follow up bug to finish your MSAA patch and > take it instead this one? Perhaps a more elegant way could be an > introduction of OuterDocAccessibleWrap class that overrides those methods. true, but meh, its mostly a waste of time for some sort of theoretical purity, and will probably need to change anyway as windows stuff gets cleaned up.

(In reply to Trevor Saunders (:tbsaunde) from comment #25) > not really, before bug 1268544 I think the ia2 / msaa / xpcom ways of poking > at children of outer docs containing top level content documents already > worked. if js-api was based on top of internal tree, then is any refactoring still required?

I'm going to land this as-is, since Trevor has already r+'d and this subject is still being debated between the two of you. I am working in "beg for forgiveness" mode as opposed to "ask for permission" mode at this point. I understand what you're saying, Alex, but right now I need to get this stuff landed now, with the intention of cleaning some of this up in bug 1289852. Of course, I am more than happy to invite you to contribute a cleaner implementation. :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/afd82c3f354101d3b7a79da8968cf45efca81cb6 Bug 1288843: Modify OuterDocAccessible so that ChildCount() and ChildAt(0) resolve RemoteChildDoc(); r=tbsaunde

(In reply to Aaron Klotz [:aklotz] from comment #29) > I'm going to land this as-is, since Trevor has already r+'d and this subject > is still being debated between the two of you. I am working in "beg for > forgiveness" mode as opposed to "ask for permission" mode at this point. > > I understand what you're saying, Alex, but right now I need to get this > stuff landed now, with the intention of cleaning some of this up in bug > 1289852. it's ok, the last thing I want is blocking your work. a follow up is totally fine > Of course, I am more than happy to invite you to contribute a cleaner > implementation. :-) thanks :)