434857 - Crash [@ nsAccessibleWrap::Next(unsigned long, tagVARIANT*, unsigned long*) ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Top crasher over the past week with a total of 13 since RC1 came out last Friday.

Sample stack trace is:
bp-7828dfe8-263f-11dd-bed3-001cc4e2bf68

The query is:http://crash-stats.mozilla.com/report/list?range_unit=weeks&query_search=signature&query_type=contains&platform=windows%2Clinux%2Csolaris&version=Firefox%3A3.0&signature=nsAccessibleWrap%3A%3ANext(unsigned+long%2C+tagVARIANT*%2C+unsigned+long*)&query=Accessi&range_value=1

Comment 1

[Aaron Leventhal]

Timeless suggested we ask luser to add symbols to the crash stats server, for some of the libraries in the stack:
<timelyx>	ask luser to add symbols for the versions of the libraries in that stack
	<timelyx>	rpcrt4.dll 5.1.2600.3173 2D7278066B8B4F0AAA1AB3E4F8CEA0DA2 rpcrt4.pdb
	<timelyx>	oleaut32.dll 5.1.2600.3266 659B514A88A24FB6ADB4F29A9BB066E42 oleaut32.pdb
	<timelyx>	ole32.dll 5.1.2600.2726 683B65B246F4418796D2EE6D4C55EB112 ole32.pdb

Comment 2

[Aaron Leventhal]

Timeless and I were looking at nsAccessibleWrap::Next() and it's a bit scary. We should rewrite it to be clearer, and not static cast.

      msaaAccessible->nsAccessNode::Release(); // this accessible will not be received by the caller
As Timeless mentioned, that line is a bit scary.

We should probably go the other direction -- grab each nsIAccessible child and then turn it into an IAccessible.

Surkov, I think if you rewrite it cleanly there's a good chance the bug will disappear.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[alexander :surkov (:asurkov)]

description of the patch
1) use AccessibleEnumerator helper class
2) recreate it on every QueryInterface(IEnumVariant), so we can be sure we have updated children collection
3) don't clone accessible object (clone accessible enumerator only) because I think it's very bad to have unattached from the main a11y tree accessible object.

Comment 5

[Aaron Leventhal]

Why should cache the children on the new class? One or more of them can be invalidated in between calls of Next(). Besides, we can also use acc->GetNextSibling() to get each next accessible. It's basically already cached somewhere else.

Comment 6

[Aaron Leventhal]

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

Also, GetChildren() doesn't take into account MustPrune() as get_accChildCount() does.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Why should cache the children on the new class? One or more of them can be
> invalidated in between calls of Next(). Besides, we can also use
> acc->GetNextSibling() to get each next accessible. It's basically already
> cached somewhere else.
> 

I am not sure just how IEnumVariant should work when collection is changed. Therefore I supposed to work with snapshot. Otherwise IEnumVariant doesn't show correct result. How should we deal if nodes has been added or removed before traversed object? We can keep traversed accessible by pointer or by index. If we will keep pointer then collection will be unusable when pointed object is shutdown. If we will keep the index of traversed object then 1) we loose accessibles before index for navigation if accessibles have been added before index and 2) we loose accessibles after index for navigation if accessibles have been removed before index.

So in any way we loose some information.

Comment 8

[alexander :surkov (:asurkov)]

Could we gather some information from our customers who uses IEnumVariant how they do it and what do they expect? Who use IEnumVariant?

Comment 9

[Aaron Leventhal]

Theoretically we need to support any usage.

I know at least one screen reader that uses it to traverse the entire tree and collect all the IAccessible nodes.

With this design I believe that will require extra memory consumption.

Comment 10

[Aaron Leventhal]

If the tree changes and the index is not currently at 0, we should probably throw an error.

Comment 11

[alexander :surkov (:asurkov)]

http://74.125.39.104/search?q=cache:y29rdtkDElQJ:www.accessinteropalliance.org/docs/Introducing_IAccessibleEx.doc+Dynamic+server+IEnumVariant&hl=ru&ct=clnk&cd=14&gl=ru

"When IEnumVARIANT is used to enumerate the children of an IAccessible, the “current child” position is stored in the IAccessible instance itself. If a single IAccessible instance is used by two clients, the clients may not get a consistent set of children returned to each of them, since they are sharing this “current child” pointer. To avoid this, clients need to ensure they have a unique IAccessible instance by cloning the IAccessible (using IEnumVARIANT::Clone) and then resetting the “current child” position using IEnumVARIANT::Reset."

That sounds to be correct way. Therefore we can use new object and don't care about extra memory consumption.

Comment 12

[alexander :surkov (:asurkov)]

Also, it seems that's the way how CComEnum works. It just return new object implementing IEnumVariant.

Comment 13

[Aaron Leventhal]

I doubt that any screen readers that use it with us actually Clone(). It would be a good thing to test.

It's really an error of some kind if the children have changed in the middle of getting them with IEnumVARIANT. At that point the screen reader isn't getting what it thinks anyway.

I think the MSDN advice is interesting but we need to be best at dealing with our use cases. If we avoid caching we will still produce the desired result for end users in those screen readers that use IEnumVARIANT, but will be more efficient.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> If we avoid caching we will still produce the desired result for
> end users in those screen readers that use IEnumVARIANT, but will be more
> efficient.
> 

could you give more details?

Comment 15

[Marco Zehe (:MarcoZ)]

This is the top crash in Firefox 3.0 and 3.0.1pre over the past 2 weeks with 173 crashes:
http://crash-stats.mozilla.com/report/list?range_unit=weeks&query_search=signature&query_type=contains&product=Firefox&platform=windows%2Clinux%2Csolaris&version=Firefox%3A3.0%2CFirefox%3A3.0.1pre&branch=1.9&signature=nsAccessibleWrap%3A%3ANext%28unsigned+long%2C+tagVARIANT%2A%2C+unsigned+long%2A%29&query=Access&range_value=2

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 17

[Aaron Leventhal]

Looks good at a quick glance.

But, instead of using parent nsAccessibleWrap with nsRefPtr<> could you just check for EVENT_REORDER and set this->mEnumVariantPosition when that happens?

Comment 18

[alexander :surkov (:asurkov)]

Yes, I would like it more of course but we don't fire reorder event if original event is show event.

