1798620 - [CTW] Consider making RemoteAccessibleBase::mParent a pointer instead of an id

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[James Teh [:Jamie]]

RemoteAccessibleBase::mParent is currently an id. This means we have to do a hash table lookup to get a parent. While this doesn't seem like a big deal, we do a lot of ancestor walks and sometimes tree searches. Making it a pointer would make this faster.

We have to be careful here because mParent can refer to an OuterDoc Accessible in another document, which might be why this was made an id in the first place. That said, we already have this problem with mChildren, so I don't think this is any less safe than that.

I think I've seen this show up in profiles, but I don't recall where now. :( It's probably not worth tackling this unless it shows up again, but I thought this worth filing while it's on my mind.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1798620 part 1: Make RemoteAccessible::mParent a pointer instead of an id.

While an id was slightly safer, it also required a hash lookup every time we retrieved a parent.
This got expensive when there was a lot of tree walking, particularly given that we walk ancestors in some cases and we have to get the parent for NextSibling and PrevSibling.
Instead, mParent is now a RemoteAccessible pointer.
mChildren is already raw pointers (even when crossing documents) and we haven't encountered problems there recently, so I don't anticipate safety problems here.

Comment 2

[James Teh [:Jamie]]

Attachment: Bug 1798620 part 2: Remove DocAccessibleParent::mParentDoc.

We have RemoteAccessible::mParent and RemoteAccessible::mDoc, so DocAccessibleParent::mParentDoc was redundant.
Aside from the redundancy, this was one extra thing we needed to keep up to date and reason about.

This involved changing the call to RemoveChildDoc in Destroy to use Unbind instead because Unbind clears the parent RemoteAccessible, but RemoveChildDoc didn't.
That meant we had a dangling RemoteAccessible::mParent pointer, which was always a problem, but is more problematic now that we no longer have mParentDoc and the destructor checks the parent document.
Since Unbind is the only caller of RemoveChildDoc, RemoveChildDoc has been merged into Unbind.
This means there is now only a single place where child documents are unbound from their parent, which should make things easier to reason about.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ed816047f45
part 1: Make RemoteAccessible::mParent a pointer instead of an id. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/8a943b8a9cc1
part 2: Remove DocAccessibleParent::mParentDoc. r=eeejay

Comment 4

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/3ed816047f45
https://hg.mozilla.org/mozilla-central/rev/8a943b8a9cc1