547027 - Create a framework for MAPI tests

Status: RESOLVED FIXED

(MailNews Core :: Simple MAPI, defect)

Description

[Rain [:rain1] (inactive)]

We should be able to test MAPI with a native code helper by dynamically linking against mozMapi32.dll at runtime -- <http://msdn.microsoft.com/en-us/library/cc963763.aspx>.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Add a simple send-mail MAPI test

This also contains a bit of MAPI cleanup to fix other issues I've discovered:

1. MAPISendMail didn't wait until the message is sent (at least in the configuration I checked). That made checking that things work properly slightly annoying.

2. The hidden DOM window doesn't necessarily exist by default. This adds code to set this up and shut it down properly.

3. The MAPI component wasn't being added to the modules on startup, which means I could never trigger it to work properly here. [This part may need to be backported to 24. At least, if anyone files a bug saying that MAPI stopped working, this ought to fix it.]

4. LOBJS is going away in mozilla-central. This stops using LOBJS properly.

I don't know how much of the MAPI infrastructure this actually goes through. I don't know if it's hitting the proxy stuff, but it definitely hits mozMapi32.dll and everybody in mapihook/src, even if another copy of Thunderbird is set up to default MAPI (as I verified after much agony).

This isn't an urgent patch. Part 4 needs to be done before cc-rework can finish (which is, incidentally, why I started this: I wanted to be sure I wasn't breaking MAPI, so I wanted a working test first), but that can be fairly easily spliced off if necessary.

Comment 2

[Karsten Düsterloh]

Comment on attachment 791889 [details] [diff] [review]
Add a simple send-mail MAPI test

