429030 - Crashes in [@ nsAccessibleWrap::accNavigate] when navigating using NVDA

Status: RESOLVED WORKSFORME

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041406 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041406 Minefield/3.0pre

Over the past few days, I have experienced many crashes which seem to relate to nsAccessibleWrap::accNavigate. The frames above this seem to have changed somewhat in the most recent build I have used. Unfortunately, I am not entirely sure how to reproduce this bug. When it does occur, it seems to occur on a focus change. I am using NVDA and we probably do query using accNavigate quite a bit on focus changes.

Crash reports:
bp-f9b79d6b-09b1-11dd-a61d-001cc4e2bf68
bp-332babc0-09b3-11dd-afcf-001cc4e2bf68
bp-ee1bd18f-09c4-11dd-bd68-0013211cbf8a
bp-bfc6d640-09e0-11dd-b8bf-001b78bc73ea
(The following reports are from a later build and do not list the nsAccessible::GetNextSibbling frame:)
bp-e8a631f0-0a65-11dd-b57b-001cc45a2ce4
bp-78fcb4a3-0a69-11dd-86f3-001cc45a2c28
bp-dec6cc4a-0a69-11dd-b594-001cc4e2bf68

Reproducible: Sometimes

Comment 1

[alexander :surkov (:asurkov)]

I assume that mNextSibling accessible dies but "this" accessible is out of dated. We keep strong reference on parent accessible. Theoretically we should be able to keep strong reference on next sibling as well but we can't do it for first child accessible. Though iirc there is similar things in DOM implementation and they somehow avoid this problem. It would be nice to get more information about this.

Comment 2

[Aaron Leventhal]

James, does the current accessible you are navigating from have the DEFUNCT state set? Or, if you avoid navigating from DEFUNCT accessibles does the crash go away?

Comment 3

[Aaron Leventhal]

Also, if you go back to an earlier version of NVDA does the crash go away? Or is it a Firefox regression? (iow if you go back to an earlier version of Firefox does the crash stop happening)

Comment 4

[Aaron Leventhal]

Does NVDA ever use accNavigate() with a VARIANT containing a child number?

Comment 5

[James Teh [:Jamie]]

Ok. I've finally managed to work out how to reproduce this reliably enough that I can provide some more information, although it is still sketchy.

The crash occurs in Firefox 3.0b5, so it isn't a regression at least as far back as b5. I can check earlier versions if that will help. I suspect it is actually caused by a recent change to NVDA.

It seems to occur under certain circumstances when I attempt to navigate next from the property page containing the iframe/document after closing a tab. Closing a tab does remove a property page object. Unfortunately, the crash does not always occur.

Using NVDA, I can reliably reproduce the issue by following these steps exactly:
1. Ensure that Firefox is closed and then start it with a blank document as the only open document. You may need to set about:blank as your home page to get this to happen.
2. Press ctrl+t to open a new tab.
3. Press ctrl+f4 to close the tab.
4. If you are focused on the location bar, press escape to stop Firefox from opening the auto complete list when you press tab. It seems that pressing ctrl+f4 while on the location bar also acts as if f4 was pressed to expand the combo box. :)
5. Tab to the document.
In an unmodified copy of a trunk build of NVDA, this will cause a crash. This is because NVDA tries to navigate next on the property page.
If this code is disabled, there will be no immediate crash. However, pressing NVDA+numpad8 twice to move the navigator to the property page and then NVDA+numpad6 to move to the next object will cause the same crash.

I suspect there are some other interactions that I am missing here, but navigating next from that propertty page is what finally causes the crash.

I've also seen many cases where navigating next from this property page returns an "Unspecified error" COM exception without crashing Firefox. Closing a tab while focused on the document using an unmodified NVDA trunk almost always reproduces this, assuming the crash doesn't occur.

To answer Aaron's other question:
NVDA doesn't use accNavigate with a child ID. However, we do use accChild passing a unique ID from time to time. As far as I know, this has never caused a crash.

Comment 6

[Aaron Leventhal]

Does the current tab that you are navigating from have an IA2 state of DEFUNCT?

Comment 7

[Aaron Leventhal]

My last question assumed you were navigating from the pagetab, which I realize it's false.

What role and MSAA+IA2 state does the object you are navigating from have?

Also, does the crash only occur if you start with a document with nothing focusable in it?

Comment 8

[Aaron Leventhal]

Another question, does the crash also happen if you use Ctrl+W instead of Ctrl+F4?

Comment 9

[James Teh [:Jamie]]

The object from which I am navigating has a role of ROLE_SYSTEM_PROPERTYPAGE, no IAccessible states and IAccessible2 states of IA2_STATE_VERTICAL and IA2_STATE_OPAQUE.

It seems that the crash only occurs after closing a tab containing about:blank. I thought it happened under other circumstances as well, but I certainly can't reproduce this at present.

The crash occurs after both ctrl+f4 and ctrl+w.

Comment 10

[Aaron Leventhal]

It's a clue that it only happens with about:blank. This might be fixed by bug 429654 or bug 429661. Those deal with invalidations or events at the root node of a document.

Comment 11

[Aaron Leventhal]

Would like to know if this is fixed in tomorrow's build.

Comment 12

[alexander :surkov (:asurkov)]

the crash report should mean mNextSibling is dead, it's possible since we don't hold strong reference on it

Comment 13

[Aaron Leventhal]

Normally it would not die but something is wrong in our cache invalidation when about:blank is involved. Invalidation probably fails because the root dom node isn't an nsIContent or something like that.

Jamie, can you retest with a nightly build? I have a feeling Surkov might have fixed it with one of his recent crash fixes.

