385070 - The parent of the find toolbar's focused entry is defunct

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Joanmarie Diggs]

It seems that a new accessible gets created for the Find toolbar each time the Find toolbar appears.  It also seems that an old/defunct instance of the toolbar is the one that claims to have focus via AT-SPI.

Steps to reproduce:
1. Launch Minefield
2. Press Control F
3. Press Escape
4. Press Control F
5. Type "hello"
6. Launch Accerciser
7. Find the entry, navigate to Accerciser's Interface Viewer, and examine the text

Expected results: "hello" would appear in the text field within Accerciser

Actual results: Nothing appears in the text field within Accerciser

Notes: 
1. If you repeat the above test but remove steps 2 and 3, "hello" does appear in the text field in Accerciser as expected.

2. What I'm seeing in Orca when I perform the above test is that focus is going to an entry named Find. However, the parent of that entry is not the Find toolbar, but a defunct object.

3. This does not seem to be a regression based on spot-checking various earlier builds.

Comment 1

[Aaron Leventhal]

I think we should work on this after Surkov does his mutation event rewrite in bug 378468.

Comment 2

[Aaron Leventhal]

See also bug 371279

Comment 3

[Evan Yan]

I debugged this one a little today.

When the find bar pop up on the second time, they are different DOM nodes than it pop up on the first time, but we still hold the old accessibles in our accessible tree.

Comment 4

[Aaron Leventhal]

Evan, are bug 371279 or bug 378468 related?

Comment 5

[Evan Yan]

bug 371279 is not related, it was about the same label with changed text.

for bug 378468, sort of...

when find toolbar pop up, we got EVENT_REORDER in doc accessible, but we didn't InvalidateCachedSubtree().
Will this problem be addressed by bug 378468?

Comment 6

[Aaron Leventhal]

I don't know, but that one is almost finished. It's probably just easier to wait 2-3 days and debug it with the new code.

Comment 7

[Joanmarie Diggs]

ping. :-)

Comment 8

[Evan Yan]

I can't reproducible it any more.

Joanie, could you please confirm it?

Comment 9

[Joanmarie Diggs]

Hmmm... The original test case is no longer reproducible but Orca still thinks that focus is in an item that is defunct.  And it thinks this even if the last thing you do is launch Orca (i.e. I don't think Orca's holding on to something it shouldn't be because it shouldn't know anything about objects that were created and destroyed before it was even launched, right??).

So, before we close this one out, let me do a bit more investigation.

Thanks for the quick follow up!

Comment 10

[Joanmarie Diggs]

New way to reproduce the same problem:

1. Launch Firefox and Accerciser
2. In Accerciser, switch to the API Browswer
3. In Firefox, press Control F  then press Control Alt A (this will cause Accerciser to display information about the focused object.  You should find the following properties/values:

childCount     0
description
name           Find:
parent         [tool bar |]

4. In Firefox, be sure you are in the Find entry.  Press Escape to hide it.  Then repeat step 3.  This time you get the following properties/values:

childCount     0
description
name           Find:
parent         [invalid |] <--- oops

So, yeah, the text is showing up in *a* find toolbar now. We're simply not in that one. :-)  The defunct instance of the Find toolbar is still claiming to have focus somehow.

Comment 11

[Evan Yan]

sorry for late reply.

Joanie, I think that's because Accerciser still hold the old Find Toolbar accessible object. If I refresh the tree in Accerciser, the problem is gone.

Comment 12

[Joanmarie Diggs]

Well.... Maybe.... :-) Okay, try this one:

0. Do NOT start Accerciser, do NOT start Orca
1. Launch Firefox
2. Control F, Escape, Control F
3. Launch gnome-terminal and start Orca
4. Alt Tab back to Firefox (focus should be in the Find entry)
5. Press Insert F7 to dump the ancestry of the focused object to the terminal window.  I get this:

+-name='Minefield' role='application' state='ENABLED SENSITIVE SHOWING VISIBLE' relations=''
  +-name='Minefield' role='frame' state='ACTIVE ENABLED HORIZONTAL OPAQUE RESIZABLE SENSITIVE SHOWING VISIBLE' relations='EMBEDS'
    +-name=None role='invalid' state='DEFUNCT' relations=''
      +-name='Find:' role='entry' state='EDITABLE ENABLED FOCUSABLE FOCUSED HORIZONTAL OPAQUE SENSITIVE SHOWING SINGLE_LINE VISIBLE' relations='LABELLED_BY'

Note that the parent of the entry has ROLE_INVALID and STATE_DEFUNCT.

If you don't kill toolbar (with Escape) and bring up a new one (with Control F), you get this: 

+-name='Minefield' role='application' state='ENABLED SENSITIVE SHOWING VISIBLE' relations=''
  +-name='Minefield' role='frame' state='ACTIVE ENABLED HORIZONTAL OPAQUE RESIZABLE SENSITIVE SHOWING VISIBLE' relations='EMBEDS'
    +-name=None role='tool bar' state='ENABLED HORIZONTAL OPAQUE SENSITIVE SHOWING VISIBLE' relations=''
      +-name='Find:' role='entry' state='EDITABLE ENABLED FOCUSABLE FOCUSED HORIZONTAL OPAQUE SENSITIVE SHOWING SINGLE_LINE VISIBLE' relations='LABELLED_BY'

