Outlook: inProcess implementation to report replied / replied all / forwarded status on mail items in the message list by michaelDCurran · Pull Request #8756 · nvaccess/nvda

michaelDCurran · 2018-09-17T03:04:38Z

Link to issue number:

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:

Fetch the info from UI Automation: UIA does expose text denoting replied/forwarded via the value pattern. However, this text is localized and therefore is impossible to match on programmatically with 100% accuracy. It is also included with other text such as read/unread status and mail item type, which we must strip.
Although the Outlook object model itself does not have a specific property on mail items conveying the last executed action (replied to, forwarded), it does have a propertyAccessor property which allows fetching MAPI properties. Fetching the status this way worked for some users, but for those who had set very high security policies, the fetching of the PropertyAccessor property caused a security dialog to appear. Further testing showed that even if ths property was fetched inProcess the security restrictions still applied.

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:

Outlook 2016/365.
NVDA installed (UIAccess), portable, and running from source.
Outlook is set to block any insecure access (I.e. if NVDA fetches MailItem.PropertyAccessor, the security dialog appears).
Focus a mail item in the message list. NVDA reports if it is forwarded, replied, or replied all. No security dialog ever appears.

Known issues with pull request:

Although security restrictions are not currently applied to MailItem.mapiObject, there is no reason why they may not get applied in a future version of Outlook.
MailItem.mapiObject is deprecated in current versions of Outlook, though still functions at this point in time. A future version of Outlook may remove support completely. If this occurs, this code is already ready to handle a COM error or a NULL COM pointer. Worse case, NVDA simply stops reporting these statuses.

Change log entry:

New features:

