300783 - Fix for bug 290354 introduced unsafe firing of events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The new events added in bug 290354 are being fired from within the guts of some
code that really can't deal with DOM mutations.  If someone happens to listen to
those events and actually change the DOM from the event listeners, we'll crash.
 We should really be firing these events once we unwind to somewhere where it's
safe to fire events (the outer event loop, basically).

Comment 1

[Aaron Leventhal]

Bz, can you point me some other code that does that so I can do it the right way?

Also, it probably makes sense only to fire these events when the containing
listbox has focus.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't believe we have such code...  Most places we fire events it's safe to do
it; we have crash bugs filed on the rest.

Comment 3

[Aaron Leventhal]

I'd like to fix everything that has been mentioned in one patch:
1) Remove "DOM" prefix from DOMItemSelected and DOMItemUnselected
2) Fix perf by only firing events for the focused element, and when aNotify is true
3) Fix leaks (not sure where they're from yet)
4) Fix unsafe firing

Bz, do you recommend I use a PLEvent to fire the event and make it safe?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, if that works for accessibility, that would be great.

Comment 5

[Aaron Leventhal]

Attachment: Why isn't this working? What event magic did I forget to add so that my plevents turn into DOM events

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'm not really that familiar with our event stuff.... that looks like it should
generally work.  But it's still firing synchronously and unsafely; I don't see
any PLEvents around.

Comment 7

[Aaron Leventhal]

Attachment: 1) Fix unsafe firing events by creating a new class for general use called nsPLDOMEvent, 2) Remove DOM prefix from new event names, 3) For perf check to see if select has focus before firing events

I haven't looked into the balsa leaks reported in bug 300833 yet. This should
fix the perf regression reported in bug 290354.

Comment 8

[Aaron Leventhal]

Actually the perf regression was bug 300818.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So I've been thinking about this... accessibility is already an
nsIDocumentObserver, right?  Is there a reason that ContentStatesChanged() can't
be used here?  Doesn't it happen exactly when we'd want it to?

Comment 10

[Aaron Leventhal]

Attachment: Use ContentStatesChanged(). This means we can get rid of the changes from bug 290354 in nsHTMLOptionElement. We still need the nsListControlFrame changes for when DOMMenuItemActive is fired.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 190373 [details] [diff] [review]
Use ContentStatesChanged(). This means we can get rid of the changes from bug 290354 in nsHTMLOptionElement. We still need the nsListControlFrame changes for when DOMMenuItemActive is fired.

>Index: accessible/src/html/nsHTMLSelectAccessible.cpp
>+  accService->GetAccessibleFor(optionNode, getter_AddRefs(optionAccessible));
>+  if (!optionNode) {

I assume that should be a check of optionAccessible?

r+sr=bzbarsky with that

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 190373 [details] [diff] [review]
Use ContentStatesChanged(). This means we can get rid of the changes from bug 290354 in nsHTMLOptionElement. We still need the nsListControlFrame changes for when DOMMenuItemActive is fired.

a=mkaply - don't forget bz's change

Comment 13

[Aaron Leventhal]

Bz's change is in

Checking in accessible/src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.70; previous revision: 1.69
done
Checking in accessible/src/html/nsHTMLSelectAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLSelectAccessible.cpp,v  <-- 
nsHTMLSelectAccessible.cpp
new revision: 1.46; previous revision: 1.45
done
Checking in accessible/src/html/nsHTMLSelectAccessible.h;
/cvsroot/mozilla/accessible/src/html/nsHTMLSelectAccessible.h,v  <-- 
nsHTMLSelectAccessible.h
new revision: 1.28; previous revision: 1.27
done
Checking in content/html/content/src/nsHTMLOptionElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLOptionElement.cpp,v  <-- 
nsHTMLOptionElement.cpp
new revision: 1.136; previous revision: 1.135
done