So something might be hanging on to the defunct item, but I don't think it's Accerciser or Orca.  (How would Orca "hang on" to a defunct item that was created before it was even launched?)

If we can prove that this is not a Firefox bug and is instead some sort of AT-SPI bug, I will happily stop bothering you about it and start bothering Li Yuan. :-) :-) :-)  Do you think it's an AT-SPI bug?

Comment 13

[Evan Yan]

em.. interesting.

My Orca didn't print anything in terminal window when I press Insert F7. But if I use Accerciser to check the Find Toolbar after step 2, every thing goes OK.

I'll try to make my Orca work and investigate again.

Comment 14

[Evan Yan]

Joanie, do you have a way of reproducing by Accerciser?

Comment 15

[Evan Yan]

ok, I looked into Orca's code:

     ancestorList = [child]
     parent = child.parent
     while parent and (parent.parent != parent):
        ancestorList.insert(0, parent)
        parent = parent.parent

Seems if we walked the ancestry tree from child to parent, we can get the defunct toolbar accessible. If using Accerciser, browsing ancestry from parent to child, the toolbar accessible was recreated, no defunct accessible exists.

Comment 16

[Aaron Leventhal]

RefreshNodes() should invalidate the descendants of a defunct find toolbar.

Comment 17

[Aaron Leventhal]

Reassign to us when there's a new testcase.

Comment 18

[Joanmarie Diggs]

Aaron reassigned this to me by mistake.  We're in a meeting and Aaron said to assign it to Alexander. :-)

Comment 19

[alexander :surkov (:asurkov)]

It sounds GetAccessibleWrap returns nsnull, it means either atkObject is null or IS_MAI_OBJECT(atkObject) == false or atkObject->IsValidObject() == false.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> 
> Seems if we walked the ancestry tree from child to parent, we can get the
> defunct toolbar accessible. 

Evan, any idea how can we get this? When accessible is deatached from the tree?

Comment 21

[Evan Yan]

Sorry I'm not sure. I haven't dug deep into it.

Comment 22

[Evan Yan]

RefreshNodes() failed to shutdown the subtree of the hid find toolbar, because the traversal algorithm doesn't work for it.

I guess we need to use GetXBLChildNodesFor(), like what we do in nsAccessibleTreeWalker.

Comment 23

[Aaron Leventhal]

Attachment: Does this help?

Evan, good detective work. Please test this patch and see if it fixes the original problem report.

This is a really leak as well. We're not shutting down accessibles in anonymous subtrees, or removing them from the cache! I've tested to make sure that we shut those down now with this patch.

Unfortunately, GetXBLChildNodesFor() is not enough, because of the anonymous content of an <input type="file">. I don't know of a method to get the anon content for that control. If we can find one that would be great.

However, this also works. For any anonymous content accessible that we Init(), it makes sure the non-anonymous accessible root is created. Then it changes RefreshNodes() to shutdown subtrees starting at the accessible children, if they were for anonymous nodes.

Comment 24

[Evan Yan]

Comment on attachment 288508 [details] [diff] [review]
Does this help?

Cool, the patch fixed the problem!

Just few comments.

>+    while (childAccessible) {
>+      // Get next sibling before node which caches that is shut down
>+      nsCOMPtr<nsIAccessible> nextSibling;
>+      childAccessible->GetNextSibling(getter_AddRefs(nextSibling));

Won't GetNextSibling() cause CacheChildren(), and make the accessible which is just shutdown get re-created?

>+  // Shutdown ordinary content subtree as well -- there may be
>+  // access node children which are not full accessible objects
>+  aStartNode->GetFirstChild(getter_AddRefs(nextNode));
>+  while (nextNode) {
>+    nextNode.swap(iterNode);
>+    RefreshNodes(iterNode);

nit: wrong indent.

Comment 25

[Aaron Leventhal]

> Won't GetNextSibling() cause CacheChildren(), and make the accessible which is
> just shutdown get re-created?
I think the shutdown happens on the current object after the GetNextSibling() is called on it. An object may sometimes be created do shutdown, but most of the time either all the child accessibles exist in the cache or none of them do.

A better way would be something like GetXBLChildNodesFor() but that works with the file control. I can't find a way.

Comment 26

[Evan Yan]

Say we have accessible subtree like below. RefreshNodes() is called on A. B, C and D are anonymous nodes.

  A
 /|\
B C D

We have this execution sequence:
1). B->GetNextSibling().
2). In recursed RefreshNodes(), B get shutdown-ed
3). C->GetNextSibling(), cause B recreated.

Is that right?

Comment 27

[Aaron Leventhal]

if c::mParent == nsnull

Is that the case?

Comment 28

[Evan Yan]

B->Shutdown() cause parent->InvalidateChildren()
So B won't be recreated, but C->GetNextSibling() will fail, right?

Comment 29

[Aaron Leventhal]

Evan, if you have time today to fix the problems you bring up that'd be great. Otherwise I'll work on it tomorrow.

