626667 - deal with cached accessible tree only

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

This is a good idea but are you thinking this needs to make it into FF4?

Comment 2

[alexander :surkov (:asurkov)]

(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 3

[alexander :surkov (:asurkov)]

Attachment: part1v1: getParent shouldn't repair the tree [landed 2011-01-29]

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 507113 [details] [diff] [review]
part1v1: getParent shouldn't repair the tree [landed 2011-01-29]

This sort of blows my mind.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: part2v1: get rid GetCachedParent [landed 2011-01-29]

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

We'll need QA of course.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Created attachment 507113 [details] [diff] [review]
> part1v1: getParent shouldn't repair the tree

try server build http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-0a433462a83f

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

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; }

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 507132 [details] [diff] [review]
part2v1: get rid GetCachedParent [landed 2011-01-29]

r=me pending QA

Comment 10

[Marco Zehe (:MarcoZ)]

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.

Comment 11

[alexander :surkov (:asurkov)]

(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

Comment 12

[Marco Zehe (:MarcoZ)]

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.

Comment 13

[alexander :surkov (:asurkov)]

ok, it's queued for landing

Comment 14

[alexander :surkov (:asurkov)]

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

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 16

[alexander :surkov (:asurkov)]

(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 17

[alexander :surkov (:asurkov)]

Attachment: patch3v1: rudiment of GetIndexInParent fix

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 19

[alexander :surkov (:asurkov)]

part3 is landed on 2.0 beta 11 - http://hg.mozilla.org/mozilla-central/rev/70b1c8dd220e