Closed Bug 1321935 Opened 4 years ago Closed 4 years ago

ProxyAccessibleBase::SetChildDoc needs to be robust against replacing one document with another

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

Details

Attachments

(1 file)

DocAccessibleParent::AddChildDoc allows it, but it calls into SetChildDoc which doesn't.
Depends on: 1321622
Attached patch PatchSplinter Review
(This applies atop bug 1321622)

This fix modifies SetChildDoc to replace the child doc if one is already present. Instead of calling SetChildDoc(nullptr) to clear the doc, I've added a ClearChildDoc method that accepts the pointer that needs to be removed, as both child docs will still attempt to remove themselves even though there is only one remaining in the actual array.
Attachment #8816650 - Flags: review?(tbsaunde+mozbugs)
Just a reminder for Trevor to review this today.
Flags: needinfo?(tbsaunde+mozbugs)
Comment on attachment 8816650 [details] [diff] [review]
Patch

"we doesn't likes it" but this document replacement stuff is all kind of a pile of ductape anyway, so I guess this is fine for now.  I do kind of wonder if this will just allow other problems though, maybe bug 1321936 is related?
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8816650 - Flags: review?(tbsaunde+mozbugs) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 8816650 [details] [diff] [review]
> Patch
> 
> "we doesn't likes it" but this document replacement stuff is all kind of a
> pile of ductape anyway, so I guess this is fine for now.  I do kind of
> wonder if this will just allow other problems though, maybe bug 1321936 is
> related?

I filed that bug in the hope that it would be the fix for this one. Sadly it didn't help in this case.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee284999b9c90feaeaf8fc61df9f54efc764b27
Bug 1321935: Make ProxyAccessibleBase::SetChildDoc tolerate replacing one document with another, and add a new ClearChildDoc for removing that document; r=tbsaunde
https://hg.mozilla.org/mozilla-central/rev/eee284999b9c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8816650 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: a11y+e10s
[User impact if declined]: Invalid a11y state, assertions in debug builds
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple fix, good test coverage
[String changes made/needed]: None
Attachment #8816650 - Flags: approval-mozilla-aurora?
Comment on attachment 8816650 [details] [diff] [review]
Patch

a11y+e10s fix for aurora52
Attachment #8816650 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.