1353094 - Accessibility causes asserts/crashes on att.com on Windows

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Parks [:handyman]]

Bug 1346518 comment 19 mentions that https://www.att.com/wireless/ exposes accessibility crashes.  Most of the issues seem to be heavily timing related -- the page loads fine for me in a debug build.  However, a release build hooked up to a debugger hits quite a few assertions/crashes.

Most of the issues that cause trouble on that page occur quickly without any user interactivity but some happen (faster, at least) with user interaction.  There is currently a toolbar at the top of the page with buttons "Shop", "myAT&T" and "Support".  There is another smaller one just above it with buttons "Personal", "Business" and "About AT&T".  Mousing over either group exposes dropdown menus.  These menus make heavy use of the aria-owns attribute, which cause most of the trouble here.  Rolling the mouse over those buttons is the best way to cause failures.

The crash in bug 1346518 is believed to have a lot of causes.  This bug may not fix all of them but it will fix issues on att.com.

Comment 1

[David Parks [:handyman]]

Attachment: 1. Fix accessible insertion to preserve child order

Technically this is the line with the bug:

 mChildren.InsertElementAt(aNewIndex - 1, aChild);

That should be aNewIndex, not aNewIndex-1.  The rest of the patch is just cleanup to simplify the logic and make the MoveChild API a bit more consistent in the case where the child doesnt change parent -- 'append' doesn't make sense if it is already attached.

Comment 2

[David Parks [:handyman]]

Attachment: 2. Allow for UpdateTreeOnRemoval altering the aria-owns map

Not crazy about the UINT_MAX here but the cause is pretty clear.

Comment 3

[David Parks [:handyman]]

Attachment: 3. Refactor out the ability to relocate one accessible child

Tees up patch #4, which uses PutChildBack.

Comment 4

[David Parks [:handyman]]

Attachment: 4. Do not delete relocated accessibles

I'm not sure if relocating back to parent is what was in mind for deleted aria-owns elements but, if we want to delete the accessible then there is a lot more cleanup we need to do, including removing its relationship to its old place in the DOM tree.

Comment 5

[David Parks [:handyman]]

Attachment: 5. Unset relocated flag when returning accessible to original position

Comment 6

[David Parks [:handyman]]

Attachment: 6. HTMLLIAccessibles must shut down their bullet

Comment 7

[David Parks [:handyman]]

Attachment: 7. Preserve child order in HTMLLIAccessible

This is way too many review requests to the same person.  Hopefully, you can delegate some...

Comment 8

[David Parks [:handyman]]

Attachment: 8. Don't update HTMLLIAccessible's bullet if it was shutdown

Comment 9

[David Parks [:handyman]]

Comment on attachment 8854107 [details] [diff] [review]
8. Don't update HTMLLIAccessible's bullet if it was shutdown

The first IsDefunct check seems necessary to me but I haven't seen an issue in the wild.  The second one does happen on att.com.  I haven't dug too far into BeforeRemoval, which I feel may be harboring the real bug here (so someone else may have a better idea...).  The case that triggered the issue was some kind of text element that seems to get special treatment in the TreeMutation code.

Comment 10

[David Parks [:handyman]]

D'oh... comment 9 is actually about patch #9 (UpdateTreeOnRemoval) coming up below, not patch #8 (HTMLLIAccessible's bullet), which needs both of its IsDefunct checks.

Comment 11

[David Parks [:handyman]]

Attachment: 9. Ignore shutdown accessibles when navigating DOM for tree removal

This is the patch I wrote about in comment 9.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to David Parks (dparks) [:handyman] from comment #6)
> Created attachment 8854097 [details] [diff] [review]
> 6. HTMLLIAccessibles must shut down their bullet

list bullet is expected to shutdown by ShutdownChildrenInSubtree, is there a case where it doesn't work?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to David Parks (dparks) [:handyman] from comment #1)
> Created attachment 8854090 [details] [diff] [review]
> 1.  Fix accessible insertion to preserve child order
> 
> Technically this is the line with the bug:
> 
>  mChildren.InsertElementAt(aNewIndex - 1, aChild);
> 
> That should be aNewIndex, not aNewIndex-1.

this makes sense. however do you have ideas why insertion at a wrong position under the parent leads to a wrong parent issue?

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 8854106 [details] [diff] [review]
7. Preserve child order in HTMLLIAccessible

Review of attachment 8854106 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/html/HTMLListAccessible.cpp
@@ +105,1 @@
>      return HyperTextAccessible::InsertChildAt(aIndex + 1, aChild);

