561932 - Make text events async/safe

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

Child of bug 423737.

We need to make our text events async where possible, and probably use static analysis otherwise. We may need to push changes down into desktop best practices in order to make our changes without breaking anyone.

I think Alexander looked at making our text events async a while back -- should be tricky. Alex can you brain dump into this bug? (If you remember)

Comment 1

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

Hmmm note:
http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/interface_i_accessible_text.html#746f479b142add18175d6c63f953cf4a

"Servers only need to save the last removed block of text and a scope of the entire application is adequate"

Comment 2

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

Attachment: early WIP (doesn't address char data changed)

Comment 3

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

I'm not sure the IA2 requirement to supply removed text in a text remove event is a good one. The fact that servers are only required to save the last removed block doesn't help us much I don't think; since we can't predict the future we'd have to be constantly updating the saved string data anways. I guess it saves memory. I'm going to find out who uses this API.

Comment 4

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

Anyways, I'll keep the old text implementation we have for now since that is a bit orthogonal to this bug. I've made the other text change event in docaccessible async locally. I'll look at this a bit more and post a patch.

Comment 5

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

Attachment: patch 1 (just add text events to the queue)

Comment 6

[alexander :surkov (:asurkov)]

I don't think we need to fire delayed events when delayed events are processed. Please find all bugs that were fixed and might be affected by this bug and ask Marco to test all of them. Then when we'll be sure we are in good shape, mochitests should be added.

Comment 7

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

(In reply to comment #6)
> I don't think we need to fire delayed events when delayed events are processed.

Of course - thanks for catch.

> Please find all bugs that were fixed and might be affected by this bug and ask
> Marco to test all of them. Then when we'll be sure we are in good shape,
> mochitests should be added.

I'll combine this and the related bugs in a try build when ready.

Comment 8

[[e-mail] (:bhearsum)]

(marked sec-sen at davidb's request)

Comment 9

[Damon Sicore (:damons)]

Bolter, can you comment as to why this is security sensitive?  Exploitable how?

Comment 10

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

(In reply to comment #9)
> Bolter, can you comment as to why this is security sensitive?  Exploitable how?

The concern is that nsContentUtils::IsSafeToRunScript can return false for cases where we are firing events synchronous events. Malicious code could reside in-process (via dll injection) and cause a crash. I think the crash exploit would need to be pretty sophisticated, but not sure of details.

(Our text events are marked unsafe here: https://intranet.mozilla.org/Accessibility/SafeA11yEvents)

...I just realized I should add some debug checks for nsContentUtils::IsSafeToRunScript to confirm.

Comment 11

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

Attachment: patch 2

As per the updated wiki,

From nsIMutationObserver, CharacterDataWillChange and CharacterDataChanged fire text text events via, nsDocAccessible::FireTextChangeEventForText.

- Fixed.

nsDocAccessible::InvalidateCacheSubtree:

- Fixed.

Comment 12

[alexander :surkov (:asurkov)]

What's about comment #7 (list of possible affected bugs and try-build)?

Also one of related bugs was we invalidate accessible children for sync text change event. It might be not need any more. Ideally we shouldn't invalidate children until all events (mutation and text change events) are about to fire to AT. That's should be investigated as well.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)

> Also one of related bugs was we invalidate accessible children for sync text
> change event. It might be not need any more. Ideally we shouldn't invalidate
> children until all events (mutation and text change events) are about to fire
> to AT. That's should be investigated as well.

Let me clarify: otherwise we might fire useless events for AT.

Comment 14

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

Comment on attachment 445403 [details] [diff] [review]
patch 2

(In reply to comment #12)
> What's about comment #7 (list of possible affected bugs and try-build)?

Yep, I should have requested f?

> Also one of related bugs was we invalidate accessible children for sync text
> change event. It might be not need any more.

If it was in order to get the correct text I think we would still need it. Can you point me to the bug?


> Ideally we shouldn't invalidate
> children until all events (mutation and text change events) are about to fire
> to AT. That's should be investigated as well.

That feels like a separate bug. Also I think it overlaps what Boris is looking into.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (From update of attachment 445403 [details] [diff] [review])
> (In reply to comment #12)
> > What's about comment #7 (list of possible affected bugs and try-build)?
> 
> Yep, I should have requested f?

> > Also one of related bugs was we invalidate accessible children for sync text
> > change event. It might be not need any more.
> 
> If it was in order to get the correct text I think we would still need it. Can
> you point me to the bug?

Since the code we have here is rather set of bug fixes than well designed code then all I can do is to look the history to understand which change is for which fix. On the another hand the best way to test it is look through history and find related bugs since this code isn't covered by mochitests. Therefore we need to find related bugs with this change. I hoped to get you for this work as assignee.

> > Ideally we shouldn't invalidate
> > children until all events (mutation and text change events) are about to fire
> > to AT. That's should be investigated as well.
> 
> That feels like a separate bug. Also I think it overlaps what Boris is looking
> into.

Yes I think it overlaps with event coalescing problems that Boris helps us to figure out. But I'm fine to deal with this in other bugs until this bug doesn't make regressions. I just afraid this problem might be not so easy as your patch.

So let's ensure this patch is well tested, no regressions and go ahead to land it.

Comment 16

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

(In reply to comment #15)
> (In reply to comment #14)
> So let's ensure this patch is well tested, no regressions and go ahead to land
> it.

Sounds like a plan! :)

Comment 17

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

https://build.mozilla.org/tryserver-builds/[e-mail]-try-365486b7a857/

Includes this patch and bug 564471 (patch 3).

Comment 18

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

Marco can you give the try build in comment 17 a spin? We can coordinate cases over IRC.

Comment 19

[Marco Zehe (:MarcoZ)]

I tested live regions, docs with frames and embedded iframes, and also pasting amounts of text or deleting it, and don't experience either delays nor missing information so far.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> I tested live regions, docs with frames and embedded iframes, and also pasting
> amounts of text or deleting it, and don't experience either delays nor missing
> information so far.

Great! What platforms, what ATs?

David, I still think we need to find bugs from cvs history related with these changes and ask Marco to test them. But I'm happy it doesn't affect on everything.

Comment 21

[Marco Zehe (:MarcoZ)]

I tested on Windows with both NVDA and JAWS. So used one that consumes MSAA/IA2 and one that uses iSimpleDom.

Comment 22

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

Thanks Marco.

(In reply to comment #20)
> (In reply to comment #19)
> David, I still think we need to find bugs from cvs history related with these
> changes and ask Marco to test them. But I'm happy it doesn't affect on
> everything.

Yeah I searched back to bug 371394 where the events were introduced and fired sync. I couldn't find bugs specific to a requirement they be sync.

I'll add some additional mochitest coverage for text change events. Good to be careful.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> I tested on Windows with both NVDA and JAWS. So used one that consumes MSAA/IA2
> and one that uses iSimpleDom.

Orca?

(In reply to comment #22)
> Yeah I searched back to bug 371394 where the events were introduced and fired
> sync. I couldn't find bugs specific to a requirement they be sync.

I'll try to look tomorrow.

Comment 24

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

(In reply to comment #23)
> (In reply to comment #22)
> > Yeah I searched back to bug 371394 where the events were introduced and fired
> > sync. I couldn't find bugs specific to a requirement they be sync.
> 
> I'll try to look tomorrow.

Great. Thanks!

Comment 25

[Marco Zehe (:MarcoZ)]

I've tested this with Orca also, and it does not appear to have any bad influence on it, either. All above tested examples also work in Orca.

Comment 26

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

Thanks Marco. Alexander, I've thought about it some but am not sure what to put under mochitest here that would be specific to this bug. Did you find any historical, related issues that we should test?

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> Thanks Marco. Alexander, I've thought about it some but am not sure what to put
> under mochitest here that would be specific to this bug.

Yes, nothing specific I can think of, at least until we find a regression. It's nice to have mochitests here, in general, but it would be a big work.

> Did you find any
> historical, related issues that we should test?

Sorry, I've started but I haven't finished yet.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> Did you find any
> historical, related issues that we should test?

No. As I figured out I was keeping in mind several bugs but they were related rather with invalidation and caret move events than with text events.

I think it should be ok to land this patch.

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 445403 [details] [diff] [review]
patch 2

>     nsRefPtr<nsAccEvent> event =
>       new nsAccTextChangeEvent(accessible, offset,
>                                renderedEndOffset - renderedStartOffset,
>                                aIsInserted, PR_FALSE);
>-    nsEventShell::FireEvent(event);
>+    FireDelayedAccessibleEvent(event);

Please make sure we create an event for DOM node. We don't use an accessible as delayed event target.
 
>       nsRefPtr<nsAccEvent> textChangeEvent =
>         CreateTextChangeEventForNode(containerAccessible, childNode, childAccessible,
>                                      PR_FALSE, isAsynch);
>       if (textChangeEvent) {
>-        nsEventShell::FireEvent(textChangeEvent);
>+        FireDelayedAccessibleEvent(textChangeEvent);

the same.

Comment 30

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

Attachment: patch 3

As per IRC I have added the eAllowDupes rule so that we short-circuit our coalescence for delayed text events. This alleviates the need to initialize and compare based on event->mNode. This is probably as close to functional parity we can get while making these two cases of text events safe.

Will post the try build link.

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 448372 [details] [diff] [review]
patch 3


>   nsAccEvent(aIsInserted ? nsIAccessibleEvent::EVENT_TEXT_INSERTED : nsIAccessibleEvent::EVENT_TEXT_REMOVED,
>-             aAccessible, aIsAsynch, aIsFromUserInput),
>+             aAccessible, aIsAsynch, aIsFromUserInput, eAllowDupes),
>   mStart(aStart), mLength(aLength), mIsInserted(aIsInserted)

It would be nice to have comment about hardcoded eAllowDupes.


>+    // Use the accessible since we have one, and we don't coalesce text events

comment like below

>+      // We use the accessible to create the event since we have one already and
>+      // we do not coalesce based on node for text change events

"since we have one already" is not a reason.

If you don't find cute to copy/paste the same comment over several places then you can add cross references between event firing and event construction code.

Comment 32

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

(In reply to comment #31)
> (From update of attachment 448372 [details] [diff] [review])
> >+      // We use the accessible to create the event since we have one already and
> >+      // we do not coalesce based on node for text change events
> 
> "since we have one already" is not a reason.

I mean it is no additional work/time.

> 
> If you don't find cute to copy/paste the same comment over several places then
> you can add cross references between event firing and event construction code.

Cross references? Do you mean something like "see event construction"?

Sometimes source comments are the hardest part :)

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)
> (In reply to comment #31)
> > (From update of attachment 448372 [details] [diff] [review] [details])
> > >+      // We use the accessible to create the event since we have one already and
> > >+      // we do not coalesce based on node for text change events
> > 
> > "since we have one already" is not a reason.
> 
> I mean it is no additional work/time.

I meant DOM node usage might a problem here. Now I see if the problem exists (if somebody shutdown the accessible) then it exists in either way. The difference in your case you don't get an assertion when you're trying to create an accessible for removed DOM node. So basically you could put this into comment. 

> > 
> > If you don't find cute to copy/paste the same comment over several places then
> > you can add cross references between event firing and event construction code.
> 
> Cross references? Do you mean something like "see event construction"?

Yes, like "fire delayed event see event construction for details" and refer to here from event construction. This is something we should address later.

> Sometimes source comments are the hardest part :)

yes.

Comment 34

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

Marco can you give this a spin like you did in comment 19? http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-eee4f2f8ad4d/tryserver-win32-debug/

Comment 35

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

Attachment: patch 3.1 (improved comment)

Comment 36

[alexander :surkov (:asurkov)]

Comment on attachment 448405 [details] [diff] [review]
patch 3.1 (improved comment)


>+// Note: we pass in eAllowDupes to the base class because we don't support text
>+// events coalescence. We fire delayed text change events in nsDocAccessible but
>+// we continue to base the event off the accessible object rather than just the
>+// node. This means we won't try to create an accessible based on the node when
>+// we are ready to fire the event and so we will no longer assert at that point
>+// if the node was removed from the document. Either way, the AT won't work with
>+// a defunct accessible so the behaviour should be equivalent

nit: dot in the end of sentence :).

Comment 37

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

Updated locally you cheeky monkey.

Comment 38

[Marco Zehe (:MarcoZ)]

The try-server build from comment #34 is also working fine for me.

Comment 39

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

I hope to land tonight/tomorrow

Comment 40

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

Landed http://hg.mozilla.org/mozilla-central/rev/08d167d1dd30