Closed Bug 1362063 Opened 3 years ago Closed 3 years ago

replace delayed ValidateARIAOwned on sync DOM tree traversal

Categories

(Core :: Disability Access APIs, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
ValidateARIAOwned keeps dead DOM/frame's accessible in the tree longer than we want and it has more sophisticated logic than the straightforward traverse DOM tree approach; I suspect it may lead to the broken tree problem. Of course on a downside, it takes us to traverse a removed tree twice: traverse an accessible tree, and then traverse a DOM tree, but that shouldn't hurt us bad.
Attachment #8864551 - Flags: review?(dbolter)
Assignee: nobody → surkov.alexander
Attachment #8864551 - Flags: review?(dbolter) → review+
Comment on attachment 8864551 [details] [diff] [review]
patch

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

::: accessible/generic/DocAccessible.cpp
@@ +1986,5 @@
> +  mt.BeforeRemoval(aChild);
> +
> +  if (aChild->IsRelocated()) {
> +    nsTArray<RefPtr<Accessible> >* owned = mARIAOwnsHash.Get(parent);
> +    owned->RemoveElement(aChild);

Is it worth asserting owned?
(In reply to David Bolter [:davidb] from comment #1)

> > +  mt.BeforeRemoval(aChild);
> > +
> > +  if (aChild->IsRelocated()) {
> > +    nsTArray<RefPtr<Accessible> >* owned = mARIAOwnsHash.Get(parent);
> > +    owned->RemoveElement(aChild);
> 
> Is it worth asserting owned?

you mean debug assert before crash? I can add it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6669f26d7f1366221b9edd3d82ac699e98101dc7
Bug 1362063 - replace delayed ValidateARIAOwned on straightforward DOM tree traversal, r=davidb
https://hg.mozilla.org/mozilla-central/rev/6669f26d7f13
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1363027
Duplicate of this bug: CVE-2017-7785
You need to log in before you can comment on or make changes to this bug.