the idea of special handling of 0 index is to insert an accessible at proper position when there's no previous sibling in DOM order, non-zero case should be ok because the insertion position is calculated based on the previous sibling index. 
So the change should break the ordering of
<ul><li>hello</li></ul>
li.insertBefore(li.fisrtChild, document.createElement('input'));
by inserting the input accessible after text node accessible.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 8854094 [details] [diff] [review]
4. Do not delete relocated accessibles

Review of attachment 8854094 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure I can see why we would need to put something back that is about to destroy. However if there are relocated children inside the destroying subtree, then it makes sense to put them back. This would help to get rid of ValidateARIAOwned.

Comment 16

[David Parks [:handyman]]

Comment on attachment 8854090 [details] [diff] [review]
1.  Fix accessible insertion to preserve child order

Lets green up tests first :(

Comment 17

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8854111 [details] [diff] [review]
9. Ignore shutdown accessibles when navigating DOM for tree removal

># HG changeset patch
># User David Parks <[e-mail]>
># Date 1491183466 25200
>#      Sun Apr 02 18:37:46 2017 -0700
># Node ID fcbb8ca4ff3b1d0608b084b91f19b097dc303ec9
># Parent  ec934e296cd24a7af1bcc90e167e7a36c8bb0087
>Bug 1353094 - Part 9: Ignore shutdown accessibles when navigating DOM for tree removal
>
>UpdateTreeOnRemoval navigates the DOM matching DOM Elements with Accessibles.  If it finds an accessible in the tree that is shutdown then it needs to skip it -- its not really in the accessible tree.

so, that's half of what this does, it doesn't mention the DropMutationEvent() business.

>   if (child) {
>-    mt.BeforeRemoval(child);
>-    MOZ_ASSERT(aContainer == child->Parent(), "Wrong parent");
>-    aContainer->RemoveChild(child);
>-    UncacheChildrenInSubtree(child);
>+    if (!child->IsDefunct()) {

hrm, it seems like that should really never be false, I tend to think the issue is GetAccessible() is returning something that's already shut down.

>+      MOZ_ASSERT(aContainer == child->Parent(), "Wrong parent");
>+      mt.BeforeRemoval(child);
>+      // BeforeRemoval calls CoalesceMutationEvents which may call
>+      // DropMutationEvent which may shut down the child.
>+      if (!child->IsDefunct()) {
>+        MOZ_ASSERT(aContainer == child->Parent(), "BeforeRemoval altered parent");
>+        aContainer->RemoveChild(child);
>+        UncacheChildrenInSubtree(child);

hrm interesting, the differences between UncacheChildrenInSubtree() and ShutdownChildrenInSubtree() are unfortunate, but assuming the accessible goes away when ShutdownChildrenInSubtree() is called I think its fine.

Comment 18

[David Parks [:handyman]]

Attachment: Make UpdateTreeOnRemoval respect aria-owns accessibles

Instead of restoring the position of relocated elements, this version deletes them in their relocated spot.   Most of the other patches were cleanup for when the accessibles were shutdown late but this avoids the late shutdown altogether.

Comment 19

[David Parks [:handyman]]

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

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 8856593 [details] [diff] [review]
Make UpdateTreeOnRemoval respect aria-owns accessibles

Review of attachment 8856593 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/DocAccessible.cpp
@@ +1989,5 @@
>  #endif
>  
> +  if (child && child->IsRelocated() && aContainer != child->Parent()) {
> +    // The subtree was relocated away from aContainer
> +    return;

this is rather incorrect because this doesn't shutdown a removing accessible, consider an example:

<div id="child"></div>
<div id="parent" aria-owns="child"></div>

then do child.remove() to remove it from DOM, which will trigger UpdateTreeOnRemoval for a child, and then we reject to shutdown it and its subtree.

I'm not sure I understand what problem you solve. Could you give me more background on it?

Comment 21

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #20)

> I'm not sure I understand what problem you solve. Could you give me more
> background on it?

it sounds like you want to shutdown a child accessible but grab a right container for it to do so.

Comment 22

[David Parks [:handyman]]

Attachment: Entries from refcount log for non-updated aria-owns accessible

> I'm not sure I understand what problem you solve. Could you give me more
> background on it?

Sure.  This bug is a spin-off of bug 1346518 -- it handles that RemoveChild crash as it appears on att.com's home page.  They are doing something like this:

<ul aria-owns="ge5p_z1_t1001">
   <li>
     <a id="ge5p_z1_t1001" href="http://www.att.com/">att.com</a>
   </li>
   [...more <li>s...]
</ul>

Typically the result is this in Accessible::RemoveChild() [1]:

  MOZ_DIAGNOSTIC_ASSERT(aChild->mParent == this, "Wrong parent");

Although we would have tripped just before that in DocAccessible::UpdateTreeOnRemoval in a debug build [2]:

  MOZ_ASSERT(aContainer == child->Parent(), "Wrong parent");

This is because the element is relocated with aria-owns so its accessible Parent() is different than its adjacent parent in the DOM but UpdateTreeOnRemoval navigates the tree... by navigating the DOM.  So, for relocated elements, there is a mismatch.

---

> > +  if (child && child->IsRelocated() && aContainer != child->Parent()) {
> > +    // The subtree was relocated away from aContainer
> > +    return;
> 
> this is rather incorrect because this doesn't shutdown a removing
> accessible

To show how ownership of the accessible works in this case, I logged the address of the accessible 'child' during a refcount-logging run.  I then pulled the refcount events for that particular accessible object and dumped them into the attached log file.  In this run, the object saw 10 refcount events.  Summarizing them:

1. nsAccessibilityService::CreateAccessible   +1 = 1
2. DocAccessible::BindToDocument              +1 = 2
3. nsAccessibilityService::CreateAccessible   -1 = 1
4. AccShowEvent::AccShowEvent                 +1 = 2
5. DocAccessible::DoARIAOwnsRelocation/
   nsTArray_Impl<RefPtr<T>>::InsertElementAt  +1 = 3
6. AccEvent::~AccEvent                        -1 = 2
7. AccStateChangeEvent::AccStateChangeEvent   +1 = 3
8. AccEvent::~AccEvent                        -1 = 2
9. PresShell::Destroy/
   DocAccessible::Shutdown                    -1 = 1
10. Accessible::LastRelease                   -1 = 0

In the case of your example, the object will still exist and won't be 'Shutdown()' there but it is cleaned up.  Maybe I should just add a call to Shutdown in the conditional?  OTOH, UpdateTreeOnRemoval currently doesn't call Shutdown on any object so this would be new behavior.

[1] https://hg.mozilla.org/mozilla-central/annotate/731639fccc70/accessible/generic/Accessible.cpp#l2136
[2] https://hg.mozilla.org/mozilla-central/annotate/731639fccc70/accessible/generic/DocAccessible.cpp#l1987

Comment 23

[alexander :surkov (:asurkov)]

(In reply to David Parks (dparks) [:handyman] from comment #22)

> Although we would have tripped just before that in
> DocAccessible::UpdateTreeOnRemoval in a debug build [2]:
> 
>   MOZ_ASSERT(aContainer == child->Parent(), "Wrong parent");
> 
> This is because the element is relocated with aria-owns so its accessible
> Parent() is different than its adjacent parent in the DOM but
> UpdateTreeOnRemoval navigates the tree... by navigating the DOM.  So, for
> relocated elements, there is a mismatch.

ok, it seems comment #21 is a way to go, i.e. do UpdateTreeOnRemoval for a relocated child but fix its 'container' before doing that.

> 9. PresShell::Destroy/
>    DocAccessible::Shutdown                    -1 = 1
> 10. Accessible::LastRelease                   -1 = 0

DocAccessible will shutdown everything for sure when the DOM document is destroyed. The point is a child should die earlier, when DOM node was removed from the document.

> In the case of your example, the object will still exist and won't be
> 'Shutdown()' there but it is cleaned up.  

What does cleaned up mean here? 

> Maybe I should just add a call to
> Shutdown in the conditional?

It appears all we should do is do UpdateTreeOnRemoval for that child and for a proper container.

>  OTOH, UpdateTreeOnRemoval currently doesn't
> call Shutdown on any object so this would be new behavior.

It's not supposed to shutdown immediately, it rather marks an accessible node for a latter shutdown, after all proper events are fired.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 8856593 [details] [diff] [review]
Make UpdateTreeOnRemoval respect aria-owns accessibles

Review of attachment 8856593 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/DocAccessible.cpp
@@ +1176,5 @@
> +    UpdateTreeOnRemoval(container, aChildNode, child);
> +    if (child && child->IsRelocated()) {
> +      nsTArray<RefPtr<Accessible> >* children = mARIAOwnsHash.Get(container);
> +      children->RemoveElement(child);
> +    }

you don't have to do this, because ValidateARIAOwned takes care of it (see below), correct?

@@ +1989,5 @@
>  #endif
>  
> +  if (child && child->IsRelocated() && aContainer != child->Parent()) {
> +    // The subtree was relocated away from aContainer
> +    return;

I think I missed that ValidateARIAOwned takes care of it, so it may be ugly but should work

Comment 25

[alexander :surkov (:asurkov)]

Attachment: simplify logic of the subtree removals

Since you're heavily debugged this code recently, then would you be interested to review this patch? It is different from the approach you taken, but it should fix the bug too.

Comment 26

[David Parks [:handyman]]

Comment on attachment 8860133 [details] [diff] [review]
simplify logic of the subtree removals

Its probably a good idea to have someone from accessibility handle this.

Comment 27

[alexander :surkov (:asurkov)]

Comment on attachment 8860133 [details] [diff] [review]
simplify logic of the subtree removals

Review of attachment 8860133 [details] [diff] [review]:
-----------------------------------------------------------------

Trevor is not that active in a11y these days, so asking Yura too, who might be around for a review.

Comment 28

[Jim Mathies [:jimm]]

(In reply to alexander :surkov from comment #27)
> Comment on attachment 8860133 [details] [diff] [review]
> simplify logic of the subtree removals
> 
> Review of attachment 8860133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Trevor is not that active in a11y these days, so asking Yura too, who might
> be around for a review.

He's on pto for a while.

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8860133 [details] [diff] [review]
simplify logic of the subtree removals

Review of attachment 8860133 [details] [diff] [review]:
-----------------------------------------------------------------

(Yes Yura and Trevor are both away right now)

::: accessible/generic/DocAccessible.cpp
@@ +1979,3 @@
>  #endif
>  
> +  // Relocated accessibles are held by ValidateARIAOwned.

How do they get removed?

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8860133 [details] [diff] [review]
simplify logic of the subtree removals

Review of attachment 8860133 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't catch any dragons.

::: accessible/generic/DocAccessible.cpp
@@ +1991,5 @@
> +void
> +DocAccessible::ContentRemoved(nsIContent* aContentNode)
> +{
> +  // If child node is not accessible then look for its accessible children.
> +  Accessible* content = GetAccessible(aContentNode);

nit: I'd prefer this called accessibleContent

Comment 31

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #29)
> > +  // Relocated accessibles are held by ValidateARIAOwned.
> 
> How do they get removed?

https://dxr.mozilla.org/mozilla-central/source/accessible/generic/DocAccessible.cpp?q=ValidateARIAOwned&redirect_type=direct#2053

(In reply to David Bolter [:davidb] from comment #30)

> > +  // If child node is not accessible then look for its accessible children.
> > +  Accessible* content = GetAccessible(aContentNode);
> 
> nit: I'd prefer this called accessibleContent

it feels long, maybe 'acc'?

Comment 32

[alexander :surkov (:asurkov)]

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

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

(Yura pinged me yesterday that he was prepared to review this -- because he's bad at PTO)

Alex, please keep in mind recent discussion on the value of commit messages. Please describe the "what" and "why" of the change in a few sentences.
https://groups.google.com/forum/#!topic/mozilla.dev.platform/qERnoJniCds

Comment 34

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #33)
> (Yura pinged me yesterday that he was prepared to review this -- because
> he's bad at PTO)

yeah, a big thanks Yura for that.

> Alex, please keep in mind recent discussion on the value of commit messages.
> Please describe the "what" and "why" of the change in a few sentences.
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/qERnoJniCds

I feel confused, whether I was inaccurate lately in giving proper commit messages. I was about to go with 'simplify the logic of accessible subtrees removal', which I hope answers to what and why same time.

Comment 35

[alexander :surkov (:asurkov)]

Attachment: simplify logic of the subtree removals v2

with removed acc->IsRelocated() condition in RemoveChild, which sneaked from previous patch versions. ValidateARIAOwned uses same RemoveChild to validate the tree, so this condition simply prevents the validation.

Comment 36

[alexander :surkov (:asurkov)]

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

Comment 37

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7636849e6f5829175bc06af8b81051ac7b61a687
Bug 1353094 - simplify the logic of accessible subtrees removal, r=davidb

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7636849e6f58

Comment 40

[Jim Mathies [:jimm]]

https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3Aa11y%3A%3AAccessible%3A%3ARemoveChild&date=%3E%3D2017-04-12T20%3A34%3A00.000Z&date=%3C2017-04-26T20%3A34%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-build_id&_sort=-date&page=1