377728 - delayed accessible events problem

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

I'll attach testcase to demonstrate the problem. Sometimes when we fire delayed accessible event then accessible is shutdowned before the event is processed. And therefore we can't fire accessible event. That is because InvalidateCacheSubtree is called and it destroys an accessible. So,
1) either we should rethink how InvalidateCacheSubtree is works
2) or we should recreate accessible in accessible event when event is processed.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: checked img

Comment 2

[alexander :surkov (:asurkov)]

Attachment: unchecked img

Comment 3

[alexander :surkov (:asurkov)]

Attachment: testcase

the original testcase is http://www.mozilla.org/access/dhtml/checkbox

steps to reproduce:
1) set breakpoint on nsAccessNode::Shutdown()
2) click on checkbox

I guess checkbox image is changed and it's cause of InvalidateCacheSubtree is called and accessible for checkbox is shutdowned.

How can it be fixed?

Comment 4

[Aaron Leventhal]

When did it break and how did it work before?
It works in Firefox 2 and I know it worked fairly recently on trunk.

Comment 5

[alexander :surkov (:asurkov)]

stack trace

>	accessibility.dll!nsDocAccessible::InvalidateCacheSubtree(nsIContent * aChild=0x044123b8, unsigned int aChangeEventType=0x00000005)  Line 1349	C++
 	accessibility.dll!nsAccessibilityService::InvalidateSubtreeFor(nsIPresShell * aShell=0x041cb648, nsIContent * aChangeContent=0x044123b8, unsigned int aEvent=0x00000005)  Line 1621 + 0x1f	C++
 	gklayout.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent=0x044123b8)  Line 11133	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList & aChangeList={...})  Line 9922	C++
 	gklayout.dll!nsCSSFrameConstructor::RestyleElement(nsIContent * aContent=0x044123b8, nsIFrame * aPrimaryFrame=0x04428b58, nsChangeHint aMinHint=0x00000000)  Line 9986	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessOneRestyle(nsIContent * aContent=0x044123b8, nsReStyleHint aRestyleHint=eReStyle_Self, nsChangeHint aChangeHint=0x00000000)  Line 12874	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessPendingRestyles()  Line 12927	C++
 	gklayout.dll!nsCSSFrameConstructor::RestyleEvent::Run()  Line 12998	C++

Comment 6

[alexander :surkov (:asurkov)]

When I click on ARIA checkbox (html:span) then style is changed:

.checkbox:before {
  content: url('https://bugzilla.mozilla.org/attachment.cgi?id=261754')
}
.checkbox[aaa|checked="true"]:before {
  content: url('https://bugzilla.mozilla.org/attachment.cgi?id=261753')
}

and RecreateFramesForContent() is called for html:span and we invalidate it. Why do we shutdown top accessible for reorder event?

Comment 7

[Aaron Leventhal]

> Why do we shutdown top accessible for reorder event?
I'm not sure that we should.

How did we avoid doing that before?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> > Why do we shutdown top accessible for reorder event?
> I'm not sure that we should.
> 
> How did we avoid doing that before?
> 

It looks it's regression from bug 374711.

+nsAccStateChangeEvent::
+  nsAccStateChangeEvent(nsIDOMNode *aNode,
+                        PRUint32 aState, PRBool aIsExtraState):
+  nsAccEvent(::nsIAccessibleEvent::EVENT_STATE_CHANGE, aNode, nsnull),
+  mState(aState), mIsExtraState(aIsExtraState)
+{
+  nsCOMPtr<nsIAccessible> accessible;
+  GetAccessible(getter_AddRefs(accessible));
+  if (accessible) {
+    PRUint32 state = 0, extraState = 0;
+    accessible->GetFinalState(&state, &extraState);
+    mIsEnabled = ((mIsExtraState ? extraState : state) & mState) != 0;
+  } else {
+    mIsEnabled = PR_FALSE;
+  }

Here I initialize mAccessible then this accessible is destroyed. Previously mAccessible was intitialized when event is processed (after reorder event). I can roll back and do not initalize mAccessible or I can change InvalidateCacheSubtree logic and do not destroy accessible that gets REORDER event. So 1) is safely 2) is right? :)

Comment 9

[Aaron Leventhal]

For delayed events pass a DOM node and don't create the accessible until you actually need it, to fire the event or whatever. Does that make sense?

