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)
Core
Disability Access APIs
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: lsocks, Unassigned)
Details
Attachments
(1 file, 4 obsolete files)
6.41 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•9 years ago
|
||
Assignee: nobody → lorien
Attachment #8637229 -
Flags: review?(tbsaunde+mozbugs)
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
I think we could also add checks to RecvBindChildDoc() and maybe other Recv methods?
Reporter | ||
Comment 4•9 years ago
|
||
Attachment #8637229 -
Attachment is obsolete: true
Attachment #8637330 -
Flags: review?(tbsaunde+mozbugs)
Reporter | ||
Comment 5•9 years ago
|
||
Attachment #8637330 -
Attachment is obsolete: true
Attachment #8637330 -
Flags: review?(tbsaunde+mozbugs)
Attachment #8637336 -
Flags: review?(tbsaunde+mozbugs)
Reporter | ||
Comment 6•9 years ago
|
||
Attachment #8637336 -
Attachment is obsolete: true
Attachment #8637336 -
Flags: review?(tbsaunde+mozbugs)
Attachment #8637338 -
Flags: review?(tbsaunde+mozbugs)
Reporter | ||
Comment 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
Trevor I see an old review request on the latest patch, still useful?
Flags: needinfo?(tbsaunde+mozbugs)
Comment 9•9 years ago
|
||
(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)
Reporter | ||
Comment 10•9 years ago
|
||
I think tbsaunde was fuzzing with this a few months ago, I'm not sure what the intention with it is re: tree integration.
Comment 11•9 years ago
|
||
Aha! OK thanks. (I get nag emails while the review request remains but that's ok)
Comment 12•8 years ago
|
||
Comment on attachment 8637353 [details] [diff] [review]
check proxy tree
Lorien, Trevor's thinking we can land this if you put it behind #ifdebug ...
Comment 13•3 years ago
|
||
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)
Comment 14•3 years ago
|
||
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.
Description
•