if (!isShowing) {
    // Fire an event so the assistive technology knows the children have changed
    // This is only used by older MSAA clients. Newer ones should derive this
    // from SHOW and HIDE so that they don't fetch extra objects

So if this is essentially for MSAA clients then we could put flag on reorder event object and filter reorder events when we fire platform events for MSAA clients. How does this sound?

Comment 19

[Aaron Leventhal]

I didn't realize we only fire REORDER when something hides. Why don't we fire it when something is shown. That makes no sense. How is an older version of JAWS supposed to know there was a new object on the page? It seems wrong.

> So if this is essentially for MSAA clients then we could put flag on reorder
> event object and filter reorder events when we fire platform events for MSAA
> clients. How does this sound?

It's in nsEventMap so it gets fired by MSAA, but ATK ignores REORDER events. Therefore that part is already correct. Perhaps I don't understand your suggestion?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> I didn't realize we only fire REORDER when something hides. Why don't we fire
> it when something is shown. That makes no sense. How is an older version of
> JAWS supposed to know there was a new object on the page? It seems wrong.
> 

We remove the REORDER event in bug 390692 comment #7 (we fired it for show/hide events) and reintroduced in bug 392243 (only for hide events). So I think you're right we can fire the reorder event for all cases.

> > So if this is essentially for MSAA clients then we could put flag on reorder
> > event object and filter reorder events when we fire platform events for MSAA
> > clients. How does this sound?
> 
> It's in nsEventMap so it gets fired by MSAA, but ATK ignores REORDER events.
> Therefore that part is already correct. Perhaps I don't understand your
> suggestion?
> 

I did mean to add the flag for nsAccEvent object that would say if we should fire reorder event for MSAA, i.e. the flag == true if it's reorder event generated for hide event (we should fire MSAA event) and flag == false if it's reorder event generated for show event (we shouldn't fire MSAA event). But if we will fire MSAA event for any case then it doesn't make sense any more.

I will change the patch to fire reorder event for show/hide events (like it was before) and use it to drop enumvariant counter.

Comment 21

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 22

[Aaron Leventhal]

Comment on attachment 328297 [details] [diff] [review]
patch3

Needs sr=. I only gave the code logic a shallow look. However, the concept of how it works is good.

Let's comment about -1 as the magic value near the declaration, or even better, have a constant for it.

Comment 23

[[e-mail]]

Comment on attachment 328297 [details] [diff] [review]
patch3

>+STDMETHODIMP
>+nsAccessibleWrap::Clone(IEnumVARIANT FAR* FAR* ppenum)
...
> STDMETHODIMP
> nsAccessibleWrap::get_nRelations(long *aNRelations)
...
>-STDMETHODIMP
>-nsAccessibleWrap::Clone(IEnumVARIANT FAR* FAR* ppenum)
Was there a good reason to move this? It made seeing changes harder...

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> (From update of attachment 328297 [details] [diff] [review])
> >+STDMETHODIMP
> >+nsAccessibleWrap::Clone(IEnumVARIANT FAR* FAR* ppenum)
> ...
> > STDMETHODIMP
> > nsAccessibleWrap::get_nRelations(long *aNRelations)
> ...
> >-STDMETHODIMP
> >-nsAccessibleWrap::Clone(IEnumVARIANT FAR* FAR* ppenum)
> Was there a good reason to move this? It made seeing changes harder...
> 

I moved this to keep all IEnumVariant methods together. Clone() method has been changed a lot therefore I thought it's not a big pain to loose evidence of history.

Comment 25

[alexander :surkov (:asurkov)]

Attachment: patch4

with Aaron's comments (I introduced the constant kIEnumVariantDisconnected).

Comment 26

[alexander :surkov (:asurkov)]

checked in http://hg.mozilla.org/mozilla-central/index.cgi/rev/0a6633de6e31

Comment 27

[Marco Zehe (:MarcoZ)]

Comment on attachment 328438 [details] [diff] [review]
patch4

Top crasher in accessibility in Firefox 3. Patch has been baking on mozilla-central for a week and has not introduced any regressions. I've been dog-fooding it around the clock.

Comment 28

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

Can we get tests for this patch before taking it in 1.9?

Comment 29

[Marco Zehe (:MarcoZ)]

Sam, is there such a thing as platform dependent tests? This is a Windows-only bug that does not affect other platforms and can only be exposed by calling Windows-native wrappers, not the platform independent APIs via Mochitest.

Comment 30

[Reed Loden [:reed]]

(In reply to comment #29)
> Sam, is there such a thing as platform dependent tests? This is a Windows-only
> bug that does not affect other platforms and can only be exposed by calling
> Windows-native wrappers, not the platform independent APIs via Mochitest.

Sure, why not? You can use Makefile ifdef checks to only run certain test directories on certain OSes. If you can test it using JS somehow, there are several ways you can specify only one OS in JS. We definitely have platform dependent tests in the tree.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)
> Sure, why not? You can use Makefile ifdef checks to only run certain test
> directories on certain OSes. If you can test it using JS somehow, there are
> several ways you can specify only one OS in JS. We definitely have platform
> dependent tests in the tree.
> 

We should test COM calls, even we can operate with COM in mozilla (iirc by installing special extension) but our COM classes doesn't support IDispatch.

Comment 32

[Marco Zehe (:MarcoZ)]

(In reply to comment #30)
> (In reply to comment #29)
> Sure, why not? You can use Makefile ifdef checks to only run certain test
> directories on certain OSes. If you can test it using JS somehow, there are
> several ways you can specify only one OS in JS. We definitely have platform
> dependent tests in the tree.

Yes, and the JS part is the main problem: That code only operates on the platform independent code, however this bug is specifically about platform-dependent code, namely Windows. But the interfaces are only exposed on platform independent code (e. g. accessible/src/base, accessible/src/html), but not the platform-dependent wrappers like accessible/src/msaa.

Comment 33

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

Comment on attachment 328438 [details] [diff] [review]
patch4

Approved for 1.9.0.2. Please land in CVS. a=ss

Marco, is there a bug on file for making this code testable? If not, can you please file one with a full description of what would be required?

Comment 34

[Marco Zehe (:MarcoZ)]

Surkov, are you going to check this one in soon? Or should I? If so, is the last patch the good one, or did you add any other comments or stuff?

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> Surkov, are you going to check this one in soon? Or should I? If so, is the
> last patch the good one, or did you add any other comments or stuff?
> 

Sorry, I missed this. I will check in once tree is green.

Comment 36

[alexander :surkov (:asurkov)]

Checking in accessible/src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.246; previous revision: 1.245
done
Checking in accessible/src/msaa/nsAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp,v  <--  nsAccessibleWrap.cpp
new revision: 1.120; previous revision: 1.119
done
Checking in accessible/src/msaa/nsAccessibleWrap.h;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleWrap.h,v  <--  nsAccessibleWrap.h
new revision: 1.34; previous revision: 1.33
done