Closed Bug 1355488 Opened 3 years ago Closed 3 years ago

Fix broken tree causing bug 1347075

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

steps to reproduce:

1) run a test case from bug 1347075
2) stop debugger at MOZ_ASSERT(origContainer == prevChild->Parent(), "Broken tree") in DocAccessible.cpp https://dxr.mozilla.org/mozilla-central/source/accessible/generic/DocAccessible.cpp#2193
Summary: hunt down a broken tree cause from bug 1347075 → Fix broken tree causing bug 1347075
Attached patch patchSplinter Review
aria-owns on a document puts back its aria owned children on next ValidateARIAOwned(), which eventually leads us to broken tree, perhaps through a series of other bugs. By fixing aria-owns on a document, I don't longer see any of assertions added in bug 1347075.
Assignee: nobody → surkov.alexander
Attachment #8863799 - Flags: review?(dbolter)
Comment on attachment 8863799 [details] [diff] [review]
patch

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

Thanks! I don't really understand the commit message. Can you rephrase it or expand on it so that I can understand what is happening without debugging?

Note in general we should only add e10s happy browser tests until we have a way forward for our mochitests surviving.

::: accessible/tests/mochitest/treeupdate/test_ariaowns.html
@@ +586,5 @@
> +        })
> +      ];
> +
> +      this.invoke = () => {
> +        getNode('t9_container').src = 

nit: unecessary trailing whitespace.
(In reply to David Bolter [:davidb] from comment #3)

> Thanks! I don't really understand the commit message. Can you rephrase it or
> expand on it so that I can understand what is happening without debugging?

technically there's no commit message to understand, right? comment #1 though has a description of the change, the first part is the most important: "aria-owns on a document puts back its aria owned children on next ValidateARIAOwned()". Eventually fixing aria-owns on a document, fixes this bug. Does this one need to be extended?

> Note in general we should only add e10s happy browser tests until we have a
> way forward for our mochitests surviving.

can you tell more, what's wrong with mochitest in e10s environment?
(In reply to alexander :surkov from comment #4)
> (In reply to David Bolter [:davidb] from comment #3)
> 
> > Thanks! I don't really understand the commit message. Can you rephrase it or
> > expand on it so that I can understand what is happening without debugging?
> 
> technically there's no commit message to understand, right? comment #1
> though has a description of the change, the first part is the most
> important: "aria-owns on a document puts back its aria owned children on
> next ValidateARIAOwned()". Eventually fixing aria-owns on a document, fixes
> this bug. Does this one need to be extended?

Yes please. I think I need to understand the sequence in detail. I know you have tests but if you can describe here that would help me.

> 
> > Note in general we should only add e10s happy browser tests until we have a
> > way forward for our mochitests surviving.
> 
> can you tell more, what's wrong with mochitest in e10s environment?

A good question for RyanVM :)
(In reply to David Bolter [:davidb] from comment #5)
> (In reply to alexander :surkov from comment #4)
> > (In reply to David Bolter [:davidb] from comment #3)
> > 
> > > Thanks! I don't really understand the commit message. Can you rephrase it or
> > > expand on it so that I can understand what is happening without debugging?
> > 
> > technically there's no commit message to understand, right? comment #1
> > though has a description of the change, the first part is the most
> > important: "aria-owns on a document puts back its aria owned children on
> > next ValidateARIAOwned()". Eventually fixing aria-owns on a document, fixes
> > this bug. Does this one need to be extended?
> 
> Yes please. I think I need to understand the sequence in detail. I know you
> have tests but if you can describe here that would help me.

* you put aria-owns on a document element
* the aria-owned children are relocated under the document by DoARIAOwnsRelocation
* then ValidateARIAOwned puts them back on next WillRefresh, which is a bug that the patch fixes

that's basically it

> > > Note in general we should only add e10s happy browser tests until we have a
> > > way forward for our mochitests surviving.
> > 
> > can you tell more, what's wrong with mochitest in e10s environment?
> 
> A good question for RyanVM :)

is there anything I should do for this bug?
Comment on attachment 8863799 [details] [diff] [review]
patch

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

Thanks. For future tests, let's add them to /browser until we know more.
Attachment #8863799 - Flags: review?(dbolter) → review+
(In reply to David Bolter [:davidb] from comment #7)
> For future tests, let's add them to /browser until we know more.

I would prefer to know more before jumping into new area for sure. Afaik, a11y browser tests infrastructure is not that rich as mochitest one, and I wouldn't invest into that much at this stage without a strong reason.
https://hg.mozilla.org/mozilla-central/rev/8a2b71463a15
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.