Closed Bug 1347667 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- fixed
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: surkov, Assigned: surkov)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
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.
Attachment #8847745 - Flags: review?(yzenevich)
Summary: unlink accessible's parent before accessible's shutdown → unlink accessible's parent before accessible's shutdown when document shutdowns
Comment on attachment 8847745 [details] [diff] [review]
patch

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

looks good. thanks
Attachment #8847745 - Flags: review?(yzenevich) → review+
Summary: unlink accessible's parent before accessible's shutdown when document shutdowns → unlink accessible's parent before accessible's shutdown when document shuts down
https://hg.mozilla.org/integration/mozilla-inbound/rev/57eb418785ab328ae4d5d32d2ca27635a67a1793
Bug 1347667 - unlink accessible's parent before accessible's shutdown when document shutdowns, r=yzen
https://hg.mozilla.org/mozilla-central/rev/57eb418785ab
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee: nobody → surkov.alexander
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.
Flags: needinfo?(surkov.alexander)
(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?
Flags: needinfo?(surkov.alexander)
Just ESR52 is fine.
Flags: needinfo?(surkov.alexander)
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
Flags: needinfo?(surkov.alexander)
Attachment #8847745 - Flags: approval-mozilla-esr52?
Comment on attachment 8847745 [details] [diff] [review]
patch

Agree with comment #5. Let's uplift it to ESR52.3.
Attachment #8847745 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.