Closed Bug 1332444 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

Details

(Keywords: access, Whiteboard: aes+)

Attachments

(1 file, 2 obsolete files)

AccessibleWrap::GetIAccessibleFor does not resolve in some cases where we are trying to get a remote IAccessible for a proxy.
Attached patch 1332444 patch (obsolete) — Splinter Review
Attachment #8828505 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8828505 [details] [diff] [review]
1332444 patch

># HG changeset patch
># User Yura Zenevich <yzenevich@mozilla.com>
>
>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.
Attachment #8828505 - Flags: review?(tbsaunde+mozbugs) → review-
Summary: ensure that AccessibleWrap::GetRemoteIAccessibleFor is only called for a proxy accessible wrap. → ensure that AccessibleWrap::GetRemoteIAccessibleFor is never called for a proxy accessible wrap.
Attached patch 1332444.1 v2Splinter Review
Attachment #8828505 - Attachment is obsolete: true
Attachment #8829459 - Flags: review?(tbsaunde+mozbugs)
Attached patch 1332444 traverse subtree (obsolete) — Splinter Review
Attachment #8829460 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8829459 [details] [diff] [review]
1332444.1 v2

># HG changeset patch
># User Yura Zenevich <yzenevich@mozilla.com>
>
>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.
Attachment #8829459 - Flags: review?(tbsaunde+mozbugs) → review+
Pushed by yura.zenevich@gmail.com:
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
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ee57647e0b
adding comment when calling GetRemoteIAccessible. r=tbsaunde
Attachment #8829460 - Flags: review?(tbsaunde+mozbugs)
Attachment #8829460 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f04c1c83233c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
Flags: needinfo?(tbsaunde+mozbugs)
To clarify: do you recall which accessibles were not resolving?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: aes+
Depends on: 1360402
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.
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(aklotz)
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.