Sorry, but I'm off Windows for almost a decade now, and hence I can't test this sufficiently. :-(

Comment 3

[[e-mail]]

(In reply to Joshua Cranmer from comment #1)
> 1. MAPISendMail didn't wait until the message is sent
It never has done, has it? (I don't like Outlook's approach whereby the whole application suddenly becomes modal to a compose window created through MAPI.)

Comment 4

[Joshua Cranmer [:jcranmer]]

(In reply to [e-mail] from comment #3)
> (In reply to Joshua Cranmer from comment #1)
> > 1. MAPISendMail didn't wait until the message is sent
> It never has done, has it? (I don't like Outlook's approach whereby the
> whole application suddenly becomes modal to a compose window created through
> MAPI.)

Looking back in the history of the MAPI code, it used to wait until the message was sent, but this was "stealthily" changed in bug 108275. To be fair, the change I've made only affects blind MAPI sends.

Comment 5

[[e-mail]]

(In reply to Joshua Cranmer from comment #4)
> To be fair, the change I've made only affects blind MAPI sends.

Ah, that makes sense.

Comment 6

[[e-mail]]

Comment on attachment 791889 [details] [diff] [review]
Add a simple send-mail MAPI test

>+        mozilla::MonitorAutoLock mal(*this);
>         m_done = true;
>+        NotifyAll();
[Shame that there's no easy way of knowing whether a monitor was notified.]

>+  	NS_IMETHOD OnSendNotPerformed(const char *aMsgID, nsresult aStatus)
>+	  {
>+	  	return OnStopSending(aMsgID, aStatus, nullptr, nullptr) ;
>+	  }
Nit: indentation fail.

>+  /// Helper for setting up the hidden window for blind MAPI.
a) Why does the compose service need a window?
b) Why don't we have a hidden window?
   (Except in an XPCshell test, but then shouldn't the test create it?)

>-  return pMsgCompose->SendMsg(WeAreOffline() ? nsIMsgSend::nsMsgQueueForLater : nsIMsgSend::nsMsgDeliverNow,
>-			      pMsgId, nullptr, nullptr, nullptr);
Heh.

>   nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
>-  while ( !((nsMAPISendListener *) pSendListener)->IsDone() )
>+  while (!sendListener->IsDone())
>   {
>-    PR_CEnterMonitor(pSendListener);
>-    PR_CWait(pSendListener, PR_MicrosecondsToInterval(1000UL));
>-    PR_CExitMonitor(pSendListener);
>+    mozilla::MonitorAutoLock mal(*sendListener);
>+    sendListener->Wait(PR_MicrosecondsToInterval(1000UL));
>     NS_ProcessPendingEvents(thread);
>   }
So the idea is that we process events every millisecond? It hardly seems worth notifying.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 791889 [details] [diff] [review]
Add a simple send-mail MAPI test

Review of attachment 791889 [details] [diff] [review]:
-----------------------------------------------------------------

The build and unit test aspects of this look reasonable, but I'll leave the mapi parts to Neil to review.

Comment 8

[Joshua Cranmer [:jcranmer]]

(In reply to [e-mail] from comment #6)
> >+  /// Helper for setting up the hidden window for blind MAPI.
> a) Why does the compose service need a window?
> b) Why don't we have a hidden window?
>    (Except in an XPCshell test, but then shouldn't the test create it?)

I'll be honest and answer that I don't know on both parts. Most of this patch was made by writing the test and then fixing code until it passed--the test actually started out calling it from a separate thread using JS workers, but I simplified that gunk out when I realized I didn't need it for the test. So all the C++ changes should have been made to make the test pass, plus a tiny bit of related code cleanup (e.g., using mozilla::Monitor instead of PRCMon).

I don't know if the lack of the hidden window is do to running in an xpcshell test or if it would be present in certain scenarios from a real MAPI code.

> So the idea is that we process events every millisecond? It hardly seems
> worth notifying.

I don't know why either, but it does seem that either the notifier or the sleep can go, and I don't know which one to kill.

Comment 9

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 10

[[e-mail]]

(In reply to Joshua Cranmer from comment #8)
> I don't know why either, but it does seem that either the notifier or the
> sleep can go, and I don't know which one to kill.
I don't see how keeping the notifier is useful, as you want to pump events anyway.

Comment 11

[:aceman]

Jorg, would you be able to refresh this for Windows?

Comment 12

[Jorg K (CEST = GMT+2)]

OK, I took note, but my next project is "cache2" (bug 1021843).

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch

Rebased, made to compile, modernised (ChromeUtils.import, Assert.equal).

No idea whether it does anything useful, will try some other day.

Comment 15

[Jorg K (CEST = GMT+2)]

Oh, all the bits in configure.in | confvars.sh | Makefile.in | moz.build fell by the wayside. No idea whether they're still needed in this modern age ;-)

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch (JK v2)

Added a hunk that got lost during rebase. Not the test runs and crashes :-(

mach xpcshell-test comm/mailnews/mapi/test/unit

PROCESS-CRASH | comm/mailnews/mapi/test/unit/test_mapisendmail.js | application crashed [unknown top frame]

The crash is when calling:
hr = pNsMapi->SendMail(lhSession, lpMessage, flFlags, ulReserved);
Exception thrown at 0x00007FFE13D3F684 (rpcrt4.dll) in xpcshell.exe: 0xC0000005: Access violation reading location 0x0000001800000008.

Something for another day.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch (JK v2b)

I tried for a while to figure out why this crashes. In the process I didn't some white-space management and added some comments to the original patch.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: WIP-dont-overwrite-input.patch

Hmm, the login function overwrites an input parameter which doesn't seem like a good idea. This patch fixes it, but it still crashes.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch (JK v2c)

Updated for import changes. There was also a bug so the "new session" flag was ignored.

Now we're back to the crashing state :-(

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch (JK v3)

I made some progress here. The crash only happens when another session of TB is running. I discovered that by coincidence :-(

There was something wrong in the test setup, no default account was set up, so MAPI logon failed and it was downhill from there. I've fixed this now.

So no logging in works and sending started to work, and we run into the next issue. Something is wrong with the monitor.

I wonder whether the initial patch ever worked for Joshua given the missing default account and now this issue :-(

0:04.33 pid:10944 ###!!! ERROR: Potential deadlock detected:
0:04.33 pid:10944 === Cyclical dependency starts at
0:04.33 pid:10944 --- Mutex : MAPISendListener monitor (currently acquired)
0:04.33 pid:10944 calling context
0:04.33 pid:10944 [stack trace unavailable]
0:04.33 pid:10944 === Cycle completed at
0:04.33 pid:10944 --- Mutex : MAPISendListener monitor (currently acquired)
0:04.33 pid:10944 calling context
0:04.33 pid:10944 [stack trace unavailable]
0:04.33 pid:10944 ###!!! Deadlock may happen NOW!
0:04.33 pid:10944 [10944, Main Thread] ###!!! ASSERTION: Potential deadlock detected:
0:04.33 pid:10944 Cyclical dependency starts at
0:04.33 pid:10944 Mutex : MAPISendListener monitor (currently acquired)
0:04.33 pid:10944 Cycle completed at
0:04.33 pid:10944 Mutex : MAPISendListener monitor (currently acquired)
0:04.33 pid:10944 ###!!! Deadlock may happen NOW!
0:04.33 pid:10944 : 'Error', file c:/mozilla-source/comm-central/xpcom/threads/BlockingResourceBase.cpp, line 279
0:04.33 pid:10944 #01: MAPISendListener::OnStopSending (c:\mozilla-source\comm-central\comm\mailnews\mapi\mapihook\src\msgMapiHook.cpp:72)
0:04.33 pid:10944 #02: nsMsgCompose::OnStopSending (c:\mozilla-source\comm-central\comm\mailnews\compose\src\nsMsgCompose.cpp:3542)
0:04.33 pid:10944 #03: nsMsgComposeSendListener::OnStopSending (c:\mozilla-source\comm-central\comm\mailnews\compose\src\nsMsgCompose.cpp:3751)

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch (JK v3b)

OK, with some more tweaks this finally works, yay \o/

Let's see what the other platforms say:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=56678a50111954636cd697d55b1aabcd3952c227

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch (JK v3c)

And also keeping the linter happy ;-)

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch (JK v3c)

Sigh, now keep static analysis happy and add explicit:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eea7606ae7b9f1dc64159330d42404c19b7a29a5

Comment 24

[Jorg K (CEST = GMT+2)]

Hmm, test doesn't pass on the server :-(

INFO - TEST-START | comm/mailnews/mapi/test/unit/test_mapisendmail.js
WARNING - TEST-UNEXPECTED-FAIL | comm/mailnews/mapi/test/unit/test_mapisendmail.js | xpcshell return code: 0
INFO - TEST-INFO took 214ms
INFO - >>>>>>>
INFO - PID 3504 | Unable to load \untrusted-startup-test-dll.dll; LoadLibraryW failed: 126
INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
INFO - Error: input is not a recognizable type! at resource:///modules/mimeParser.jsm:81
INFO - MimeParser_parseSync@resource:///modules/mimeParser.jsm:81:13
INFO - MimeParser_extractHeaders@resource:///modules/mimeParser.jsm:172:5
INFO - run_test@Z:/task_1549126101/build/tests/xpcshell/tests/comm/mailnews/mapi/test/unit/test_mapisendmail.js:33:25
INFO - _execute_test@Z:\task_1549126101\build\tests\xpcshell\head.js:521:7
INFO - @-e:1:1
INFO - <<<<<<<

It works locally, so this will be fun to debug :-(

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch (JK v4)

Updated after bug 1524751.

Here's a try with
mozilla::LogModule::Get("MAPI")->SetLevel(mozilla::LogLevel::Debug);
added, maybe we can see why it's not working.

I'm assuming that MAPI may not be installed on machines in automation.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=171f85f6f39bee8e6871c9c2b7568f3ab473a2ef

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: 547027-mapi-test.patch (JK v4b)

Grrr, the explicit went into the wrong patch :-(

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ebe4402e49c76c209fa87f012a172b39ae36ffd1

Comment 27

[Jorg K (CEST = GMT+2)]

Sadly no output, still the same Unable to load \untrusted-startup-test-dll.dll; LoadLibraryW failed: 126 as in comment #24.

Do our Windows specialists have any idea? David, sorry to trouble you, any idea why that MAPI stuff works locally but not in automation?

Comment 28

[Masatoshi Kimura [:emk]]

The message comes from here:
https://searchfox.org/comm-central/rev/582bd5d57be824ff97c13c7b6e097b9cf666a953/mozilla/mozglue/build/WindowsDllBlocklist.cpp#784

GetCurrentDirectoryW returns \ for some reason.

Comment 29

[Jorg K (CEST = GMT+2)]

Sigh, those "mozilla via comm-central" links don't work :-( - You mean:
https://searchfox.org/mozilla-central/rev/69d3c6c61dca9a41f14797cd9924733289238d1a/mozglue/build/WindowsDllBlocklist.cpp#784

Comment 30

[Jorg K (CEST = GMT+2)]

Which DLL is this about? untrusted-startup-test-dll.dll? What's that? The test does:
https://hg.mozilla.org/try-comm-central/rev/78b3350064bc19e51c83934e50d546323b184f85#l5.117
let mapi = ctypes.open("mozMapi32.dll");

Maybe that needs a path?

Comment 31

[Mike Kaganski]

(In reply to Joshua Cranmer [:jcranmer] from comment #4)

(In reply to [e-mail] from comment #3)

(In reply to Joshua Cranmer from comment #1)

1. MAPISendMail didn't wait until the message is sent
   It never has done, has it? (I don't like Outlook's approach whereby the
   whole application suddenly becomes modal to a compose window created through
   MAPI.)

Looking back in the history of the MAPI code, it used to wait until the
message was sent, but this was "stealthily" changed in bug 108275. To be
fair, the change I've made only affects blind MAPI sends.

Just FYI.
MAPISendMail API includes MAPI_DIALOG_MODELESS flag, in addition to MAPI_DIALOG [1]; so the proper MAPI implementation should use modal dialogs, unless MAPI_DIALOG_MODELESS is passed. In this sense, the change in bug 108275 was wrong.

[1] https://docs.microsoft.com/en-us/windows/desktop/api/mapi/nc-mapi-mapisendmailw

Comment 32

[Mike Kaganski]

(In reply to Jorg K (GMT+1) from comment #30)

Which DLL is this about? untrusted-startup-test-dll.dll? What's that? The test does:
https://hg.mozilla.org/try-comm-central/rev/78b3350064bc19e51c83934e50d546323b184f85#l5.117
let mapi = ctypes.open("mozMapi32.dll");

Maybe that needs a path?

IIUC, that is a DLL with a testing framework (searching for that in my build tree gives several, e.g. C:\mozilla-source\mozilla-central\obj-x86_64-pc-mingw32\toolkit\components\telemetry\tests\untrusted-startup-test-dll\untrusted-startup-test-dll.dll). So likely something is missing not in the test code itself, but in the way the test is invoked (comment 28 suggests some initialization, like passing paths, missing?).

Comment 33

[Mike Kaganski]

Comment on attachment 9041020 [details] [diff] [review]
547027-mapi-test.patch (JK v4b)

Review of attachment 9041020 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/mapi/mapiDll/MapiDll.cpp
@@ +166,5 @@
>          return MAPI_E_UNKNOWN_RECIPIENT ;
>  
>      if (!lhSession || pNsMapi->IsValidSession(lhSession) != S_OK)
>      {
> +        FLAGS LoginFlag = flFlags & (MAPI_LOGON_UI | MAPI_NEW_SESSION);

Why is that?

The change would (1) pass more flags to MAPILogin than before, (2) would require LogonUI unconditionally, regardless of what caller wanted, and (3) would unconditionally prohibit reusing possible shared sessions?

Previously the checks *sanitized* the flags, because only the two flags (MAPI_LOGON_UI and MAPI_NEW_SESSION) that may be passed to MAPISendMail API are applicable to MAPILogon; so the checks simply revoved everything else. Being bitmasks, the sanitization could be simply

    FLAGS LoginFlag = flFlags & MAPI_LOGON_UI | MAPI_NEW_SESSION;

but this change seems inconsistent.

Comment 34

[Mike Kaganski]

Oh! /me only can say he is still recovering from illness - so sorry for the noise.

Comment 35

[Jorg K (CEST = GMT+2)]

The change is what you suggested. The test uses "new session" (0x02) and the hand-rolled sanity check lost that, see comment #19.

Comment 36

[(Away)]

(In reply to Masatoshi Kimura [:emk] from comment #28)

The message comes from here:
https://searchfox.org/comm-central/rev/
582bd5d57be824ff97c13c7b6e097b9cf666a953/mozilla/mozglue/build/
WindowsDllBlocklist.cpp#784

GetCurrentDirectoryW returns \ for some reason.

I noticed this on the arm64 builds too, in the taskcluster logs in bug 1524114:

INFO - PID 9868 | Unable to load \untrusted-startup-test-dll.dll; LoadLibraryW failed: 126

But I wasn't aware of this behavior of GetCurrentDirectory. Aaron, have you seen this before?

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/67e20f7993b0
Create a framework for MAPI tests. r=standard8,jorgk

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6798eb6964aa
Follow-up: Switch to reentrant monitor as originally intended. r=me DONTBUILD

Comment 39

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to David Major [:dmajor] from comment #36)

(In reply to Masatoshi Kimura [:emk] from comment #28)

The message comes from here:
https://searchfox.org/comm-central/rev/
582bd5d57be824ff97c13c7b6e097b9cf666a953/mozilla/mozglue/build/
WindowsDllBlocklist.cpp#784

GetCurrentDirectoryW returns \ for some reason.

I noticed this on the arm64 builds too, in the taskcluster logs in bug 1524114:

INFO - PID 9868 | Unable to load \untrusted-startup-test-dll.dll; LoadLibraryW failed: 126

But I wasn't aware of this behavior of GetCurrentDirectory. Aaron, have you seen this before?

Can't say I have.

Comment 40

[Intermittent Failures Robot]

1 failures in 2658 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 1

Platform breakdown:

• windows10-aarch64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=547027&startday=2019-02-04&endday=2019-02-10&tree=all

Comment 41

[Jorg K (CEST = GMT+2)]

(In reply to Pulsebot from comment #38)

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6798eb6964aa
Follow-up: Switch to reentrant monitor as originally intended. r=me DONTBUILD

For the record, I discussed this with Joshua on IRC:
00:26:57 - jorgk: why was there a need for one at the first call site? [1]
00:27:20 - jcranmer: because stuff can be on different threads
00:27:58 - jcranmer: I thought monitor was reentrant on single threads

[1] https://hg.mozilla.org/comm-central/rev/67e20f7993b0f4ce493efca29b24edbe8448e5dc#l2.45

The original patch had:

-        PR_CEnterMonitor(this);
-        PR_CNotifyAll(this);
+        mozilla::MonitorAutoLock mal(*this);
         m_done = true;
-        PR_CExitMonitor(this);
-        return NS_OK ;
+        NotifyAll();
+        return NS_OK;

but that caused a deadlock situation since Monitor is not reentrant:
https://searchfox.org/mozilla-central/rev/5e3bffe964110b8d1885b4236b8abd5c94d8f609/xpcom/threads/Monitor.h#16

See comment #20. But the tweak to just remove mozilla::MonitorAutoLock mal(*this); wasn't the right way to fix this, hence the follow-up that restores a reentrant monitor at that call site.