Make text events async/safe

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: davidb, Assigned: davidb)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][critsmash:patch])

Attachments

(1 attachment, 4 obsolete attachments)

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)
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"
Created attachment 443690 [details] [diff] [review]
early WIP (doesn't address char data changed)
Assignee: nobody → bolterbugz
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.
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.
Created attachment 444119 [details] [diff] [review]
patch 1 (just add text events to the queue)
Attachment #443690 - Attachment is obsolete: true
Attachment #444119 - Flags: review?(surkov.alexander)
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.
(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.
Group: core-security
(marked sec-sen at davidb's request)
Bolter, can you comment as to why this is security sensitive?  Exploitable how?
(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.
Created attachment 445403 [details] [diff] [review]
patch 2

As per the updated wiki,

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

- Fixed.

nsDocAccessible::InvalidateCacheSubtree:

- Fixed.
Attachment #444119 - Attachment is obsolete: true
Attachment #445403 - Flags: review?(surkov.alexander)
Attachment #444119 - Flags: review?(surkov.alexander)
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.
(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 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.
Attachment #445403 - Flags: review?(surkov.alexander)
(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.
(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! :)
Marco can you give the try build in comment 17 a spin? We can coordinate cases over IRC.
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.
(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.
I tested on Windows with both NVDA and JAWS. So used one that consumes MSAA/IA2 and one that uses iSimpleDom.
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.
(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.
(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!

Updated

9 years ago
Whiteboard: [sg:critical?][critsmash:investigating]
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.
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?
(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.
(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 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.
Created attachment 448372 [details] [diff] [review]
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.
Attachment #445403 - Attachment is obsolete: true
Attachment #448372 - Flags: review?(surkov.alexander)
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.
Attachment #448372 - Flags: review?(surkov.alexander)
(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 :)
(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.
Created attachment 448405 [details] [diff] [review]
patch 3.1 (improved comment)
Attachment #448372 - Attachment is obsolete: true
Attachment #448405 - Flags: review?(surkov.alexander)
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 :).
Attachment #448405 - Flags: review?(surkov.alexander) → review+
Updated locally you cheeky monkey.
The try-server build from comment #34 is also working fine for me.
I hope to land tonight/tomorrow
Status: NEW → ASSIGNED

Updated

9 years ago
Whiteboard: [sg:critical?][critsmash:investigating] → [sg:critical?][critsmash:patch]
Landed http://hg.mozilla.org/mozilla-central/rev/08d167d1dd30
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical][critsmash:patch]
Depends on: 619002

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.