Last Comment Bug 420993 - "Dead" accessible objects should not be present in the hierarchy
: "Dead" accessible objects should not be present in the hierarchy
Status: RESOLVED FIXED
orca:urgent
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Linux
: -- major (vote)
: ---
Assigned To: alexander :surkov
:
Mentors:
Depends on: 576838 576777
Blocks: orca treea11y 191a11y fox3access
  Show dependency treegraph
 
Reported: 2008-03-04 17:40 PST by Joanmarie Diggs
Modified: 2014-04-26 02:21 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+


Attachments
test case (431 bytes, text/html)
2008-03-04 17:40 PST, Joanmarie Diggs
no flags Details
help? (3.08 KB, patch)
2008-03-20 22:24 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
Accerciser screenshot (103.64 KB, image/png)
2008-03-20 22:40 PDT, Joanmarie Diggs
no flags Details
patch (8.16 KB, patch)
2010-06-28 21:39 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
New test case (352 bytes, text/html)
2010-06-29 21:24 PDT, Joanmarie Diggs
no flags Details
Accerciser event monitor output: Event flood (235.19 KB, text/plain)
2010-06-29 21:28 PDT, Joanmarie Diggs
no flags Details
wip (13.93 KB, patch)
2010-07-02 07:00 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (3.31 KB, patch)
2010-07-04 01:05 PDT, alexander :surkov
dbolter: review+
dbolter: approval2.0+
Details | Diff | Splinter Review
Screenshot showing the dead accessible still present in the hierarchy (264.10 KB, image/png)
2010-08-20 13:17 PDT, Joanmarie Diggs
no flags Details
patch 2.0 (next issue) (640 bytes, patch)
2010-10-04 02:37 PDT, alexander :surkov
dbolter: review+
dbolter: approval2.0+
Details | Diff | Splinter Review

Description Joanmarie Diggs 2008-03-04 17:40:56 PST
Created attachment 307369 [details]
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 2008-03-04 22:03:28 PST
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 2008-03-20 15:00:01 PDT
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 2008-03-20 21:54:58 PDT
Joanie, can you should your accessible tree?
Comment 4 alexander :surkov 2008-03-20 22:13:05 PDT
patch3 of bug 400066 might be helpful for the bug, though let's look at accessible tree firstly.
Comment 5 alexander :surkov 2008-03-20 22:24:02 PDT
Created attachment 310929 [details] [diff] [review]
help?

Can you look does it help?
Comment 6 Joanmarie Diggs 2008-03-20 22:40:17 PDT
Created attachment 310932 [details]
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 2008-03-20 23:21:42 PDT
> 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 2008-03-24 03:45:25 PDT
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 2008-05-10 19:44:35 PDT
Friendly ping. :-)

Dead accessibles are a problem. Thanks!!
Comment 10 Marco Zehe (:MarcoZ) 2008-05-12 00:55:29 PDT
Evan, could your patch for bug 430758 have had any positive impact on this?

Anyone else have any thoughts?
Comment 11 Evan Yan 2008-05-12 05:42:35 PDT
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 2008-05-12 06:56:56 PDT
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 2008-05-13 19:07:51 PDT
I think we should deal with this after firefox3
Comment 14 alexander :surkov 2008-05-13 20:13:15 PDT
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 2008-11-05 12:30:57 PST
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 2009-05-21 14:01:57 PDT
Looks like it's time for my semi-annual ping on this bug. :-)
Comment 17 David Bolter [:davidb] 2009-06-16 11:58:51 PDT
Mass un-assigning bugs assigned to Aaron.
Comment 18 Joanmarie Diggs 2010-06-06 14:28:02 PDT
A year has passed since the last time I pinged this bug. This is still a problem. Thanks.
Comment 19 Joanmarie Diggs 2010-06-07 05:38:55 PDT
http://azui.com causes Orca + Firefox to hang.
Comment 20 Joanmarie Diggs 2010-06-27 14:07:28 PDT
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 2010-06-27 15:32:17 PDT
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) 2010-06-27 23:42:54 PDT
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 2010-06-28 21:39:11 PDT
Created attachment 454764 [details] [diff] [review]
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) 2010-06-28 23:13:50 PDT
Alex, did you push to try-server? I'd like to ask Joanie to take a look.
Comment 25 alexander :surkov 2010-06-28 23:38:08 PDT
(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 27 Joanmarie Diggs 2010-06-29 15:31:04 PDT
(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 2010-06-29 21:24:00 PDT
Created attachment 455064 [details]
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 2010-06-29 21:28:54 PDT
Created attachment 455065 [details]
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 2010-06-29 22:20:47 PDT
Comment on attachment 454764 [details] [diff] [review]
patch

Ok, thanks Joanie, I think I have an idea.
Comment 31 alexander :surkov 2010-06-30 07:18:22 PDT
(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 2010-06-30 07:24:17 PDT
(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 2010-07-02 05:06:26 PDT
(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 2010-07-02 07:00:59 PDT
Created attachment 455677 [details] [diff] [review]
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] 2010-07-02 07:18:48 PDT
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 2010-07-02 07:22:43 PDT
(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] 2010-07-02 07:28:19 PDT
(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 2010-07-02 08:31:18 PDT
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 2010-07-02 08:49:41 PDT
(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 2010-07-04 01:05:06 PDT
Created attachment 455930 [details] [diff] [review]
patch
Comment 41 alexander :surkov 2010-07-04 01:06:15 PDT
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] 2010-07-05 06:29:40 PDT
Comment on attachment 455930 [details] [diff] [review]
patch

r=me
Comment 43 Ginn Chen 2010-07-05 20:09:08 PDT
(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 2010-07-05 20:19:44 PDT
(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 2010-07-06 23:19:21 PDT
(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 2010-07-20 21:35:10 PDT
Needed an approval for 2.0 - major fix for Linux screen readers (like Orca).
Comment 47 alexander :surkov 2010-07-27 01:53:16 PDT
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] 2010-08-09 07:45:37 PDT
Note this patch depends on GetEmbeddedChildCount which will land as part of bug 576777.
Comment 49 alexander :surkov 2010-08-15 04:32:00 PDT
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/3812ddb37a35
Comment 50 Joanmarie Diggs 2010-08-20 13:17:54 PDT
Created attachment 467867 [details]
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 2010-08-20 19:28:31 PDT
(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 2010-10-04 02:37:57 PDT
Created attachment 480586 [details] [diff] [review]
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 2010-10-04 02:39:29 PDT
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] 2010-10-04 06:06:26 PDT
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] 2010-10-04 06:16:37 PDT
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 57 alexander :surkov 2010-10-06 02:06:54 PDT
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] 2010-10-06 06:12:56 PDT
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] 2010-10-06 06:24:04 PDT
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 2010-10-06 06:50:48 PDT
landed on Gecko 2.0 (beta 7) - http://hg.mozilla.org/mozilla-central/rev/52b06ea6b602

Note You need to log in before you can comment on or make changes to this bug.