Closed Bug 1443150 Opened 2 years ago Closed 2 years ago

Shutdown doc accessible children before un-setting its document node.

Categories

(Core :: Disability Access APIs, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: yzen, Assigned: yzen)

Details

Attachments

(1 file, 2 obsolete files)

Right now in doc accessible we unset doc node before we loop through its accessible children. That in turn can result in a crash, that happens 100% of the time when we log doc lifycycle for example.
Attached patch 1443150 patch (obsolete) — Splinter Review
Attachment #8956068 - Flags: review?(surkov.alexander)
Comment on attachment 8956068 [details] [diff] [review]
1443150 patch

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

::: accessible/generic/DocAccessible.cpp
@@ +450,5 @@
> +      accessible->mParent = nullptr;
> +      accessible->Shutdown();
> +    }
> +    iter.Remove();
> +  }

It looks a bit weird we shutdown children before child documents (because shutdown children will shutdown their child documents if connected), so I would avoid changing ordering here.

On the other side it should be safe to move mDocumentNode = nullptr to the end instead, so loggging can have access to mDocumentNode at late shutdown stages, which will fix this bug.
Attached patch 1443150 patch v2 (obsolete) — Splinter Review
Attachment #8956068 - Attachment is obsolete: true
Attachment #8956068 - Flags: review?(surkov.alexander)
Attachment #8956939 - Flags: review?(surkov.alexander)
Comment on attachment 8956939 [details] [diff] [review]
1443150 patch v2

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

::: accessible/generic/DocAccessible.cpp
@@ +487,5 @@
>      iter.Remove();
>    }
>  
> +  nsCOMPtr<nsIDocument> kungFuDeathGripDoc = mDocumentNode;
> +  mDocumentNode = nullptr;

do we really need kungFu thing at all? mDocument = nullptr should work fine, I don't think it can trigger sync cycle collection.
Attached patch 1443150 patch v3Splinter Review
Attachment #8956939 - Attachment is obsolete: true
Attachment #8956939 - Flags: review?(surkov.alexander)
Attachment #8957220 - Flags: review?(surkov.alexander)
Comment on attachment 8957220 [details] [diff] [review]
1443150 patch v3

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

might make sense to land it separately for easier regression finding
Attachment #8957220 - Flags: review?(surkov.alexander) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f263f5dceef3
unset doc accessible's document node at the very end of its shutdown. r=surkov
https://hg.mozilla.org/mozilla-central/rev/f263f5dceef3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.