166994 - Make our MSAA objects also support IDispatch methods for scripters

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P5)

Description

[Aaron Leventhal]

We have a place to support IDispatch interfaces on our Accessible objects here:

http://bugzilla.mozilla.org/show_bug.cgi?id=166993

According to Adam:

> Basically you just let the OLE system to implement the IDispatch methods 
> for you using the type library to marshal calls to your dual interface 
> methods. I have a VB sample which demonstrates the object being called 
> via IDispatch and directly on the dual interface to demonstrate it works.

Comment 1

[Aaron Leventhal]

Oops, pasted the wrong link.

Here's the correct LXR link:

http://lxr.mozilla.org/seamonkey/source/widget/src/windows/Accessible.cpp#857

Comment 2

[Adam Lock]

Attachment: Zipped up simple sample

Contained is a simple COM object demonstrating the principle. I'll attach the
most pertinent source file seperately next to save people the pain of unzipping
to see what's involved.

Comment 3

[Adam Lock]

Attachment: TlbDemoObject.cpp

This is a simple COM object with a few dummy methods. It exposes a
dual-interface (i.e. derived from IDispatch) just like IAccesible. The
IDispatch implementation is the most important thing to note - Replace the impl
in mozilla/widget/src/windows/Accessible.cpp with this changing the Typelib
GUID and major/minor version numbers, ensuring that QI works for IDispatch and
IAccesible and that's about all there is to it.

The file win32 file oleacc.h defines the typelib GUID as LIBID_Accessibility,
and probably 1,0 or 1,1 will do for the major, minor versions. I have 1.1
installed on my machine, but I don't know what the baseline version is.

Comment 4

[Aaron Leventhal]

Attachment: Okay, here's a patch -- how do I test this?

Was I suppoed to use IID_IAccessible here? 
  hr = typeLib->GetTypeInfoOfGuid(IID_IAccessible, ppTInfo);

Anyway, this builds okay, but how do we test it?

Comment 5

[Adam Lock]

Yes, IID_IAccessible.

Doesn't the Accessibility SDK have tools to test it's working? They must make
calls via IDispatch of course.

If the tools go via IAccesible, you'll have to throw something together in VB to
test it. The trickiest thing will be getting the IDispatch from the window,
which presumably means calling AccessibleObjectFromPoint or something similar.

Comment 6

[Adam Lock]

The MSAA SDK 1.3 has a sample called VBInsp which uses the IDispatch impl. In
its current state it should make a good sanity test (hover over the Moz/NS
window and see if it can get back some info) and you could probably add some
extra tests to it too.

Comment 7

[Aaron Leventhal]

I haven't tried VBInsp yet, but checking "Use Invoke" in Inspect.exe seems to
test the stuff.

Unfortunately it goes into an infinite recursive loop when Invoke() gets called.

Invoke() calls GetTypeInfo(), and then Invoke() again, which ends up being
Invoke() on the same object.

I better read up on MSDN. I still don't understand what these IDispatch methods
are supposed to do.

Comment 8

[David Bradley]

Ideally you implement IDispatch::Invoke, put any specialized code in and then
forward to ITypeInfo::Invoke which then does the actual invoke. Are you trying
to invoke "Invoke"? I would imagine that would cause problems.

Comment 9

[Adam Lock]

Attachment: Updated patch

This is a slightly updated patch which caches the type info instead of
refetching it everytime. Otherwise it's the same.

Comment 10

[Adam Lock]

The patch works for me. The invoke method gets called from the inspector exe and
then the appropriate dual interface implementation. The patch lets the system do
the marshaling.

I don't understand why the ITypeInfo::Invoke would call the IDispatch::Invoke()
again. It doesn't for me. The actual implementation does little more than grab
the ITypeInfo object representation of the interface and call Invoke on it,
passing the accessible instance in as the object whose methods are to be called.

Comment 11

[Aaron Leventhal]

When I build with the new patch, I get these errors:
C:\run\vstudio\VC98\atl\include\atlcom.h(2945) : error C2065: '_Module' :
undeclared identifier
C:\run\vstudio\VC98\atl\include\atlcom.h(2945) : error C2228: left of '.Lock'
must have class/struct/union type
C:\run\vstudio\VC98\atl\include\atlcom.h(2947) : error C2228: left of '.Unlock'
must have class/struct/union type
C:\run\vstudio\VC98\atl\include\atlcom.h(3318) : error C2228: left of
'.m_csTypeInfoHolder' must have class/struct/union type
C:\run\vstudio\VC98\atl\include\atlcom.h(3340) : error C2228: left of
'.m_csTypeInfoHolder' must have class/struct/union type
C:\run\vstudio\VC98\atl\include\atlcom.h(3341) : error C2228: left of
'.AddTermFunc' must have class/struct/union type

