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)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: bugzilla, Assigned: bugzilla)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
tbsaunde
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Is my scenario in the description actually plausible, or am I missing something?
Flags: needinfo?(tbsaunde+mozbugs)
Comment 2•8 years ago
|
||
(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)
Assignee | ||
Comment 3•8 years ago
|
||
It can't hurt...
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8819118 -
Flags: review?(tbsaunde+mozbugs)
Comment 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
Comment on attachment 8819118 [details] [diff] [review]
Patch
a11y+e10s fix for aurora52
Attachment #8819118 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 9•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•