Closed Bug 1186427 Opened 9 years ago Closed 3 years ago

Add more checks to ensure that proxy tree structure is valid

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: lsocks, Unassigned)

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch check proxy tree (obsolete) — Splinter Review
Assignee: nobody → lorien
Attachment #8637229 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8637229 [details] [diff] [review] check proxy tree >+void >+DocAccessibleParent::CheckProxies() >+{ >+ // Ensure that mAccessibles is correct. >+ for (auto iter = mAccessibles.Iter(); !iter.Done(); iter.Next()) { >+ ProxyAccessible* proxy = iter.Get()->mProxy; >+ >+ // Check that this proxy's parent has it as a child. >+ bool validParent = false; >+ ProxyAccessible* parent = proxy->Parent(); >+ for (size_t i = 0; i < parent->ChildrenCount(); i++) { >+ if (!mAccessibles.GetEntry(parent->ID())) >+ MOZ_CRASH("found proxy unknown by mAccessibles as proxy's parent"); blank line please and actually you should be able to hoist this out of the loop right? >+ if (proxy == parent->ChildAt(i)) { >+ validParent = true; >+ break; >+ } >+ } >+ if (!validParent) >+ MOZ_CRASH("mAccessibles broken - proxy's parent does not have it as a child"); >+ >+ if (proxy->ChildrenCount() == 0) >+ continue; >+ >+ // Check that this proxy's children have it as its parent. >+ bool validChildren = false; >+ for (size_t i = 0; i < proxy->ChildrenCount(); i++) { >+ ProxyAccessible* child = proxy->ChildAt(i); >+ if (!mAccessibles.GetEntry(child->ID())) >+ MOZ_CRASH("found proxy unknown by mAccessibles in proxy's children"); nit, blank line >+ if (proxy == child->Parent()) { >+ validChildren = true; >+ break; isn'this inverted? your checking that *1* child has parent as its parent, not that *all* of them do. >+DocAccessibleParent::CheckProxyTree(ProxyAccessible* root) aRoot >+{ >+ size_t sum = 1; >+ for (size_t i = 0; i < ChildrenCount(); i++) { you mean root->ChildrenCount() right? >+ ProxyAccessible* child = root->ChildAt(i); >+ sum = sum + CheckProxyTree(child); what does the var buy you? > void CheckDocTree() const; >+ void CheckProxies(); >+ size_t CheckProxyTree(ProxyAccessible* root); const for both
Attachment #8637229 - Flags: review?(tbsaunde+mozbugs)
I think we could also add checks to RecvBindChildDoc() and maybe other Recv methods?
Attachment #8637330 - Attachment is obsolete: true
Attachment #8637330 - Flags: review?(tbsaunde+mozbugs)
Attachment #8637336 - Flags: review?(tbsaunde+mozbugs)
Attached patch crash in loop instead of after (obsolete) — Splinter Review
Attachment #8637336 - Attachment is obsolete: true
Attachment #8637336 - Flags: review?(tbsaunde+mozbugs)
Attachment #8637338 - Flags: review?(tbsaunde+mozbugs)
Attached patch check proxy treeSplinter Review
Fixed an issue in the previous version where it was miscounting the proxies due to counting documents which aren't part of mAccessibles.
Attachment #8637338 - Attachment is obsolete: true
Attachment #8637338 - Flags: review?(tbsaunde+mozbugs)
Attachment #8637353 - Flags: review?(tbsaunde+mozbugs)
Trevor I see an old review request on the latest patch, still useful?
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to David Bolter [:davidb] from comment #8) > Trevor I see an old review request on the latest patch, still useful? not really clear
Flags: needinfo?(tbsaunde+mozbugs)
I think tbsaunde was fuzzing with this a few months ago, I'm not sure what the intention with it is re: tree integration.
Aha! OK thanks. (I get nag emails while the review request remains but that's ok)
Comment on attachment 8637353 [details] [diff] [review] check proxy tree Lorien, Trevor's thinking we can land this if you put it behind #ifdebug ...

The bug assignee didn't login in Bugzilla in the last 7 months.
:Jamie, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: lorien → nobody
Flags: needinfo?(jteh)

It's not really clear what the precise intent was here. I think we can just add checks as we determine we need them.

Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(jteh)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: