1332444 - ensure that AccessibleWrap::GetRemoteIAccessibleFor is never called for a proxy accessible wrap.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

AccessibleWrap::GetIAccessibleFor does not resolve in some cases where we are trying to get a remote IAccessible for a proxy.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1332444 patch

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8828505 [details] [diff] [review]
1332444 patch

># HG changeset patch
># User Yura Zenevich <[e-mail]>
>
>Bug 1332444 - ensure that GetRemoteIAccessibleFor is only called for a proxy accessible wrap. r=tbsaunde

That's not really what this is doing, but anyway.

>   // find it here and should look remotely instead.
>-  if (XRE_IsParentProcess() && !sIDGen.IsChromeID(varChild.lVal)) {
>+  if (XRE_IsParentProcess() && IsProxy() && !sIDGen.IsChromeID(varChild.lVal)) {
>     return GetRemoteIAccessibleFor(varChild);

So, this code is a bit of a mess, but I was wrong, I think the idea is GetRemoteIAccessible() will handle the case this accessible is in the chrome process and is for part of a xul browser window, and the child id points in the content documents.  Some code farther down in GetIAccessibleFor() is supposed to handle the case that this accessible is related to a proxy and is for content in the child process.

So I think the thing to do is add a check this *isn't* a proxy, and leave GetRemoteIAccessible() alone.

However, in addition to missing that when reviewing this code there's a bug in the code farther down trying to deal with the case of this being a proxy, it doesn't deal with this being a proxy in a document that is not top level.  So we need to fix that somehow.

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1332444.1 v2

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1332444 traverse subtree

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8829459 [details] [diff] [review]
1332444.1 v2

># HG changeset patch
># User Yura Zenevich <[e-mail]>
>
>Bug 1332444 - only call GetRemoteIAccessible for non-proxies. r=tbsaunde

s/for/on/ since your talking about the this object is.  Really it wouldn't hurt to explain more why you are doing this.

>   // If the MSAA ID is not a chrome id then we already know that we won't
>   // find it here and should look remotely instead.
>-  if (XRE_IsParentProcess() && !sIDGen.IsChromeID(varChild.lVal)) {
>+  if (XRE_IsParentProcess() && !IsProxy() && !sIDGen.IsChromeID(varChild.lVal)) {

this is unclear enough it deserves explanation.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f04c1c83233c
only call GetRemoteIAccessible on non-proxies that have a child id pointing in content documents. r=tbsaunde

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ee57647e0b
adding comment when calling GetRemoteIAccessible. r=tbsaunde

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f04c1c83233c

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/87ee57647e0b

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

The description of this bug is not useful. Trev, do you remember what the original motivation for this bug was? I think that this fix actually broke resolution of MSAA IDs but I'd like to know what the original problem was that we were attempting to solve here.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

To clarify: do you recall which accessibles were not resolving?

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

I believe comment 2 helps with a case (misunderstood by me initially). I believe this was an issue with b-c tests, specifically (if I remember correctly) where I was requesting gecko custom id for an xpcom accessible.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8e808199f776ef97ce93dd961adb5cef6eb64bc

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2e089e2163129d9fc0367a194975035b36abfd
Bug 1332444: Backed out changesets 87ee57647e0b and f04c1c83233c for causing bug 1354077; r=backout

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed out the backout in https://hg.mozilla.org/integration/mozilla-inbound/rev/a5234158960e because the backout caused bug 1360402.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

OK, between Yura's comment, paging this stuff back into my head, and reviewing the existing code, I think I figured out what went on in this bug and why I was mistaken to reopen it. Making notes so that this is clear:

* Since GetRemoteIAccessibleFor searches across all top-level docs for the content process whose ID matches that in the MSAA ID, it only needs be called when |this| is a chrome-based accessible and the desired MSAA ID is content-based. We don't know which document we need to search in, so we use GetRemoteIAccessibleFor to restrict that search to the top-level docs who reside in the correct content process. Restricting this via |!IsProxy()| works. An alternate condition could be |sIDGen.IsChromeID(GetExistingID())|, but the former is more efficient.

* If we already know our document (by virtue of being a proxy), we can skip that other stuff and go straight to resolving the accessible in our subtree via GetProxiedAccessibleInSubtree.

Why are we hitting stack overflows in bug 1354077, then? Based on what I'm seeing, the invariant that proxies to top-level docs should always have valid COM proxies is broken. But that has nothing to do with this bug.

Comment 17

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.