delayed accessible events problem

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access, regression})

Trunk
x86
Windows XP
access, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 261753 [details]
checked img
(Assignee)

Comment 2

12 years ago
Created attachment 261754 [details]
unchecked img
(Assignee)

Comment 3

12 years ago
Created attachment 261755 [details]
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

12 years ago
When did it break and how did it work before?
It works in Firefox 2 and I know it worked fairly recently on trunk.
Blocks: 343213
Keywords: access, regression

Updated

12 years ago
Assignee: aaronleventhal → surkov.alexander
(Assignee)

Comment 5

12 years ago
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++
(Assignee)

Comment 6

12 years ago
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

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

How did we avoid doing that before?

(Assignee)

Comment 8

12 years ago
(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

12 years ago
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?
(Assignee)

Comment 10

12 years ago
(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

12 years ago
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

12 years ago
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.
(Assignee)

Comment 13

12 years ago
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?
(Assignee)

Comment 14

12 years ago
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?).
(Assignee)

Comment 15

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

Comment 16

12 years ago
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.
(Assignee)

Comment 17

12 years ago
(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?
(Assignee)

Comment 18

12 years ago
Created attachment 264081 [details] [diff] [review]
patch [delayed events]

do not store accessible when it not necessary.
Attachment #264081 - Flags: review?(aaronleventhal)

Comment 19

12 years ago
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

12 years ago
>  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

12 years ago
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.
(Assignee)

Comment 22

12 years ago
(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.
(Assignee)

Comment 23

12 years ago
(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.

Updated

12 years ago
Attachment #264081 - Flags: review?(aaronleventhal) → review+
(Assignee)

Comment 24

12 years ago
I filed bug 380174 to have a deal with delayed accessible events.
(Assignee)

Comment 25

12 years ago
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

12 years ago
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
(Assignee)

Comment 27

12 years ago
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?
Attachment #264081 - Flags: superreview?(neil)

Comment 28

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

I can't see anything relevant, sorry.
Attachment #264081 - Flags: superreview?(neil)

Comment 29

12 years ago
See also the new crash bug 380412.
Depends on: 380482
(Assignee)

Comment 30

11 years ago
I guess the bug is fixed because regression from the bug is fixed by bug 380412.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.