Replied / Forwarded status is now reported on mail items in the Microsoft Outlook message list. (NVDA not inform about if an e-mail in Outlook 2013 is answered or forwarded #6911)

Section: New features, Changes, Bug fixes

…s in the message list, by fetching info from the mail item's MAPI object in-process, bypassing security restrictions.

LeonarddeR

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.

LeonarddeR · 2018-09-17T05:10:44Z

+#include <windows.h>
+
+// A Smart Library handle.
+// Constrcut it with a handle returned by LoadLibrary or similar.


Constrcut > Construct

LeonarddeR · 2018-09-17T05:20:02Z

+		pGIT->GetInterfaceFromGlobal(cookie,IID_IUnknown,(void**)&mapiObject);
+		if(res!=S_OK) {
+			LOG_ERROR(L"Could not marshal object");
+			return;


Shouldn't this be return res?

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.

… the inner lambda.

LeonarddeR · 2018-09-17T06:36:13Z

I'm getting the following error while arrowing through an outlook messages list

INFO - 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: 0000019A74245B80

ERROR - RPC process 11932 (OUTLOOK.EXE) (08:33:23.921):
Thread 1356, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp, 62:
Could not locate function HrGetOneProp in mapi32.dll

This is outlook '16.0.10730.20088 X64

michaelDCurran · 2018-09-17T06:39:33Z

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.

michaelDCurran · 2018-09-17T06:40:46Z

Also, is it 64 or 32 bit? and how did you install Ofice... was it a stand-alone installer, or from the Windows Store?

LeonarddeR · 2018-09-17T06:46:09Z

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:

HrGetOneProp@12	0x1000d2d0	0x0000d2d0	135 (0x87)	mapi32.dll	C:\Windows\syswow64\mapi32.dll	Exported Function	

HrGetOneProp@12	0x1000d2d0	0x0000d2d0	135 (0x87)	mapi32.dll	C:\Windows\system32\mapi32.dll	Exported Function

LeonarddeR · 2018-09-17T06:51:18Z

Actually, the HrGetOneProp docs indicate that this support is limited to Outlook >=2013.

michaelDCurran · 2018-09-17T06:56:20Z

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.

LeonarddeR · 2018-09-17T07:00:22Z

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.

LeonarddeR · 2018-09-17T07:12:58Z

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

…GetOneProp@12.

michaelDCurran · 2018-09-17T07:19:34Z

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.

LeonarddeR · 2018-09-17T07:57:39Z

Yes, this works like a charm.

I just pushed a try build for this, so I can test this on an Outlook 2010 system.

LeonarddeR · 2018-09-17T09:20:43Z

This definitely causes heavy crashes when used with Outlook 2010. The outlook process is terminated as soon as a message gets focus.

Error log

ERROR - 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

INFO - RPC process 11776 (OUTLOOK.EXE) (11:12:56.298):
Thread 4112, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp::<lambda_d6a1fe29f2f3f1f2a36dfe801802c01b>::operator (), 98:
MAPI object in GUI thread: 1

ERROR - RPC process 8808 (nvda_slave.exe) (11:13:09.305):
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 3008 (OUTLOOK.EXE) (11:13:12.579):
Thread 7516, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp, 43:
MAPI object in rpc thread: 0000000010486920

INFO - RPC process 3008 (OUTLOOK.EXE) (11:13:12.585):
Thread 2188, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp::<lambda_d6a1fe29f2f3f1f2a36dfe801802c01b>::operator (), 98:
MAPI object in GUI thread: 1

ERROR - RPC process 5100 (nvda_slave.exe) (11:13:25.823):
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 11572 (OUTLOOK.EXE) (11:13:39.536):
Thread 15284, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp, 43:
MAPI object in rpc thread: 000000000A2DC1A0

INFO - RPC process 11572 (OUTLOOK.EXE) (11:13:39.536):
Thread 14540, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp::<lambda_d6a1fe29f2f3f1f2a36dfe801802c01b>::operator (), 98:
MAPI object in GUI thread: 1

michaelDCurran · 2018-09-17T09:24:17Z

ouch. A memory address of 1. How nice.

…erfaceTable::getInterfaceFromGlobal, and return on error.

michaelDCurran · 2018-09-17T10:12:13Z

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.

LeonarddeR · 2018-09-17T11:30:00Z

Still crashes:

ERROR - RPC process 76 (nvda_slave.exe) (13:27:34.888):
__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 5740 (OUTLOOK.EXE) (13:27:39.892):
Thread 3540, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp, 43:
MAPI object in rpc thread: 00000000109B4150

INFO - RPC process 5740 (OUTLOOK.EXE) (13:27:39.903):
Thread 3972, build\x86_64\remote\outlook.cpp, nvdaInProcUtils_outlook_getMAPIProp::<lambda_8eeec166f96c7951c3b21da844cd2873>::operator (), 98:
MAPI object in GUI thread: 1

michaelDCurran · 2018-09-17T21:54:53Z

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.

michaelDCurran · 2018-09-17T23:10:08Z

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?

michaelDCurran · 2018-09-18T03:58:53Z

Here is a new try build: https://ci.appveyor.com/api/buildjobs/bdiqb730rs4et0jf/artifacts/output%2Fnvda_snapshot_try-i6911-16053%2C4d74915c.exe

LeonarddeR · 2018-09-18T06:45:34Z

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.

LeonarddeR

No strange errors in the Outlook 2010 instance NVDA log either.

LeonarddeR · 2018-09-18T06:46:29Z

+
+// 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);


Might make sense to make this debug before merging.

LeonarddeR · 2018-09-18T06:47:45Z

+			LOG_ERROR(L"Could not unmarshal object, code "<<res);
+			return;
+		}
+		LOG_INFO(L"MAPI object in GUI thread: "<<static_cast<IUnknown*>(mapiObject));


Same as above

LeonarddeR · 2018-09-18T07:00:27Z

 //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)) {


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.

~ in c++ is a unary operator I think. So it would have to be wParam&~PM_REMOVE.

feerrenrut · 2018-09-19T02:56:08Z

+// 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


Rather than #defines, can these be changed constexpr

feerrenrut · 2018-09-19T03:51:53Z

+		// 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;


Rather than -1 can we have a named constant please.

feerrenrut · 2018-09-19T03:54:33Z

+	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));


(void**)static_cast<IUnknown**>(&mapiObject) looks wrong to me. Why cast to IUnknown** and then c-style cast to void**

feerrenrut · 2018-09-19T03:59:33Z

