427841 - TreeViewChanged event fired on a particular tree table more than once breaks that table's AT-SPI hierarchy.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

When TreeViewChangedEvent is fired on a particular tree table more than once, its accessibility hierarchy on Linux ATK/AT-SPI most likely breaks. There are very few cases where things go well, but in 95% of the time, it breaks.

This currently primarily affects Thunderbird, where the tree view is switched on the messages list with every folder change, but could also potentially be exposed by any extension in any Mozilla product that uses a tree view and changes views as a response to user action.

To reproduce:
0. Wait for the checkin to bug 424656, which fixes the accessible hierarchy, or build TB yourself.
1. Then, launchh Thunderbird with that patch in.
2. Focus the Inbox that usually comes up.

Expected and actual result: Message view is OK.

3. Shift+TAB to the Folder tree and change folders using the ARROW keys.
4. Press TAB to go back into the messages list.

Expected: Messages should be navigable and information should be present.
Actual result: In 95% of the time, the tree table is screwed up: The rowCount is 0, there is no accessible information for any tree row/cell, the only thing that is accurate is the count of visible columns.

Comment 1

[Damon Sicore (:damons)]

Sounds like we could take this in a dot release.  Also, could use test cases here.

re-nom if you strongly believe we would stop ship for this.

Comment 2

[alexander :surkov (:asurkov)]

Marco, does the patch from bug 424161 help?

Comment 3

[Marco Zehe (:MarcoZ)]

Well, the testcases we have actually don't cover this particular case. test_bug368835.xul only fires TreeViewChanged *once*. If we make a testcase that has two views and switches between them and makes sure everything is updated, we would see the breackage immediately.

Comment 4

[alexander :surkov (:asurkov)]

In test_bug368835.xul I catch tree "treeViewChanged" events. It's possible with the fix of bug bug 424161 we'll handle this problem.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

marco, could you test the patch?

Comment 6

[Marco Zehe (:MarcoZ)]

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

r=me with the following nits/error fixed:
> Index: accessible/public/nsIAccessibleTreeCache.idl

> +   * Invalidates сhildren created for previous tree view.


Please use a regular latin c here instead of a cyrillic one. :-)

> Index: accessible/src/base/nsAccessible.h

> +   * in xul:tree accessible to lie AT. Must be overriden in wrap classes.

must be "to lie to AT. " and "overridden" (double d).

> Index: accessible/src/xul/nsXULTreeAccessible.cpp

> +  // Fire notification only destroy/create events on accessible tree to lie AT

"Fire only notification", and "to lie to AT".

> Index: accessible/src/msaa/nsAccessibleWrap.cpp

> +  return FireToolkitEvent(aEvent);

Must be FirePlatformEvent.

This is a real speed boost on Linux. Changing folders is instantaneous.
Also works nicely on Windows.

Comment 7

[Marco Zehe (:MarcoZ)]

Still there is the problem that sometimes one more column header is being reported visible than there actually are columns. In my Drafts folder, for example, the "From" column header is present, but the "From" column is not. Is this a different issue?

Comment 8

[Marco Zehe (:MarcoZ)]

Also with this patch, I get a ton of warnings in the console that say this:

"Warning: NS_ENSURE_TRUE(parentObject) failed", pointing to this line of code:
http://mxr.mozilla.org/seamonkey/source/accessible/src/atk/nsAccessibleWrap.cpp#1446.

Comment 9

[Marco Zehe (:MarcoZ)]

