Closed Bug 626667 Opened 9 years ago Closed 9 years ago

deal with cached accessible tree only

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b11

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(3 files)

the work has been started in bug 606924, however accessible tree traversal methods may trigger accessible tree update (like GetParent()) at unsafe time (for example when AT handles hide event for display: none element). We should deal with cached accessible tree only to accessible tree creation at wrong time.
This is a good idea but are you thinking this needs to make it into FF4?
(In reply to comment #1)
> This is a good idea but are you thinking this needs to make it into FF4?

Yes, we need to get rid all cases when we may create accessible at wrong time.
Comment on attachment 507113 [details] [diff] [review]
part1v1: getParent shouldn't repair the tree [landed 2011-01-29]

This sort of blows my mind.
Attachment #507132 - Flags: review?(bolterbugz)
Attachment #507132 - Flags: approval2.0?
We'll need QA of course.
Comment on attachment 507113 [details] [diff] [review]
part1v1: getParent shouldn't repair the tree [landed 2011-01-29]

OK r=me pending QA.

>-  virtual nsAccessible* GetParent();
>+  nsAccessible* GetParent() const { return mParent; }
Attachment #507113 - Flags: review?(bolterbugz) → review+
Comment on attachment 507132 [details] [diff] [review]
part2v1: get rid GetCachedParent [landed 2011-01-29]

r=me pending QA
Attachment #507132 - Flags: review?(bolterbugz) → review+
Running with the try-server build for this patch, I got a non-reproducible crash:
http://crash-stats.mozilla.com/report/index/bp-e32411a3-d44c-4a6a-be6f-6135d2110126

Performing the same steps didn't bring the crash a second time.
Attachment #507113 - Flags: approval2.0? → approval2.0+
Attachment #507132 - Flags: approval2.0? → approval2.0+
(In reply to comment #10)
> Running with the try-server build for this patch, I got a non-reproducible
> crash:
> http://crash-stats.mozilla.com/report/index/bp-e32411a3-d44c-4a6a-be6f-6135d2110126

no, stack trace unfortunately
Haven't seen another crash yet, and David and I agreed that it would be good to land this so if there is a crasher that's being introduced, we have a good chance of catching it in a regular nightly, with a valid stack trace.
ok, it's queued for landing
Blocks: 629137
part1: getParent shouldn't repair the tree
http://hg.mozilla.org/mozilla-central/rev/4b8113a528f0

part2: get rid GetCachedParent
http://hg.mozilla.org/mozilla-central/rev/e006628cc089

landed on 2.0 beta 11
Attachment #507113 - Attachment description: part1v1: getParent shouldn't repair the tree → part1v1: getParent shouldn't repair the tree [landed 2011-01-29]
Attachment #507132 - Attachment description: part2v1: get rid GetCachedParent → part2v1: get rid GetCachedParent [landed 2011-01-29]
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#2894

2894 PRInt32
2895 nsAccessible::GetIndexInParent()
2896 {
2897   // XXX: call GetParent() to repair the tree if it's broken.
2898   GetParent();
2899   return mIndexInParent;
2900 }

Looks like this needs an update.
(In reply to comment #15)

> 2897   // XXX: call GetParent() to repair the tree if it's broken.
> 2898   GetParent();
> 2899   return mIndexInParent;
> 2900 }
> 
> Looks like this needs an update.

right, thank you!
Comment on attachment 508229 [details] [diff] [review]
patch3v1:  rudiment of GetIndexInParent fix

r=me.

>-nsAccessible::GetIndexInParent()
>+nsAccessible::GetIndexInParent() const
> {
>-  // XXX: call GetParent() to repair the tree if it's broken.
>-  GetParent();
>   return mIndexInParent;

Great.
Attachment #508229 - Flags: review?(bolterbugz) → review+
part3 is landed on 2.0 beta 11 - http://hg.mozilla.org/mozilla-central/rev/70b1c8dd220e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b11
You need to log in before you can comment on or make changes to this bug.