+			res=HrGetOneProp(mapiObject,mapiPropTag,&_propValue);
+			propValue.reset(_propValue);
+		}
+		if(res!=S_OK||!propValue) {


In this case, res could be returned by the outer function with S_OK even though there was an error due to !propValue

feerrenrut · 2018-09-19T04:00:42Z

+			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:


Please use a name for 0 instead.

michaelDCurran · 2018-09-19T04:45:47Z

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.

feerrenrut · 2018-09-19T05:47:28Z

Actually, no cast is necessary to cast to void** I believe this will happen implicitly. However, if its important to ensure that &mapiObject is dynamically convertible to IUnknown** then I would prefer this is done separately with a comment to explain. Like so:

// static_cast to ensure that the smart pointer's address-taking operator (&) correctly uses the encapsulated IUnknown* pointer
auto mapiObjPtr = static_cast<IUnknown**>(&mapiObject) 
res=pGIT->GetInterfaceFromGlobal(cookie, IID_IUnknown, mapiObjPtr);

That said, it's not necessary at all, mapiObject is a pointer type and thus can't override the & operator, so the result of &mapiObject has to be IUnknown**. So I think my preference would be to simplify the code to:

res=pGIT->GetInterfaceFromGlobal(cookie, IID_IUnknown, &mapiObject);

feerrenrut · 2018-09-19T05:54:09Z

+		LOG_ERROR(L"NULL MAPI object");
+		return E_INVALIDARG;
+	}
+	if((mapiPropTag&0xffff)!=PT_LONG) {


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

feerrenrut · 2018-09-19T05:55:53Z

+				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))


Also, since this line is quite long, please split the arguments onto multiple lines.

michaelDCurran · 2018-09-19T06:02:20Z

Nope, the compiler refuses to implicitly cast from IUnknown** to void**.

michaelDCurran · 2018-09-19T06:18:47Z

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**.

Outlook: report replied / replied all / forwarded status on mail item…

a290640

…s in the message list, by fetching info from the mail item's MAPI object in-process, bypassing security restrictions.

michaelDCurran requested review from LeonarddeR and feerrenrut September 17, 2018 03:04

Add translator comments.

13cf35c

LeonarddeR suggested changes Sep 17, 2018

View reviewed changes

michaelDCurran added 3 commits September 17, 2018 15:36

Fix typo in comment.

3fd8fd2

nvdaHelperRemote, outlook_getMAPIProperty: return any error code from…

ada9b18

… the inner lambda.

Fix case of path.

663158e

nvdaHelperRemote, outlook_getMAPIProperty: try HrGetOneProp before Hr…

da9fb4c

…GetOneProp@12.

NVDAHelperRemote, outlook_getMAPIProperty: check result of IGlobalInt…

c8c2d8c

…erfaceTable::getInterfaceFromGlobal, and return on error.

outlook_getMAPIProperty: fix logging.

aaf0c1c

NvDAHelperRemote, inProcess_getMessageHook: wParam can have flags oth…

4d74915

…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.

LeonarddeR previously approved these changes Sep 18, 2018

View reviewed changes

feerrenrut suggested changes Sep 19, 2018

View reviewed changes

Address review comments.

7015b63

michaelDCurran dismissed LeonarddeR’s stale review via 7015b63 September 19, 2018 04:41

feerrenrut reviewed Sep 19, 2018

View reviewed changes

Address further review comments.

150970d

feerrenrut previously approved these changes Sep 19, 2018

View reviewed changes

Update what's new

a71adc1

michaelDCurran dismissed feerrenrut’s stale review via a71adc1 September 19, 2018 07:05

michaelDCurran merged commit 4eb7aa5 into master Sep 19, 2018

nvaccessAuto added this to the 2018.4 milestone Sep 19, 2018

Uh oh!

Conversation

michaelDCurran commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented Sep 17, 2018

Uh oh!

michaelDCurran commented Sep 17, 2018 via email

Uh oh!

michaelDCurran commented Sep 17, 2018 via email

Uh oh!

LeonarddeR commented Sep 17, 2018

Uh oh!

LeonarddeR commented Sep 17, 2018

Uh oh!

michaelDCurran commented Sep 17, 2018 via email

Uh oh!

LeonarddeR commented Sep 17, 2018

Uh oh!

LeonarddeR commented Sep 17, 2018

Uh oh!

michaelDCurran commented Sep 17, 2018 via email

Uh oh!

LeonarddeR commented Sep 17, 2018

Uh oh!

LeonarddeR commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelDCurran commented Sep 17, 2018 via email

Uh oh!

michaelDCurran commented Sep 17, 2018 via email

Uh oh!

LeonarddeR commented Sep 17, 2018

Uh oh!

michaelDCurran commented Sep 17, 2018 via email

Uh oh!

michaelDCurran commented Sep 17, 2018 via email

Uh oh!

michaelDCurran commented Sep 18, 2018 via email

Uh oh!

LeonarddeR commented Sep 18, 2018

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Sep 19, 2018 via email

Uh oh!

michaelDCurran commented Sep 17, 2018 •

edited

Loading

LeonarddeR commented Sep 17, 2018 •

edited

Loading