Comment 30

[Evan Yan]

Attachment: Aaron's idea, but use GetChildren() instead of GetNextSibling()

Comment 31

[Aaron Leventhal]

Comment on attachment 288811 [details] [diff] [review]
Aaron's idea, but use GetChildren() instead of GetNextSibling()

Good work. Please add a comment for this if statement, because it's not obvious right away. Something, we only need to shutdown the accessibles here if one of them has been created.
     privateAccessible->GetCachedFirstChild(getter_AddRefs(childAccessible));
    if (childAccessible) 

Also, you can simplify the following code by QI'ing directly to nsIAccessNode:
        children->QueryElementAt(index, NS_GET_IID(nsIAccessible),
                                 getter_AddRefs(childAccessible));
        nsCOMPtr<nsIAccessNode> childAccessNode(do_QueryInterface(childAccessible));

to:
        nsCOMPtr<nsIAccessNode> childAccessNode;
        children->QueryElementAt(index, NS_GET_IID(nsIAccessNode),
                                 getter_AddRefs(childAccessNode));

Comment 32

[Aaron Leventhal]

Robert, we only need r+/sr+ for the layout bits.

Comment 33

[Mike Connor [:mconnor]]

Comment on attachment 288811 [details] [diff] [review]
Aaron's idea, but use GetChildren() instead of GetNextSibling()

a=mconnor on behalf of endgame drivers

Comment 34

[Aaron Leventhal]

Checked in for Evan.

Comment 35

[Evan Yan]

The patch seems broke the tree. see
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Ports/1195178460.1195179342.4249.gz

core stack:

-----------------  lwp# 1 / thread# 1  --------------------
 ff0c4a08 _lwp_kill (4, ffbfe6f0, 0, 0, fffffffc, 0) + 8
 fd9393b0 void nsProfileLock::FatalSignalHandler(int) (4, fee12150, cfab4, c, feda6a50, fd939198) + 1cc
 ff0c3944 __sighndlr (4, 0, ffbfe878, fd9391e4, 0, 0) + c
 ff0b8254 call_user_handler (4, 0, ffbfe878, 3, 48288, ff100650) + 41c
 0094a2a0 ???????? (ffbfec7c, ffbfec78, 94a2a0, b681b0, 0, 4)
 fe592ed8 unsigned nsDocAccessible::FlushPendingEvents() (933f18, 2, ffbfec70, ffbfec74, fef8b940, ffbfec78) + 7c
 fe6b4400 void nsTimerImpl::Fire() (2, b72c98, fe5934cc, 0, fefb83a8, a56b079e) + 118
 fe6b4538 unsigned nsTimerEvent::Run() (b86088, 1, fe6b44f8, 1, 93, 93) + 40
 fe6b03b8 unsigned nsThread::ProcessNextEvent(int,int*) (cfec8, 1, ffbfee1c, 1, cfee0, 1) + 154
 fe656c10 int NS_ProcessNextEvent_P(nsIThread*,int) (fe6b0264, 1, fefb8054, 6d10, 74fde0, cfec8) + 50
 fe554c5c unsigned nsBaseAppShell::Run() (cff78, 1, 0, cfec8, 0, 0) + 40
 fe335d90 unsigned nsAppStartup::Run() (fe554c1c, fef86404, 1, fefb3c50, 0, 467a18) + 34
 fd9306c8 XRE_main (ffbff2b8, ffbff31c, ffbff348, ffbff370, fd93663c, ffbff3a4) + 2104
 00011b18 main     (4, ffbff4c4, fffeff10, 220f8, 43c78, ffbff45c) + 224
 00011428 _start   (0, 0, 0, 0, 0, 0) + 108

Comment 36

[Evan Yan]

patch backed out, to make nightly build safe.

Comment 37

[Evan Yan]

An easy way to produce the crash: just start firefox, then press ctrl + B to show bookmarks.

Comment 38

[Aaron Leventhal]

Evan, can you work on it?

Comment 39

[Evan Yan]

Sure, I'm working it and will keep the latest status updated.

Comment 40

[Evan Yan]

Attachment: patch v3

We shutdown-ed doc accessible by mistake in the former patches.

This patch made two changes based on patch v2.
1) move shutdown code into the |if| statement of |accessNode != this|;
2) remove the invoke of InvalidateChildren() in FlushPendingEvents(), because we will do it when we shutdown accessible in RefreshNodes().

Comment 41

[Aaron Leventhal]

#1 is good

#2 is good, but it's not obvious at first:

Right now FlushPendingEvents() uses GetParent() before parent->InvalidateChildren(), and thus it will create the parent before the invalidation if the parent didn't exist. The Shutdown() method will only InvalidateChildren() on mParent -- the parent already had to exist. Does this difference matter?

I think it's okay, because if we needed to InvalidateChildren() on the parent, that means that the parent would already know what children it has, and they would know what their parent is. A parent wouldn't just know some of its children. Because when we get a child we always get all of them.

Comment 42

[Aaron Leventhal]

Comment on attachment 289461 [details] [diff] [review]
patch v3

Already have sr= for layout parts, so we don't need that again.