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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
major
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: Joanmarie Diggs, Assigned: surkov)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {access})

unspecified
x86
Linux
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: orca:urgent)

Attachments

(7 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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!!
(Reporter)

Updated

10 years ago
Whiteboard: orca:urgent
(Reporter)

Comment 1

10 years ago
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!!
(Reporter)

Comment 2

10 years ago
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?
(Assignee)

Comment 3

10 years ago
Joanie, can you should your accessible tree?
(Assignee)

Comment 4

10 years ago
patch3 of bug 400066 might be helpful for the bug, though let's look at accessible tree firstly.
(Assignee)

Comment 5

10 years ago
Created attachment 310929 [details] [diff] [review]
help?

Can you look does it help?
(Reporter)

Comment 6

10 years ago
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!!
(Reporter)

Comment 7

10 years ago
> 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

10 years ago
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?
(Reporter)

Comment 9

9 years ago
Friendly ping. :-)

Dead accessibles are a problem. Thanks!!
Evan, could your patch for bug 430758 have had any positive impact on this?

Anyone else have any thoughts?

Comment 11

9 years ago
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?
(Reporter)

Comment 12

9 years ago
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?
(Assignee)

Comment 13

9 years ago
I think we should deal with this after firefox3
Blocks: 391535
(Assignee)

Comment 14

9 years ago
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.
(Reporter)

Comment 15

9 years ago
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.
Summary: Poor markup kills children (of the accessible object variety) → "Dead" accessible objects should not be present in the hierarchy
(Reporter)

Comment 16

8 years ago
Looks like it's time for my semi-annual ping on this bug. :-)
Mass un-assigning bugs assigned to Aaron.
Assignee: aaronleventhal → nobody
(Reporter)

Comment 18

7 years ago
A year has passed since the last time I pinged this bug. This is still a problem. Thanks.
(Assignee)

Updated

7 years ago
Blocks: 540399
Severity: normal → major
(Reporter)

Comment 19

7 years ago
http://azui.com causes Orca + Firefox to hang.
(Reporter)

Comment 20

7 years ago
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. :-(
(Reporter)

Comment 21

7 years ago
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.
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.
(Assignee)

Comment 23

7 years ago
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.
Assignee: nobody → surkov.alexander
Attachment #310929 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #454764 - Flags: review?(ginn.chen)
Alex, did you push to try-server? I'd like to ask Joanie to take a look.
(Assignee)

Comment 25

7 years ago
(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.
(Assignee)

Comment 26

7 years ago
try-server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-9d54d8b83bb4
(Reporter)

Comment 27

7 years ago
(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!!
(Reporter)

Comment 28

7 years ago
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.
(Reporter)

Comment 29

7 years ago
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>
(Assignee)

Comment 30

7 years ago
Comment on attachment 454764 [details] [diff] [review]
patch

Ok, thanks Joanie, I think I have an idea.
Attachment #454764 - Flags: review?(ginn.chen)
(Assignee)

Comment 31

7 years ago
(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?
(Reporter)

Comment 32

7 years ago
(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.

Updated

7 years ago
Attachment #455065 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 33

7 years ago
(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?
(Assignee)

Comment 34

7 years ago
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.
Attachment #454764 - Attachment is obsolete: true
Did anyone try this with about:config html5.enable true? HTML5 pulls unexpected table children out and places them after the table element...
(Assignee)

Comment 36

7 years ago
(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?
(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.
(Assignee)

Comment 38

7 years ago
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.
(Assignee)

Comment 39

7 years ago
(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 :)
(Assignee)

Updated

7 years ago
Depends on: 576838
(Assignee)

Comment 40

7 years ago
Created attachment 455930 [details] [diff] [review]
patch
Attachment #455677 - Attachment is obsolete: true
Attachment #455930 - Flags: review?(bolterbugz)
(Assignee)

Comment 41

7 years ago
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 on attachment 455930 [details] [diff] [review]
patch

r=me
Attachment #455930 - Flags: review?(bolterbugz) → review+

Comment 43

7 years ago
(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.
(Assignee)

Comment 44

7 years ago
(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

7 years ago
(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);
}
(Assignee)

Comment 46

7 years ago
Needed an approval for 2.0 - major fix for Linux screen readers (like Orca).
blocking2.0: --- → ?
(Assignee)

Comment 47

7 years ago
Comment on attachment 455930 [details] [diff] [review]
patch

Needed an approval for 2.0 - major fix for Linux screen readers (like Orca). Trivial fix.
Attachment #455930 - Flags: approval2.0?

Updated

7 years ago
Attachment #455930 - Flags: approval2.0? → approval2.0+

Updated

7 years ago
blocking2.0: ? → beta4+
Note this patch depends on GetEmbeddedChildCount which will land as part of bug 576777.
Depends on: 576777
(Assignee)

Comment 49

7 years ago
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/3812ddb37a35
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 50

7 years ago
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.(??)
(Assignee)

Comment 51

7 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

7 years ago
blocking2.0: beta4+ → final+
(Assignee)

Comment 52

7 years ago
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.
Attachment #480586 - Flags: review?(bolterbugz)
(Assignee)

Comment 53

7 years ago
Can we get blocking 7+ status since we have a patch for this? It doesn't make sense to wait.
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.
Attachment #480586 - Flags: review?(bolterbugz) → review+
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."
(Assignee)

Comment 56

7 years ago
try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-f9e55a723f8c/
(Assignee)

Comment 57

7 years ago
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.
Attachment #480586 - Flags: approval2.0?
Comment on attachment 480586 [details] [diff] [review]
patch 2.0 (next issue)

Yep, approving this patch for beta7 landing (preferably as a ride along).
Attachment #480586 - Flags: approval2.0? → approval2.0+
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
(Assignee)

Comment 60

7 years ago
landed on Gecko 2.0 (beta 7) - http://hg.mozilla.org/mozilla-central/rev/52b06ea6b602
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.