Closed Bug 1048658 Opened 10 years ago Closed 6 years ago

implement MAPISendMailW for unicode messages and Windows 8

Categories

(MailNews Core :: Simple MAPI, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(thunderbird65 fixed, thunderbird66 fixed)

RESOLVED FIXED
Thunderbird 66.0
Tracking Status
thunderbird65 --- fixed
thunderbird66 --- fixed

People

(Reporter: unicorn.consulting, Assigned: mikekaganski)

References

Details

Attachments

(3 files, 7 obsolete files)

Apparently windows 8 does not support MAPISendMAil and the new MAPISendMailW replaces it. Right click > send mail to among other functions.
Windows 8 and later do support MAPISendMail. Still, a number of problems are associated with the 8-bit MAPI, that are listed in the proposed change commit message: 1. Text may be garbled. 2. Attachments may be inaccessible. 3. Attachments may fail to save. The problem #2 is when the attachment temporary file name includes characters not representable using 8-bit CP_ACP. Replacing correct character in names naturally makes files not accessible. The problem #3 is when attachment temporary file is accessible, but its user-visible name is different and contains such characters. In that case, the 8-bit string encoded in CP_ACP would contain "?", which are illegal in file names on Windows. TB tries to save the file locally with the user-visible name, which would fail, and then fail to send or save the message. The change may be tested using current master of LibreOffice, which includes a series of changes by me (starting https://cgit.freedesktop.org/libreoffice/core/commit/?id=c2f7759e85f3e5cc9f56aaf03eefa0f1ba834734) that allow using MAPISendMAilW and pass UTF-16. To test, use an attached document, which is a simple text document, that is named "aбγ.odt", thus containing a Latin, Cyrillic and Greek characters, and so isn't representable in any single codepage that could be used as active codepage on Windows (which still lacks an ability to use UTF-8 as ACP). Open the file in a LibreOffice daily from https://dev-builds.libreoffice.org/daily/master/, and use its File->Send-E-Mail Document... (of course, the TB in comm-central\obj-i686-pc-mingw32\dist\bin must be registered by using its uninstall\helper.exe /SetAsDefaultAppGlobal or /SetAsDefaultAppUser, and also regsvr32 path\to\comm-central\obj-i686-pc-mingw32\dist\bin\MapiProxy.dll; I also had to replace mozMapi32_InUse.dll with mozMapi32.dll in DLLPath under HKEY_LOCAL_MACHINE\SOFTWARE\Clients\Mail\Mozilla Thunderbird).

Mike, this bug has no audience and you didn't ask anyone for review. In the meantime, review board has been phased out. Can you please attach the a simple text patch.

Also, I've tried to do the same thing in bug 1506488. That patch actually doesn't work, I don't know why. It's similar to yours. As you've seen, there's a flurry of activities in bug 393302, and we're actually removing code like

+	else
+		lpFiles = lpMessage->lpFiles;
+
+	hr = pNsMapi2->SendMailW(lhSession, lpMessage,
+		(short)lpMessage->nRecipCount, lpRecips,
+		(short)lpMessage->nFileCount, lpFiles,
+		flFlags, ulReserved);

in favour of copying the memory.

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

well - let me postpone updating this until the changes are settled in bug 393302 then...

Attached patch MAPISendMailW.diff (obsolete) (deleted) — Splinter Review

This is updated after bug 393302. I saw that in the latter, the COM interface was changed easily, thus I decided to not create a "nsIMapi2", but simply extend existing nsIMapi.

After this patch, both LibreOffice and Explorer's "Send to -> mail recipient" start using this method in my testing on Windows 10.

Of course, the change has big parts copy-pasted. Yet, I didn't try to refactor the duplicated code. Unable to afford this at the moment, sorry.

Comment on attachment 9037406 [details] [diff] [review] MAPISendMailW.diff Review of attachment 9037406 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/mapi/mapihook/src/msgMapiHook.cpp @@ +478,5 @@ > + pFile->InitWithPath (nsDependentString(aFiles[i].lpszPathName)); > + > + bool bExist ; > + rv = pFile->Exists(&bExist) ; > + MOZ_LOG(MAPI, mozilla::LogLevel::Debug, ("nsMapiHook::HandleAttachments: filename: %s path: %s exists = %s \n", (const char*)aFiles[i].lpszFileName, (const char*)aFiles[i].lpszPathName, bExist ? "true" : "false")); I think %s is wrong here for wchar file names. See attachment 9024554 [details] [diff] [review], and there msgMapiHook.cpp and HandleAttachmentsW.

Oh thank you! - of course you are right. Will update.

Attached patch MAPISendMailW.diff (obsolete) (deleted) — Splinter Review

Patch updated after comment 8

Attachment #8957557 - Attachment is obsolete: true
Attachment #9037406 - Attachment is obsolete: true
Assignee: nobody → mikekaganski
Status: NEW → ASSIGNED
Attached patch 1048658-MAPISendMailW.patch (obsolete) (deleted) — Splinter Review

Added HG header and commit message. Fixed white-space issues, overlong lines, one copy/paste error (in debug output).

Attachment #9037420 - Attachment is obsolete: true
Attached patch 1048658-MAPISendMailW.patch (obsolete) (deleted) — Splinter Review

More copy/paste errors.

Attachment #9037500 - Attachment is obsolete: true
Attached patch 1048658-MAPISendMailW.patch (obsolete) (deleted) — Splinter Review

... and one more :-(

Attachment #9037503 - Attachment is obsolete: true

Jorg: thank you so much! Sorry for all these errors. I admit that I didn't pay attention to comments/debug string :-( - but I hope that the executed code (apart from debug strings) should be OK? I tested it before sending, be assured!

Comment on attachment 9037507 [details] [diff] [review] 1048658-MAPISendMailW.patch Works for me. My copy of LibreOffice 5.4.6.2 doesn't use the new interface, but Window own St>Mr does as per my previous observation in the duplicate bug I started. I can now send a file named テスト.txt which was possible via the non-W interface only with the system locale set to UTF-8.
Attachment #9037507 - Flags: review+
Attached patch 1048658-MAPISendMailW.patch (deleted) — Splinter Review

More white-space tweaks.

Attachment #9037507 - Attachment is obsolete: true

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/aaf39a9bd368
Implement MAPISendMailW(). r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0

I backported it to SeaMonkey 2.53 based on 56 and this seems to work fine there in Windows 8.1 and Server 2016. But under Windows 7 files names with unicode chars no longer result in a mail window shown. Previously they would show ?? for the unicode chars in the attachment.

Could someone test this with a Nighly Thunderbird under Windows 7. Regular file names still work so it is probably a unicode only regression if any. I am still on TB 60.5 and don't want to mess with the profile.

Thanks for testing!

There's actually no such thing as a "unicode character". Unicode is character encoding, so for example Ä or テ can be encoded in many different ways, UTF-8 being one of them.

So what you mean is that on a Windows system with a Western code page, like windows-1252, you used a file name which can't be encoded in that code page, right?

Unless I'm reading something incorrectly, MAPISendMailW is not available on Windows 7 (https://docs.microsoft.com/en-us/windows/desktop/api/mapi/nc-mapi-mapisendmailw). So this would be a regression of bug 393302, or maybe bug 1521007? Are you 100% sure that ever worked, see my comment in bug 393302 comment #69 where I tried with Japanese and Polish file names and no compose window came up. I'm also not sure where the ? replacement would have happened and how attaching a file could have worked, since it didn't exist with the ? name.

I know. Was just mean't as a simpification. Used the attached test file here.

It seems Virtualbox might be the culprit. If I try to use a unicode file from a virtual box shared folder in Win 7 it won't work. With a local file it seems to work but it displays the short Windows name for the attachment name. Both are ok under 8.1 and up. SeaMonkey has some different attachment handling which might be the cause here and it is also a downlevel Gecko 56 albeit heavily patched up. I wouldn't bother if the unicode case generally works in Win 7. Great work so far.

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

I backported it to SeaMonkey 2.53 based on 56 and this seems to work fine there in Windows 8.1 and Server 2016. But under Windows 7 files names with unicode chars no longer result in a mail window shown. Previously they would show ?? for the unicode chars in the attachment.

It's very important to also state which program was used as calling application. E.g., last year I have changed LibreOffice to create a temporary file with ASCII-only names on send, and pass that filename to MAPI, to workaround that deficiency (in https://git.libreoffice.org/core/+/5566df47de17eca8c0a5b447bcb906051c869db6). So using LibreOffice, that would previously work (and now, too). Word also uses that technique. But Windows Explorer right-click menu launches some other application, which never could send files with names not representable in ACP ("Unicode names") previously to this patch. And Windows 7 doesn't have the W function in MAPI, as Jorg mentioned.

Please check the "Previously they would show ??" part, I guess it's some mistake.

Or an artefact of using the VM? Kai in bug 393302 comment #59 and cmt #64 claimed that he could send files with "unicode names" on his VM when I couldn't on "real" Windows 10.

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

I cannot say anything definitive regarding what could that be, except that Kai should have debugged that using VS debugger back then. I doubt, though, that UTF-8, even if actually sent by Explorer in the data, could be processed properly by TB back then, without what we implemented in bug 1521007, unless one used beta UTF-8 ACP in Windows 10. And I cannot see anything in our code that could describe VM differences.

Thank you guys for movement in this area. This looks like a reasonably standalone patch, would it be a candidate for backport to ESR60? Looks like bug 393302 is too.

Sure both (and more) are on my mental list. I plan to ship all MAPI fixes in TB 60.6.

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

But under Windows 7 files names with unicode chars no longer result in a mail window shown.

(In reply to Mike Kaganski from comment #22)

And Windows 7 doesn't have the W function in MAPI, as Jorg mentioned.

MS says:
On Windows 8 and later: Call MAPISendMailW to send a message.
On Windows 7 and earlier: Use MAPISendMailHelper to send a message.
and:
MAPISendMailHelper function:
Takes Unicode message information and sends the message using MAPISendMailW or, if necessary, converts the message to ANSI and sends the message using MAPISendMail.
(https://docs.microsoft.com/de-de/windows/desktop/api/mapiunicodehelp/nf-mapiunicodehelp-mapisendmailhelper)

I think that first MAPISendMailHelper must be implemented so that unicode chars in the attachment works under Windows 7.

(In reply to bblack from comment #27)

I think that first MAPISendMailHelper must be implemented so that unicode chars in the attachment works under Windows 7.

No. MAPISendMailHelper is not something anyone needs to implement, because it's a function fully implemented in MS in Windows SDK (in MAPIUnicodeHelper.h). Its source code is open (in the said header), and is built-in into any application that decides to use this helper function.

Its deficiencies (related to handling of CP_UTF8) are raised in my request [1].

[1] https://developercommunity.visualstudio.com/content/problem/431508/mapisendmailhelper-implementation-is-inconsistent.html

Attachment #9037531 - Flags: approval-comm-esr60+
Attachment #9037531 - Flags: approval-comm-beta+
Attached patch 1048658-MAPISendMailW.patch - ESR60 version (obsolete) (deleted) — Splinter Review
Attachment #9038001 - Flags: approval-comm-esr60+
Attachment #9037531 - Flags: approval-comm-esr60+ → review+
Attachment #9038001 - Attachment is obsolete: true
Attachment #9038024 - Flags: approval-comm-esr60?
Attachment #9038001 - Flags: approval-comm-esr60+
Comment on attachment 9038024 [details] [diff] [review] 1048658-MAPISendMailW.patch - ESR60 version Let's leave some of the fun for TB 68. We had enough MAPI chagrin in TB 60. The patch doesn't apply anyway any more.
Attachment #9038024 - Flags: approval-comm-esr60?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: