424656 - Accessible information for Thunderbird message and Firefox bookmarks tree tables wrong

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

Originally reported to me by Mike Pedersen on IRC: On Linux, tree table information wrong. Column information mixes from different messages, some tree rows appear empty. This is after bug 421922 has been fixed and accessibles have been restored. The whole problem started with the 2008030603 build of Thunderbird on Linux.

Windows is unaffected.

Comment 1

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> Column information mixes from different messages, some tree
> rows appear empty.

Sorry, could you describe this in details?

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to comment #1)
> (In reply to comment #0)
> > Column information mixes from different messages, some tree
> > rows appear empty.
> Sorry, could you describe this in details?

For example if you have 10 messages, on the first row, subject column may be filled with information from message1, but "From", or "Date" come from different messages like message2 or message4.

Mike, you've had several examples, could you share them here?

Comment 3

[Mike Schroepfer]

We should nom this for blocking tbird3...

Comment 4

[Mike Schroepfer]

Er - missed the FF part - is there a good testcase of this?

Comment 5

[Mike Pedersen]

Attachment: expected orca output when arrowing down the list of messages

This output was created using a thunderbird from a couple weeks ago.  Specifically march 4th.

Comment 6

[Mike Pedersen]

Attachment: incorrect output

Output from the same thunderbird message list using todays thunderbird.  Notice that information is mixed from multiple rows.

Comment 7

[Marco Zehe (:MarcoZ)]

From a discussion on IRC, it became evident that we're somehow treating the column picker column as a column too and that this may throw us off.
Furthermore, I am able to reproduce the problem on Linux with Orca 100% of the time.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 9

[Evan Yan]

Marco, could you help test the patch? I can't reproduce the bug.

Comment 10

[Marco Zehe (:MarcoZ)]

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

This patch causes a crash in Thunderbird on Linux as soon as the tree gets visible (e. g. after the "Default client" query that comes up on my system). Without the patch, TB starts fine.

Comment 11

[Marco Zehe (:MarcoZ)]

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

>    // The last child could be column picker. In that case, we need to minus the
>    // number of columns by 1
   > nsCOMPtr<nsIAccessible> lastChildAccessible;
   > acc->GetLastChild(getter_AddRefs(lastChildAccessible));

> +  if (acc & Role(acc) != nsIAccessibleRole::ROLE_COLUMNHEADER)
     (*aColumns)--;

Did you mean to write:
   acc->GetLastChild(getter_AddRefs(lastChildAccessible));

+  if (lastChildAccessible & Role(lastChildAccessible) != nsIAccessibleRole::ROLE_COLUMNHEADER)
     (*aColumns)--;

instead?
Did you want to query the role for the last child or the first one?

Comment 12

[Marco Zehe (:MarcoZ)]

Attachment: Patch that delays my crash a bit

This patch delays the crash I'm seeing until one of the trees (folders or messages) actually gets focus. Anyone got any idea on this?

Comment 13

[Marco Zehe (:MarcoZ)]

More info:
I crash at this line:
http://mxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessible.cpp#744

In addition, in the output window, I get lots of messages like these:

###!!! ASSERTION: nsLoadGroup not thread safe: '_mOwningThread.GetThread() PR_GetCurrentThread(), file http://mxr.mozilla.org/seamonkey/source/netwerk/base/src/nsLoadGroup.cpp#204

###!!! nsChromeTreeOwner not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file http://mxr.mozilla.org/seamonkey/source/xpfe/appshell/src/nsChromeTreeOwner.cpp#127

###!!! ASSERTION: nsWeakReference not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file http://mxr.mozilla.org/seamonkey/source/xpcom/glue/nsWeakReference.cpp#146

Comment 14

[Marco Zehe (:MarcoZ)]

Strike the asserts, they are the result of nsImapProtocol doing some stuff it probably should not do.

Comment 15

[Marco Zehe (:MarcoZ)]

Also tried both patches in Firefox, and as soon as I open Organize Bookmarks..., it crashes, but with a float operation exception, not with a memory fault.

Comment 16

[Marco Zehe (:MarcoZ)]

Well I managed to build with the original patch Surkov posted.  And it is definitely not good. :-(

Here is the hierarchy from a good build of Firefox (March 5, 2008):

Tree Table
- List with no name
-- Column header "Name"
-- Column Header "Tags"
-- column header "Location"
-- Menu with no name
- Cell1 row 1
- Cell2 row 1
- Cell3 row 1
- Cell1 row 2
- cell2 row 2
- cell3 row 2
- cell1 row 3
- cell2 row 3
- cell3 row 3

With a current nightly, the hierarchy looks like this:
Tree table
- List with no name
-- Column header "Name"
-- Column header "Tags"
-- Column header "Location"
-- Accessible with unknown role and no name
--- menu with no name and no children
- Cell1 row 1
- Cell2 row 1
- Cell3 row 1
- Cell1 row 2
- Cell2 row 2
- Cell3 row 2
- Cell1 row 3
- Cell2 row 3
- Cell3 row 3
- Cell? row ?
- cell? row ?
- cell? row ?
- cell? row ?

These last cells are not associated with anything: The tree table only displays 3 rows and 3 columns!

With the patch, the accessible hierarchy is totally different/broken:

Tree table
- List with no name and no children
- cell1 row 1 (or whichever may currently have focus)

So without the patch, what changed is this:
1. Instead of the menu being the fourth item in the list, an unknown accessible that contains the menu is there.
2. There are extra cells added that don't contain any data.

Comment 17

[Marco Zehe (:MarcoZ)]

OK, after a morning of investigation, I found out the following:
1. The override of getChildCount in nsXULTreeAccessibleWrap.cpp for ATK is bogus. It assumes that the column headers are added right below the tree table accessible.  However, this is not true, since we put our column headers inside of a list accessible.  In other words, the child count is (rowCount * colCount) + "ListContainingHeaders".  So in a tree table consisting of 3 rows and 3 columns, the child value is 10.  In such a list, I get a value of 14 returned, which adds the 3 column headers and the column picker.

As a result: getRowAtIndex, getColumnAtIndex are all being thrown into chaos because they operate with bogus values. Also, Accerciser tries to get to children that aren't there.

2. the getColumns function operates under the false assumption that the column picker is still a menu. However, that is not the case, it is sort of an unknown accessible type.  For this, Alexander's patch was almost correct.

WIP patch coming up that addresses these issues.

Comment 18

[Marco Zehe (:MarcoZ)]

More notes: The failure to get any childCount happens in the call to GetColumns that Alexander's original patch adds. If I replace the line with the original "mFirstChild->GetChildCount(&colCount)" call, stuff suddenly starts working.

Comment 19

[Marco Zehe (:MarcoZ)]

Attachment: WIP/Proof of concept

This patch has almost all of Surkov's changes, except where noted/commented earlier.
Also, it has fixes for the algoritms used in getIndexAt, GetColumnAtIndex and GetRowAtIndex.

This patch gives me the right number of children, makes Orca read right, and also makes lookups and reverse lookups look correct with Accerciser/Orca.

Question: Why is GetColumns failing when called from within GetChildCount?  It fails at the line

rv = acc->GetChildCount(aColumns);
the return value rv is not NS_OK (found that out by letting it thrown an assertion there).

Evan, Alexander, any comments/ideas?

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 21

[Evan Yan]

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

I'd like to have Marco to help test the patch.

Comment 22

[Marco Zehe (:MarcoZ)]

(In reply to comment #20)
> Created an attachment (id=313923) [details]
> patch

This patch is almost good!  The only thing that fails is getRowAtIndex if executed on a non-column index, in my example, index 0. In that case, -1 should be returned, but 0 is being returned, because -1 / column number rounds to 0 rather than -1 in some cases.

Suggestion: Write:
  NS_ENSURE_ARG_POINTER(aRow);
  *aRow = -1;

  PRInt32 indexCorrelationNumber = GetIndexCorrelationNumber();
  if (aIndex < indexCorrelationNumber) // Index pointing to child before first cell accessible
    return NS_OK;

  PRInt32 columns;
  nsresult rv = GetColumns(columns);
  NS_ENSURE_SUCCESS(rv, rv);

  *aRow = (aIndex - indexCorrelationNumber) / columns;

Comment 23

[Marco Zehe (:MarcoZ)]

This still does not work in Thunderbird where the tree gets updated while messages are being downloaded and added. While it worked fine in the Firefox Organize Bookmarks dialog, it still fails with Thunderbird. The number of children added is not right/being updated as rows/cells get added. Maybe a caching problem? Surkov, did you run the Mochitests on the build with this patch in?

Comment 24

[Marco Zehe (:MarcoZ)]

More info: It mostly seems to be a problem when loading huge amounts of data for the first time, e. g. when downloading messages from an IMAP folder. I had 400 or so messages in the Inbox folder when i started Thunderbird with this patch in for the first time. It would take some time to download the messages, and that threw off the child count and subsequent cell accessible creation.
Ona second start, when most of this was already in the local cache, things worked much better.

Alexander, we should take a look where the cache might get out of sync when adding/removing cells as a result of downloading messages. At some point we're still working with old cached child counts and other such properties.

Comment 25

[Marco Zehe (:MarcoZ)]

This is getting more strange. I closed down the build and restarted it, and then everything looked fine. I also tried downloading newsgroup headers, and switching focus to the message tree while the download was still in progress, but right now, it's not going bad at all any more.

Alexander, can your test for bug 368835/bug 308564 be adapted to add a random number of rows in 5 or 10 runs? e. g. make a loop that runs ten times and everytime choose a different number of rows to add, and check the events to make sure they are firing, and stuff is being updated properly.

Comment 26

[Evan Yan]

Attachment: patch

Marco, could you try this patch?

The regression is caused by bug 415704. nsXULElement::IsFocusable()'s behavior was changed since that bug, so that we created additional accessibles.

In http://mxr.mozilla.org/seamonkey/source/content/base/public/nsIContent.h
The document of nsIContent::IsFocusable() says
"
most callers should use nsIFrame::IsFocusable() instead as it checks visibility and other layout factors as well.
"

So the fix of bug 421922 just worked around the problem. I reversed that change in this patch.

Comment 27

[Marco Zehe (:MarcoZ)]

Evan, this does not make any difference in regards to the wrong accessible problem. Bug 421922 was to fix the fact that no accessibles were created at all, but this problem is to address the totally wrong accessible info Mike's output is showing.
So your patch does prove that there is a different fix for bug 421922, but the original problem, the fact that there are now totally wrong accessibles being created, is still valid.

The problem is that in the column count, the column picker is being seen as a column. There is a specific check for role_menu in the original code that is no longer valid. The accessible for the column picker has changed to an "unknown" accessible. As a result, a wrong column count is generated, leading to all sorts of errors: Wrong child count, wrong RowAtIndex/ColumnAtIndex results, etc. The only true value I currently get is the number of rows. Everything else is broken, also with your patch.

Comment 28

[Marco Zehe (:MarcoZ)]

I am now going to try a build that has both Surkov's patch plus my nit, and Evan's patches.

Comment 29

[Evan Yan]

Marco, good catch!

When the column picker accessible is first introduced, I fixed the problem of nsXULTreeAccessibleWrap::GetColumns() in bug 387710. I should have fixed GetChildCount() as well. With fix to GetChildCount(), everything should be ok.

I'll post a new patch tomorrow when I'm at office.

Comment 30

[Marco Zehe (:MarcoZ)]

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

This patch restores the Windows a11y hierarchy for xul:tretables. Please resubmit in bug 421922 to replace the patch that is currently checked in for it.

Comment 31

[Marco Zehe (:MarcoZ)]

(In reply to comment #29)
> When the column picker accessible is first introduced, I fixed the problem of
> nsXULTreeAccessibleWrap::GetColumns() in bug 387710. I should have fixed
> GetChildCount() as well. With fix to GetChildCount(), everything should be ok.

Evan, please make sure that GetIndexInParent, executed from a cell accessible, and the reverse lookup of getRowAtIndex and getColumnAtIndex return the correct result for row and column numbers. 
Say I have a tree table of 3 columns and 3 rows. The expected child count is 10 (3 columns x 3 rows + 1 for the child accessible containing the list of column headers). 
In Accerciser's IPython console, I use the following when focused on column 1 of the second row:

in[1]: print acc.getIndexInParent()
out[1]: 4 // 0 is the list, 1 through 3 are first row
In[2]: parent = acc.parent
In[3]: table = parent.queryTable()
In[4): print table.getColumnAtIndex(4)
Out[4]: 0 // First of 3 columns
In[5]: print table.getRowAtIndex(4)
Out[5]: 1 // Second row.
In[6]: print table.getColumnAtIndex(0) // ask for index of the list
Out[6]: -1 // not a cell
In[7]: print table.getRowAtIndex(0)
Out[7]: -1 // not a table cell

Please make sure that this is what comes out, or stuff will still be messed up.

Comment 32

[Marco Zehe (:MarcoZ)]

The loss of tree info happened to me again while playing with my new build.  Here are the symptoms:
1. The child count went to 1.
2. The column count was correct.
3. The row count was 0, meaning it somehow lost contact with the tree object.
Really looks like a caching problem.

Comment 33

[Evan Yan]

Attachment: patch v2

This patch needs to be applied with the patch for bug 421922 together.

I used Surkov's fix of GetColumns() instead of checking treecolpicker by tag.

Comment 34

[alexander :surkov (:asurkov)]

Evan, is your patch+patchv2 equivalent with my patch?

Possibly frame->IsFocusable() is more correct than content->IsFocusable(), though I would like to know the difference :) But I think it's worth to override CacheChildren() for tree accessible and we shouldn't call base class's NextSibling on treecolumns because if we will find some accessible then we won't expose tree items/cells.

Another point: it's possible when there is no visible treecolumns (headers) but there are tree cells then should we expose treecolumns as accessible or it should have empty children set?

Comment 35

[Marco Zehe (:MarcoZ)]

Comment on attachment 314252 [details] [diff] [review]
patch v2

> +  if (aIndex > treeCols) {

Must be
+  if (aIndex >= treeCols) {

in both nsXULTreeAccessibleWrap::GetColumnAtIndex and nsXULTreeAccessibleWrap::GetRowAtIndex. The index is 0-based, so only indices that are actually less than the count are to be minus-one'd. Otherwise, getting the row or column number for the first column fails.

Comment 36

[Marco Zehe (:MarcoZ)]

OK I've now run with Evan's patch from attachment 314252 [details] [diff] [review] plus my correction, and the first hunk of attachment 314250 [details] [diff] [review]. In Firefox, everything's fine now. In Thunderbird, everything is fine with the first tree, which is my Inbox. If I then:
1. Shift+Tab to the folder tree,
2. DownArrow to Drafts, which has 2 messages (Inbox has over 400),
3. Tab forward to the message tree table,

the tree table goes bad.

It seems that I can now also reproduce that reliably.

Do you want a separate bug for this that we deal with once this and bug 421922 are checked in?

Comment 37

[Marco Zehe (:MarcoZ)]

More info: Also applying the second hunk of the latest patch to bug 421922 doesn't change the situation.  In Firefox, navigating the "Library" dialog always gives me consistent results, but with Thunderbird, as soon as I change folders, I now get a broken tree.
Any way to debug this?

Comment 38

[Marco Zehe (:MarcoZ)]

OK, in a debug build, once the failure happens, I get a ton of these outputs:
WARNING: atk_table_ref_at: assertion `row >= 0' failed: 'glib warning', file nsSigHandlers.cpp, line 197

** (thunderbird-bin:4263): CRITICAL **: atk_table_ref_at: assertion `row >= 0' failed

Evan, any idea what this exactly means?

Comment 39

[Evan Yan]

(In reply to comment #34)
> Evan, is your patch+patchv2 equivalent with my patch?
> 
> Possibly frame->IsFocusable() is more correct than content->IsFocusable(),
> though I would like to know the difference :)

The document says it checks visibility and other layout factors as well. I just follow the document's suggest.
Here are the code for the different implementations:
http://mxr.mozilla.org/seamonkey/source/content/xml/content/src/nsXMLElement.cpp#197
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#5727

> But I think it's worth to
> override CacheChildren() for tree accessible and we shouldn't call base class's
> NextSibling on treecolumns because if we will find some accessible then we
> won't expose tree items/cells.
> 
We shouldn't create accessibles for treerows and treechildren. So I would prefer not override CacheChildren(). It's not necessary and might hide issue in some case.

> Another point: it's possible when there is no visible treecolumns (headers) but
> there are tree cells then should we expose treecolumns as accessible or it
> should have empty children set?
> 
Good point.
If there is no treecols at all, I think we don't create treecolumns accessible. If there is treecols, I don't know whether we can make all column invisible. If there is really such a case, I would prefer having a treecolumns accessible with empty children set.

Comment 40

[Evan Yan]

(In reply to comment #35)
> (From update of attachment 314252 [details] [diff] [review])
> > +  if (aIndex > treeCols) {
> 
> Must be
> +  if (aIndex >= treeCols) {
> 
> in both nsXULTreeAccessibleWrap::GetColumnAtIndex and
> nsXULTreeAccessibleWrap::GetRowAtIndex. The index is 0-based, so only indices
> that are actually less than the count are to be minus-one'd. Otherwise, getting
> the row or column number for the first column fails.
> 
Thanks for the catch!

Comment 41

[Evan Yan]

Marco,
The Thunderbird changing folder issue sounds like some other issue related with "TreeViewChanged" event handling. That reminded me of bug 419474, which is still not reproducible on my box.

The warning message is print from atktable.c:atk_table_ref_at(). It seems the caller passed in some negative row index. I'm not sure how that happened.

Comment 42

[Marco Zehe (:MarcoZ)]

I found out that Firefox, in the "Library" dialog, only fires two TreeViewChanged events: One for each tree. The updates to the tree table happen without switching views.
In Thunderbird, on the other hand, a new view is being switched to whenever the folder is changed.
Those TreeViewChanged events make it go bad, if fired on a tree table object more than once.

I will file a separate bug for this.

In the meantime, get this patch r+'d and checked in ASAP, please, at 23:59 PDT/08:00 UTC is deadline for RC1 freeze.
This patch will allow the Library dialog to function properly, we can concentrate on the TreeViewChanged bug separately.

Comment 43

[alexander :surkov (:asurkov)]

Comment on attachment 314252 [details] [diff] [review]
patch v2

r=me since the bug fixes some problems, I'll file the new bug to make things clearer.

Comment 44

[Evan Yan]

Checking in nsXULTreeAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsXULTreeAccessibleWrap.cpp,v  <--  nsXULTreeAccessibleWrap.cpp
new revision: 1.25; previous revision: 1.24
done