(In reply to comment #5)
> marco, could you test the patch?

I managed to crash it in nsRootAccessible.cpp:
> +  if (isTree) {
> +    nsCOMPtr<nsIAccessibleTreeCache> treeAcc(do_QueryInterface(accessible));
> +
> +    if (eventType.EqualsLiteral("TreeViewChanged"))
> +      return treeAcc->TreeViewChanged();
> +

Adding an NS_ENSURE_TRUE(treeAcc, NS_OK); before the first if
fixed that crash for me.

I did this:
1. Started Thunderbird with that patch.
2. Pressed TAB twice to get to the message folder tree.
3. Hit DownArrow to focus on "Drafts".
4. Pressed TAB to focus on messages tree table.
5. Started DOM Inspector.
6. Chose File/Inspect Chrome Document/Drafts - Mail/News.
7. Pressed TAB and Shift+TAB once to get focus to the tree.
8. Clicked the Node type button and selected Accessible tree.
9. Pressed ENTER.

Boom! Repetitive crash.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Still there is the problem that sometimes one more column header is being
> reported visible than there actually are columns. In my Drafts folder, for
> example, the "From" column header is present, but the "From" column is not. Is
> this a different issue?
> 

Please file bug for this. Let's deal with this separately.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> (In reply to comment #5)
> > marco, could you test the patch?
> 
> I managed to crash it in nsRootAccessible.cpp:

Additionally I put assertion there because I believe tree accessible must implement that interface. With assertion it should be easier to handle the problem.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch2

Not sure the patch address comment Marco's comment 8. I'll look more deeply.

Comment 13

[Marco Zehe (:MarcoZ)]

(In reply to comment #10)
> (In reply to comment #7)
> > Still there is the problem that sometimes one more column header is being
> > reported visible than there actually are columns. In my Drafts folder, for
> > example, the "From" column header is present, but the "From" column is not. Is
> > this a different issue?
> Please file bug for this. Let's deal with this separately.

Done, bug 428240.

Comment 14

[Evan Yan]

Comment on attachment 314775 [details] [diff] [review]
patch2

So when we get "TreeViewChanged" event, we just fire platform event to lie to AT, but actually we do nothing to our cached tree cells, right?

I think it may be ok for tree cells, but could be problem for column headers. Because not like tree cell accessibles, column header accessibles have corresponding DOM nodes. And it seems when "TreeViewChanged" event come, the DOM nodes for column headers are actually changed, but we didn't update the column header accessibles.

I think that may be the cause of the problems in comment #7 and comment #8.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (From update of attachment 314775 [details] [diff] [review])
> So when we get "TreeViewChanged" event, we just fire platform event to lie to
> AT, but actually we do nothing to our cached tree cells, right?

We clean up the cache: remove cached tree cells.

> I think it may be ok for tree cells, but could be problem for column headers.
> Because not like tree cell accessibles, column header accessibles have
> corresponding DOM nodes. And it seems when "TreeViewChanged" event come, the
> DOM nodes for column headers are actually changed, but we didn't update the
> column header accessibles.

Since column headers are usual DOM nodes then we should handle their notification by usual order. treeViewChanged event and column header reorder isn't related things I think.

> I think that may be the cause of the problems in comment #7 and comment #8.
> 

Can you give me a hint how it can be related with comment #8. I just don't get why tree accessible hasn't parent. It should mean tree accessible is old and out of tree. But why do we get treeViewChanged event.

Btw, Marco, if it won't take a lot of your time then could you test the latest patch to check the problem of comment #8?

Comment 16

[Evan Yan]

Sorry for the later reply, I forgot to cc myself.

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 314775 [details] [diff] [review] [details])
> > So when we get "TreeViewChanged" event, we just fire platform event to lie to
> > AT, but actually we do nothing to our cached tree cells, right?
> 
> We clean up the cache: remove cached tree cells.
> 
Yes, I missed that line.

> > I think it may be ok for tree cells, but could be problem for column headers.
> > Because not like tree cell accessibles, column header accessibles have
> > corresponding DOM nodes. And it seems when "TreeViewChanged" event come, the
> > DOM nodes for column headers are actually changed, but we didn't update the
> > column header accessibles.
> 
> Since column headers are usual DOM nodes then we should handle their
> notification by usual order. treeViewChanged event and column header reorder
> isn't related things I think.
> 
I checked it, the DOM nodes of column headers are not changed. So my guess was wrong.

> > I think that may be the cause of the problems in comment #7 and comment #8.
> > 
> 
> Can you give me a hint how it can be related with comment #8. I just don't get
> why tree accessible hasn't parent. It should mean tree accessible is old and
> out of tree. But why do we get treeViewChanged event.
> 
We not only get treeViewChanged event, but also get RowCountChanged and TreeInvalidate events. Could that be related?
Do you want to deal with this issue in separate bug?

Comment 17

[Evan Yan]

Ok, there are different situations.

When switch from Inbox to Draft, the column headers are changed. If switch from Inbox to some other folder like Trash, the column headers keep the same. I tried to log the dom events we received when switching folder. I didn't see any extra event related to invalidate the dom nodes of column headers in the case that column headers are changed. So might there still be chance that we didn't invalidate column header accessibles?

As to comment #8, could the atk object be the old column headers? 

Just some guess, need more investigation of course.

Comment 18

[Marco Zehe (:MarcoZ)]

(In reply to comment #16)
> We not only get treeViewChanged event, but also get RowCountChanged and
> TreeInvalidate events. Could that be related?

Do we need all these events? If a tree is destroyed and then recreated, do we really need to fire rowcount change etc.? Isn't that obvious from the fact that the tree gets destroyed and recreated? This sounds like too many events to process.

(In reply to comment #17)
> When switch from Inbox to Draft, the column headers are changed. If switch from
> Inbox to some other folder like Trash, the column headers keep the same. I
> tried to log the dom events we received when switching folder. I didn't see any
> extra event related to invalidate the dom nodes of column headers in the case
> that column headers are changed. So might there still be chance that we didn't
> invalidate column header accessibles?

This would correspond with what I am seeing. When switching to "Drafts", we may at some point notice that columns have changed visibility, and try to update, but not always get things right. Since these column headers are part of the tree, they should be destroyed and recreated just like the tree cells do. After all, there might be a different set of column headers alltogether for different folders, depending on what the user choses to show and hide.

> As to comment #8, could the atk object be the old column headers? 

I would suggest cleaning everything up first, meaning we make sure we always recreate everything correctly, and if we still see such errors then, we can take another look. From the current state, it may very well be that these phantom column headers bite us.

Comment 19

[alexander :surkov (:asurkov)]

When I switch between folders my tree columns are the same everywhere. How do you get them different.

(In reply to comment #17) 
> As to comment #8, could the atk object be the old column headers? 

it's possible I think because with my static columns case I don't get any message from comment #8

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> (In reply to comment #16)
> > We not only get treeViewChanged event, but also get RowCountChanged and
> > TreeInvalidate events. Could that be related?
> 
> Do we need all these events? If a tree is destroyed and then recreated, do we
> really need to fire rowcount change etc.? Isn't that obvious from the fact that
> the tree gets destroyed and recreated? This sounds like too many events to
> process.

I think we don't but how we can manipulate with them?

Comment 21

[alexander :surkov (:asurkov)]

Since it's very like the problems go from columns handling (the bug 428240) then let's continue the reivew process and try get the bug in the tree since it helps to thunderbird accessibility, what do you think?

Comment 22

[Evan Yan]

Comment on attachment 314775 [details] [diff] [review]
patch2

r=me, let's deal with the issue of column headers in bug 428240.

> nsresult
> nsRootAccessible::HandleTreeRowCountChangedEvent(nsIDOMEvent *aEvent,
>-                                                 nsIAccessible *aAccessible,
>-                                                 const nsAString& aTargetName)
>+                                                 nsIAccessibleTreeCache *aAccessible)

nit: I think it would be less confusion to use a name of aAccTreeCache or aTreeAcc other than aAccessible.

> nsresult
> nsRootAccessible::HandleTreeInvalidatedEvent(nsIDOMEvent *aEvent,
>-                                             nsIAccessible *aAccessible,
>-                                             const nsAString& aTargetName)
>+                                             nsIAccessibleTreeCache *aAccessible)

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314775 [details] [diff] [review]
patch2

Drivers need risk/reward assessments for approval these days. I'm seeing a lot of last minute changes here, and would like to make sure we're not just throwing patches against the tree to see what sticks.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> (From update of attachment 314775 [details] [diff] [review])
> Drivers need risk/reward assessments for approval these days. I'm seeing a lot
> of last minute changes here, and would like to make sure we're not just
> throwing patches against the tree to see what sticks.
> 

Reward is much because currently thunderbird can't be well accessible, i.e. there is big chance user can't reed his messages.  It's major issue and we must fix it before thunderbird achieve release status.

I think it shouldn't be very big risk because all we do here is remove cached treeitems accessible when tree view is changed. Currently we recreate tree accessible what is not totally right and not clear issue.

Comment 25

[Marco Zehe (:MarcoZ)]

To more clarify the issue: This is NOT a problem in Firefox. But it is a BLOCKER for Thunderbird on Linux. If we don't take this one, Thunderbird 3.0a1 won't be accessible on Linux for blind users. It basically means that, whenever the user changes folders in TB, that the accessibility info is lost in over 90% of the use cases. Only a very small percentage gets the current implementation/event firing right.
I would be fine with the following:
1. Ship Thunderbird 3.0a1 documenting that we are currently not accessible on Linux.
2. Land this one as soon as Firefox has shipped on Trunk or whatever Branch Thunderbird will then use.

In the meantime, I would suggest that we find a way to enhance the try-servers to also make Thunderbird builds. Try Servers are a great way to test out this kind of thing before committing it. Our problem is that we couldn't throw this at people in a try-server build because they currently can't make Thunderbird builds.

Comment 26

[alexander :surkov (:asurkov)]

cc'ing Mike to get an opinion on previous two comments

Comment 27

[Marco Zehe (:MarcoZ)]

Alexander, can you put an updated patch in? The current one no longer applies cleanly against trunk. Thanks!

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch3 (updated to trunk)

Comment 29

[Marco Zehe (:MarcoZ)]

Committed in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/be010ac940e0

Comment 30

[Peter Weilbacher]

This patch left nsAccessibleWrap in src/other unchanged, so that accessibility doesn't compile any more. Could you please at least add a dummy FirePlatformEvent? (Sorry, not really set up for Hg yet, otherwise I would post a patch.)

Comment 31

[Marco Zehe (:MarcoZ)]

Peter, I just took a look at other/nsAccessibleWrap.cpp, and notice that this only contains empty implementations for the constructor and destructor. None of the other methods usually found in nsAccessibleWrap implementations is present in this file.

What exact compiler error do you get?

Comment 32

[Peter Weilbacher]

I got

make.exe[5]: Entering directory `X:/central/fx/accessible/src'
make.exe[6]: Entering directory `X:/central/fx/accessible/src/other'
nsAccessNodeWrap.cpp
nsAccessibleWrap.cpp
nsRootAccessibleWrap.cpp
Creating library file `accessibility_toolkit_s.lib'
accessibility_toolkit_s.lib
make.exe[6]: Leaving directory `X:/central/fx/accessible/src/other'
make.exe[6]: Entering directory `X:/central/fx/accessible/src/base'
nsAccessNode.cpp
X:/central/src/accessible/src/base/nsAccessNode.cpp: In function `static nsIAccessibilityService* nsAccessNode::GetAccService()':
X:/central/src/accessible/src/base/nsAccessNode.cpp:103: warning: unused variable `nsresult rv'
X:/central/src/accessible/src/base/nsAccessNode.cpp: In static member function `static already_AddRefed<nsApplicationAccessibleWrap> nsAccessNode::GetApplicationAccessible()':
X:/central/src/accessible/src/base/nsAccessNode.cpp:246: error: cannot allocate an object of type `nsApplicationAccessibleWrap'
X:/central/src/accessible/src/base/nsAccessNode.cpp:246: error:   because the following virtual functions are abstract:
X:/central/src/accessible/src/base/nsAccessible.h:257: error: 	virtual nsresult nsAccessible::FirePlatformEvent(nsIAccessibleEvent*)
make.exe[6]: *** [nsAccessNode.o] Error 1
make.exe[6]: Leaving directory `X:/central/fx/accessible/src/base'

and fixed it like this (plain diff, not having Hg):

--- accessible\src\other\nsAccessibleWrap.h.orig   2006-06-27 15:42
:08.000000000 +0000
+++ accessible\src\other\nsAccessibleWrap.h     2008-06-25 23:50:24.000000000 +0
000
@@ -51,6 +51,11 @@ class nsAccessibleWrap : public nsAccess
   public: // construction, destruction
     nsAccessibleWrap(nsIDOMNode*, nsIWeakReference *aShell);
     virtual ~nsAccessibleWrap();
+
+  protected:
+    virtual nsresult FirePlatformEvent(nsIAccessibleEvent *aEvent) {
+      return NS_ERROR_NOT_IMPLEMENTED;
+    }
 };
 
 #endif

Comment 33

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Patch based on Peter's diff.

Comment 34

[alexander :surkov (:asurkov)]

Comment on attachment 326864 [details] [diff] [review]
Patch

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff --git a/accessible/src/other/nsAccessibleWrap.h b/accessible/src/other/nsAccessibleWrap.h
>--- a/accessible/src/other/nsAccessibleWrap.h
>+++ b/accessible/src/other/nsAccessibleWrap.h
>@@ -51,6 +51,11 @@
>   public: // construction, destruction
>     nsAccessibleWrap(nsIDOMNode*, nsIWeakReference *aShell);
>     virtual ~nsAccessibleWrap();
>+
>+  protected:
>+    virtual nsresult FirePlatformEvent(nsIAccessibleEvent *aEvent) {
>+      return NS_ERROR_NOT_IMPLEMENTED;

should be NS_OK I think because if there is no platform event then it's ok

thank you for the patch

Comment 35

[Marco Zehe (:MarcoZ)]

Pushed with surkov's review comment addressed in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/0847e34786a9

Thanks Peter for the patch!

Comment 36

[Samuel Sidler (old account; do not CC)]

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.