Although, are we really destroying an accessible for a REORDER event? We should be destroying its children I think, and doing InvalidateChildren() on it. But, it doesn't need to be destroyed.

When we call into the invalidation from layout and we don't know it's a SHOW/HIDE, we fire REORDER. But, reorder should be on the container, whereas show/hide should be on the child. Do we get that right?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> When we call into the invalidation from layout and we don't know it's a
> SHOW/HIDE, we fire REORDER. But, reorder should be on the container, whereas
> show/hide should be on the child. Do we get that right?
> 

1) PresShell::RecreateFramesFor fires reorder event for content. It looks it should be for content parent.

2) PresShell::RecreateFramesFor calls nsCSSFrameConstructor::ProcessRestyledFrames that calls nsCSSFrameConstructor::RecreateFramesForContent that will fire HIDE/SHOW/REORDER events. If there is no new frame for content then reorder event is fired for content. Should it be parent content again?

Aaron, please comment this.

Comment 11

[Aaron Leventhal]

I think we fire REORDER when the frame is changing from one type to another, but is still visible. Correct? Are there any other times?

The work of going up to the parent for the REORDER event should be in our code. If we decide to do that, the layout should have these 3 choices:
CHANGE_SHOW, CHANGE_HIDE, CHANGE_OTHER
The event would always be fired on the changed node.

Then in our InvalidateSubtree() we can fire internal 
nsIAccessibleEvent:: CHANGE_SHOW, CHANGE_HIDE and CHANGE_OTHER

Then in msaa/nsAccessibleWrap::FireAccessibleEvent() we can map it like this:
- CHANGE_SHOW to SHOW + REORDER on parent
- CHANGE_HIDE to HIDE + REORDER on parent
- CHANGE_OTHER to REORDER on parent

How does that sound?

Comment 12

[Aaron Leventhal]

So in other words, we would no longer have REORDER in nsIAccessibleEvent, only in MSAA.

For ATK, the mapping would be:
- CHANGE_SHOW to children-changed:add on parent
- CHANGE_HIDE to children-changed:remove on parent
- CHANGE_OTHER to children-changed or maybe children-changed:add with index = -1 (as we do now)

For notices via nsIObserver they would get those 3 new nsIAccessibleEvent types.

Comment 13

[alexander :surkov (:asurkov)]

Sounds good. So in this bug
1) do not shutdown for reorder event in InvalidateCachedSubtree
2) fix nsAccessibleEvent to don't initialize accessible

Atk changes can be fixed in another bug (we have two open for that).

Ok?

Comment 14

[alexander :surkov (:asurkov)]

Though internally we fire REORDER event when for example attribute is changed (like role or onclick) and therefore we need to recreate accessible and actually it's not add or remove events (though probably remove+add?).

Comment 15

[alexander :surkov (:asurkov)]

Though RecreateFramesForContent means frames was changed and therefore accessible may be changed too. No?

Comment 16

[Aaron Leventhal]

Though RecreateFramesForContent means frames was changed and therefore
accessible may be changed too. No?
Yes.

I think we need to separate the use of REORDER, which means children are changing from EVENT_CHANGE_OTHER, with means the type of child accessible may change, but not a show or hide.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> I think we need to separate the use of REORDER, which means children are
> changing from EVENT_CHANGE_OTHER, with means the type of child accessible may
> change, but not a show or hide.
> 

Sorry, not sure I caught you. If I get right then we fire REORDER event when node is changed (not shown or hidden) so that its accessible may be changed. In that case we always should invalidate accessible tree for that accessible and then we should notify platfrom listeners that parent accessible reordered. So do you mean we should rename REORDER event to CHANGE event and use REORDER event to fire it for parent accessibes only?

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch [delayed events]

do not store accessible when it not necessary.

Comment 19

[Aaron Leventhal]

A REORDER should be fired any time an object has changes how many or which children it has, or the order of those children.

If an accessible node changes from one kind to another there is no special event to fire on that object, but we would fire a REORDER on its parent. But, we also should fire MSAA REORDER on a parent when a child is show or hidden (which we do).

The problem is we are using REORDER to mean 2 things. 
1) A change (not show or hide) on a node (it means this when we call into a11y service from layout)
2) A change in an objects' children (it means this when we fire the event from inside a11y)

