289188 - IAccessible is not thread safe

Status: RESOLVED INVALID

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

When I run with a screen reader in debug mode, I get a fair number of the
following  assertions from  NS_ASSERT_OWNINGTHREAD(_class);    
http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsISupportsImpl.h#135   and #178
It tells me that  "nsAccessNode is not thread-safe"

This assertion seems to occur because nsAccessNode is tied in via both COM and
XPCOM. Users of IAccessible can be in or out of process, and are usually not
in-process on the same thread. Most of the methods they call are getters,
although they can set the focus, cause a link or button to be activated or check
a checkbox.

The consequnces of not doing anything, apparently, would 
be crashes, data inconsistencies, deadlocks do to accessing critical 
sections (namely the mRefCnt of nsISupports as well as DOM specific 
data) at the same time.

How the objects are created:
The AT gets the first object it cares about by calling
AccessibleObjectFromWindow(). This in affect sends a WM_GETOBJECT message to the
window, which we respond to by creating an object via new nsRootAccessible and
using LResultFromObject to send the pointer back. This first object is then used
by the AT to get any other  objects in that window, via IAccessible getters like
get_accChild().

nsRootAccessible and all of the other kinds of accessibles inherit from
nsAccessibleWrap, which implements IAccessible when built on Windows. The class
nsAccessibleWrap in turns inherits from nsAccessible, which implements the cross
platform interfaces.

Comment 1

[Aaron Leventhal]

Comments from Dougt via email:
> There are a couple of different things that you can do.  Where is the 
> /nsAccessNode/ tied to a COM object.  Maybe there is a MSCOM apartment 
> attribute you can set when you CoCreateInstance or something when you 
> create the object't CoClass.

> Another approach would be to make the object serialized.  This may lead 
> to performance problems in the best case, and if you don't do it perfect 
> you will run into deadlocks and crashes.

(Note that I don't really understand either of these options yet)

Comment 2

[Benjamin Smedberg]

Aaron, what threading model is the COM component using? It should almost
certainly be using a single-threaded model.

Comment 3

[Doug Turner (:dougt)]

what ben said.  sounds like the object is free-threaded.  in COM there is a way
to say only call this object on this thread.  this magic needs to be applied to
your COM object.

Comment 4

[Aaron Leventhal]

(In reply to comment #3)
> what ben said.  sounds like the object is free-threaded.  in COM there is a way
> to say only call this object on this thread.  this magic needs to be applied to
> your COM object.

Doug, Ben -- do you mean:
CoMarshalInterThreadInterfaceInStream and
CoGetInterfaceAndReleaseStream

This MS COM threading is beyond me. Any more pointers and hints would be
appreciated.

Comment 5

[Benjamin Smedberg]

I would be willing to look at this if I had all the proper tools... is there a
free screen reader that I can test this with?

I lost our IRC conversation, but external clients are obtaining the IAccessible
COM object by a win32 message sent to the WindowProc, correct?

http://lxr.mozilla.org/mozilla/source/widget/src/windows/nsWindow.cpp#4853

As far as I can tell, this call should always happen on the main thread; we
should add assertions to check this. At this point, we are returning an
IAccessible interface that has not been registered with COM. Has CoInitialize
already been called? If so, this interface should be treated by COM as a
single-threaded apartment, and we should not have threading problems.

We should then add assertions to the IAccessible implementations to check
whether those methods are all being called on the main thread as well.

Comment 6

[Aaron Leventhal]

Surkov, this may become more of an issue since some IA2 methods can set text etc. Probably an issue for setting focus and DoAction() as well.

Comment 7

[Marco Zehe (:MarcoZ)]

Alex, do we still need to do anything here? last activity was in February 2007, and these methods have long been implemented, and there were no complaints or crashes AFAIK.

Comment 8

[alexander :surkov (:asurkov)]

Jim, could you look at this issue please (especially Benjamin's comment #5). What exactly should we do here?

Comment 9

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

(In reply to alexander :surkov from comment #8)
> Jim, could you look at this issue please (especially Benjamin's comment #5).
> What exactly should we do here?

Alex I think this bug is obsolete? ...and there is no Jim on the bug ;)

Comment 10

[alexander :surkov (:asurkov)]

Cc'ing Jim and Aaron, if it rings a bell for them. If not then we can close it as being inactive for 5 years.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

I think we should resolve as invalid, with caveats.

As far as local objects are concerned, we should always be accessing them on the main thread. I suppose that badly-written injected DLLs might try to do otherwise, but we should consider that to be unsupported and block accordingly.

OTOH, remote objects are just fine. That's because COM proxies those objects depending on the apartment that the main thread resides in. We initialize our main thread in the single-threaded apartment, so we're good. E10s content don't use the single-threaded apartment, but we use our own proxying mechanism that always forwards requests to the main thread at all times.