Comment 14

[James Teh [:Jamie]]

Unfortunately, I can still reproduce this in the current nightly build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042407 Minefield/3.0pre).
Crash report: bp-65273e2e-1231-11dd-9862-0013211cbf8a

Comment 15

[James Teh [:Jamie]]

This no longer affects NVDA in normal usage. However, a user can still cause this using the object navigator, so it still needs to be fixed. I can still reproduce this in a recent nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008061105 Minefield/3.0pre).

Comment 16

[Marco Zehe (:MarcoZ)]

I can still reproduce this in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008071103 Minefield/3.1a1pre. I used the following steps:

1. Start NVDA.
2. Start Firefox with a blank page.
3. Press CTRL+T to open a new tab.
4. Press CTRL+W to close this tab.
5. Press TAB until NVDA says "embedded frame, about:blank document".
6. Press NVDA+NUM PAD 8 once. NVDA is normally the Insert key on the number pad. NVDA will say "Embedded frame".
7. Press NVDA+NUM PAD 8 again. NVDA will say "page".
8. Press NVDA+NUM PAD 6.

Result: Crash.
Crash report:
bp-a9a80f9e-4fdb-11dd-a384-001a4bd43ef6

Comment 17

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Noticed that I did not crash with NVDA+NUM PAD 4, which stands for "Previous sibling". I then looked in nsAccessible::GetPreviousSibling and found that it uses an addref on the parent where nsAccessible::GetNextSibling did not.

Added similar logic as in nsAccessible::GetPreviousSibling, fixing the missing addref in the process and making the function bail out if no parent (e. g. we're at the root of the accessibles which does not have children).

This fixes the crashes for me.

Comment 18

[Aaron Leventhal]

That's really weird. 
nsCOMPtr<nsIAccessible> accessible(GetParent()) 
should addref in the non-null case.

Are you sure that it's not the early return that fixed it?

Is the GetParent() help in nsAccessible.h broken? If so we should remove that helper and always use GetParent(getter_AddRefs())

Comment 19

[Marco Zehe (:MarcoZ)]

Attachment: Patch V2

Aaron, you're right, it's the return early part that fixes it. With this simpler patch, I also don't get a crash. Thanks for the pointer!

Comment 20

[Aaron Leventhal]

Comment on attachment 329267 [details] [diff] [review]
Patch V2

Surkov, any idea why this fixes the bug? I'm confused.

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 329267 [details] [diff] [review]
Patch V2

(In reply to comment #20)
> (From update of attachment 329267 [details] [diff] [review])
> Surkov, any idea why this fixes the bug? I'm confused.
> 

Ok. The accessible object (on which we call GetNextSibling()) is unattached from the tree (because mParent is null and getParent() caln't calculate new parent). But eventually we didn't shutdown the accessible object completely because obviously mNextSibling points to the dead object. If before the patch we tried to addref dead mNextSibling but now we will return NS_ERROR_FAILURE when the acc object is unattached. That sounds correct. But it doesn't look like real fix. That's the unique idea I have that explains the fix I guess.

Comment 22

[Marco Zehe (:MarcoZ)]

Alex, my first guess also was that we don't properly adjust the hierarchy when closing the second tab. Here are the two accessible hierarchies:

With only 1 tab open:

App Frame "Minefield"
- ... several menus and toolbars
- Grouping
- - Property page
- - - internal frame
- - - - Document "about:blank"

With two about:blank documents open:

App frame Minefield
- ... several toolbars and menus
- Grouping
- - Tool tip
- - Tabs
- - - Tab 1
- - - Tab 2
- - Propertypage "abount:blank" no. 1
- - - internal frame
- - - - document "about:blank"
- - propertypage "about:blank" no. 2
- - - internal frame
- - - - document "about:blank"

My suspicion was that we somehow don't update so when navigating from the document to the internal frame to the page, navigate "next" would try to get to the second page, which is no longer there.

But when I performed the steps to open Firefox, open a second tab, closing that tab, and then looking at the a11y hierarchy from accProbe, I didn't find anything that indicated something dangling around which does not belong there.

And furthermore: GetPreviousSibling did not crash me. So I started to compare the two and found two things: 1. The addref in GetPreviousSibling, and the fact that it returns early.

If you have a better idea and we have a way to get to the root cause, that's fine!

I'll hold off checking this one in until I hear from either of you about this.

Comment 23

[alexander :surkov (:asurkov)]

Marco, would assertion !mParent && !parent && mNextSibling help us to debug the stuff? where the 'parent' we get from GetParent(getter_AddRefs(parent))

Comment 24

[Marco Zehe (:MarcoZ)]

OK, got a better stack trace. According to that in
bp-34c42f0a-4fe0-11dd-a1ee-001a4bd43e5c
mNextSibling is not a dead_end_accessible, but addref'ing it still causes the crash.
      NS_IF_ADDREF(*aNextSibling = mNextSibling);

Add to that the fact that I landed in nsCycleCollector.cpp when I had the crash in my debug build and told VS to break. Should our caching code have been updated for the cycle collector somehow?

Comment 25

[Marco Zehe (:MarcoZ)]

Jamie, Mick, are you still seeing this happen? Since we introduced garbage collection, I haven't seen this, and currently can no longer reproduce. Should we close as worksforme?

Comment 26

[James Teh [:Jamie]]

I can't reproduce this in: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090102 Shiretoko/3.1b3pre

As a matter of curiosity, when was garbage collection introduced?

Comment 27

[Marco Zehe (:MarcoZ)]

(In reply to comment #26)
> As a matter of curiosity, when was garbage collection introduced?

Somewhere in the first half of the 3.1 cycle.