I should not have used REORDER on the object when calling the a11y service from layout (#2). It's a confusing thing to do, since the REORDER is really happening on the parent of that object.

Comment 20

[Aaron Leventhal]

>  So do you mean we should rename REORDER event to CHANGE event and use REORDER
> event to fire it for parent accessibes only?

>  So do you mean we should rename REORDER event to CHANGE event.
Yes, for nsIAccessibleEvent.

Yes, but we should only fire MSAA REORDER from inside accessible/src/msaa, and not deal with it inside accessible/src/base.
Inside Wrap classes, when it receives:
SHOW: fire SHOW plus REORDER on parent
HIDE: fire HIDE plus REORDER on parent
CHANGE: fire REORDER on parent

Comment 21

[Aaron Leventhal]

Comment on attachment 264081 [details] [diff] [review]
patch [delayed events]

Is mIsEnabled being calculated too early?

Should mIsEnabled be removed and instead be provided as a GetIsEnabled()? That way it can be calculated when the event is finally fired.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> (From update of attachment 264081 [details] [diff] [review])
> Is mIsEnabled being calculated too early?

no, while it works :)

> Should mIsEnabled be removed and instead be provided as a GetIsEnabled()? That
> way it can be calculated when the event is finally fired.
> 

Not sure because GetIsEnabled() will be shortcut of accessilbe->state&DESIRED_STATE.

Probably we need some concept of delayed events because it looks delayed event is not just event that get fired a bit later. It looks delayed event should be initialized (accessible related part) when event is fired. So we need to have some special interface for delayed events creation or something like.

We can pass this patch and file new common bug for delayed events interface.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)

> Probably we need some concept of delayed events because it looks delayed event
> is not just event that get fired a bit later. It looks delayed event should be
> initialized (accessible related part) when event is fired. So we need to have
> some special interface for delayed events creation or something like.

Probalby accessible event should have a mehtod Initialize() that should be called from FireAccessibleEvent() so that we can do not distinguish delayed events from usual events.

Comment 24

[alexander :surkov (:asurkov)]

I filed bug 380174 to have a deal with delayed accessible events.

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 264081 [details] [diff] [review]
patch [delayed events]

checked in.

leaving this bug open until reorder/change events won't have own bug (probably one among of exisiting bugs).

Comment 26

[Evan Yan]

there is a crash regression seems caused by this check in.

it can be simply reproduced by selecting another tab in pref pane.

core stack:

 fef2e6c5 _lwp_kill (1, b) + 15
 feee6922 raise    (b, 0) + 22
 fedcb68e void nsProfileLock::FatalSignalHandler(int) (b, 0, 804615c) + 16e
 fef2d1bf __sighndlr (b, 0, 804615c, fedcb520) + f
 fef226ab call_user_handler (b, 0, 804615c) + 2b8
 fef22852 sigacthandler (b, 0, 804615c) + c2
 --- called from signal handler with signal 11 (SIGSEGV) ---
 febaaae1 NS_LogCOMPtrAddRef_P (95ad560, f80536d4) + 41
 fead5718 void nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) (95ad560, f80536d4) + 48
 f7ee9331 nsCOMPtr<nsIAccessible>&nsCOMPtr<nsIAccessible>::operator=(const already_AddRefed<nsIAccessible>&) (95ad560, 8046408) + 31
 f7ee7440 unsigned nsAccEvent::GetAccessible(nsIAccessible**) (95ad550, 80464b4) + 90
 f7ef116d unsigned nsDocAccessible::FlushPendingEvents() (94b8f38, 0) + 15d
 f7ef15e6 void nsDocAccessible::FlushEventsCallback(nsITimer*,void*) (95df7b8, 94b8f90) + 66
 feb96f07 void nsTimerImpl::Fire() (95df7b8, 0) + 2a7
 feb9713e unsigned nsTimerEvent::Run() (95b6a10, 0) + 10e
 feb8d90b unsigned nsThread::ProcessNextEvent(int,int*) (809c7b0, 1, 8046654) + 28b

Comment 27

[alexander :surkov (:asurkov)]

Comment on attachment 264081 [details] [diff] [review]
patch [delayed events]

Neil, can you look is there here potential problems that leads to crash described in comment #26?

Comment 28

[[e-mail]]

Comment on attachment 264081 [details] [diff] [review]
patch [delayed events]

I can't see anything relevant, sorry.

Comment 29

[Aaron Leventhal]

See also the new crash bug 380412.

Comment 30

[alexander :surkov (:asurkov)]

I guess the bug is fixed because regression from the bug is fixed by bug 380412.