397112 - Crash [@ nsAccessible::GetFirstChild] moving text out of an option

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build (today's build) after 1s or so.
You need to download the testcase to your computer, because of the use of
enhanced privileges.

It doesn't seem to crash on branch (I used the branch specific code for that),
but it doesn't seem to crash there, so it might be a regression, somehow.
I can look for a regression range, if wanted.

http://crash-stats.mozilla.com/report/index/4c411ca8-688d-11dc-ae5d-001a4bd46e84
0  	nsAccessible::GetFirstChild(nsIAccessible**)  	 mozilla/accessible/src/base/nsAccessible.cpp:659
1 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
2 	AutoJSSuspendRequest::SuspendRequest() 	mozilla/js/src/xpconnect/src/xpcprivate.h:3317
3 	xul.dll@0x6aa947

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

crash is because mFirstChild is not addrefed and the object is destoyed.

Comment 2

[Mats Palmgren (inactive)]

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

This isn't the right way to go IMHO.  While holding a strong ref might
prevent some crashes it's still referring to the wrong accessible,
which can cause other obscure errors.  The root cause for this
crash is that the nsHTMLSelectOptionAccessible isn't notified that
the child (nsHTMLTextAccessible) is removed.

This crash is a recent regression,  2007082805 -- 2007082905,
which points to bug 391846.

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip1

FWIW, this adds an assert that detects this condition.

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip2

FWIW, this makes the crash go away (and the asserts from the last patch too).

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> Created an attachment (id=282048) [details]
> wip2
> 
> FWIW, this makes the crash go away (and the asserts from the last patch too).
> 

I'm not 100% sure patch is correct but you pointed right reason. We're going to invalidate container accessible after timeout but before we'll do it accessible can move through the tree and therefore we invalidate wrong accessible.

Comment 6

[alexander :surkov (:asurkov)]

Aaron, why do we need delayed events for hidden accessibles?

Comment 7

[Aaron Leventhal]

Surkov, because especially for style changes involving visibility: hidden , we get asynch notifications for every object in the subtree that's being hidden. We fire it delayed so the duplicate events can be removed from the event queue.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

do not invalidate accessible tree until actual event firing

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 282047 [details] [diff] [review]
wip1

I think it would be fine to have an assertion

Comment 11

[alexander :surkov (:asurkov)]

Attachment: comptr and assertion

use nsCOMPtr for nsIAccessible (pointer is cause of crash) and add an assertion if we get that accessible wasn't invalidated.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> (From update of attachment 282047 [details] [diff] [review])
> I think it would be fine to have an assertion
> 

Aaron, why canceled review for this assertion patch?

Comment 13

[Aaron Leventhal]

Comment on attachment 282047 [details] [diff] [review]
wip1

Okay, putting review flag back on, but "wip" usually means work in progress.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (From update of attachment 282047 [details] [diff] [review])
> Okay, putting review flag back on, but "wip" usually means work in progress.
> 

Just thought it's worth to have more assertions to see an errors.

Comment 15

[Aaron Leventhal]

Comment on attachment 282373 [details] [diff] [review]
comptr and assertion

I think we will leak with circular references if a child and parent both point to each other with strong refs.

Comment 16

[Aaron Leventhal]

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

Surkov, can you explain why this patch works?

Mats is right, we should be  calling privateContainerAccessible->InvalidateChildren() even when just hiding a node. I'm not sure why I changed that with bug 391846 which is when this regression started happening.

Comment 17

[Aaron Leventhal]

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

Actually, now I understand the patch better after thinking of it. We are still doing InvalidateChildren() when we hide the node.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> (From update of attachment 282373 [details] [diff] [review])
> I think we will leak with circular references if a child and parent both point
> to each other with strong refs.
> 

Ok, what if I say an assertion from the patch only?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> (In reply to comment #15)
> > (From update of attachment 282373 [details] [diff] [review] [details])
> > I think we will leak with circular references if a child and parent both point
> > to each other with strong refs.
> > 
> 
> Ok, what if I say an assertion from the patch only?
> 

s/say/save

Comment 20

[Aaron Leventhal]

Comment on attachment 282373 [details] [diff] [review]
comptr and assertion

Yes, I'll give r+ on the assertion but please post a clean patch for that.

Also, is the static cast necessay?

Comment 21

[alexander :surkov (:asurkov)]

Attachment: assertion

Comment 22

[alexander :surkov (:asurkov)]

Attachment: assertion

Comment 23

[Aaron Leventhal]

Checked in for Mats and Surkov.

Comment 24

[Martijn Wargers (dead)]

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