376468 - Firefox crashes when user submit a bug with orca running.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Tim Miao]

Attachment: stack trace on solaris

This bug makes firefox crash.
Steps to reproduce:
1. Run orca and firefox.
2. Navigate to landfill.bugzilla.org and file a bug there.
3. Tab to submit button and press Enter.

Bug observation:
Firefox crashed.

Additional info:
This bug can be found with firefox Gecko/20070206 Minefield/3.0a2pre and orca2.19.0 on both ubuntu and opensolaris.

Comment 1

[Tim Miao]

This bug can not be reproduced with firefox 20070331 nightly trunk build. Move to close.

Comment 2

[Tim Miao]

This bug can be found on yesterday nightly trunk build 20070409. Reopen it.

Additional comment: The crash comes randomly, sometimes it crashes when I press the submit button, sometimes it crashes when I tab and navigate browse the page, sometimes it even crashes on firefox starting. It's hard to give a unique way to reproduce this bug, but if you keep orca and firefox running, you will find the crash eventually.

Comment 3

[Aaron Leventhal]

Evan, since you're looking at the other crash, I also assigned this to you, oka?

Comment 4

[Evan Yan]

sure, no problem.

Comment 5

[Aaron Leventhal]

Is this fixed now, from the other crash fixes?

Comment 6

[Evan Yan]

no, this bug still exists.

Comment 7

[Evan Yan]

It's not easy for me to debug, since I can't reproduce this but reliably.

What I know so far is it crashes at shutting down nsHTMLComboboxListAccessible, the accessible's mFirstChild is a dangling pointer.

Comment 8

[Evan Yan]

When Firefox crashes, there are some assertions of docAccessible failed at nsAccessible.cpp:587.
And after added print into the code, I saw the child, which the dangling pointer pointed to, has been destructed, while I didn't see it being shutdown.
So I suspect the child hasn't been added into doc accessible's cache.

Comment 9

[Aaron Leventhal]

Mats' patch in bug 377470 fixes a crash related to nsHTMLComboboxListAccessible. When that goes in, can you test and see if it fixes this?

Comment 10

[Aaron Leventhal]

I'm unable to duplicate this crash.

Comment 11

[Evan Yan]

bug 377470 doesn't help on this one.

The cause of this bug:

We created nsHTMLComboboxListAccessible ourselves. The first time when combobox do CacheChildren(), everything is OK. Things are like below:

ComboboxAcc
  --ComboboxListAcc
    --Option accessibles

Then for some reason the combobox did InvalidateChildren() (submitting a form may cause that, I guess). So the next time the same combobox do CacheChildren(), it will create a brand new nsHTMLComboboxListAccessible. 

Then every option accessibles which were the former ComboboxListAcc's children, are now all belong to this new one. But the former ComboboxListAcc still hold the option accessibles as its children.

So once the option accessible shutdown, but the former ComboboxListAcc visit its children, which is dangling pointer now, Firefox crashes.

Comment 12

[Aaron Leventhal]

Great summary!

Comment 13

[Aaron Leventhal]

When the ComboBoxACc gets Shutdown() it should Shutdown() it's children, which should cause the ComboboxListAcc to be Shutdown() which should cause all the option accessibles to be shutdown.

That way when the new combo box accessible is created with a new list, it should not use the old children.

Comment 14

[Aaron Leventhal]

Actually, is the combo box accessible Shutdown() at all? Or do we only InvalidateChildren() on it?

Also, if the form is being submitted, and a new page is loading, why are we creating another accessible for the combo box? I would think we don't need it again since it's on a page that's going away.

Comment 15

[Evan Yan]

(In reply to comment #14)
> Actually, is the combo box accessible Shutdown() at all? Or do we only
> InvalidateChildren() on it?
> 
The combobox accessible doesn't shutdown, just InvalidateChildren().

I'm not very sure when it took place, since it crashes randomly and not easy to debug.
What I did right now is adding lots of print into the code, and try to reproduce the crash, then analyze the log.

Comment 16

[Aaron Leventhal]

I think there are 2 problems that we should fix:
1) nsDocAccessible::RefreshNodes(childNode, aChangeEventType) is called by InvalidateCacheSubtree(). It's supposed to shut down the entire subtree. However it does this by iterating the DOM subtree, and the listbox is not in the DOM subtree. It also needs to shut down any attached accessibles that are created by CacheChildren() but aren't part of the DOM subtree.

