Last Comment Bug 166994 - Make our MSAA objects also support IDispatch methods for scripters
: Make our MSAA objects also support IDispatch methods for scripters
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Windows 2000
: P5 normal (vote)
: mozilla1.3alpha
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: ia2 tbird3access 539699
  Show dependency treegraph
 
Reported: 2002-09-05 19:36 PDT by Aaron Leventhal
Modified: 2010-01-14 07:49 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Zipped up simple sample (30.16 KB, application/octet-stream)
2002-09-06 06:59 PDT, Adam Lock
no flags Details
TlbDemoObject.cpp (2.74 KB, text/plain)
2002-09-06 07:15 PDT, Adam Lock
no flags Details
Okay, here's a patch -- how do I test this? (2.59 KB, patch)
2002-09-06 21:40 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Updated patch (5.01 KB, patch)
2002-09-09 14:19 PDT, Adam Lock
no flags Details | Diff | Splinter Review
Fixes EnterCriticalSection problem, by using InitializeCriticalSection() (6.14 KB, patch)
2002-09-10 19:27 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
patch (8.01 KB, patch)
2007-03-10 12:37 PST, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (7.29 KB, patch)
2010-01-05 08:14 PST, alexander :surkov
dbolter: review+
Details | Diff | Splinter Review

Description Aaron Leventhal 2002-09-05 19:36:03 PDT
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 2002-09-05 19:37:38 PDT
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 2002-09-06 06:59:00 PDT
Created attachment 98121 [details]
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 2002-09-06 07:15:08 PDT
Created attachment 98123 [details]
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 2002-09-06 21:40:30 PDT
Created attachment 98250 [details] [diff] [review]
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 2002-09-09 08:11:53 PDT
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 2002-09-09 08:25:15 PDT
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 2002-09-09 10:49:11 PDT
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 2002-09-09 13:26:02 PDT
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 2002-09-09 14:19:33 PDT
Created attachment 98487 [details] [diff] [review]
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 2002-09-09 14:26:19 PDT
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 2002-09-09 15:03:21 PDT
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 2002-09-09 15:32:11 PDT
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 2002-09-09 15:38:29 PDT
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 2002-09-09 15:40:47 PDT
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 2002-09-10 07:19:04 PDT
Can you try clobbering the whole widget dir and rebuilding? It could be bustage
caused by broken dependencies.
Comment 16 Aaron Leventhal 2002-09-10 18:47:38 PDT
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 2002-09-10 19:27:27 PDT
Created attachment 98671 [details] [diff] [review]
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 2002-09-10 19:39:10 PDT
  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 2002-09-10 20:25:54 PDT
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 2002-09-11 06:57:00 PDT
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 2002-09-11 11:11:48 PDT
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 2006-11-17 10:46:39 PST
Apparently this is needed for JAWS scripting.
Comment 23 Aaron Leventhal 2007-02-15 07:48:29 PST
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 2007-02-20 09:02:39 PST
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 2007-02-20 09:59:01 PST
> 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 2007-03-10 12:37:38 PST
Created attachment 258137 [details] [diff] [review]
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 2007-03-10 12:38:19 PST
(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 2007-03-10 14:47:29 PST
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 2007-03-11 01:59:32 PST
(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 2007-03-11 07:49:55 PDT
Maybe bsmedberg?
Comment 31 alexander :surkov 2007-03-11 10:30:04 PDT
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 2007-03-11 10:35:15 PDT
(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 2007-06-08 07:44:14 PDT
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 2008-07-09 11:20:04 PDT
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 2008-07-09 20:36:59 PDT
(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 2009-12-20 03:26:09 PST
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 2009-12-20 21:31:16 PST
(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 2009-12-21 01:04:11 PST
(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 2010-01-05 08:14:26 PST
Created attachment 420083 [details] [diff] [review]
patch2
Comment 40 David Bolter [:davidb] 2010-01-05 08:23:21 PST
How do we test this?
Comment 41 alexander :surkov 2010-01-05 08:26:48 PST
(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] 2010-01-05 08:34:28 PST
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 2010-01-06 02:49:40 PST
(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] 2010-01-06 06:15:46 PST
Can you do a build and ask Martin or Adam to test it?
Comment 45 alexander :surkov 2010-01-06 10:56:04 PST
(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/surkov.alexander@gmail.com-try-07e98c881a2a/
Comment 46 martin.feger 2010-01-07 05:25:26 PST
(In reply to comment #45)
> Sure, try server build is here
> https://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-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] 2010-01-07 05:52:37 PST
(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 2010-01-07 09:03:12 PST
> 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 2010-01-07 15:38:17 PST
(In reply to comment #46)
> (In reply to comment #45)
> > Sure, try server build is here
> > https://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-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 2010-01-07 22:55:21 PST
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/1f9ae6f1f9c5
Comment 51 alexander :surkov 2010-01-07 23:02:49 PST
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 2010-01-07 23:27:43 PST
(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 2010-01-11 10:21:43 PST
(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 2010-01-13 22:38:23 PST
cc'ing Benjamin again to help in answer on comment #53.
Comment 55 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-01-14 06:43:06 PST
Yes, the main thread pumps all messages that I know of.
Comment 56 alexander :surkov 2010-01-14 07:49:29 PST
Ok, thanks. I filed bug 539699 to deal with this.

Note You need to log in before you can comment on or make changes to this bug.