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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

DocAccessibleParent::AddChildDoc allows it, but it calls into SetChildDoc which doesn't.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

(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.

Comment 2

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

Just a reminder for Trevor to review this today.

Comment 3

[Trevor Saunders (:tbsaunde)]

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?

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

(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.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eee284999b9c

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 8816650 [details] [diff] [review]
Patch

a11y+e10s fix for aurora52

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06fd54cb0558