Martijn Wargers (dead) Reporter
	Description • 17 years ago

See testcase, which crashes current trunk build after 500ms.
You need to download the testcase to your computer to see the crash, because of the use of enhanced privileges.

It also crashes current Firefox branch when I close the browser, so I'm marking it security sensitive because of that.

http://crash-stats.mozilla.com/report/index/ea8e8c51-6ed3-11dc-a666-001a4bd43e5c
0  	nsAccessible::GetFinalRole(unsigned int*)  	 mozilla/accessible/src/base/nsAccessible.cpp:1996
1 	nsAccessible::Role(nsIAccessible*) 	mozilla/accessible/src/base/nsAccessible.h:147
2 	nsHTMLSelectOptionAccessible::GetRole(unsigned int*) 	mozilla/accessible/src/html/nsHTMLSelectAccessible.cpp:487
3 	nsAccessible::GetFinalRole(unsigned int*) 	mozilla/accessible/src/base/nsAccessible.cpp:1996
4 	nsAccessible::Role(nsIAccessible*) 	mozilla/accessible/src/base/nsAccessible.h:147
5 	nsHTMLSelectOptionAccessible::GetRole(unsigned int*) 	mozilla/accessible/src/html/nsHTMLSelectAccessible.cpp:487
6 	nsAccessible::GetFinalRole(unsigned int*) 	mozilla/accessible/src/base/nsAccessible.cpp:1996
7 	nsAccessible::Role(nsIAccessible*) 	mozilla/accessible/src/base/nsAccessible.h:147
8 	nsHTMLSelectOptionAccessible::GetRole(unsigned int*) 	mozilla/accessible/src/html/nsHTMLSelectAccessible.cpp:487
9 	nsAccessible::GetFinalRole(unsigned int*) 	mozilla/accessible/src/base/nsAccessible.cpp:1996
10 	nsAccessible::Role(nsIAccessible*) 	mozilla/accessible/src/base/nsAccessible.h:147
etc..

	alexander :surkov (:asurkov)
	Comment 1 • 17 years ago

the idea is we should freeze the accessible tree until we fire events. Now after document is modified we try to traverse the tree and create new accessibles. Patch chop off children of accessible if they weren't initialized. Aaron, does idea looks correct?

	alexander :surkov (:asurkov)
	Comment 2 • 17 years ago

btw, the patch has a ring of bug 398205

	Aaron Leventhal
	Comment 3 • 17 years ago

> btw, the patch has a ring of bug 398205
Surkov, does it prevent the situation that originally caused us to need the null check for bug 394198?

	alexander :surkov (:asurkov)
	Comment 4 • 17 years ago

