Closed Bug 1353094 Opened 3 years ago Closed 2 years ago

Accessibility causes asserts/crashes on att.com on Windows

Categories

(Core :: Disability Access APIs, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: handyman, Assigned: handyman)

References

(Blocks 1 open bug)

Details

(Whiteboard: aes+)

Crash Data

Attachments

(3 files, 10 obsolete files)

4.12 KB, patch
Details | Diff | Splinter Review
39.37 KB, text/plain
Details
11.00 KB, patch
Details | Diff | Splinter Review
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.
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.
Assignee: nobody → davidp99
Attachment #8854090 - Flags: review?(tbsaunde+mozbugs)
Not crazy about the UINT_MAX here but the cause is pretty clear.
Attachment #8854091 - Flags: review?(tbsaunde+mozbugs)
Tees up patch #4, which uses PutChildBack.
Attachment #8854092 - Flags: review?(tbsaunde+mozbugs)
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.
Attachment #8854094 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854097 - Flags: review?(tbsaunde+mozbugs)
This is way too many review requests to the same person.  Hopefully, you can delegate some...
Attachment #8854106 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854107 - Flags: review?(tbsaunde+mozbugs)
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.
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.
This is the patch I wrote about in comment 9.
Attachment #8854111 - Flags: review?(tbsaunde+mozbugs)
(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?
Whiteboard: aes+
(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 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 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 on attachment 8854090 [details] [diff] [review]
1.  Fix accessible insertion to preserve child order

Lets green up tests first :(
Attachment #8854090 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854091 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854092 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854094 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854095 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854097 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854106 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854107 - Flags: review?(tbsaunde+mozbugs)
Attachment #8854111 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8854111 [details] [diff] [review]
9. Ignore shutdown accessibles when navigating DOM for tree removal

># HG changeset patch
># User David Parks <dparks@mozilla.com>
># 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.
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.
Attachment #8854090 - Attachment is obsolete: true
Attachment #8854091 - Attachment is obsolete: true
Attachment #8854092 - Attachment is obsolete: true
Attachment #8854094 - Attachment is obsolete: true
Attachment #8854095 - Attachment is obsolete: true
Attachment #8854097 - Attachment is obsolete: true
Attachment #8854106 - Attachment is obsolete: true
Attachment #8854107 - Attachment is obsolete: true
Attachment #8854111 - Attachment is obsolete: true
Attachment #8856593 - Flags: review?(surkov.alexander)
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?
(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.
> 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
Attachment #8858056 - Attachment mime type: text/x-log → text/plain
(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 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
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.
Attachment #8860133 - Flags: review?(davidp99)
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.
Attachment #8860133 - Flags: review?(davidp99) → review?(tbsaunde+mozbugs)
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.
Attachment #8860133 - Flags: review?(yzenevich)
Attachment #8856593 - Flags: review?(surkov.alexander)
(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.
Attachment #8860133 - Flags: review?(yzenevich) → review?(dbolter)
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 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
Attachment #8860133 - Flags: review?(dbolter) → review+
(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'?
(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
(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.
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.
Attachment #8860133 - Attachment is obsolete: true
Attachment #8860133 - Flags: review?(tbsaunde+mozbugs)
https://hg.mozilla.org/mozilla-central/rev/7636849e6f58
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Crash Signature: [@ mozilla::a11y::Accessible::RemoveChild ]
Duplicate of this bug: 1346518
Depends on: 1360122
Depends on: 1363027
Depends on: 1372221
You need to log in before you can comment on or make changes to this bug.