Closed Bug 1316789 Opened 3 years ago Closed 3 years ago

UncacheChildren should not go between documents

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(1 file)

No description provided.
discovered while dealing with bug 1270916 but its more or less a separate issue.  Probably should have a test for this on its own, but that bug depends on this, and it needs to land so that'll have to wait.
Attachment #8809715 - Flags: review?(dbolter)
Comment on attachment 8809715 [details] [diff] [review]
UncacheChildren should not go between documents

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

::: accessible/generic/DocAccessible.cpp
@@ +2277,5 @@
> +    Accessible* child = aRoot->ContentChildAt(idx);
> +    if (!child->IsDoc()) {
> +      UncacheChildrenInSubtree(child);
> +    }
> +  }

I'm so rusty on life cycle. Is it okay that we still set eIsNotInDocument for when IsDoc succeeeds?
(In reply to David Bolter [:davidb] from comment #2)
> Comment on attachment 8809715 [details] [diff] [review]
> UncacheChildren should not go between documents
> 
> Review of attachment 8809715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/DocAccessible.cpp
> @@ +2277,5 @@
> > +    Accessible* child = aRoot->ContentChildAt(idx);
> > +    if (!child->IsDoc()) {
> > +      UncacheChildrenInSubtree(child);
> > +    }
> > +  }
> 
> I'm so rusty on life cycle. Is it okay that we still set eIsNotInDocument
> for when IsDoc succeeeds?

the only original callers pass in a accessible that is being removed from the document that is |this|.  So the only document it can make sense to pass as a Root is the same one as |this|.  I'm not totally sure if setting it for the document itself makes sense, but that behavior doesn't change.  Then if aRoot is a document other than this the caller is wrong.  Other than the recursive call site the two callers are both removing accessibles from the tree so I'm pretty sure they aren't doing anything wrong.  That means the only case where something bad can happen is the recursive case which is fixed in this patch.
Comment on attachment 8809715 [details] [diff] [review]
UncacheChildren should not go between documents

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

If you can think of a comment to help the reader understand this please add one.
Attachment #8809715 - Flags: review?(dbolter) → review+
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd21f10a7d6e
UncacheChildren should not go between documents r=davidb
https://hg.mozilla.org/mozilla-central/rev/dd21f10a7d6e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8809715 [details] [diff] [review]
UncacheChildren should not go between documents

dependency of bug 1270916
Attachment #8809715 - Flags: approval-mozilla-aurora?
Comment on attachment 8809715 [details] [diff] [review]
UncacheChildren should not go between documents

More uplift material for aurora 52, e10s/a11y fix along with bug 1270916.
Attachment #8809715 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.