(In reply to comment #3)
> > btw, the patch has a ring of bug 398205
> Surkov, does it prevent the situation that originally caused us to need the
> null check for bug 394198?
> 

testcase from the bug 394198 doesn't crash with this patch.

	Aaron Leventhal
	Comment 5 • 17 years ago

Right, but what if you reverse the patch from bug 394198? It was a "wallpaper" null check fix. We already don't crash with the testcase from that patch, because of the temporary fix.

I want to know if this is the real fix for that one.

	alexander :surkov (:asurkov)
	Comment 6 • 17 years ago

(In reply to comment #5)
> Right, but what if you reverse the patch from bug 394198? It was a "wallpaper"
> null check fix. We already don't crash with the testcase from that patch,
> because of the temporary fix.
> 
> I want to know if this is the real fix for that one.
> 

Sure, I removed that patch before test.

	alexander :surkov (:asurkov)
	Comment 7 • 17 years ago

And I think it's worth to change NS_WARNING there on NS_ASSERTION since shell is not null there anymore.

	Aaron Leventhal
	Comment 8 • 17 years ago

Surko

	alexander :surkov (:asurkov)
	Comment 9 • 17 years ago

(In reply to comment #8)
> Surko
> 

Heh? Eventually letters are finished on your computer? :)

	Aaron Leventhal
	Comment 11 • 17 years ago

Comment on attachment 284155 [details] [diff] [review]
rough patch

1. Why do we only fire this event if cached child count == -1
2. Why do we need SetCachedChildCount()?
3. Why do we still need InvalidateChildren() in FlushPendingEvents() for SHOW/CREATE/HIDE/DESTROY
4. Could you add answers these things as comments in the code.

	alexander :surkov (:asurkov)
	Comment 13 • 17 years ago

(In reply to comment #12)
> Created an attachment (id=285097) [details]
> the same idea another way
> 

idea is to save tree until it is invalidated, so if node is pending to invalidate then we shoulnd't create accessible for it or if it exists then to initialize children of its accessible.

	Aaron Leventhal
	Comment 14 • 17 years ago

1. IsPendingToInvalidate() should probably use nsAccUtils::IsAncestorOf()

2. In the past, we would invalidate the children after we called FireAccessibleEvent() for a hide event.
Now we invalidate the children first. I'm not sure what that will do to us. We might want to fire INVALIDATE_CACHE_INTERNAL after 
we fire the hide event from InvalidateCacheSubtree(). But, that would make RefreshNodes() come first, and I'm not sure what that will change.

3. I'm not sure if we want to remove to remove event dupes with INVALIDATE_CACHE_INTERNAL, and what it will do if we do that. Related: it looks like there is a case where we will pass -1 back for GetAccChildCount. If INVALIDATE_CACHE_INTERNAL happens twice in a row.
After the first, the child count is set to mAccChildCount == eChildCountUninitialized. When CacheChildren() is called it will return that for the number of children.

4. We have more implementations of CacheChildren() that would probably need to change.

5. Nit: if we go this route I want to change the name of the event to INVALIDATE_CACHE_INTERNAL -- we have another INTERNAL method already.

6. In the following case, if accessible subtree is changing for child B
 Top
A B C
this new code is saying that A and C cannot be created. Is that what we want? 

7. What if, instead of adding a new event, we just froze the accessible subtree under a hide event. Would that fix this bug as well?

	Aaron Leventhal
	Comment 15 • 17 years ago

I'm curiois -- what was actually trying to create the accessible object before the tree was invalidated?

	alexander :surkov (:asurkov)
	Comment 16 • 17 years ago

(In reply to comment #15)
> I'm curiois -- what was actually trying to create the accessible object before
> the tree was invalidated?
> 

in tescase beginAccessible() function called after timeout. So we get something like:
1. doe1()
2. beginAccessible()
3. nsDocAccessible::flushPendingEvents()

	alexander :surkov (:asurkov)
	Comment 17 • 17 years ago

(In reply to comment #14)
> 1. IsPendingToInvalidate() should probably use nsAccUtils::IsAncestorOf()

I thought about this but IsPendingToInvalidate() starts from this node but IsAncestorOf does from parent node.

> 2. In the past, we would invalidate the children after we called
> FireAccessibleEvent() for a hide event.
> Now we invalidate the children first. I'm not sure what that will do to us. We
> might want to fire INVALIDATE_CACHE_INTERNAL after 
> we fire the hide event from InvalidateCacheSubtree(). But, that would make
> RefreshNodes() come first, and I'm not sure what that will change.

It looks correct if we will invalidate after show/hide event proccessing. That do you mean?

> 4. We have more implementations of CacheChildren() that would probably need to
> change.

Right, I rember about this but eventually I missed this.

> 5. Nit: if we go this route I want to change the name of the event to
> INVALIDATE_CACHE_INTERNAL -- we have another INTERNAL method already.

Ok.

> 6. In the following case, if accessible subtree is changing for child B
>  Top
> A B C
> this new code is saying that A and C cannot be created. Is that what we want? 

I'm not sure about A, sounds we can create accessible for it but we shouldn't intialize its children. But do we need such accessible?

Accessible for C shouldn't be created.

> 7. What if, instead of adding a new event, we just froze the accessible subtree
> under a hide event. Would that fix this bug as well?
> 

Just under show/hide events. I guess it should. I'll try.

	Aaron Leventhal
	Comment 18 • 17 years ago

I think we should chat on IRC when you're available.

	Aaron Leventhal
	Comment 19 • 17 years ago

> It looks correct if we will invalidate after show/hide event proccessing. 
> That do you mean?
I mean we currently invalidate after we fire MSAA/ATK's hide event, but before we fire the show event. We should keep it that way.

Is the basic problem with this bug that there is more than one invalidation at about the same time? The processinging of the 2nd invalidation in InvalidateCacheSubtree() can cause the subtree to be rebuilt before the DOM is ready.

>> A B C
>Accessible for C shouldn't be created.
Why?

> Just under show/hide events. I guess it should. I'll try.
I guess we still need the new event because of the problem explained in bug 398205

But, I wonder if we need to freeze the accessible tree like this for show events.

	Aaron Leventhal
	Comment 20 • 17 years ago

I think we should pull the INVALIDATE_CACHE_INTERNAL event into a separate patch and put in bug 398205. That way we can check it in to see if it's a good change on it's own.

	Aaron Leventhal
	Comment 21 • 17 years ago

I don't crash, but unfortunately I get an assertion:
###!!! ASSERTION: Two accessibles have the same first child accessible.: '!realP
arent || realParent == this', file c:/moz/mozilla/accessible/src/base/nsAccessib
le.cpp, line 691

Surkov, can you take a look?

	Aaron Leventhal
	Comment 22 • 17 years ago

Good sign, it fixes the test case in bug 394404, although I get this assertion:
###!!! ASSERTION: Creating accessible for node with no document: 'content->GetDo
cument()', file c:/moz/mozilla/accessible/src/base/nsAccessibilityService.cpp, l
ine 1293

	alexander :surkov (:asurkov)
	Comment 23 • 17 years ago

I think there is two problems:
1) you do not freeze the tree for show events

A
 C
B

Let I remove C and append it to B. With your patch C should have two parents A and B because A subtree is freezed and not invalidated. B subtree is not freezed.

2) you do not freeze the tree for random access to the tree

If I get accessible from point or for certain node (for example C after adding) then it should lead to the same stuff.

It's safely I guess to freeze the whole modified tree until we sync it again. At least because it's hard to track what's going on even for simple tests :).

	alexander :surkov (:asurkov)
	Comment 24 • 17 years ago

(In reply to comment #23)

> 2) you do not freeze the tree for random access to the tree
> 
> If I get accessible from point or for certain node (for example C after adding)
> then it should lead to the same stuff.

Ah, no, it shouldn't. But in any way I afraid this.

	alexander :surkov (:asurkov)
	Comment 26 • 17 years ago

(In reply to comment #22)
> Good sign, it fixes the test case in bug 394404, although I get this assertion:
> ###!!! ASSERTION: Creating accessible for node with no document:
> 'content->GetDo
> cument()', file c:/moz/mozilla/accessible/src/base/nsAccessibilityService.cpp,
> l
> ine 1293
> 

latest patch fixes bug 394404 too but the assertion is there.

	Aaron Leventhal
	Comment 27 • 17 years ago

nsDocAccessible::ShutDown() should clear the mNodesToInvalidate array to avoid leaks, where it does the same for mEventsToFire:
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsDocAccessible.cpp#572

isInStaleSubtree
* @param  canBeRoot  if true then the given node can be a root of stale subtrees
This is a bit hard to understand. How about: 
Having the new parameter seems like this is less of a radical change, which I like. The root of a change can still be part of normal processing, but the descendants aren't until processed the root is no longer stale.

I think the parameter name is a bit confusing. I suggest something like:
* @param mustBeDescendant
if true then the root of a changing subtree will not be considered stale
Please also add comments to all the places explaining why you pass that parameter as you do.

Nit: I suggest renaming s/isPending/isStale now because of the new method name.

	Aaron Leventhal
	Comment 28 • 17 years ago

Comment on attachment 285634 [details] [diff] [review]
one more try

r+, but I don't mind reviewing another patch with those nits fixed.

Also if you could figure out the assertion (comment 22) that would be great.

	Aaron Leventhal
	Comment 29 • 17 years ago

One more thing, please take special care to look at all the CacheChildren() methods, 
1) that they don't have a problem if they upcall to nsAccessible:CacheChildren() and mAccChildCount becomes -1, and 
2) that they check for staleness

Also for #1 check for any other callers of CacheChildren() and GetChildCount() and make sure that if a node is not shut down but returns -1 children it's still okay.

	Aaron Leventhal
	Comment 32 • 17 years ago

> // Do not initialize children unless the accessible is pending to
> // invalidate
s/unless/when ?

In nsIAccessible.idl it says
 94   /**
 95    * Number of accessible children
 96    */
 97   readonly attribute long childCount;
It doesn't say what it means if a childCount of -1 is returned
Should ::GetChildCount() return a failure code if the current element is stale and mAccChildCount is uninitialized?

I'm still concerned that the callers of GetChildCount() haven't taken that into account, that they could get -1 (or fail, which is probably what GetChildCount() should do if it only knows -1)
For example:
http://lxr.mozilla.org/seamonkey/source/accessible/src/atk/nsXULTreeAccessibleWrap.cpp#73
   mFirstChild->GetChildCount(&colCount);
  *aAccChildCount += rowCount * colCount;
If colCount is -1, and we have 100 rows, then *aAccChildCount will end up as -100. I see other similar problems elsewhere in the tree code.

Or here:
http://lxr.mozilla.org/seamonkey/source/accessible/src/msaa/nsAccessibleWrap.cpp#1069
It looks like mEnumVARIANTPosition can become 65535 because of this

Or if I'm an AT vendor, and I call:
ULONG childCount;
pAcc->get_accChildCount(&childCount);
for (count = 0; count < childCount; count ++) { ... }

	Aaron Leventhal
	Comment 33 • 17 years ago

Comment on attachment 285884 [details] [diff] [review]
patch -w

Minus because childCount can still be returned as -1

	alexander :surkov (:asurkov)
	Comment 34 • 17 years ago

If isn't initialized then return 0 and throw an exception/return failure. Sounds good?

	Aaron Leventhal
	Comment 35 • 17 years ago

I think it's good, but make sure everything that calls GetChildCount() checks the return value now.

	Aaron Leventhal
	Comment 38 • 17 years ago

Comment on attachment 286395 [details] [diff] [review]
patch2 -w

What about nsAccessibleWrap::get_accChildCount() -- shouldn't it return E_FAIL if GetChildCount() fails?

	alexander :surkov (:asurkov)
	Comment 39 • 17 years ago

(In reply to comment #38)
> (From update of attachment 286395 [details] [diff] [review])
> What about nsAccessibleWrap::get_accChildCount() -- shouldn't it return E_FAIL
> if GetChildCount() fails?
> 

Not sure it's good idea, actually there is no error if children wasn't initialized and accessible is stale. For gecko I proposed this to get more attention (probably it wasn't correct though too). But client developers can tract that E_FAIL unpredictable. I mean in gecko we can dictate our's will but we can't do it outside to require special proccessing for firefox because http://msdn2.microsoft.com/en-us/library/ms696128.aspx doesn't say a word about something similar.

	Aaron Leventhal
	Comment 40 • 17 years ago

Surkov, is the only time we need this fix when there are 2 mutations that happen at the same time, where the 2nd mutation happens before the 1st one is fired?

I'm still not sure exactly what's happening here and how this bug fixes the problem. I will fire up a debugger and investigate when I can but haven't been able to yet. If you could provide a nice, thorough description of the problem and solution that would be great. 

	Evan Yan
	Comment 41 • 17 years ago

I found a regression of the patch.

When input a first character into an entry, no text-changed event get fired.

	Aaron Leventhal
	Comment 42 • 17 years ago

Comment on attachment 286395 [details] [diff] [review]
patch2 -w

We need Evan's comment addressed and a better description of what this patch does. I'm concerned about regressions with it generally.

	Aaron Leventhal
	Comment 43 • 17 years ago

Surkov, I think this may be a much simpler way to deal with the problem. It also doesn't regress the text insertion event for the first char in an input field.

	alexander :surkov (:asurkov)
	Comment 44 • 17 years ago

(In reply to comment #43)
> Created an attachment (id=287000) [details]
> Flush event queue before operating more on tree
> 
> Surkov, I think this may be a much simpler way to deal with the problem. It
> also doesn't regress the text insertion event for the first char in an input
> field.
> 

It sounds this get rid your idea of delayed events. Iirc we use delayed events to coalesce them. Right?

	alexander :surkov (:asurkov)
	Comment 45 • 17 years ago

with fixed Evan's regression.

Evan can you look it works fine?

	alexander :surkov (:asurkov)
	Comment 48 • 17 years ago

Can you give some hints how it fixes the bug, why there is no reparenting or rechilding?

Probably it's better to put the adoption into GetAccessible() to catch all cases?

It sounds it's more correct to use CacheChildren directly not to introduce new method (I mean you might want to put it into interface).

	Aaron Leventhal
	Comment 49 • 17 years ago

It fixes a problem where you have 

    X
  /  \
C1   C2

  and it becomes

    X 
    | 
    Y 
   / \
 C1   C2

The new node Y was inserted and it needs to adopt C1 and C2. Otherwise mParent for C1 and C2 are incorrect, and mFirstChild of X is incorrect. Also, C2->mNextSi bling may need to be set to null because Y may have captured some of the children but X may have had other children.

It's only a problem when the tree mutates, otherwise the parents/children of the objects nearby are not in danger of being incorrect. That's why I did not put it in GetAccessible(). It cause a lot of unnecessary processing while walking the tree there, and we'd have to watch out for recursion.

	Evan Yan
	Comment 50 • 17 years ago

Does the latest patch still fix the regression caused by delayed invalidation?

By "delayed invalidation", I mean that we now invalidate the tree after events are fired. So if we try to query accessible after the corresponding dom node changed but before the delayed invalidate happen, we would get wrong information.

The delayed invalidation caused bug 394493 and probably bug 401395 also, those two bugs are depending on this one.

	Aaron Leventhal
	Comment 51 • 17 years ago

Evan, I'm not sure. Can you check please?

	Evan Yan
	Comment 52 • 17 years ago

(In reply to comment #51)
> Evan, I'm not sure. Can you check please?
> 
My check shows that the latest patch doesn't help the two depending bugs.

	alexander :surkov (:asurkov)
	Comment 53 • 17 years ago

Evan, what about my patch3, the same?

	Aaron Leventhal
	Comment 54 • 17 years ago

I still want the first crash fix to go in first. Then I will look at tree freezing. But, I'm very cautious about tree freezing.

	alexander :surkov (:asurkov)
	Comment 55 • 17 years ago

Aaron, do you want to put new patch?

	Aaron Leventhal
	Comment 56 • 17 years ago

Surkov, what would you like to see in the new patch?

I'd like to get this in and work on the dependencies, from this as a base. I'm not sure whether we will need tree freezing. I need to prove it to myself because it's not trivial.

	alexander :surkov (:asurkov)
	Comment 57 • 17 years ago

(In reply to comment #56)
> Surkov, what would you like to see in the new patch?

You said it's worth to try adoption after GetParentInChain in InvalidateCHildren instead of inside. And if you like to move CacheChildren into PI interface.

	Aaron Leventhal
	Comment 58 • 17 years ago

> You said it's worth to try adoption after GetParentInChain in
> InvalidateCHildren instead of inside. 
Now that I'm working on another fix that will use GetAccessibleInParentChain() with aCanCreate == PR_TRUE, I'd like to keep it in that method. I think it's safer to keep it there. Do you see a big reason to move it outside that method?

> And if you like to move CacheChildren
> into PI interface.
That's bug 398021. For now I'd like to leave it as it is.

So, seeking r+ on this patch.

	alexander :surkov (:asurkov)
	Comment 59 • 17 years ago

(In reply to comment #58)
> > You said it's worth to try adoption after GetParentInChain in
> > InvalidateCHildren instead of inside. 
> Now that I'm working on another fix that will use GetAccessibleInParentChain()
> with aCanCreate == PR_TRUE, I'd like to keep it in that method. I think it's
> safer to keep it there. Do you see a big reason to move it outside that method?

I have the one reason actually is why. I get we need this in InvalidateCachedSubtree, but GetAccessibleInParentChain() is used more widely. So do you need actually to keep an adoption inside GetAccessibleInParentChain for your future patch?

	Aaron Leventhal
	Comment 60 • 17 years ago

Actually, for the future patch I don't think it matters.

But, I will need to add a new out parameter to GetAccessibleInParentChain(), which tells me whether accessible was created or a cached one is used.

Because if it just returns an already-cached accessible we don't need to adopt children. Everything is already correct.

That seems unnecessary.

> I have the one reason actually is why. I get we need this in
> InvalidateCachedSubtree, but GetAccessibleInParentChain() is used more widely.
GetAccessibleInParentChain() is used widely but not very often with aCanCreate == PR_TRUE. Since we only do this extra work when aCanCreate == PR_TRUE, I don't understand the concern. Do we really need to move it?

	alexander :surkov (:asurkov)
	Comment 61 • 17 years ago

Comment on attachment 287150 [details] [diff] [review]
A little better at adopting children

Ok, I do not see explicit problems with the patch, excepting probably redurant  adoption in GetAccessibleInParentChain. Maybe new argument in the future patch would be nicer :)

	Martijn Wargers (dead) Reporter
	Comment 62 • 17 years ago

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111605 Minefield/3.0b2pre

	Aaron Leventhal
	Comment 63 • 17 years ago

I have backed this out because of bug 404343.

	Aaron Leventhal
	Comment 64 • 17 years ago

aab

abcc

	Aaron Leventhal
	Comment 65 • 17 years ago

This may have also caused bug 404582

	Aaron Leventhal
	Comment 66 • 17 years ago

Looks like my technique didn't work. Fortunately these crashes aren't happening in the wild, at least according to the crash-stats.

While I want to look more into both Surkov and my approaches, I'm not sure what's safe enough for Firefox 3. 

The "tree freezing" technique may well be the right approach, but I really need a document on the wiki that throroughly describes tree freezing and what it does. I'm thinking we should check that into the the trunk after Firefox 3 branches, and let it bake for a while. We may be able to take it into a point release if it proves to be the right thing.

	Jesse Ruderman
	Comment 67 • 17 years ago

This is a null dereference, right?  So not a security hole?

In general, please decide whether to mark a bug as security-sensitive based on whether the crash looks exploitable, not based on whether a crash also occurs on branch.  http://www.squarefree.com/2006/11/02/determining-whether-a-crash-looks-exploitable/ has useful hints.

	Steve Lee
	Comment 68 • 17 years ago

I'm just chipping in on a tangent as while I don't follow all the details of this a couple of comments make me wonder if you sometimes re-create accessibles? I have a couple of cases where that seems to be happening and is causing problems. 

Basically I keep a reference to an accessible so when an event occurs I can compare the source to see if it is that particular node. If it gets recreated than the comparison fails.

In the absence of good practice guidelines for AT-SPI I guess that might be a valid thing to do but it makes life harder. IMHO it is reasonable for an accessible to have a lifetime as long as the object it represents.

	Ginn Chen
	Comment 69 • 17 years ago

Steve, can you call getRole() to check if the AtkObject you ref-ed is still alive in Gecko?

	Steve Lee
	Comment 70 • 17 years ago

hi Ginn, yes I'm doing that anyway to do a property compare rather than identity check. Actually it's the top level ROLE_FRAME and Peter Parent says that is managed by the window manager not gecko. I had another case but worked round that so I'll try to find it again.

	Ginn Chen
	Comment 71 • 17 years ago

ROLE_FRAME of Firefox window should be managed by Gecko not libgail.

	Steve Lee
	Comment 72 • 17 years ago

(In reply to comment #71)
> ROLE_FRAME of Firefox window should be managed by Gecko not libgail.

I'm trying to reproduce it again now.

	Mike Schroepfer
	Comment 73 • 16 years ago

Comment on attachment 287150 [details] [diff] [review]
A little better at adopting children

clearing the approval flag since this got backed out

	Daniel Veditz [:dveditz]
	Comment 74 • 16 years ago

This is a stack overflow, doesn't need to remain hidden.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 75 • 15 years ago

Mass un-assigning bugs assigned to Aaron.

	alexander :surkov (:asurkov)
	Comment 76 • 14 years ago

worksforme on 22/10/2010 (local build)

	alexander :surkov (:asurkov)
	Comment 77 • 14 years ago

Can anybody with rights cc me on bug 306663 that blocks this one?

	alexander :surkov (:asurkov)
	Comment 78 • 14 years ago

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

doesn't make any sense for Firefox 4.