561741 - use nsAccessible outside an accessible module

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Morph nsIAccessiblityService::GetAccessibleInShell to return raw pointer to nsAccessible instance, it's safe because the object is returned from cache, util nsAccessibleEvent processing is sync then it's safe to use not addrefed nsAccessible*.

Comment 1

[alexander :surkov (:asurkov)]

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

David - a11y
Olli - content
Steven - widgets os x
Neil - widgets windows
Robert - layout, widgets

Comment 2

[alexander :surkov (:asurkov)]

this patch gets rid the bug 560185

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Can you say more about how this fixes bug 560185?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Can you say more about how this fixes bug 560185?

Actually that was an idea that helped. I still don't know what "non-virtual thunk" means in this case. But since it's related with virtual table then I thought what if we'll stop to use interfaces and start to work with object directly.

This fix is not about bug 560185 only. We're switching internally (in a11y module) to nsAccessible and I think it's worth to use it outside an accessibility module (no query interface and more quick methods).

Comment 5

[Steven Michaud [:smichaud] (Retired)]

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

I'm glad you had other motivations for these changes than just working
around bug 560185 :-)

Your changes to nsChildView.mm look fine to me.  Presuming (of course)
that it's safe to use a non-addrefed nsAccessible*.  Josh should
probably also look at them, so I'll add him to the review queue.

-  nsCOMPtr<nsIAccessible> accessible;
-  mGeckoChild->GetDocumentAccessible(getter_AddRefs(accessible));
+  nsRefPtr<nsAccessible> accessible = mGeckoChild->GetDocumentAccessible();
+  // XXXsurkov: why can the accessible getting destroy mGeckoChild?
   if (!mGeckoChild)

The handling of DOM events is complex, and who knows what
listeners/handlers might have been installed on a given event, and
what they might do.  So (in Cocoa widgets) we generally assume that
the processing of any DOM event might have destroyed the current
widget.

Comment 6

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

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

r=me. Can you post a tryserver build link?

(In reply to comment #4)
> (In reply to comment #3)
> > Can you say more about how this fixes bug 560185?
> 
> Actually that was an idea that helped. I still don't know what "non-virtual
> thunk" means in this case. But since it's related with virtual table then I
> thought what if we'll stop to use interfaces and start to work with object
> directly.

I'd like to know the real cause. Is there a known compiler bug on OS X related to this?

Comment 7

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

Note either way, I see this as an improvement. Nice one Alexander :)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> I'd like to know the real cause. Is there a known compiler bug on OS X related
> to this?

I failed to find known issue. I've seen couple similar problems of linking. All of them related with multiple inheritance, perhaps like in our case. The first idea I had it's because our classes have methods named like interface methods the class is inherited from (overloaded methods, have same name but different arguments or types), but it sounds that wasn't a reason.

I could file a bug but the bug description would be "build firefox and try gdb" what it's not very sucesful bug report. I need to spend amount of time to find a cause and make bug descriptive but I think it might be not right time for investigations in the area I do not work.

Comment 9

[alexander :surkov (:asurkov)]

try-server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-a0bbf0bbb997

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch2

updated to trunk

Comment 11

[Marco Zehe (:MarcoZ)]

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

I built with this patch and accessibility enabled, and it doesn't cause any change. That means no change for the better in regards to VoiceOver interaction, but also no change for the worse (breaking communication alltogether).

Comment 12

[alexander :surkov (:asurkov)]

Marco, thank you for testing. Could you please check Windows and Linux since the patch affects on all platforms?

Comment 13

[Marco Zehe (:MarcoZ)]

(In reply to comment #12)
> Marco, thank you for testing. Could you please check Windows and Linux since
> the patch affects on all platforms?

All's well.

Comment 14

[alexander :surkov (:asurkov)]

great. thank you much.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Why you need to #ifdef ACCESSIBILITY in nsGUIEvent and in nsDOMEvent?

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> Why you need to #ifdef ACCESSIBILITY in nsGUIEvent and in nsDOMEvent?

I think it's not necessary to expose nsAccessibleEvent if a11y is disabled. That's usual practice of a11y code usage in gecko.

Comment 17

[Josh Aas]

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

+  // XXXsurkov: why can the accessible getting destroy mGeckoChild?

Any time we send an event we assume that some chain of events could end up destroying the gecko nsIWidget. To be safe we always check for that. Please leave this check (as you did), and remove the XXX comment.

Comment 18

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

(In reply to comment #17)
> (From update of attachment 441718 [details] [diff] [review])
> +  // XXXsurkov: why can the accessible getting destroy mGeckoChild?
> 
> Any time we send an event we assume that some chain of events could end up
> destroying the gecko nsIWidget. To be safe we always check for that. Please
> leave this check (as you did), and remove the XXX comment.

Good to know Josh (I was curious as well).

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> Please
> leave this check (as you did), and remove the XXX comment.

done

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

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

backed out due to reports of Linux crashes on startup
http://hg.mozilla.org/mozilla-central/rev/2108d2458719

http://crash-stats.mozilla.com/report/index/80688273-e67f-4879-8068-fa8c82100512

Comment 21

[alexander :surkov (:asurkov)]

I reproduced the crash on my local machine, hope to come with the fix soon.

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch3

Robert, please review gtk part
Marco, please test it on linux

the problem was activate/deactive events were sent for wrong accessible what crashes firefox in the end.

Comment 23

[alexander :surkov (:asurkov)]

try server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-a15902942f1b

Comment 24

[Marco Zehe (:MarcoZ)]

Comment on attachment 445130 [details] [diff] [review]
patch3

Doesn't crash for me. I'm using it to r+ this patch. :)

Comment 25

[alexander :surkov (:asurkov)]

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