1321936 - NotificationController needs to check whether "new" children are defunct before doing IPC binding

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

I am not sure whether this can actually happen, but I'm filing this just in case:

Scenario:

One outer doc, denoted O.
Two DocAccessibles, denoted A and B.

1) A is created as the child of O and enqueued to parent's NotificationController during that creation.
2) B is created as the child of O and enqueued to same NotificationController as A.
3) NotificationController::WillRefresh fires;
4) A is bound to O;
5) B is bound to O. This implicitly unbinds A from O and calls A->Shutdown();
6) Later on in the same method, we iterate through the new DocAccessibles. We touch A and B and construct their remote actors in order.
7) But A is already *defunct*!

We need to add a check to ensure that each DocAccessible is still alive before  initiating remote doc construction. Note that we have to add this check when iterating through newChildDocs due to the fact that, when iterating through mHangingChildDocuments, an outer doc binding might affect a previous DocAccessible that was already processed.

This bug is very timing sensitive since both A and B need to be enqueued to the NotificationController during the same quiescent period; if a WillRefresh were to fire between the enqueuing of A and B, everything would work correctly.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Is my scenario in the description actually plausible, or am I missing something?

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to Aaron Klotz [:aklotz] from comment #1)
> Is my scenario in the description actually plausible, or am I missing
> something?

I'd like to think that before the B document can be created a page hide notification for A must be fired, and so the doc accessible for A should be shut down before we try and bind child docs.  That would mean we should never try and send a constructor message for A because it doesn't get into the newChildDocs array.  However I'm not completely sure that is actually true.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

It can't hurt...

Comment 4

[Trevor Saunders (:tbsaunde)]

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

seems reasonable, maybe it'll help with the PDocAccessible::Transiton() crashes, we'll see.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a6af42c741deb13656fb868bb2ce1c5f26108dd
Bug 1321936: Check whether new child docs are defunct before doing IPC binding; r=tbsaunde

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/5a6af42c741d

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

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

Approval Request Comment
[Feature/Bug causing the regression]: a11y on e10s
[User impact if declined]: Crashes
[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?]: Trivial patch
[String changes made/needed]: None

Comment 8

[Julien Cristau [:jcristau]]

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

a11y+e10s fix for aurora52

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f14d6231bac