Closed Bug 1321936 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bugzilla, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Is my scenario in the description actually plausible, or am I missing something?
Flags: needinfo?(tbsaunde+mozbugs)
(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.
Flags: needinfo?(tbsaunde+mozbugs)
Attached patch Patch (deleted) — Splinter Review
It can't hurt...
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8819118 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8819118 [details] [diff] [review] Patch seems reasonable, maybe it'll help with the PDocAccessible::Transiton() crashes, we'll see.
Attachment #8819118 - Flags: review?(tbsaunde+mozbugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a6af42c741deb13656fb868bb2ce1c5f26108dd Bug 1321936: Check whether new child docs are defunct before doing IPC binding; r=tbsaunde
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Attachment #8819118 - Flags: approval-mozilla-aurora?
Comment on attachment 8819118 [details] [diff] [review] Patch a11y+e10s fix for aurora52
Attachment #8819118 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: