According to the information in this bug, for some users it sometimes works, and sometimes it doesn't. And several users had mentioned they might have a mix of 32bit and 64bit software installed. Then I saw some reports on the web, which claimed that the sendto isn't working on 64bit systems, or that the way mapi is working on recent windows systems has changed.

I suggest to begin analyzing this issue with a fresh 64-bit machine, with a thunderbird 64-bit the only version ever installed. We should learn if that works, or fails, and how often, and if necessary, add some printf statements for logging to a debug build.

I tried to start with that approach on Windows 10, but apparently it doesn't work at all.

I haven't used a TB installer. I have a local comm-central debug build. On start if offers to set it as the default mail client, and I accepted.

I used a web page with a <a href="mailto:..."> link, and when clicked from Firefox, I confirmed that the TB debug build is started, and the mail compose window opens.

Then I used file explorer, right click, send to mail recipient. I get an error, that no matching application is installed or configured.

I went back to the Windows default apps. I changed the preferred Mail application to the Windows Mail program. Attempted again to "send to", and still the same failure message is shown!

If you open the Windows SendTo folder (that's the place where the shortcuts for the send-to right click menu are managed), I learned that the "email recipient" file has a ".mapimail" extension.

In Windows "default apps", if you scroll down, there's an option to "choose default applications by file type". Click, and scroll down, until you see ".mapimail" on the left hand side. On my system, the right hand side is unconfigured, it offers "+ choose a default". When clicked, the only selection I'm offered is: "Look for an app in the app store".

I conclude, neither the default Windows Mail app, nor Thunderbird are registered with the system as appropriate destinations for this message type.

Could this explain why users have trouble with the 64-bit version of Thunderbird and the send-to feature?

Could the reports, that it sometimes works, be related to 32-bit applications still being registered somewhere in the system, and Windows still using them? (potentially because it works differently on the win32 subsystem)

I found several discussion threads with people not being able to use the send-to email feature. The only solutions I found, which were confirmed by some users, were about Outlook.

I suggest that someone researches how to make Thunderbird compatible with .mapimail, or make it compatible with it.

Additional experiment. In the SendTo folder (user/appdata/roaming/microsoft/windows/sendto) I double clicked the mail recipient file, and was offered to select an application. I navigated to the dist/bin directory of my local build and selected thunderbird.exe

This started thunderbird composer. When double clicking the mail recipient icon again, thunderbird starts again.

However, when using the file explorer to look at other folders, and use the right click menu to open the "send to" menu, the mail recipient option is gone!

Back to the default programs, choose by file type, the entry .mapimail now has an assignment to "thunderbird daily". So it's configured, but for some reason Windows believes it's incompatible, and removes it from the sendto menu???

Before we can consider a bug in Thunderbird, we need a learn about reliable way to configure this on a fresh system.

Umm, you must run an installer. To make one: mach pacakge. If you use Windows, "Everything" desktop search is a great little tool. It will help you to locate
... obj-x86_64-pc-mingw32\dist\install\sea\thunderbird-66.0a1.en-US.win64.installer.exe

For the default registration see bug 1509918 and bug 1517955.

Well, mach package.

Jörg, thanks for clarifying that the integration performed by the Thunderbird executable is insufficient, and thanks for pointing me to the installer.

After running the installer, "file explorer right-click send-to mail" no longer complains "no app", but it runs Thunderbird.

After playing with it for a while, I've observed:

If you skip running thunderbird immediately (disable checkbox at the end of the installer), and if you don't run thunderbird manually, but if "right-click send-to" is used as the very first action, then it doesn't work.

I could reproduce that with my debug build and my local installer. Because the debug binary is slow, when using send-to, I can see that a thunderbird process is being started (the debug build opens a terminal window, and I can see some trace statements).

This window closes after a few seconds, without bringing up any Thunderbird window. Using the thunderbird debug build, I can reproduce this multiple times.

Now, I started the thunderbird program, and waited until it opens up. During the first execution, it might be slow, and show the "spinning mouse cursor" for a longer amount of time. If I attempted to use "send-to" very quickly after starting thunderbird, I saw it once or twice, that the main thunderbird window closed (crashed?). But not always, sometimes, the compose window open fine, even if I clicked it quickly.

However, as soon as I've allowed thunderbird (after installation) to open, and have given it sufficient time (10-20 seconds), the right-click send-to mail ALWAYS works for me. After it worked once, it never failed for me again. Even after a reboot, if I don't start thunderbird yet, but use the right-click send-to mail as the first thunderbird action since boot, it works fine, and both the compose window and the thunderbird come up, and work.

If I uninstall, reboot, install again, I can again reproduce the behavior described above.

This could mean that Thunderbird triggers some system registration in the background. Only after that registration has succeeded once, it works reliably.

In addition to the above tests, which used my own debug binary and installer, I've also test using a download 60.4 release.

I want to note that the default download site sent me to a 32-bit Thunderbird installer, despite me using a Firefox 64 bit on a 64 bit windows. Initially, I didn't notice and installed the 32-bit build. With that, again I didn't allow the installer to run thunderbird immediately, and I used the right-click send-to mail as the first action. That immediately worked. So maybe the 32-bit version is indeed better.

I uninstalled 32bit, rebooted, manually searched for the 64-bit release, installed. Skipped launching ob thunderbird by installer. Use right-click send-to as first action. And it failed. Nothing happened.

Then, I immediately tried right-click send-to again. Now it worked.

So, my theory from above, that it needs a full start of thunderbird before it works, isn't true for the release binary.

Again, from this point, it always worked. I rebooted, and send-to as first action still worked.

My summary of the above is:

• I can reproduce a send-to failure immediately after install
• I cannot reproduce a failure as soon as thunderbird has been allowed to fully start up once

This brings up two questions:

(a)
Can anyone reproduce the failure more reliably?
After an install has worked for you once, do you ever see it nonworking?
(Please consider that each time after you have updated, you might allow it to start fully, before send-to works.)

If yes, then you have a problem that I cannot reproduce yet.

(b)
If the answer to (a) is "no", in other words, if some kind of (small) delay is necessary, until a new install works fine, is it worth to to spend more time to debug this further?

Further analysis that could be made are:

• does the windows registry look differently before "first attempt with failure" and "once it works"?
• same question for thunderbird profile?
  (if differences are found, could these changes be responsible for the occassional initial failure, and could we apply them immediately?)

If no changes are found, we should do some more experiments, to investigate if there's any time delay involved. If we wait for a longer time after the installer, will the first send-to attempt work immediately? If yes, this could mean some delay inside the windows registration stuff, which we might not be able to influence (this is just a wild guess, but it might be useful to rule this possibility out).

IMHO, only if the answer to (a) was "yes", or if the suggested investigations for (b) produces no solution, then we could spend more time on debugging this at the code level. It would require to identify the code pathes that are triggered by the right-click send to, and add some printf style tracing to a logfile. Probably two processes are involved, so we'd have to log to a file that includes the process id in its logfile. I'm guessing that it doesn't "crash", because crashes should trigger some crash reporter, right? Rather, I guess that some assumptions/expectations are not met, and the code decides to exit.

Hmm, yes, if you run TB, is does some system integration stuff (badly these days), you can see a discussion in bug 1509918.

Since x64 versions are not fully supported, we don't advertise them, but you can still get them, for example from here:
http://ftp.mozilla.org/pub/thunderbird/releases/60.4.0/win64/

So you didn't see the crash? Try this: Have TB running and then do the "Send to > Mail recipient". It takes down the running TB :-( - The bug is getting (too) long, but the crash is there, see comment #19 and corresponding attachment. You can even get a local build to crash.

Confirmed in comment #33. No "exit", it's a crash :-(

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

So you didn't see the crash? Try this: Have TB running and then do the "Send to > Mail recipient". It takes down the running TB :-( -

No crash for me. Windows 10 64bit, Thunderbird 60.4.0 64bit release installed.

Thunderbird is running, click on file in file explorer, send-to mail, new compose windows open in thunderbird, no crash.

Now! I can reproduce. Using my local debug build. As you said. Thunderbird exited three times in a row, on each attempt.

Just when I was gearing up to reproduce it ;-)

Yep, tried just now using TB 60.4 x64 bit. Running app just died :-(

I was able to reproduce it in the debugger.

I understand the bug. It's a memory cleanup issue.

When send-to is used, Windows loads the mozMapi32.dll, calls the MAPISendMail callback, and gives it memory that contains information about the requested message.

mozMapi32.dll then uses inter-process-communication to forward the request to the thunderbird process.

At the time we crash, thunderbird has completed executing the CMapiImp::SendMail function (which implements the NSIMapi.SendMail MS-COM interface). Then the OS runtime libraries attempt to free the memory received through the IPC interface. This crashes.

The reason is that the initial mozMapi32.dll simply forwards the memory that it has received from Windows. But Windows it allowed to free that memory, after the initial process is done sending the message to thunderbird.

The solution is to change mozMapi32.dll to copy the memory it has received by Windows, pass it with IPC to thunderbird, and don't free it. The pointers are given to thunderbird, and the OS runtime libraries will free them.

I have a fix in hand, which fixes the crash for me. I need to spend some more time to clean it up, and handle all possible scenarios, before I can attach it.

Also, I found a misdefined type in msgMapi.idl, which I need to clean up, too.

Great. Why doesn't it crash on x86 though? Pure luck?

Difficult to say. Maybe luck, maybe the order of events is different on x86. If mozMapi32.dll stays alive on x86 until after the CMapiImp::SendMail is completed, the freeing inside thunderbird succeeds, and the inability to free in the calling dll is ignored? This is just wild guessing. But duplicating the memory and passing it over should be safe on all architectures.

Jörg, does this patch work for you?

FYI, the existing code registers the MAPISendMail callback. That API uses parameters which are defined by the Windows/MAPI SDK.

In particular, parameter lpMessage is defined here:
https://docs.microsoft.com/en-us/windows/desktop/api/mapi/ns-mapi-mapimessage

Our code attempts to emulate this nested data type, see nsMAPI.idl, using the MS-COM IDL rules.

It has the nested element lpFiles, with the type defined here:
https://docs.microsoft.com/en-us/windows/desktop/api/mapi/ns-mapi-mapifiledesc

It uses LPSTR for path and file names.

However, our existing code deviates, and use type LPTSTR instead.
This is a mistake. We must use the types defined in the API.

While there is a W version of the APIs, which uses unicode and LPTSTR, we cannot mix the types, it must be either all without W or all with W.

Despite not using the W versions, sending a file from a local path containing Unicode characters, sending such files works fine for me on Windows 10. I'm guessing it uses UTF-8 for transport, which works with LPSTR.

I don't know the background of using LPTSTR in MapiFileDesc in the Thunderbird code. Maybe it was a mistake. Maybe it was an attempted hack to fix Unicode paths on some Windows versions? But I cannot imagine how changing the type could have helped, because the API as defined by the SDK defines what Windows will put into the parameters, and changing the type of the transport structure doesn't change what we receive.

kaie, where have you been these 12 years? :)

Thanks for touching this code. WRT LPSTR, this may not be terribly insightful but the decision perhaps was made in the simple mapi days, eg bug 108275. Note also, there was an effort to change simple mapi to LPSTR in bug 594239

I'll try the patch some other day/night.

Some remarks. Please do not use tabs. Or indentation is two spaces, although some old code is inconsistent. There are also no spaces between function names and (, so foo(x). We typically fix these things in the vicinity, like here in
hr = pNsMapi->SendMail ( ....

I've last tweaked this code in bug 1505315: https://hg.mozilla.org/comm-central/rev/e1449ad9e4d6

There was also an attempt to implement the MAPISendMailW interface in bug 1506488.

I think it makes no difference whether we use LPSTR or LPTSTR as long a the content is treated as char. But there's no harm in correcting it.

Finally I don't know what you mean by "local path containing unicode characters". Windows uses its own code pages, like windows1252 for most "Western" installation, or cp932 for Japanese. There is also UTF-8 as experimental system charset in Windows 10 ([ ] Beta: Use Unicode UTF-8 for worldwide language support"). Have you tried a filename with some CJK in it? You could see what arrives at the interface. NS_CopyNativeToUnicode() would assume the local code page as input, not UTF-8.

I'm not sure that the Unicode path that you're removing isn't triggered, see:
https://searchfox.org/comm-central/rev/f383ce2e20ebab18bc89bb305a3d6a1349020f3a/mailnews/mapi/mapihook/src/msgMapiImp.cpp#219
In fact, if that were never triggered, we could remove one of the PopulateCompFields* functions, no?

Bug 594239 is certainly interesting ;-)

Comment on attachment 9036164 [details] [diff] [review]
393302-v1.patch

Tested with SeaMonkey 2.53 x64 and no longer crashes under Server 2016. Mail window with attachment is created in the background. But the z-order problem is either something SeaMonkey specific or present since ages too.

Great job.

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

Some remarks. Please do not use tabs. Or indentation is two spaces, although some old code is inconsistent.

Agreed, mixing tabs and spaces is painful. The editor inside the Windows VM wasn't setup correctly when I started writing the code. I'll fix this.

There are also no spaces between function names and (, so foo(x). We typically fix these things in the vicinity, like here in
hr = pNsMapi->SendMail ( ....

Besides tab/space mixing, I'm don't mind whitespace details much. I'm ok to fix the places that you point out and don't like, but I have a slight preference to avoid whitespace changes, as they can make a patch unreadable quickly (and make it more difficult to see where the relevant changes are).

Finally I don't know what you mean by "local path containing unicode characters".

I wanted to ensure that this patch doesn't make it worse regarding those characters. Yes, I had test a directory name consisting of japaneses characters, and it worked.

I'm not sure that the Unicode path that you're removing isn't triggered, see:
https://searchfox.org/comm-central/rev/f383ce2e20ebab18bc89bb305a3d6a1349020f3a/mailnews/mapi/mapihook/src/msgMapiImp.cpp#219

I'm not removing this code you're pointing to.
With the change of type LPTSTR to LPSTR, we muse use set aIsUnicode to false in function HandleAttachments. After this change, all calls to HandleAttachments use parameter false, so the parameter can be removed, and the logic for "true" can be removed in this function, because it's never used.

v2 fixes whitespace, no other changes

Thanks. I agree, too many white-space changes can be confusing, but we can certain fix issues on lines which are already touched, like:

+        memset (Recipients, 0, sizeof (nsMapiRecipDesc) );
should be
+        memset(Recipients, 0, sizeof(nsMapiRecipDesc));
+        memset (Files, 0, sizeof (nsMapiFileDesc) );
+  rv = HandleAttachments (aCompFields, aMessage->nFileCount, aMessage->lpFiles) ;  - 2 times
+            pFile->InitWithNativePath (nsDependentCString(aFiles[i].lpszPathName));
and more.

I know you're not removing the code I pointed out, but I'm asking myself whether we should do a broader clean-up here and remove all dead or non-functioning code including nsMapiHook::PopulateCompFields() which I doubt will be triggered or work. Refer to attachment 473834 [details] [diff] [review] of bug 594239 which did remove the code I pointed out.

JFTR:
MAPISendMailW

(In reply to Kai Engert (:kaie:) from comment #59)

Despite not using the W versions, sending a file from a local path containing Unicode characters, sending such files works fine for me on Windows 10. I'm guessing it uses UTF-8 for transport, which works with LPSTR.

MapiMessage structure
| MapiMessage structure:
[...]
| ulReserved
|
| Reserved; must be zero or CP_UTF8. If CP_UTF8, the following are UTF-8
| instead of ANSI strings: lpszSubject, lpszNoteText, lpszMessageType,
| lpszDateReceived, lpszConversationID.

vs.

MapiMessageW structure
| ulReserved

| Reserved; must be zero.

Comment on attachment 9036207 [details] [diff] [review]
393302-v2.patch

With these registry settings
[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\CLSID{29F458BE-8866-11D5-A3DD-00B0D0F3BAA7}\LocalServer32]
""=""C:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\bin\thunderbird.exe" /MAPIStartup"

[HKEY_LOCAL_MACHINE\SOFTWARE\Clients\Mail\Mozilla Thunderbird]
@="Mozilla Thunderbird"
"DLLPath"="C:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\comm\mailnews\mapi\mapiDLL\mozMapi32.dll"

Window's "Sent to > Mail recipient" will start the local build, no installer required (see comment #19 from two years ago.

Without the patch, my x64 build crashes, with the patch, it works :-)

I tried a filename with non-ASCII characters but within the local code page of windows-1252 (äö€ß.txt) and also a filename with Japanese (テスト.txt) and that doesn't work at all. TB isn't even launched as far as I can see. Most likely, whatever representation Windows uses for the filename internally, it detects that Japanses cannot be converted to the local ANSI code-page and refuses to work. That's good news since internally we now use NS_CopyNativeToUnicode() and that would mess up completely if UTF-8 were interpreted as native ANSI. Of course, say, Polish users will use windows-1250 and it will work for them. For me a file with many "Polish" characters (ąęćżźłóńś.txt) launches TB but doesn't launch the compose window, so that's inconsistent with Japanese. For those cases to work, we'd need to implement MAPISendMailW (bug 1506488). I also tried sending files with ASCII-names from folders called テスト and ąęćżźłóńś. In the former case, nothing happens, in the latter case TB starts but with no compose window.

So Kai, please let us know how you want to proceed here. I see these options:

1. Fix the white-space nits and get it landed as it is. I can fix those nits to avoid further noise.
2. We also remove the dead/non-functioning code I pointed out in comment #67, or I can do that in bug 594239.

For full unicode support we need bug 1506488.

Anyway, thanks for picking this up and finding the root cause of the problem. It looks like it needed a fresh pair of eyes since everyone else was busily trying to find out why 32bit worked and 64bit didn't when apparently 32bit only worked by luck.

Addition: Switching the Windows system locale to UTF-8, see comment #61, regional settings, [ ] Beta: Use Unicode UTF-8 for worldwide language support, makes the テスト.txt and ąęćżźłóńś.txt files work as well :-) - So MS did get something right after all ;-)

I guess Kai used this setting in his VM since he reported Japanese filenames to be working. Surely if NS_CopyNativeToUnicode() detects UTF-8 as local code page, it will do the right thing.

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

I guess Kai used this setting in his VM since he reported Japanese filenames to be working.

FYI, in my VM it's not checked.

Jörg, thanks for the explanations.

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

So Kai, please let us know how you want to proceed here. I see these options:

1. Fix the white-space nits and get it landed as it is. I can fix those nits to avoid further noise.

Thanks, I accept your offer.

(I haven't looked at the other cleanup suggestions. Handling it separately sounds good.)

OK, I'll get this landed and backported.

(In reply to Kai Engert (:kaie:) from comment #71)

FYI, in my VM it's not checked.

Hmm, hard to tell what's going on there. The underlying Linux file system surely uses UTF-8 names, so maybe it's all UTF-8 under the cover even without checking the box. Somehow the VM maps the local file system into the VM's file system. What I observed on a real system certainly makes sense. Anyway, the entire encoding discussion is separate from fixing the crash by copying the data and correcting the types. And with that, we should be very happy since it was the blocker for the x64 roll-out on Windows.

As anxious as we may be to ship this to users, I think this should have a good settle time in beta before hitting esr

Here's the patch with the white-space issues fixed. I've also added checks after each call to CoTaskMemAlloc. Use interdiff to see the changes.

Comment on attachment 9036358 [details] [diff] [review]
Kai's 393302-v2.patch, plus Jörg's whitespace cleanup and more memory checking.

I've looked at this for a while now and I've come up with some questions and noticed some omissions:

1. Can you clarify comment #56: "The pointers are given to thunderbird, and the OS runtime libraries will free them." - The way I understand it, Windows interfaces with MAPISendMail in MapiDll.cpp and that somehow calls into a TB processes (via IPC) and the SendMail() function. What is the process of freeing the memory we allocate?
2. Do you understand why the SendMail() interface passes the message and the counts of recipients and file attachments? Why can't the message structure hold those pointers and the interface be reduced to just passing the message? These assignment on the receiving end aMessage->lpRecips = aRecips; appear strange when the could be done in the caller. Hint: I tried doing what I just said, and it doesn't work any more, so there must be some reason.
3. I think you forgot to populate flFlags in the file copy, as well as ulRecipClass in the recipients and flFlags and lpOriginator in the message itself. Whilst these fields might not matter for the "Sent to > Mail recipient" function, they would matter in a full MAPI implementation, do you agree?

Please start further work with the patch I attached.

Here's my attempt to simplify the interface, and it doesn't work :-(

I tried to understand how the invocation mechanism works and whether there's anything special about the SendMail() interface. In this patch I've renamed it, I'm pretending that I'm passing one file and I'm setting the message in MapiDll.cpp. It's still working, and one can create a message with more than one file. As soon as you change the interface like I did in the other patch, things stop working. Windows must somehow know about that interface since in one experiment I passed one file with an allocated but empty structure in it seemed to crash Windows Explorer. I'm not really doing this for fun, I want to work out how it hangs together and whether we really don't need to free what we allocate, and also, why we need the extra parameters when we can pass everything in the message structure.

User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 SeaMonkey/2.53
Build-Identifikator: 20190114130007

Works for me. Great job.

Jörg, if you want to make more changes, we'll have to invest much more time in this and learn in more detail how MS-COM works. Does this part of the code deserve that much attention?

The inter-process communication here apparently involves serialization and deserialization of data. If it works when having the lists (recip and files) as separate parameters in the IDL, then possibly Windows will use the instructions in the function signature of the IDL to correctly handle them. You said it doesn't work if the lists are transfered inside nsMapiMessage. So apparently, despite the nested type nsMapiMessage also using statements like size_is (nRecipCount), it doesn't work. In order to understand why, we'd have to become experts for MS-COM message exchange.

The code in MapiDll.cpp contains the following comment:
// The MS COM type lib code generated by MIDL for the MS COM interfaces checks for these parameters
// to be non null, although null is a valid value for them here.

Because of that comment, I allocate an empty structure for the lpRecips parameter and memset it to zero.

However, when I was debugging on the receiving side inside CMapiImp::SendMail, parameter aRecips pointed to random memory, not filled with zero. That's fine, because we ignore it, as aRecipCount is zero.

Apparently the Windows MS-COM IPC code decided it doesn't matter what aRecips is set to, because aRecipCount is zero.

It also shows that CMapiImp::SendMail doesn't receive the direct values that were originally given to the SendMail call inside MapiDll.cpp, but some transformation is in between.

I'm suggesting that we do the minimal amount of changes to fix this bug, because apparently, the code mostly works. The knowledge the original authors had on this, might have been broader than what we have currently. We shouldn't make more, unnecessary changes, unless we're willing to learn about how it all works. I'd like to avoid an trial and error approach.

The original bug was clearly a memory issue, because it crashed inside NdrOleFree, a wrapper for CoTaskMemFree. So obviously incorrectly allocated, or incorrectly owned memory is involved. That's all that's really obvious, without being an expert on MS-COM IPC.

The correct way to construct and manage the memory exchanged, might be to use functions like CoCreateInstance to instantiate the data we want to send, use AddRef() on the calling side, and use Release() on the receiving side, once we're done. I don't know how close the existing code is to use that approach. As I said, if we want to go the route to make it completely correct, we'll need to learn and spend much more time on this, and it might result in a bigger rewrite.

https://docs.microsoft.com/en-us/windows/desktop/api/combaseapi/nf-combaseapi-cocreateinstance
https://docs.microsoft.com/en-us/previous-versions/ms810016(v=msdn.10)

Well, I've looked through the MAPI code, and it's really in a deplorable state, I'll file a new bug for it. Here some examples:

MapiDll.cpp: SetPointerArray() never used, hence, memArray never written, hence MAPIFreeBuffer() most likely doing nothing. MAPISaveMail() doing a bunch of stuff, then returning MAPI_E_FAILURE unconditionally.

As for your question: Does this part of the code deserve that much attention?. MAPI is the official interface where Windows communicates with the e-mail client, this interface consists of various functions. We have no knowledge which functions actually work and which ones don't, and worse, and we have no knowledge which function are used by other Windows software, like MS Office or LibreOffice, interfacing with TB through MAPI. All we have looked at in this patch is where Windows itself interfaces with TB through "Send to > Mail recipient". In this case, as debugging showed, there is no recipient and we only get a bunch of files.

I'm about 100% sure now that your patch breaks the MAPISendMail() for other applications, for the simple fact mentioned in comment #77 point 3. You don't copy over various fields which are used at the receiving side, I've checked the code. So if some Office or scheduling software tries to send a message with To, CC and BCC, and originator set, we've just broken that. So despite the joy of having the issue fixed, the patch cannot land in its current form.

In general, fixing software we don't understand, is very treacherous ground. I can understand that we've run into some memory ownership problems here which you fixed by taking (so far incomplete) copies. I'm not convinced that this memory isn't leaked. We would have to make the conscious decision to accept that.

As you correctly point out, some transformation happens between MapiDll.cpp and CMapiImp::SendMail(), as my investigation patch shows, the message is passed intact, so I don't understand why we need to pass recipients and files separately. You can pass one file, and if the message specifies two, two get attached.

I think key to a fix that will hold water in the medium term is to understand who the passing works and who is responsible for freeing the memory.

At the very least, to move this patch forward, we need to copy over all the fields I mentioned, and personally I've like to assign the recipients and files into the message structure in MapiDll.cpp and not at the receiving end. It would be great to understand why that CMapiImp::SendMail() interface apparently cannot be changed since MAPISendMail() does not have separate arguments for recipients and files.

Masatoshi-san, do you know?

One more thing: I know from other bugs that MAPI is used by all sorts of weird 3rd party applications, not only the mentioned office packages. So the very least we need to do is to maintain it in working state.

I agree that we should copy over the integer flags. Yes, the suggested patch might potentially leak. If we want to ensure this is avoided, we'll probably have to instantiate COM objects, and use IUnknown reference counting (not XPCOM).

If you're certain the suggested workaround has negative side effects, the patch should probably be marked as r- or obsolete.

I don't know what the way forward here is. At the very least, we need to copy over the integer flags and the lpOriginator.

You mentioned the risk of potential regression. Do you know how to test and confirm that a new patch don't introduce
MAPI regressions?

Jörg, it's your call if you accept a minimal patch that potentially leaks and doesn't implement your mentioned API change.

Perhaps Mike can offer some advice (but he hasn't been around in a while)

(In reply to Kai Engert (:kaie:) from comment #87)

You mentioned the risk of potential regression. Do you know how to test and confirm that a new patch don't introduce
MAPI regressions?

Jörg, it's your call if you accept a minimal patch that potentially leaks and doesn't implement your mentioned API change.

At the very least, we need to copy over the mentioned fields. Any objections to do that? Then we can try at least LibreOffice to send.

Further before we ship this in TB 60.x, we should involve the reporters and commenters from these bugs: bug 1367141 (using SOCMAPI.EXE to create MAPI emails from our custom Helpdesk software in Thunderbird for years), bug 1370814 and bug 1366196.

That "helpdesk software" might pump out lots of messages and leaking memory may be an issue, I can't tell.

Personal note: While bug 1367141 was a WONTFIX, I've written an add-on for these people and the ones from bug 1367716, so they could keep their workflow.

Hi! :-) Nice to meet you.

@Wayne: I'm sorry, tried to briefly follow the thread, but it's quite lengthy... could you brief me wrt what my advise should be about?

By the way, as a LibreOffice developer working on Windows x64 and using TB, I can say that personally I don't see any problems (except for lack of Unicode MAPI which I had prepared a patch for, but which didn't get any review so far ;-))

BTW, with the patch, I can send a document from LibreOffice, but it's the "no recipient, single file case" as well.

Hi Mike, long time, no see ;-) - You're working for LibreOffice now. Do you have knowledge of MS-COM? What do you think of the patch? And were is your patch awaiting review? Oh, for LibreOffice, not TB?

Hi Jorg! o/

Well - wrt COM - I need to re-learn things every time I need to touch it ;-)
But which patch exactly is being discussed - I see some obsolete, and some not...

The patch I referred to is for bug 1048658 (/me doesn't loose a chance to advertise own patches when possible, in the hope nobody notices the offtopick :-P)

FYI, I found a sample application for Aimple MAPI here:
https://support.microsoft.com/en-us/help/171096/simple-mapi-console-application

Comment on attachment 9036387 [details] [diff] [review]
API experiment 1

Better description for this attachment

Comment on attachment 9036440 [details] [diff] [review]
API experiment 2

Better description for this attachment

Comment on attachment 9036358 [details] [diff] [review]
Kai's 393302-v2.patch, plus Jörg's whitespace cleanup and more memory checking.

Renaming this patch as well.

I intend to attach a further enhancement of this patch, which copies the additional fields that Jörg mentioned.

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

FYI, I found a sample application for Aimple MAPI here:
https://support.microsoft.com/en-us/help/171096/simple-mapi-console-application

Interesting. Yes, that exercised the various MAPI functions without telling us how to implement them on the other side. Could be good for testing though.

(In reply to Mike Kaganski from comment #93)

Well - wrt COM - I need to re-learn things every time I need to touch it ;-)
But which patch exactly is being discussed - I see some obsolete, and some not...

(v3) is what we're trying to land with some additions. I'd like to understand why the "follow-up" on top of that breaks things. And the investigation patch tweaks (v3) so it still works.

Oops, they've been renamed, so experiment 1 doesn't work, but experiment 2 does.

This copies the additional fields. I've moved the redundant copy code to local (static) helper funtions.

Comment on attachment 9036603 [details] [diff] [review]
393302-v4.patch

This is still working ;-) - and copying all the fields, so we don't lose functionality. Oh, great clean-up of the ugly duplication.

I'll see whether Mike has some input, he used to be at GMT+10 or so. Otherwise I'm happy to go with this for now.

Comment on attachment 9036387 [details] [diff] [review]
API experiment 1

Mike, this used to apply on top of (v3), but you get the idea. Any idea why this patch breaks the functionality completely? Where are the arguments of SendMail() set in stone? Also please take a look "experiment 2" which doesn't change the function, but transports the information inside the message object.

I guess we're not so worried about memory management here, more about fixing the crash. It's highly unlikely that we run out of memory. As far as I can see, all the memory allocated here to create a clone of the message is leaked anyway, at least we don't understand the mechanism of it being free'd.

The feedback we're seeking from you is what you think of the approach in general. Since the 32bit binaries don't crash, the memory passing between the MAPI DLL and the callee in TB must be different for 64bit. I was dreaming of a one-line fix where we tell Windows to handle the 64bit case the same as the 32bit case so no further changes would be necessary here.

So right now, we either go with the proposed solution of making a copy, mismanaging it a bit if some later allocation fails, and then leaking it all in the end ;-(

Sigh. I need to setup TB development environment. Sorry for the silly question (I try to save a couple of search cycles): could you give a direct link to the up-to-date manual how to setup it?

I'll need that anyway for the MAPISendMailW bug ;-) - Note comment #69 (registry entries) to get your local build to handle the MAPI requests. Other than that, it's still the same instructions:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Thunderbird_build
Also the "Windows Build Prerequisites" on that page.

This also might be worth a read:
http://lists.thunderbird.net/pipermail/maildev_lists.thunderbird.net/2018-April/001152.html
Ignore the P.S., the build instructs were brought up-to-date.

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

The feedback we're seeking from you is what you think of the approach in general. Since the 32bit binaries don't crash, the memory passing between the MAPI DLL and the callee in TB must be different for 64bit.

It could be a timing issue, and a different order of cleanup routines. For example, potentially the 32-bit MAPI/MS-COM routine could block until its done, while on 64-bit potentially it could be implemented inside Windows fully asynchronously. This could potentially cause the MS-COM cleanup inside the 64-bit thunderbird process to happen earlier than in the 32-bit.

Mike, our suggested hack allocates new memory, which we pass on into MS-COM, and never free it, in the hope it avoids all crashes.

I suspect the correct fix would be to instantiate real MS-COM objects, and use MS-COM reference counting.

But we currently haven't learned how to do that correctly.

Currently, API nsIMAPI.SendMail passes an object of type nsMapiMessage. How would we correctly instantiate an MS-COM object of that type, allowing us to use AddRef() and Release() on it? (Which probably would avoid the crash and wouldn't leak.)

I believe the original implementor of the code didn't know that (and didn't know about the need to use reference counting on it, because the old code simply forwarded the same pointer, as received from windows, into the remote MS-COM call nsIMAPI.SendMail.)

But the original implementor had apparently noticed issues when forwarding nested arrays inside nsMapiMessage, when forwarding it with the nsIMAPI.SendMail call. That's why the original implementor had chosen to move the nested arrays (lpRecips and lpFiles) to parameters of the nsIMAPI.SendMail call.

(Jörg's experimental patch attempted to remove the separate parameters from the call, and instead move them into the structure - which didn't work.)

Jörg's thought is, can we avoid learning all the MS-COM object handling details ourselves, and rely on your experience to help us out?

Is there something missing in the "Building Firefox for Windows" referenced from "Simple Thunderbird build" with a note "To build on Windows, please read the prerequisites so you don't skip preparing the MAPI headers needed to compile Thunderbird"? I don't see anything there mentioning MAPI, and I had got missing MAPI headers errors. The directions for MAPI are down there on the "Simple TB Build", so maybe move them above to replace the warning?

I have built TB now (since I started my work for LO, I always admire how fast TB is built) - but it turned out to be 32-bit. I see some mentions of mozconfigs for 64-bit, but they all lack anything 64-bit-specific. Of should it be somewhere outside of the mozconfig?

(In reply to Mike Kaganski from comment #110)

I have built TB now (since I started my work for LO, I always admire how fast TB is built) - but it turned out to be 32-bit. I see some mentions of mozconfigs for 64-bit, but they all lack anything 64-bit-specific. Of should it be somewhere outside of the mozconfig?

from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites#Common_problems_hints_and_restrictions

If you want to build 64-bit Firefox, add the two lines below to your mozconfig file:
ac_add_options --target=x86_64-pc-mingw32
ac_add_options --host=x86_64-pc-mingw32

(In reply to Kai Engert (:kaie:) from comment #108)

(Jörg's experimental patch attempted to remove the separate parameters from the call, and instead move them into the structure - which didn't work.)

Hmm, well, using the content from within the message does in fact work, see experiment 2 where I pass one file separately, yet you can still attach two. What doesn't work is dropping the parameters from the function, experiment 1.

Mike, I fixed https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Thunderbird_build now. The MAPI stuff was on the Windows pre-requisites page, but it got removed and placed in the TB troubleshooting section :-(

As for 64bit: Yes, this is needed in your .mozconfig:

ac_add_options --enable-application=comm/mail
ac_add_options --enable-calendar
ac_add_options --enable-debug
ac_add_options --target=x86_64-pc-mingw32  <==
ac_add_options --host=x86_64-pc-mingw32    <==
ac_add_options --enable-warnings-as-errors
ac_add_options --enable-tests

A couple more of off-topic things while I'm rebuilding:

1. I believe ac_add_options --host=x86_64-pc-mingw32 should be redundant (a minor thing possibly not worth mentioning) - but overall, putting the 64-build hint to the TB build page would be fine (somewhere in Build configuration)
2. On TB page, the requirement to run ./mach bootstrap is missing; and the logic of the text implies that ./mach build is run after cloning and creating mozconfig (as these steps are explicit on the page, it looks natural that the reference to Prerequisites ends with, say, installing VS for Windows) - but skipping the step will fail. Also, it's not evident which options to choose from the four options all mentioning FF; and if it's required to say "Yes" when it asks to create a .mozconfig (iirc) directory under user profile (it is).

Well - while I'm testing, some thoughts.

The IP communications here are quite involved.

1. An application finds the default mail provider in registry;
2. It tries to load its DLL - it must be of correct bitness (32- or 64-);
3. If it can, the DLL is run in the context of the caller, and talks to TB using COM; if it cannot, then it loads Windows MAPI dll, which possibly uses a shim to provide access to the default application with different architecture (as is the case when 64-bit application tries to send using 32-bit TB).

So all kinds of combinations are possible:

• 64-bit app + 64-bit TB MAPI DLL -> 64-bit TB;
• 32-bit app + 32-bit TB MAPI DLL -> 32-bit TB;
• 64-bit TB app + 64-bit Windows MAPI dll -> shim -> (? 32-bit TB MAPI DLL ?) -> 32-bit TB;
• 32-bit TB app + 32-bit Windows MAPI dll -> shim -> (? 64-bit TB MAPI DLL ?) -> 64-bit TB.

Of course, the memory management details may differ in these cases.

Oh, there's another thing in play here: whether the calling app uses MAPISendMail or MAPISendMailW. Because in case of Windows 8+ (where the function is implemented in Windows' MAPI32.DLL), the latter would check if the TB MAPI DLL supports the W variant, and if not, it will internally convert passed arguments to 8-bit (allocating for that); so here the memory management may be complicated even more. The details are available in Windows SDK's MapiUnicodeHelp.h, which allows using MAPISendMailHelper (with wide-string semantics) even for Windows 7 (where MAPISendMailW is absent), using MAPISendMailW if available, and doing all the conversions itself.

I'm really sorry for spamming. What is a better channel for asking questions like this:

I have built TB 64-bit; I followed comment 69 to register it (since I have 32-bit TB installed, I had to change HKEY_CLASSES_ROOT\WOW6432Node\CLSID{29F458BE-8866-11D5-A3DD-00B0D0F3BAA7}); when I use an application (LO) to send mail, I get MAPI_E_FAILURE from InitMozillaReference (CoCreateInstance fails with "no such interface" result for {29F458BE-8866-11D5-A3DD-00B0D0F3BAA7}).

I suspect it's because of mismatch between 64-bit TB I test and 32-bit registry entries. So possibly I need to copy everything from 32-bit registry to 64-bit; or use an installer (mentioned above). I tried to copy HKEY_CLASSES_ROOT\WOW6432Node\CLSID{29F458BE-8866-11D5-A3DD-00B0D0F3BAA7} to HKEY_CLASSES_ROOT\CLSID{29F458BE-8866-11D5-A3DD-00B0D0F3BAA7}, but it didn't change things. I couldn't find a 64-bit TB installer on thunderbird.net, neither did I find it in the build tree (checked for msi; for exe; for large-sized files - to no avail).

So what is the intended procedure to register it (I'd prefer to have manual steps with specific registry keys to change, to be able to revert it later - but of course, anything working is fine)?

I love the information, keep it coming ;-)

You have two options: mach package and install that, see comment 48. The other option would be to install a TB x64 version.

60.4.0: http://ftp.mozilla.org/pub/thunderbird/releases/60.4.0/win64/
Daily: http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central/thunderbird-66.0a1.en-US.win64.installer.exe

A sidenote.

Since nsMapiHook::HandleAttachments depends hard on nsMapiFileDesc.lpszPathName being wchar_t*, LPTSTR in nsMapiFileDesc (mailnews/mapi/mapihook/build/msgMapi.idl) must be LPWSTR (to not depend on some UNICODE defines).

Ok, this is my analysis.

mozMAPI32.dll's MAPISendMail may be called with lpMessage == nullptr. The memory which lpMessage points to is never modified inside the function; but nsIMapi::SendMail (CMapiImp::SendMail) is made sure to be called with all valid pointers; the pointers are:

• aMessage
• aRecips
• aFiles

Here, aRecips and aFiles may point to aMessage->lpRecips and aMessage->lpFiles, or they may point to some zeroed memory blocks in case aMessage has those nullptrs. The latter may be because lpMessage had these nullptrs, or because lpMessage was nullptr itself, thus a zeroed memory block is used instead.

When passed to MAPI hook via COM, the data is marshaled using MapiProxy.dll (built from .c files auto-generated by MIDL compiler). The marshaling copies every block of data which (recursively for all pointers inside structs!), so when it arrives into CMapiImp::SendMail, the pairs [aMessage->lpRecips and aRecips], [aMessage->lpFiles and aFiles] are guaranteed to point to different addresses. Note that aMessage->lpRecips and aMessage->lpFiles may or may not be nullptr here!

Then aMessage->lpRecips and aMessage->lpFiles are unconditionally reset to aRecips and aFiles resp.; thus the previous addresses of memory they pointed to are lost (no idea if they are somehow tracked by COM), and aRecips and aFiles blocks are referenced twice by data managed by COM.

What it means in the end is that some memory may be destroyed twice, while some may be lost.

What I'd expect to be the correct approach here would be this:

1. Change the CMapiImp::SendMail to follow MAPISendMail signature (as Jorg did in experiment 1);
2. Just send lpMessage to CMapiImp::SendMail as it arrives to MAPISendMail (msgMapi.idl already has this param set to [unique], so the comments about the need to handle nullptrs specially are likely obsolete - see [1]).
   The CMapiImp::SendMail and following processing already is prepared for nullptrs in aMessage->lpRecips and aMessage->lpFiles. Just need a little tweak to handle aMessage itself.

In the meanwhile, it's unclear to me how does passing of nsMapiFileDesc happen, when lpszPathName and lpszFileName are actually char* (and so aFlags & MAPI_UNICODE is 0). The IDL used by TB knows that they are wchar_t*, so the marshaling should use wchar_t* zero-termination to copy strings; but if char* is passed, then it's not guaranteed to have the two trailing zeroes... but possibly that's another story, and maybe it somehow is handled on the caller side, if it uses the IDL "properly" (shim code is auto-created?).

It's incorrect to ignore the MAPI_UNICODE flag in Flags (as in the v4) - it's the caller's decision how to use this, as part of MS MAPI.

It should not be required to do the copy of the data in the MAPISendMail.

I'm preparing my attempt.

[1] https://blogs.msdn.microsoft.com/oldnewthing/20140919-00/?p=44023

(In reply to Mike Kaganski from comment #118)

Since nsMapiHook::HandleAttachments depends hard on nsMapiFileDesc.lpszPathName being wchar_t*, LPTSTR in nsMapiFileDesc (mailnews/mapi/mapihook/build/msgMapi.idl) must be LPWSTR (to not depend on some UNICODE defines).

I don't understand this comment. The patch changes lpszPathName to LPSTR and HandleAttachments() does pFile->InitWithNativePath(nsDependentCString(aFiles[i].lpszPathName)); - No wide string far and wide to be seen. So you're saying this is wrong?

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

Well - actually I only started to look into the patch details after I had a chance to look around in the original code (so the comment 118 was made before I realized that change).

Now I'm confused about the possibility to use whcar_t* in MAPISendMail. Possibly I'm mistaken, and wchar_t* in only sent to MAPISendMailW... then the change is fine. Well - it's orthogonal to the memory handling / crashing here.

Heh, [1], [2] and [3] suggests only one thing: MAPI_UNICODE is invalid flag to MAPISendMail, and it's possible to have UTF-8 there using CP_UTF8 in MapiMessage::ulReserved. Well - that UTF-8 support should be outside of the scope here.

[1] https://docs.microsoft.com/en-us/windows/desktop/api/mapi/nc-mapi-mapisendmail
[2] https://docs.microsoft.com/ru-ru/windows/desktop/api/mapi/nc-mapi-mapisendmailw
[3] https://docs.microsoft.com/ru-ru/windows/desktop/api/mapi/ns-mapi-mapimessage

This works for me :-) - this includes changing LPTSTR to LPSTR, as in v4.

Mike, thanks a lot for your great help and discovering the double free is caused by manipulating the incoming aMessage parameter to CMapiImp::SendMail.

I think the cleanup and fixes you've implemented seem great, and if it works, we should absolutely take your patch. However, IIUC, this smaller patch is the minimal cure for the crash.

Comment on attachment 9037162 [details] [diff] [review]
cleanup.diff

I like the clean-up. I'll try it when I'm back at my desk.

Wrt the LPCTSTR stuff, as I see the discussion here started from comment 59:

That wasn't a mistake on Mozilla side. I know that because I tried to work with this code 10 months ago, when MS haven't yet cleaned up their MAPI documentation pages into the new help system, as it is now. Then, there was no mention of MapiMessageW, and both MAPISendMail and MAPISendMailW documentation referred to common MapiMessage which actually had those LPCTSTR. That was the reason I thought it's how it's designed by MS, and wasn't terribly amazed seeing handling of MAPI_UNICODE in TB code. So it was just a matter of literally following existing MS documentation at that time actually.

Wrt comment 81 ("The MS COM type lib code generated by MIDL for the MS COM interfaces checks for these parameters to be non null, although null is a valid value for them here"):

the relevant pieces here are:

1. Initial import of comm-central into Mercurial from CVS - bug 437643 (2008) [1] which already contained the comment about inability to pass nullptrs.
2. Change by Hiroyuki Ikezoe - bug 594224 (2010) which had changed (fixed) that.

So now indeed there's no need to make sure to not pass nullptrs; just need to make sure nullptr is handled correctly.

Wrt Jorg's comments re:tabs and other formatting: of course, my patch has problems with that - sorry.

[1] https://hg.mozilla.org/comm-central/rev/e4f4569d451a
[2] https://hg.mozilla.org/comm-central/rev/b9feb6dbd2a9

Mike, once again, thanks to much for your help. I know you're busy with LibreOffice these days, so this is even more appreciated. Once we're finished here (I guess today), we'll look at your patch in bug 1048658 after it's refreshed and benefited from all the knowledge gained here. And then, as a last favour I'd like to ask you to finish off bug 594239, so we have a trilogy ;-) - Part of the clean-up there is already happening here. After that, we'd have MAPI in a pretty good state :-)

Oh, one thing about UTF-8: This works today if you set your system locale to UTF-8, see comment #70. You don't need the MAPISendMailW if you use that option on Windows 10. However, prior experiments showed that Windows prefers to call the W interface, so no doubt we should implement it, also for users not using the "beta UTF-8" option.

Never mind the white-space issues, I'll fix those before checking it in. (Still trying to get TB built after switching office/machine.)

Please disregard comment 126: my memory apparently failed me - as seen in my bug 1048658 comment 1 (so yes, MS documentation always defined the strings as 8-bit).

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

I'm a TB user; and thus it's the least I can do to give back a tiny bit how your great product helps me :-)

Wrt UTF-8 and comment 70 my comment 122 is a different story. Of course, if setting Windows ACP to UTF-8, then all ACP-related conversions would work automatically. The CP_UTF8 as the value of MapiMessage::ulReserved is another story, which started initially as an undocumented "feature" of Outlook that gave them opportunity to use Unicode in Simple MAPI; they had documented that since then. It allows an application to pass UTF-8 to Simple MAPI regardless of ACP, and without (then-absent) MapiMessageW. I don't know if supporting that is of significant priority; just mention here that not it's a part of the official API.

See Bug 1430766 - Remove unused code from msgMapiHook.cpp
Should that be included in the cleanup?

Thanks, we'll look into it, not in this bug here, that's too long already.

This is Mike's patch with the tabs replaced, white-space removed and a comment tweaked, and with a proper commit message.

Sadly ... is doesn't work for me. Sending via "Send to > Mail recipient" starts TB, but no compose window. Running it from LibreOffice starts TB and then gives an error.

I've gone back to Kai's (v4) and that works.

This patch, just as my experiment 1 modifies CMapiImp::SendMail() and in my experiments that broke things when before Kai's (v3) and (v4) worked.

Since it's working for Mike, there must be something very subtle going on. Maybe I should build the TB 66 installer and install TB properly?

It needs to also tweak the registry path to MapiProxy.dll, since the marshalling would not work properly if your setup uses proxy from a different build (can't check the correct path ATM).

Sigh, this stuff is so brittle :-(

OK, I built the installer, and sending from LibreOffice works now. However, "Send to > Mail recipient" (tired of typing that, it'll be St>Mr in the future) doesn't. I've checked the two registry entries I mentioned before:

[HKEY_LOCAL_MACHINE\SOFTWARE\Classes\CLSID{29F458BE-8866-11D5-A3DD-00B0D0F3BAA7}\LocalServer32]
@=""C:\Program Files\Thunderbird Daily 66-self\thunderbird.exe" /MAPIStartup"

[HKEY_LOCAL_MACHINE\SOFTWARE\Clients\Mail\Mozilla Thunderbird]
@="Mozilla Thunderbird"
"DLLPath"="C:\Program Files\Thunderbird Daily 66-self\mozMapi32_InUse.dll"

What's wrong still?

Comment on attachment 9037227 [details] [diff] [review]
393302-fix-crash-via-cleanup.patch

Sigh, it worked after logout/logon. No wonder my experiments never worked :-( - Maybe Windows as caching the DLL? I don't get it.

Let's go with this nice clean-up and move onto the follow-up bugs - yay \o/

Comment on attachment 9037227 [details] [diff] [review]
393302-fix-crash-via-cleanup.patch

Fast track this to TB 65 beta 3.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/335361a67f2d
Correct memory handling in MAPISendMail() and CMapiImp::SendMail() to fix "Send to > Mail Recipient" crash. r=jorgk

Let's move onto bug 1048658, bug 594239 and potentially bug 1430766.

TB 65 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/c331d58a7d3199e34c38a27692ccf0821acd1e8a

Fix commit message.

By the way: the special treatment in the MAPISendMail:

// we are seeing a problem when using Word, although we return success from the MAPI support
// MS COM interface in mozilla, we are getting this error here. This is a temporary hack !!
if (hr == 0x800703e6)
hr = SUCCESS_SUCCESS;

is for 0x800703e6 which is wrapped Win32 error 998: "ERROR_NOACCESS Invalid access to memory location." [1]
Possibly it was the problem we fixed here.

[1] https://docs.microsoft.com/en-us/windows/desktop/Debug/system-error-codes--500-999-

Testing with Word 2016 on my system, I see that it indeed does call MAPISendMail (even when MAPISendMailW is available!) with MapiMessage::ulReserved set to 65501 (CP_UTF8). Sigh.

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

Let's move onto bug 1048658, bug 594239 and potentially bug 1430766.

see also bug 547027 - Create a framework for MAPI tests
and bug 594243 - nsMapiHook::PopulateCompFieldsForSendDocs should create unique temporary file for attachments.

there are already patches for both bugs.

(In reply to Mike Kaganski from comment #145)

Testing with Word 2016 on my system, I see that it indeed does call MAPISendMail (even when MAPISendMailW is available!) with MapiMessage::ulReserved set to 65501 (CP_UTF8). Sigh.

I've seen MAPISendMailW() being called by St>Mr.

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

I've seen MAPISendMailW() being called by St>Mr.

Just to make sure: are you talking about MS Word in-built function, or about Windows Explorer context menu function (the latter indeed uses MAPISendMailW in my testing)? If the former, then possibly that's a Word version-dependent difference? (and possibly better discuss this at bug 1521007)

As I said: St>Mr = "Send to > Mail recipient" ;-) - You made me type it again.

I'm sorry, and I got the meaning of the abbreviation; just I'm not sure if you refer to Explorer (which indeed has this), or Word (which could, possibly, has the same path in its menu in some version - I have only one at hand, and there are many Word versions released out there).

This fix will be available for TB60ESR (if i got it right).

Does anyone knows in which version it will be on?

Likely in TB 60.6, 2019-03-19.

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

Likely in TB 60.6, 2019-03-19.

OK, Thank you :)

(In reply to Mike Kaganski from comment #145)

Testing with Word 2016 on my system, I see that it indeed does call MAPISendMail (even when MAPISendMailW is available!) with MapiMessage::ulReserved set to 65501 (CP_UTF8). Sigh.

Umm, should we cater for this in a new bug? We always use NS_CopyNativeToUnicode() which will mess up if the system charset isn't UTF-8, but the data is. Do you get mojibake? If so, let's file a new bug, easy to fix checking that flag and using UTF-8 to UTF-16 instead. You're the person to fix it since I don't have Word 2016 ;-)

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

Umm, should we cater for this in a new bug?

Heh, see last words of comment 150 ;-)

I've decided to uplift some white-space changes, to this needed adjustment.

The fix is also included in TB 65.0b3 x64?

Yes, and the follow-ups, bug 1048658 and bug 1521007 for full unicode support, will be in TB 65 beta 4 x64.

I tested TB 65 beta 3 x64. TB don't crash any more, but compose window won't open.

Please try a logon/logoff of better, restart of your machine. TB 65 beta 3 was installed with the official installer and nothing got installed afterwards? If not: Download the beta, install it, at least logon/off.

Still REPRODUCIBLE with unzipped installer of unofficial (by wg9s) en-US SeaMonkey 2.53 (NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 Build 20190122130014 (Default Classic Theme, German Language Pack active) on German WIN7 64bit

Rainer, if you simply unzipped, but didn't install, it cannot work. The updated DLL must be registered in the Windows system registry, please try to run the installer.

(In reply to Kai Engert (:kaie:) from comment #165)

Rainer, if you simply unzipped,

I used the "Make SM default Email application", and WIN7 went to admin mode. I think, that should have been sufficient to make SM "default mail application", but of course, I additionally I can run the installer. We will see.

Only the installer sets the registry entries that point to the MAPI DLL. This stuff is quite brittle, but if you get it installed properly, it works.

Rainer,
as Jorg and Kai told just you running the unzipped intaller won't work. The MAPI dll needs to be registered and this is only done by the installer. I tested with both libreoffice and "send to email" and 2.53 works fine here now.

(In reply to Rainer Bielefeld from comment #166)

Hi Rainer,
Nice to see you here! o/ Just wanted to thank you for all the tremendous work you've done for LO!

(In reply to Frank-Rainer Grahl (:frg) from comment #168)

MAPI dll needs to be registered and this is only done by the installer.

Yup, so it is. I now use an installed Version of above mentioned Build 20190122130014, and now 'Sendfile as emailfrom WIN files Explorer' and also 'Send document by email'from LibO work fine.

It's a little strange, currently the buttons 'Make SeaMonkey default application for [Mail] and [News] both are active, so SeaMonkey seems to think that that currently it might NOT be the default application?
But if it's a problem that should be discussed for SM, not here in the Bug.
I think this one is FIXED for SM, and so I adapt the status flag (also considering frg's result).

I think this one is FIXED for SM, and so I adapt the status flag (also considering frg's result).

Keep in mind that these are official flags and mine and Bills version is anything but official. So unless it is checked into an, as of now not existing, SeaMonkey comm-release branch it is still affected.

Keep in mind that these are official flags

you are right.

(In reply to Frank-Rainer Grahl (:frg) from comment #63)

Mail window with attachment is created in the background. But the z-order problem is either something SeaMonkey specific or present since ages too.

see Bug 758646 - (MAPI) TB Composition window opens in background when started from Windows Explorer > RightClick > Send to > Email recipient

Bug 758646 should be changed from MailNews Core to SeaMonkey if that does not happen in TB anymore.

TB 60.5.2 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/9a591621c69539ad40e272363b6e4eb4a49bb170