Outlook: inProcess implementation to report replied / replied all / forwarded status on mail items in the message list #8756
Conversation
…s in the message list, by fetching info from the mail item's MAPI object in-process, bypassing security restrictions.
There was a problem hiding this comment.
I'm not a C++ expert, so it might be good to have @feerrenrut have a quick look at the NVDAHelper code, especially the new outlook module.
nvdaHelper/common/libraryLoader.h
Outdated
| #include <windows.h> | ||
|
|
||
| // A Smart Library handle. | ||
| // Constrcut it with a handle returned by LoadLibrary or similar. |
nvdahelper/remote/outlook.cpp
Outdated
| pGIT->GetInterfaceFromGlobal(cookie,IID_IUnknown,(void**)&mapiObject); | ||
| if(res!=S_OK) { | ||
| LOG_ERROR(L"Could not marshal object"); | ||
| return; |
There was a problem hiding this comment.
Shouldn't this be return res?
There was a problem hiding this comment.
Currently execInThread only takes callables that return void. I made res be passed into the lambda by reference thus once execInThread returns, res contains what ever error was inside. However, I should at least be returning res from the outer rpc function, which I forgot to do.
I'm getting the following error while arrowing through an outlook messages listINFO - RPC process 11932 (OUTLOOK.EXE) (08:33:23.914): Thread 1356, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp, 43: MAPI object in rpc thread: 0000019A74245B80ERROR - RPC process 11932 (OUTLOOK.EXE) (08:33:23.921): This is outlook '16.0.10730.20088 X64 |
|
So you do have mapi32.dll at least, or it would not have got that far.
Are you able to take a look at what functions that dll exports please?
I usually do it with a utility in Linux such as strings.
But I'm sure there are other utilities to do it.
|
|
Also, is it 64 or 32 bit? and how did you install Ofice... was it a
stand-alone installer, or from the Windows Store?
|
|
This was a standalone clikc to run installer Strange enough, the HrGetOneProp function is there, so I don't get why neither ctypes nor NVDAHelper can't get it: |
|
Actually, the HrGetOneProp docs indicate that this support is limited to Outlook >=2013. |
|
I'm guessing your Office is 64 bit?
I've just noticed that the 64 bit version of mapi32.dll doesn't have the
@12 at the end of HrGetProp.
I think 64 bit has a different calling convention and exposes its
symbols slightly different.
My Office is 32 bit.
|
|
Yes, My office is 64 bit indeed. dllex is reporting the @12 suffix for both the x86 and x64 instance on my system. File version of mapi32.dll is 1.0.2536.0. Product version is 10.0.17134.1. This one looks like to be bundled with the OS, so I wonder whether Outlook version installed makes a difference. |
|
A plain Windows 7 x86 VM without outlook installed also has the mapi32.dll variant with the @12 suffix. IN this article, Microsoft explains how to link to MAPI functions. There's also an article to choose a specific version |
|
I pushed a commit to try HrGetOneProp before HrGetOneProp@12 just in
case. Give that a try and let me know how you go.
I don't think MAPIFreeBuffer will have a similar issue.
It is possible that Outlook loads its own mapi32.dll from Program Files,
not system32.
|
|
Yes, this works like a charm. I just pushed a try build for this, so I can test this on an Outlook 2010 system. |
|
This definitely causes heavy crashes when used with Outlook 2010. The outlook process is terminated as soon as a message gets focus. Error logERROR - RPC process 8236 (nvda_slave.exe) (11:12:53.848): __main__.main: slave error Traceback (most recent call last): File "nvda_slave.pyw", line 94, in main File "comHelper.pyc", line 22, in _lresultFromGetActiveObject File "comtypes\client\__init__.pyc", line 180, in GetActiveObject File "comtypes\__init__.pyc", line 1247, in GetActiveObject File "_ctypes/callproc.c", line 950, in GetResult WindowsError: [Error -2147221021] Bewerking is niet beschikbaar INFO - RPC process 11776 (OUTLOOK.EXE) (11:12:56.298): Thread 14208, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp, 43: MAPI object in rpc thread: 0000000010BB8430 |
|
ouch. A memory address of 1. How nice.
|
…erfaceTable::getInterfaceFromGlobal, and return on error.
|
Pretty sure I've solved the crash now. I was not bothering to check the
returned error code of IGlobalInterfaceTable::getInterfaceFromGlobal. I
am guessing it was returning an error on Outlook 2010. I'm also logging
that error code now as well.
|
|
Still crashes: |
|
I am unable to reproduce the crash on Outlook 2010 32 bit.
Your Outlook 2010 is 64 bit, so it may be specific to that.
Unless of course it has something to do with the implementation of the
MAPI object itself. What kind of email account are you testing on? pop3,
imap, exchange?
|
…er than PM_REMOVE, so check for PM_REMOVE specifically rather than PM_NOREMOVE. This stops messages being processed more than once if fetched with peakMessage with no PM_REMOVE but other flags.
|
I noticed in my log for Outlook 2010, that the lambda given to
execInThread was executing twice. This is very dangerous and I'm
surprised Outlook was not crashing for me.
I tracked it down to getMessageHook function in inProcess.cpp. Microsoft
documentation states that wParam can be either PM_NOREMOVE (0x00 or
PM_REMOVE (0x01).
We need to ignore any window message that is not removed from the queue,
otherwise we may see it more than once.
However, debugging this a bit, sometimes the hook in Outlook 2010 is
being called with other flags from PeakMessage. And therefore our check
for PM_NOREMOVE does not match.
I have changed it to check for the absence of PM_REMOVE in the flags,
rather than a literal equality check for PM_NOREMOVE. And we now only
handle our execInThread window message once.
I also fixed another bug in the lambda to do with logging: that 1 was
not the address of the object, but rather the logging stream was casting
it to bool because it didn't know how to handle a COM smart pointer. Now
it correctly prints the COM object address.
No idea if these changes fix the crash for you?
|
|
The Outlook 2010 system is actually my colleague's system, but it indeed has Outlook 2010 X64. With the last try build, there are no crashes and things are reported as expected. |
nvdaHelper/remote/outlook.cpp
Outdated
|
|
||
| // Our RPC function | ||
| error_status_t nvdaInProcUtils_outlook_getMAPIProp(handle_t bindingHandle, const long threadID, IUnknown* mapiObject, const unsigned long mapiPropTag, VARIANT* retVal) { | ||
| LOG_INFO(L"MAPI object in rpc thread: "<<mapiObject); |
There was a problem hiding this comment.
Might make sense to make this debug before merging.
nvdaHelper/remote/outlook.cpp
Outdated
| LOG_ERROR(L"Could not unmarshal object, code "<<res); | ||
| return; | ||
| } | ||
| LOG_INFO(L"MAPI object in GUI thread: "<<static_cast<IUnknown*>(mapiObject)); |
| @@ -114,7 +114,7 @@ bool unregisterWindowsHook(int hookType, HOOKPROC hookProc) { | |||
|
|
|||
| //GetMessage hook callback | |||
| LRESULT CALLBACK inProcess_getMessageHook(int code, WPARAM wParam, LPARAM lParam) { | |||
| if(code<0||wParam==PM_NOREMOVE) { | |||
| if(code<0||!(wParam&PM_REMOVE)) { | |||
There was a problem hiding this comment.
Isn't this equal to
if(code<0||wParam~PM_REMOVE) {
I consider that to be somewhat more readble, but that's just a personal preference.
There was a problem hiding this comment.
~ in c++ is a unary operator I think. So it would have to be wParam&~PM_REMOVE.
nvdaHelper/remote/outlook.cpp
Outdated
| // The following declarations come from MAPIDEFS.h which is no longer included in the Windows SDK | ||
|
|
||
| #define PT_LONG ((ULONG)3) | ||
| #define MAPI_E_NOTFOUND 0x8004010f |
There was a problem hiding this comment.
Rather than #defines, can these be changed constexpr
nvdaHelper/remote/outlook.cpp
Outdated
| // Right now this function only supports MAPI properties with a type of long. | ||
| // To support more types, we would need to know how to correctly pack them into a VARIANT. | ||
| LOG_ERROR(L"Unsupported MAPI prop type"); | ||
| return -1; |
There was a problem hiding this comment.
Rather than -1 can we have a named constant please.
nvdaHelper/remote/outlook.cpp
Outdated
| execInThread(threadID,[=,&res](){ | ||
| // Unmarshal the IUnknown pointer from the COM global interface table. | ||
| IUnknownPtr mapiObject=nullptr; | ||
| res=pGIT->GetInterfaceFromGlobal(cookie,IID_IUnknown,(void**)static_cast<IUnknown**>(&mapiObject)); |
There was a problem hiding this comment.
(void**)static_cast<IUnknown**>(&mapiObject) looks wrong to me. Why cast to IUnknown** and then c-style cast to void**
nvdaHelper/remote/outlook.cpp
Outdated
| res=HrGetOneProp(mapiObject,mapiPropTag,&_propValue); | ||
| propValue.reset(_propValue); | ||
| } | ||
| if(res!=S_OK||!propValue) { |
There was a problem hiding this comment.
In this case, res could be returned by the outer function with S_OK even though there was an error due to !propValue
source/appModules/outlook.py
Outdated
| if mapiObject: | ||
| v=comtypes.automation.VARIANT() | ||
| res=NVDAHelper.localLib.nvdaInProcUtils_outlook_getMAPIProp(self.appModule.helperLocalBindingHandle,self.windowThreadID,mapiObject,PR_LAST_VERB_EXECUTED,ctypes.byref(v)) | ||
| if res==0: |
There was a problem hiding this comment.
Please use a name for 0 instead.
|
I believe I addressed all your review comments, except for the query
around the c-style cast.
The static_cast to IUnknown** is to ensure that the smart pointer's
address-taking operator (&) correctly uses the encapsulated IUnknown*
pointer. But getInterfaceFromGlobal takes a void**, so a second
non-static cast from IUnknown** to void** is needed.
|
|
Actually, no cast is necessary to cast to That said, it's not necessary at all, |
nvdaHelper/remote/outlook.cpp
Outdated
| LOG_ERROR(L"NULL MAPI object"); | ||
| return E_INVALIDARG; | ||
| } | ||
| if((mapiPropTag&0xffff)!=PT_LONG) { |
There was a problem hiding this comment.
Rather than having 0xffff hard coded here, it might be better to give it a name. This ties in with the comment in nvdaInProcUtils right? Perhaps this could be called tagMask
source/appModules/outlook.py
Outdated
| mapiObject=None | ||
| if mapiObject: | ||
| v=comtypes.automation.VARIANT() | ||
| res=NVDAHelper.localLib.nvdaInProcUtils_outlook_getMAPIProp(self.appModule.helperLocalBindingHandle,self.windowThreadID,mapiObject,PR_LAST_VERB_EXECUTED,ctypes.byref(v)) |
There was a problem hiding this comment.
Also, since this line is quite long, please split the arguments onto multiple lines.
|
Nope, the compiler refuses to implicitly cast from IUnknown** to void**.
|
|
I have addressed your further review comments.
You're right that the cast to IUnknown** is not needed. So now I just
have a reinterpret_cast to void**.
|
Link to issue number:
Fixes #6911
Summary of the issue:
Outlook's message list shows if a mail item has been replied to or forwarded, however NVDA does not report this information.
Previous tries at implementing this which failed were:
Description of how this pull request fixes the issue:
The Outlook object model provides direct access to the MAPI object via a mapiObject property on mail items. However, as the IMAPIProp interface cannot be marshalled by COM, all calls must be made on the object directly, in the object's own COM apartment (the Outlook GUI thread).
This PR implements a new outlook_getMAPIProperty RPC call which marshals the given MAPI object back into its own thread, and fetches the requested property from there.
Testing performed:
Known issues with pull request:
Change log entry:
New features:
Section: New features, Changes, Bug fixes