2) After #1 is fixed, we need to look at why the combo box accessible is getting  invalidated and recreated in the first place. We should look into why all that extra work is happening.

Comment 17

[Aaron Leventhal]

Additional info: LSR does not crash with it.

Comment 18

[Evan Yan]

(In reply to comment #16)
> I think there are 2 problems that we should fix:
> 1) nsDocAccessible::RefreshNodes(childNode, aChangeEventType) is called by
> InvalidateCacheSubtree(). It's supposed to shut down the entire subtree.
> However it does this by iterating the DOM subtree, and the listbox is not in
> the DOM subtree. It also needs to shut down any attached accessibles that are
> created by CacheChildren() but aren't part of the DOM subtree.
> 
Yes, I agree. I think the accessible who created attached accessible should in charge of shutdown the attached one as well.

For this case, the combobox list accessible actually was added to the cache when it Init(). I think we got another problem about cache here.

Some attached accessibles were created using mDOMNode of the owner accessible (like comboboxListAcc using the mDOMNode of comboboxAcc). When it Init() and added to cache, the same unique id will be used for multiple accessibles, because unique id is come from mDOMNode.

I think we should never added an attached accessible into cache. Instead, the creater should take care of what it created.

If you think so, we can have another bug to address this.

> 2) After #1 is fixed, we need to look at why the combo box accessible is
> getting  invalidated and recreated in the first place. We should look into why
> all that extra work is happening.
> 
The combobox accessible is not recreated, but the combobox _list_ accessible was recreated by the combobox accessible.

Comment 19

[Evan Yan]

Attachment: patch

1) use mListAccessible to hold the combobox list accessible, and don't Init() it to prevent adding it to cache.
   I used nsRefPtr, since nsHTMLTextAccessible also uses nsRefPtr to hold its attached accessible, but I'm not very sure whether there could be a problem.

2) implement nsHTMLComboboxAccessible::Shutdown(), to shutdown combobox list accessible as well;
   deleted useless nsHTMLSelectableAccessible::Shutdown() declaration.

Comment 20

[Evan Yan]

With this patch, I cannot reproduce this bug any more, while I can reproduce it once in 3 tries in average before.

Comment 21

[Aaron Leventhal]

I think combo box accessible doesn't need special mListAccessible. 

nsHTMLComboboxAccessible::Shutdown() could just run through accessible children and shut them down.

That way the list accessible will be recreated next time.

Comment 22

[Aaron Leventhal]

+ nsAccessibleWrap(aDOMNode, aShell), mListAccessible(nsnull)

Should not need to set it to null, because nsRefPtr<> does that for you automatically.

Comment 23

[Aaron Leventhal]

If we look at bug 377737, it seems that the destructor for every class that overrides Shutdown() should call Shutdown() directly. And if we do that, I'm not sure if we really need mListAccessible->Shutdown()
That should happen automatically when refcount == 0. After all it's not in the cache anymore. But, we could keep that one line  just to be safe.

Has this been leak tested? Is there any concern with circular references?

Comment 24

[Aaron Leventhal]

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

This is very similar to what we do for list bullet accessible.

Should not need 
mListAccessible(nsnull)
Please remove that if I'm correct.

Comment 25

[Aaron Leventhal]

Filed bug 377891 for extra invalidations in comboboxes that occur whenever an option changes.

Comment 26

[Evan Yan]

Attachment: patch to check in

Comment 27

[Aaron Leventhal]

Enter passphrase for key '/home/aleventhal/.ssh/id_dsa':
Checking in accessible/src/html/nsHTMLSelectAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLSelectAccessible.cpp,v  <--  nsHTMLSelectAccessible.cpp
new revision: 1.76; previous revision: 1.75
done