Comment 12

[Aaron Leventhal]

I can get it to compile of I move

#define _ATLBASE_IMPL
#include <atlbase.h>
extern CComModule _Module;
#define _ATLCOM_IMPL
#include <atlcom.h>

into Accessible.h (replace the old <atlbase.h>, <atlcom.h> includes in the .h
with those lines)

Comment 13

[Aaron Leventhal]

This is really weird. With the new patch this line in GetTypeInfo()

  EnterCriticalSection(&_Module.m_csTypeInfoHolder);

seems to call GetTypeInfo() again.

Comment 14

[Aaron Leventhal]

Maybe some kind of exception is getting thrown on my system.
The page I'm using to test is www.google.com.

Comment 15

[Adam Lock]

Can you try clobbering the whole widget dir and rebuilding? It could be bustage
caused by broken dependencies.

Comment 16

[Aaron Leventhal]

I tried clobbering and rebuilding with a completely fresh tree, and it made no
difference.

And I verified that it's not coming back to me from EnterCriticalSection() by
using printfs. It doesn't reach the printf right after that. Next time my code
is entered is via Invoke() with a different dispid.

Comment 17

[Aaron Leventhal]

Attachment: Fixes EnterCriticalSection problem, by using InitializeCriticalSection()

There is still the problem of typeInfo->Invoke() not returning. It's behaving
similarly, as if it had it's own uninitialized critical section.

Comment 18

[Aaron Leventhal]

  EnterCriticalSection(&_Module.m_csTypeInfoHolder);
  if (!gmTypeInfo)
  {
    CComPtr<ITypeLib> typeLib;
    hr = LoadRegTypeLib(LIBID_Accessibility, wMajor, wMinor, lcid, &typeLib);
    if (SUCCEEDED(hr)) 
      hr = typeLib->GetTypeInfoOfGuid(IID_IAccessible, &gmTypeInfo);
  }
  LeaveCriticalSection(&_Module.m_csTypeInfoHolder);


Wouldn't it be better to put the EnterCriticalSection and LeaveCriticalSection
inside the if?

Comment 19

[Aaron Leventhal]

Do I need to initialize CComModule _Module ?
_Module->Init( _ATL_OBJMAP_ENTRY* p, HINSTANCE h );

I don't have a clue what's wrong with it.

Comment 20

[David Bradley]

If you're not linking against ATL, which has it's own instance, then you'll need
to declare it and initialize it. Either in DLLMain or main. If you're linking
against ATL then you should be able to just declare it extern, and ATL will do
the rest.

And you should leave the Enter/Leave calls outside of the if. The if depends on
the setting of gmTypeInfo in the call to GetTypeInfoOfGuid. So the if needs to
be within the critical section as well.

Comment 21

[Adam Lock]

I thought that the m_csTypeInfoHolder was initialised in the CComModule
constructor, but looks like it is done in CComModule::Init. 

Unless you want to hack your code to be properly conformant with ATL I suggest
declaring a CComAutoCriticalSection static and just using that instead.

Making it use ATL properly wouldn't be a bad thing. ATL has some useful macros
for taking the drudgery out of implementing COM objects. It also has an
IDispatchImpl template which means you wouldn't need to implement it for
youself. It is possible to statically link to ATL too, though it is 95% inline
anyway.

Comment 22

[Aaron Leventhal]

Apparently this is needed for JAWS scripting.

Comment 23

[Aaron Leventhal]

I believe we'll want IDispatch support for our IA2 support as well, so it works with JAWS scripting. Check with #ia2 or IAccessible2 mailing list.

Comment 24

[alexander :surkov (:asurkov)]

Neither JAWS or Window-Eyes need IDispatch for IA2. Do we need IDispatch for IAccessible. If we need then it may be a problem to make MSAA object implements both IA and IA2 interfaces.

Comment 25

[Aaron Leventhal]

> If we need then it may be a problem to make MSAA object implements
> both IA and IA2 interfaces.

What kind of problem?

Comment 26

[alexander :surkov (:asurkov)]

Attachment: patch

I tried to call accName property, GetIDsOfNames() for it returns S_OK but Invoke() doesn't work. Debugger doesn't show me HRESULT for DispInvoke() due to unknown reasons for me.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> > If we need then it may be a problem to make MSAA object implements
> > both IA and IA2 interfaces.
> 
> What kind of problem?
> 

Ah, ignore this. I was wrong.

Comment 28

[Aaron Leventhal]

Are you looking for review of last patch? (I would not be the right person -- need someone who knows COM better).

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)
> Are you looking for review of last patch? (I would not be the right person --
> need someone who knows COM better).
> 

Actually my patch is much like previous one excepting I don't use ATL and use helper methods like DispInvoke(). Can we find MS COM guru to help us?

Comment 30

[Aaron Leventhal]

Maybe bsmedberg?

Comment 31

[alexander :surkov (:asurkov)]

I tried with my patch to call get_accChildCount from JScript. Invoke() didn't fail. But nsAccessibleWrap::get_accChildCount() wasn't called. Therefore I wonder when I load ITypeInfo and when I pass nsAccessibleWrap to ITypeInfo::Invoke() then how will Invoke know what method to call? Can anybody clarify how Invoke() works?

On the other hand we can easy I guess to implement IDispatch support without typeinfo. Here we can just to implement manually GetIDsOfNames() and Invoke() like it is shown on http://lists.kde.org/?l=kde-accessibility&m=103849116623673&w=2.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)
> I tried with my patch to call get_accChildCount from JScript. Invoke() didn't
> fail. But nsAccessibleWrap::get_accChildCount() wasn't called. Therefore I
> wonder when I load ITypeInfo and when I pass nsAccessibleWrap to
> ITypeInfo::Invoke() then how will Invoke know what method to call? Can anybody
> clarify how Invoke() works?

