1347667 - unlink accessible's parent before accessible's shutdown when document shuts down

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

here's a couple reasons:

1) crashes like https://crash-stats.mozilla.com/report/index/826eaf7c-d1ff-4bd4-9a98-60a202170311

I assume that parent accessible may be destroyed on document's shutdown before its children, since order of a shutdown is defined by order in the cache. I'm not sure I have a good explanation though why the crash ratio is relatively small.

2) it doesn't make children adjustments, i.e. remove a child after a child during the document's shutdown, it's just overhead work.

Comment 1

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8954fdeed03b

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8847745 [details] [diff] [review]
patch

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

looks good. thanks

Comment 3

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57eb418785ab328ae4d5d32d2ca27635a67a1793
Bug 1347667 - unlink accessible's parent before accessible's shutdown when document shutdowns, r=yzen

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/57eb418785ab

Comment 5

[Ryan VanderMeulen [:RyanVM]]

I came across this bug again while looking at bug 1346518. It remains a low-frequency crash on ESR52. This patch looks pretty small and hasn't caused any obvious issues that I'm aware of, but I'm wondering if it's worth the risk of backporting given how fragile the a11y code seems to be in general.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> I came across this bug again while looking at bug 1346518. It remains a
> low-frequency crash on ESR52. This patch looks pretty small and hasn't
> caused any obvious issues that I'm aware of, but I'm wondering if it's worth
> the risk of backporting given how fragile the a11y code seems to be in
> general.

the patch is safe enough, basically this is one line patch mParent = null during document's shutdown. It makes the things less complicated (since no latter mParent processing), so it can be backported  I think. Should I request release and esr52 backports both?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Just ESR52 is fine.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 8847745 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: crashes, see bug description for details
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): low risk, see comment #6
String or UUID changes made by this patch: no

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8847745 [details] [diff] [review]
patch

Agree with comment #5. Let's uplift it to ESR52.3.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/ef1d17821945