477551 - nsDocAccessible::FlushPendingEvents isn't robust

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Marking blockin as bug 472809 (meta mutaiton events) though this bug is not mutation events only.

We fire delayed events and process them nsDocAccessible::FlushPendingEvents and fire a11y events. If there is a11y event listener that is cause of delayed events then these events won't be fired. Scenario is

1) collect delayed events
2) process them (in FlushPedningEvents)
2.1) cache count of collected delayed events
2.2) run through events
2.3) fire event
3) if handler for fired event is cause of new delayed events then
3.1) collect delayed events
3.3) do not start their processing because it is started already
4) initial event processing is finished and clear all collected events, that's the reason we won't process newly collected events.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Assuming I understand this bug, I wonder if having 2 (or more) event collections would be a solution here.

We could collect events on a temporary collection while processing (#2 above), and after flushing (#4 above), copy them over (or keep a pointer to the current one and alternate).

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> Assuming I understand this bug, I wonder if having 2 (or more) event
> collections would be a solution here.
> 
> We could collect events on a temporary collection while processing (#2 above),
> and after flushing (#4 above), copy them over (or keep a pointer to the current
> one and alternate).

Yes, that would work. Another way that should work is remove processed events only.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[Marco Zehe (:MarcoZ)]

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

r=me for the test part.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

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

r=me thanks. Probably best to still get Ginn's review on this.

>+      // Kung fu death grip to prevent crash in callback.

We should probably advance to Brazilian Jujitsu at some point.

> void
> nsDocAccessible::FlushPendingEvents()

>+  // Process events existed in the queue at this moment. Do not process any
>+  // newly appended events.

Maybe:
// Process only currently queued events. In the meantime, newly appended events will not be processed.

>   for (PRUint32 index = 0; index < length; index ++) {
>-    nsCOMPtr<nsIAccessibleEvent> accessibleEvent(
>-      do_QueryInterface(mEventsToFire[index]));
>+    nsCOMPtr<nsIAccessibleEvent> accessibleEvent =
>+      do_QueryInterface(mEventsToFire[0]);
>+    mEventsToFire.RemoveObjectAt(0);

I wonder how expensive RemoveObjectAt is. I'd like to know how big our queue
gets. I can do some perf tests of this algorithm and a dual queue approach, but
that needn't hold up this patch since I can do a follow up if necessary.


> protected:
>-    PRBool mIsAnchor;
>-    PRBool mIsAnchorJumped;
>-    PRBool mInFlushPendingEvents;
>+  PRBool mIsAnchor;
>+  PRBool mIsAnchorJumped;
>+
>+  PRBool mInFlushPendingEvents;
>+  PRBool mFireEventTimerStarted;
>+

Nit: probably nicer to follow the existing file indenting.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> I wonder how expensive RemoveObjectAt is. I'd like to know how big our queue
> gets. I can do some perf tests of this algorithm and a dual queue approach, but
> that needn't hold up this patch since I can do a follow up if necessary.

they use memmove, it sounds it copies elements so it might be slow. The size of the queue depends on the page. It might be big (hundreds I think) in DoJo case for example. We could remove them all together but neither nsCOMArray nor underlying nsVoidArray provides method for this. Let's ask Neil for sr, it might be he will help.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (From update of attachment 398363 [details] [diff] [review])

> 
> Nit: probably nicer to follow the existing file indenting.

there is no existing file indenting, somewhere it is 2 spaces, somewhere it is 4. So I would suggest to have 2 spaces where we are around. I could remove those indenting changes for mAnchor variables.

Comment 8

[Ginn Chen]

I have same concern of the performance of RemoveObjectAt.

Can we use RemoveElementsAt()?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> I have same concern of the performance of RemoveObjectAt.
> 
> Can we use RemoveElementsAt()?

Do you suggest to extend nsCOMArray to introduce removeObjectsAt method (redirection to nsVoidArray:: RemoveElementsAt)?

Comment 10

[[e-mail]]

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

>+  // Process events existed in the queue at this moment. Do not process any
>+  // newly appended events.
>   for (PRUint32 index = 0; index < length; index ++) {
>-    nsCOMPtr<nsIAccessibleEvent> accessibleEvent(
>-      do_QueryInterface(mEventsToFire[index]));
>+    nsCOMPtr<nsIAccessibleEvent> accessibleEvent =
>+      do_QueryInterface(mEventsToFire[0]);
>+    mEventsToFire.RemoveObjectAt(0);
What you could try is
nsCOMPtr<nsIAccessibleEvent> accessibleEvent(mEventsToFire[index]);
if (!accessibleEvent) continue;
mEventsToFire.ReplaceObjectAt(nsnull, index);
...
>-  mEventsToFire.Clear(); // Clear out array
if (mEventsToFire.Length() == length)
  mEventsToFire.Clear(); // No new events, safe to clear out array

Otherwise, you could change mEventsToFire into an nsTArray<nsCOMPtr<nsIAccessibleEvent> > which would let you do mEventsToFire.RemoveElementsAt(0, length); instead. I'm not sure how general performance compares with an nsCOMArray, but a slight optimisation would be to use nsIAccessibleEvent *accessibleEvent = mEventsToFire[index]; which wouldn't be possible with the first idea as you're nulling out the array's reference.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> (From update of attachment 398363 [details] [diff] [review])
> >+  // Process events existed in the queue at this moment. Do not process any
> >+  // newly appended events.
> >   for (PRUint32 index = 0; index < length; index ++) {
> >-    nsCOMPtr<nsIAccessibleEvent> accessibleEvent(
> >-      do_QueryInterface(mEventsToFire[index]));
> >+    nsCOMPtr<nsIAccessibleEvent> accessibleEvent =
> >+      do_QueryInterface(mEventsToFire[0]);
> >+    mEventsToFire.RemoveObjectAt(0);
> What you could try is
> nsCOMPtr<nsIAccessibleEvent> accessibleEvent(mEventsToFire[index]);
> if (!accessibleEvent) continue;
> mEventsToFire.ReplaceObjectAt(nsnull, index);
> ...
> >-  mEventsToFire.Clear(); // Clear out array
> if (mEventsToFire.Length() == length)
>   mEventsToFire.Clear(); // No new events, safe to clear out array

That won't be good, since it's not good to have null values if this condition is not fulfilled because every time when new event is appeared in the queue then we traverse whole queue to coalesce dupe events, that would be mean we will traverse through null values.

> Otherwise, you could change mEventsToFire into an
> nsTArray<nsCOMPtr<nsIAccessibleEvent> > which would let you do
> mEventsToFire.RemoveElementsAt(0, length); instead. I'm not sure how general
> performance compares with an nsCOMArray, but a slight optimisation would be to
> use nsIAccessibleEvent *accessibleEvent = mEventsToFire[index]; which wouldn't
> be possible with the first idea as you're nulling out the array's reference.

That might be a best solution, though I didn't realized yet whether nsCOMPtr will be nulled when it is removed from the array.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> That might be a best solution, though I didn't realized yet whether nsCOMPtr
> will be nulled when it is removed from the array.

Ah, it sounds nsTArrayElementTraits will care to construct/destruct nsCOMPtrs.

Comment 13

[alexander :surkov (:asurkov)]

Neil, could you give some info about array resizing please? How is it optimal to add elements to the array one by one? Wouldn't it lead to array resizing and copying of the array elements each time?

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 15

[[e-mail]]

Comment on attachment 399040 [details] [diff] [review]
patch2

>+    nsCOMPtr<nsIAccessibleEvent> accessibleEvent =
>+      do_QueryInterface(mEventsToFire[index]);
Isn't mEventsToFire[index] already an nsCOMPtr<nsIAccessibleEvent>? In which case, you could just assign it.

Comment 16

[[e-mail]]

(In reply to comment #13)
> Neil, could you give some info about array resizing please? How is it optimal
> to add elements to the array one by one? Wouldn't it lead to array resizing and
> copying of the array elements each time?
nsTArray and nsVoidArray have slightly different reallocation strategies; nsTArray just doubles all the time, while nsVoidArray grows by 8 elements at a time until 24, then doubling until 1024, then by 1024 elements at a time.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> (From update of attachment 399040 [details] [diff] [review])
> >+    nsCOMPtr<nsIAccessibleEvent> accessibleEvent =
> >+      do_QueryInterface(mEventsToFire[index]);
> Isn't mEventsToFire[index] already an nsCOMPtr<nsIAccessibleEvent>? In which
> case, you could just assign it.

right, I'll fix this. thank you.

Comment 18

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/c51f70b22873