I mean it looks something was called and invoked get_accChildCount returned untitialized value (and debugger showed that it wasn't our nsAccessibleWrap::get_accChildCount). Really I can't understand magic of loaded ITypeInfo::Invoke (ITypeInfo I get via boundle of calls LoadRegTypeLib/GetTypeInfoOfGuid).

Benjamin, can you explain this?

Comment 33

[Aaron Leventhal]

It would still be helpful to have this for MSAA even though we're not planning to do it for IA2

Comment 34

[Aaron Leventhal]

Surkov, how hard is this to fix? We're getting fairly regular criticism for not support IDispatch on our COM objects.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> Surkov, how hard is this to fix? We're getting fairly regular criticism for not
> support IDispatch on our COM objects.
> 

The problem is the latest patch doesn't work (I tried it with accExplorer) but it looks correctly with me. I really haven't idea what's wrong.

Comment 36

[martin.feger]

Is this bug still "helpwanted"? It seems current version of oleacc.dll have learned a few new tricks and now handle the dispinterface part of IAccessible themselves.

Anyway, I believe (I didn't actually test this theory) the first patch was already almost there and just missing one essential detail. In this line

hr = typeInfo->Invoke(this, dispIdMember, wFlags, pDispParams, pVarResult, pExcepInfo, puArgErr);

'this' is an Accessible * , but ITypeInfo::Invoke requires an IAccessible *. IOW, you were passing the wrong vtable and the symptoms of that are exactly what you've described in comments 7 and 32: seemingly random method calls all over the place. Replace |Invoke(this, | with |Invoke(static_cast<IAccessible *>(this), | and you should be golden.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> Is this bug still "helpwanted"?

I think so.

> It seems current version of oleacc.dll have
> learned a few new tricks and now handle the dispinterface part of IAccessible
> themselves.

Really? That's great. So then we don't need to do anything to expose IAccessible. But will it harm us if we do?

We could expose IAccessible2 interfaces as well I guess.

> 
> Anyway, I believe (I didn't actually test this theory) the first patch was
> already almost there and just missing one essential detail. In this line
> 
> hr = typeInfo->Invoke(this, dispIdMember, wFlags, pDispParams, pVarResult,
> pExcepInfo, puArgErr);
> 
> 'this' is an Accessible * , but ITypeInfo::Invoke requires an IAccessible *.
> IOW, you were passing the wrong vtable and the symptoms of that are exactly
> what you've described in comments 7 and 32: seemingly random method calls all
> over the place. Replace |Invoke(this, | with |Invoke(static_cast<IAccessible
> *>(this), | and you should be golden.

Could you look at the last patch please? Does it have the same problem in HRESULT hr = DispInvoke(this, ?

Comment 38

[martin.feger]

(In reply to comment #37)
> Really? That's great. So then we don't need to do anything to expose
> IAccessible. But will it harm us if we do?

It won't harm you, your code simply won't be called with these versions of oleacc. But Windows 2000 SP4 still has an older version, here Inspector shows E_NOTIMPL for tons of stuff when you try the "Use Invoke" option with Firefox. Users of that OS would still benefit if you'd implement IDispatch yourself.

> Could you look at the last patch please? Does it have the same problem in
> HRESULT hr = DispInvoke(this, ?

Yes, since nsAccessibleWrap also uses multiple inheritance. DispInvoke is really a quite useless API: DispInvoke(p, typeInfoPtr, ...) is exactly the same as typeInfoPtr->Invoke(p, ...) .

Comment 39

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 40

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

How do we test this?

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #40)
> How do we test this?

Only if you have Windows 2000 (see comment #36, comment #38). We could ask Martin to look again at the patch if he can see any other issue.

Comment 42

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

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

r=me with comments:

(In reply to comment #41)
> We could ask
> Martin to look again at the patch if he can see any other issue.

OK so this is a tweaked version of Aaron's patch? I'm very very rusty on this part of COM... so yes let's get his review.

It needs to be tested before landing.

Comment 43

[alexander :surkov (:asurkov)]

(In reply to comment #42)
> (From update of attachment 420083 [details] [diff] [review])
> r=me with comments:
> 
> (In reply to comment #41)
> > We could ask
> > Martin to look again at the patch if he can see any other issue.
> 
> OK so this is a tweaked version of Aaron's patch? I'm very very rusty on this
> part of COM... so yes let's get his review.

I think so. I don't remember though already. Do you mean review from Aaron? I'm not sure it's good idea to summon him for these stuffs.

> It needs to be tested before landing.

Ideally yes. If someone of us can get windows 2000 then it would be easy, otherwise I don't have any idea. Also I think it's not very big area of screen reader users who use windows 2000 and write scripts. Actually I think we can trust Martin comment so we could think the fix is right and don't spare time for testing.

Comment 44

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

Can you do a build and ask Martin or Adam to test it?

Comment 45

[alexander :surkov (:asurkov)]

(In reply to comment #44)
> Can you do a build and ask Martin or Adam to test it?

Sure, try server build is here https://build.mozilla.org/tryserver-builds/[e-mail]-try-07e98c881a2a/

Comment 46

[martin.feger]

(In reply to comment #45)
> Sure, try server build is here
> https://build.mozilla.org/tryserver-builds/[e-mail]-try-07e98c881a2a/

The patch looks ok to me and Inspect32 under Windows 2000 likes this build very much (same information with or without "Use Invoke" checked). Loading the type library once for every Invoke call is less than ideal performance-wise, but probably not worth fixing unless somebody complains about it.

Comment 47

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

(In reply to comment #46)
> (In reply to comment #45)
> Loading the type
> library once for every Invoke call

Does it? I thought this guarded against reload:

+  if (mTypeInfo)
+    return mTypeInfo;

Comment 48

[martin.feger]

> Does it? I thought this guarded against reload:
> 
> +  if (mTypeInfo)
> +    return mTypeInfo;

Yes, you're absolutely right, don't know how I missed those lines.

Comment 49

[alexander :surkov (:asurkov)]

(In reply to comment #46)
> (In reply to comment #45)
> > Sure, try server build is here
> > https://build.mozilla.org/tryserver-builds/[e-mail]-try-07e98c881a2a/
> 
> The patch looks ok to me and Inspect32 under Windows 2000 likes this build very
> much (same information with or without "Use Invoke" checked). 

Great. Thank you much!

Removing "helpwanted" keyword.

Comment 50

[alexander :surkov (:asurkov)]

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

Comment 51

[alexander :surkov (:asurkov)]

Martin, btw, could we turn mTypeInfo into static variable, right (and GetTI while we are here) (just noticed this after landing)?

Comment 52

[alexander :surkov (:asurkov)]

(In reply to comment #51)
> Martin, btw, could we turn mTypeInfo into static variable, right (and GetTI
> while we are here) (just noticed this after landing)?

ah, then we should deal with critical sections when type lib is created and used?

Comment 53

[martin.feger]

(In reply to comment #51)
> Martin, btw, could we turn mTypeInfo into static variable, right (and GetTI
> while we are here) (just noticed this after landing)?

You probably could, yes. I know nothing about Gecko's threading architecture, it has only one thread pumping messages for all windows and thus this thread handles all WM_GETOBJECT messages and creates all accessible objects? If so, you won't need to use a critical section, AFAICS MSAA doesn't require your objects to be free threaded.

If you revisit this patch you could also remove the ResultFromScode macro, it's obsolete and doesn't do anything.

Comment 54

[alexander :surkov (:asurkov)]

cc'ing Benjamin again to help in answer on comment #53.

Comment 55

[Benjamin Smedberg]

Yes, the main thread pumps all messages that I know of.

Comment 56

[alexander :surkov (:asurkov)]

Ok, thanks. I filed bug 539699 to deal with this.