420993 - "Dead" accessible objects should not be present in the hierarchy

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: test case

Step to reproduce:

1. View the attached test case in Accerciser

Expected results:  There'd be no "<dead>" accessibles in the hierarchy.

Actual results:  There is one "<dead>" accessible in the hierarchy.

The dead accessible (the middle child of the outermost table) seems to be the product of a combination of things:

1. the "<br>" floating in space immediately after the start of the 2nd of the inner tables

2. the styles present

If you get rid of the <br> or "display:block;" or "overflow:hidden;" the dead accessible goes away and the parent table's child count reflects 2 children rather than 3.

This test case is based on a real site that an Orca user is trying to access (I can't make markup this bad up. ;-) ).  In some cases I can hack around the dead accessible, but I'm not sure I can do that for the collections-related code.  I'll try.  But in the meantime, if a fix for this can be squeezed in for 3.0, that would be awesome!  (Blocking 396346 as a request) Thanks!!

Comment 1

[Joanmarie Diggs]

Okay, I'm relatively certain that I can hack around the problem for our collections-related code.  However, if there's a way to eliminate the dead accessible for 3.0 (and thus the need to hack at all), that would still be very much appreciated.

In the meantime, I have a question (being brand new to collections and all).  Using Accerciser's iPython console, if I use getMatchesFrom() to find the next 10 headings from the last accessible in the test case, what is returned is a list of four accessibles -- all headings, all of which follow the dead accessible.  If I modify the test case to eliminate the dead accessible and repeat the test, getMatchesFrom() returns an empty list.  Returning an empty list seems quite logical to me since, after all, there are no more headings (or any other objects for that matter) after the last object on the page. :-)

So here's the question:  Ignoring the dead accessible for a moment, whose bug is the problem of returning accessibles which should not be returned via getMatchesFrom()?  FF/Gecko or AT-SPI?  Or is it all the dead accessible?

Thanks again!!

Comment 2

[Joanmarie Diggs]

Looking at this some more.... Having to check children on the off chance there's a "dead" accessible will hurt performance-wise if an object has quite a few children.  Any chance we can get this fixed on the FF side of things?

Comment 3

[alexander :surkov (:asurkov)]

Joanie, can you should your accessible tree?

Comment 4

[alexander :surkov (:asurkov)]

patch3 of bug 400066 might be helpful for the bug, though let's look at accessible tree firstly.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: help?

Can you look does it help?

Comment 6

[Joanmarie Diggs]

Attachment: Accerciser screenshot

I've attached a screenshot of the hierarchy as seen in Accerciser.  Hopefully it will help some.  

(Alexander, I saw your comment on the other bug to which I attached screenshot.  I'll see what I can come up with to clarify things.  In the meantime, perhaps a system of 'I give you a screenshot of Accerciser, you tell me when you need more info' might work. :-) )

I will try your patch now.  Thanks!!

Comment 7

[Joanmarie Diggs]

> Can you look does it help?

Actually, unless I'm missing something, that makes the entire hierarchy inaccessible.  

1. Applied the patch, built, and installed.   The *only* thing I see in the hierarchy under the app is one dead accessible.

2. Reversed the patch, built, and installed.  Things are back to normal (not counting the dead accessible reported in this bug)

3. Re-applied the patch, built, and installed.  Again, the accessible hierarchy is broken.

Am I doing something wrong?

Comment 8

[Evan Yan]

The problem is, we didn't create accessible for the td.

nsAccessible tree is

-table
 (we didn't create table cell accessible here)
  -table
  -br  (the dead accessible in ATK)
  -table

table accessible didn't implement hyperText, so the accessible for br (a text leaf accessible) is shown "dead".

In fact, there is a general problem for html table.

If we have a td with display:block or something, so that table cell will have a different frame other than TableCellFrame. Then we will create some other accessible (or even no accessible) other tan table cell accessible. That will make GetCellAt() fail.

For example,

<table>
  <td style="display:block">cell0</td>
  <td>cell1</td>
</table>

tableAcc.getCellAt(0,0) returns null.

Why not we always create table cell accessible as long as it's under a table accessible?

Comment 9

[Joanmarie Diggs]

Friendly ping. :-)

Dead accessibles are a problem. Thanks!!

Comment 10

[Marco Zehe (:MarcoZ)]

Evan, could your patch for bug 430758 have had any positive impact on this?

Anyone else have any thoughts?

Comment 11

[Evan Yan]

No, they're different issues.

Joanie,
What's your opinion about comment #8? To always expose table cell accessible as long as it's inside a table accessible, no matter what style attribute the table cell could have?

Comment 12

[Joanmarie Diggs]

Evan, to be honest I'm not sure. :-)

On the one hand, given the new table-cell-index attribute which (pending testing) will be used by Orca, I *believe* what you suggest will be okay.  As long as the insertion doesn't impact the values returned for rows and columns.

On the other hand, that's not a table cell.  :-) It's just a line break floating in space.  Therefore, could it have some other role?  I've seen rows added in as ROLE_TEXT.  That way, we would be confused into thinking we had an actual cell.

Thoughts?

Comment 13

[alexander :surkov (:asurkov)]

I think we should deal with this after firefox3

Comment 14

[alexander :surkov (:asurkov)]

In general I think we should start from investigation of layout tree (possibly http://developer.mozilla.org/en/docs/Debugging_Frame_Reflow can help us) because I think accessible tree should be correspond with the rendered things rather than with DOM tree in the case of tables.

Comment 15

[Joanmarie Diggs]

Does "after firefox3" mean "after firefox3.0" or "for firefox4"? If the former, ping. :-)

Dead accessibles in the hierarchy cause Orca to "get stuck". If we have to question every single object on the off chance that it might be dead, we will see a performance hit.

Thanks.

Comment 16

[Joanmarie Diggs]

Looks like it's time for my semi-annual ping on this bug. :-)

Comment 17

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

Mass un-assigning bugs assigned to Aaron.

Comment 18

[Joanmarie Diggs]

A year has passed since the last time I pinged this bug. This is still a problem. Thanks.

Comment 19

[Joanmarie Diggs]

http://azui.com causes Orca + Firefox to hang.

Comment 20

[Joanmarie Diggs]

I begged for attention to this bug via Twitter. (Desperate times, desperate measures, yadda, yadda, yadda.) :-)

Marco responded very quickly (thanks!) with the following suggestion:

> A shot in the dark, but can you try the try-server build posted to
> https://bugzilla.mozilla.org/show_bug.cgi?id=573955 ?

I just tried it. Makes no difference. The dead accessibles persist. :-(

Comment 21

[Joanmarie Diggs]

Here's another site with a related dead accessible issue which can hang Orca: https://www.ncsecu.org/

Just beyond the 'user name' entry, there is an accessible which goes from 'dead' to having a name when viewed in Accerciser. But this zombie child only appears in 3.6.x; not 3.5.x.

Comment 22

[Marco Zehe (:MarcoZ)]

Hi Joanie, thanks for testing the try-server build I pointed you to! Since that try-server build also contains the patch for bug 574312, which deals with a correct accessible tree, I was hoping it might fix this bug as well.

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch

It's fast but not optimal for memory usage, though it shouldn't hit us more than on windows since web pages usually are text accessibles. So I think we can leave memory optimizations until next Firefox release.

Comment 24

[Marco Zehe (:MarcoZ)]

Alex, did you push to try-server? I'd like to ask Joanie to take a look.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> Alex, did you push to try-server? I'd like to ask Joanie to take a look.

Sure, though it might be not very handy for Jaonie since she on Solaris iirc and there's no try server build for this platform. I checked with accerciser and I don't see any dead nodes.

Comment 26

[alexander :surkov (:asurkov)]

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

Comment 27

[Joanmarie Diggs]

(In reply to comment #25)
> (In reply to comment #24)
> > Alex, did you push to try-server? I'd like to ask Joanie to take a look.
> 
> Sure, though it might be not very handy for Jaonie since she on Solaris iirc

You recall correctly.

Though I have various and sundry Linux installations for testing things. And I can confirm that I'm not seeing dead accessibles any longer. Yay! (And thanks!!) Something is still hanging Orca + Firefox in the vicinity of the formerly-dead nodes, however. :-( And once I was able to crash your try server build just by using Accerciser to examine the hierarchy of one of the offending sites. So something still isn't quite right....

I need a *stable*, linux dev environment to test further, so I'm wiping one of my laptops as I write this. I'll try to get to the bottom of the remaining problem.

Thanks for jumping on this one Alex!!

Comment 28

[Joanmarie Diggs]

Attachment: New test case

The attached test case demonstrates a variant of this bug and (maybe?) a problem with the fix. Or another, serious bug. Either way:

Prior to the proposed fix, if you view this new test case with Accerciser, within the table you'll see an accessible go from 'dead' to having a name ('bad checkbox'). It still doesn't have a role, and otherwise still appears to be 'dead'.

After the proposed fix, that dead accessible is no longer in the hierarchy. Yay! The thing is.... Try the following:

1. Launch Orca from a terminal window
2. Load the test case
3. Arrow from the first line to the second and wait
4. Arrow from the second line to the third and wait
5. Arrow from the third line to the fourth and wait

At this point you'll be on the 'bad' checkbox. And soon Orca and Firefox and more or less your entire Desktop will hang. :-(

6. Kill the Orca process.

In the terminal window you'll have a BUNCH of the following errors:

~~~~~~~~~~~~~~~~~~~~~~~~
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/pyatspi/registry.py", line 271, in notifyEvent
    ev = event.Event(ev)
  File "/usr/lib/python2.6/dist-packages/pyatspi/event.py", line 137, in __init__
    self.type = EventType(event.type)
  File "/usr/lib/python2.6/dist-packages/pyatspi/event.py", line 233, in __init__
    for i in xrange(len(split)):
RuntimeError: maximum recursion depth exceeded while calling a Python object
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/pyatspi/registry.py", line 271, in notifyEvent
    ev = event.Event(ev)
  File "/usr/lib/python2.6/dist-packages/pyatspi/event.py", line 137, in __init__
    self.type = EventType(event.type)
  File "/usr/lib/python2.6/dist-packages/pyatspi/event.py", line 233, in __init__
    for i in xrange(len(split)):
RuntimeError: maximum recursion depth exceeded while calling a Python object

~~~~~~~~~~~~~~~~~~~~~~~~

Given that the only difference between the first checkbox and second checkbox is that there used to be a dead accessible that you've since removed, I'm wondering if the problem/bug/loop is being triggered on the Gecko side of things. I suspect this for an additional reason which I'll attach next.

Comment 29

[Joanmarie Diggs]

Attachment: Accerciser event monitor output: Event flood

If you repeat the steps I described in my previous comment, but also launch Accerciser and turn event monitoring on to monitor for object:property-change:accessible-parent events for the current application, here's what you get when you arrow from the third line to the fourth (i.e. to the unsafe checkbox) with Orca running:

object:property-change:accessible-parent(0, 0, [table | ])
	source: [check box | ]
	application: [application | Minefield]

Only you don't get one event, or two. You get not quite 2,000 if I did my math right. So.... I don't know why Gecko is now flooding us with these events. And it doesn't seem to want to do that with the first checkbox.

<shrugs>

Comment 30

[alexander :surkov (:asurkov)]

Comment on attachment 454764 [details] [diff] [review]
patch

Ok, thanks Joanie, I think I have an idea.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #28)

> At this point you'll be on the 'bad' checkbox. And soon Orca and Firefox and
> more or less your entire Desktop will hang. :-(
> 
> 6. Kill the Orca process.

How to kill the Orca process if everything hangs?

Comment 32

[Joanmarie Diggs]

(In reply to comment #31)
> (In reply to comment #28)
> 
> > At this point you'll be on the 'bad' checkbox. And soon Orca and Firefox and
> > more or less your entire Desktop will hang. :-(
> > 
> > 6. Kill the Orca process.
> 
> How to kill the Orca process if everything hangs?

In Linux you can get into a virtual console with Ctrl+Alt+F{1,2,3,4,...}. Then 

  ps -elf | grep orca

(We really need to make the process name be Orca, but anyhoo....)

Then kill -9 pidof <orca process>

Then you can Ctrl+Alt+F7 back into your desktop environment.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)

>   ps -elf | grep orca
> 
> (We really need to make the process name be Orca, but anyhoo....)

How can I get orca process number from these scrolled off several pages of output?

Comment 34

[alexander :surkov (:asurkov)]

Attachment: wip

Orca still hangs on 'new test case', no crashed for me any more but still not have any idea what it can hang: firefox mai debug output isn't helpful.

Comment 35

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

Did anyone try this with about:config html5.enable true? HTML5 pulls unexpected table children out and places them after the table element...

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> Did anyone try this with about:config html5.enable true? HTML5 pulls unexpected
> table children out and places them after the table element...

How does it help to fix the bug?

Comment 37

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

(In reply to comment #36)
> (In reply to comment #35)
> > Did anyone try this with about:config html5.enable true? HTML5 pulls unexpected
> > table children out and places them after the table element...
> 
> How does it help to fix the bug?

Just catching up on this bug. It seemed that the examples involved tables... updating my linux FF source now.

Comment 38

[alexander :surkov (:asurkov)]

Ginn, could you give me a hint when atk object is addrefed and released. I see it's released in nsAccessibleWrap::Shutdown. I would assume it should be addrefed in nsAccessibleWrap::GetNativeInterface, I see there atk_object_initialize, probably it makes addref but I'm not sure. Ok, I assume it's addrefed in GetNativeInterface when atk object is created and released in Shutdown() when gecko accessible is about to destroy.

But I don't completely understand ATK function implementation then. They sometimes addref atk objects, somethimes they don't.

For example,
refChildCB addrefs returned atk object
getParentCB doesn't addref if parent was intialized

I assume all function should act by common pattern, so either they should return addrefed object or not. Therefore refChildCB and getParentCB implementation confuses me.

Another issue refChildCB calls atk_object_set_parent for obtained child but if getParentCB has been called early then it doesn't make sense.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)

> I assume all function should act by common pattern, so either they should
> return addrefed object or not. Therefore refChildCB and getParentCB
> implementation confuses me.

Probably not. Does "ref" in the name means it should addrefed, "get" means it shouldn't? At least I see webkit makes the same if they weren't inspired by our code :)

Comment 40

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 41

[alexander :surkov (:asurkov)]

The patch get rids dead accessibles from the tree. That's this bug is about. I filed bug 576838 to get rid Orca hang.

Comment 42

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

Comment on attachment 455930 [details] [diff] [review]
patch

r=me

Comment 43

[Ginn Chen]

(In reply to comment #39)

> Probably not. Does "ref" in the name means it should addrefed, "get" means it
> shouldn't? At least I see webkit makes the same if they weren't inspired by our
> code :)

Correct.

The comment should be more clear.
atk_object_set_parent() will always unref the old parent and then ref the new parent.

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)

> The comment should be more clear.
> atk_object_set_parent() will always unref the old parent and then ref the new
> parent.

It looks like atk_object_set_parent() sends object:property-change:accessible-parent event, and thus that might be a cause of excess addref/release calls and events. Does it make sense to check at this point if we have ATK parent already, right?

Comment 45

[Ginn Chen]

(In reply to comment #44)

> It looks like atk_object_set_parent() sends
> object:property-change:accessible-parent event, and thus that might be a cause
> of excess addref/release calls and events. Does it make sense to check at this
> point if we have ATK parent already, right?

Yes, we should do the same as we did in getParentCB().
if (!childAtkObj->accessible_parent) {
   atk_object_set_parent(childAtkObj, aAtkObj);
}

Comment 46

[alexander :surkov (:asurkov)]

Needed an approval for 2.0 - major fix for Linux screen readers (like Orca).

Comment 47

[alexander :surkov (:asurkov)]

Comment on attachment 455930 [details] [diff] [review]
patch

Needed an approval for 2.0 - major fix for Linux screen readers (like Orca). Trivial fix.

Comment 48

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

Note this patch depends on GetEmbeddedChildCount which will land as part of bug 576777.

Comment 49

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/3812ddb37a35

Comment 50

[Joanmarie Diggs]

Attachment: Screenshot showing the dead accessible still present in the hierarchy

Mozilla/5.0 (X11; Linux i686; rv:2.0b5pre) Gecko/20100820 Minefield/4.0b5pre

Still seeing the dead accessible. :-( And Orca still cannot navigate around it. From the comments, I am gathering this should not be the case.(??)

Comment 51

[alexander :surkov (:asurkov)]

(In reply to comment #50)

> Still seeing the dead accessible. :-( And Orca still cannot navigate around it.
> From the comments, I am gathering this should not be the case.(??)

You're right they shouldn't be here. I'll look. Reopen for now.

Comment 52

[alexander :surkov (:asurkov)]

Attachment: patch 2.0 (next issue)

ATK object is created if IsEmbeddedObject() returns true, the same time AppendChild/InsertChild() use IsText() to detect if there are embedded or text obejcts. IsText and IsEmbeddedObejct() aren't opposite, they are different in ROLE_WHITESPACE. We encountered this case in this example. I don't think whitespaces should be considered as embedded objects (it affects on nsIHyperTextAccessible interface as well where whitespace accessible is not link any more and that's right I think). So change IsText to !IsEmbeddedObject.

Comment 53

[alexander :surkov (:asurkov)]

Can we get blocking 7+ status since we have a patch for this? It doesn't make sense to wait.

Comment 54

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

Comment on attachment 480586 [details] [diff] [review]
patch 2.0 (next issue)

r=me.

So this essentially changes an IsText check to a check that the role is one of:
ROLE_TEXT_LEAF
ROLE_WHITESPACE
ROLE_STATICTEXT

Makes sense to me.

Comment 55

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

Comment on attachment 480586 [details] [diff] [review]
patch 2.0 (next issue)

Alexander, we need to explain why this patch should land. Can you post a comment here when you get a full try server run? Something like "Note to beta7 drivers. This patch is low risk and make the Linux screen reader stop hanging randomly. Etc."

Comment 56

[alexander :surkov (:asurkov)]

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

Comment 57

[alexander :surkov (:asurkov)]

Comment on attachment 480586 [details] [diff] [review]
patch 2.0 (next issue)

can we get an approval for beta7? the patch is safe, tests are passed, the bug has big impact on Orca screen reader making it hang while the web browsing.

Comment 58

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

Comment on attachment 480586 [details] [diff] [review]
patch 2.0 (next issue)

Yep, approving this patch for beta7 landing (preferably as a ride along).

Comment 59

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

Comment on attachment 480586 [details] [diff] [review]
patch 2.0 (next issue)

If anyone lands this please remove from: https://wiki.mozilla.org/LandingQueue#Ride-along_patches

Comment 60

[alexander :surkov (:asurkov)]

landed on Gecko 2.0 (beta 7) - http://hg.mozilla.org/mozilla-central/rev/52b06ea6b602