1739784 - Messages in folders with non-ascii characters can't be restored on restart, possible breakage for add-ons and other issues relating to getUriForMsg

Status: RESOLVED FIXED

(MailNews Core :: Networking, defect)

Description

[Mark Banner (:standard8)]

I believe this is a regression from bug 1571672.

nsIMsgFolder.getUriForMsg returns an ACString, whereas nsIMsgMessageService.messageURIToMsgHdr expects a AUTF8String

https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/base/public/nsIMsgFolder.idl#453
https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/base/public/nsIMsgMessageService.idl#223

This causes the save/restore mechanism for tabs to break as it uses both of those functions combined from javascript, which breaks as the encoding gets messed up: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mail/base/content/mailTabs.js#405,409

This also has the potential for knock-on effects in other areas of the code (though I couldn't currently find any specific breakage) and for add-ons that are still using getUriForMsg in experiment code.

I'm a little surprised that bug 1571672 went the route of trying to make URIs utf-8 rather than simply encoding the relevant parts. However, I guess that's probably a bit too late to change now.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1739784 - Fix nsIMsgFolder.getUriForMsg, nsIMsgMessengerService.DisplayMessage and nsIMsgMessengerServices.streamMessage to handle non-ascii URIs. r?mkmelin!

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1739784 - Fix nsIMsgComposeParams.originalMsgURI to be able to handle non-ascii URIs. r?mkmelin!

Depends on D130554

Comment 3

[Mark Banner (:standard8)]

I've fixed here what I think is necessary for Conversations. There's still a lot of potentially broken APIs that I'm filing a follow-up for.

Try push: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=0f4da1acf0cf519e669081c1824d39f15b0246cc

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1739784 - Fix nsIMsgCompose.quoteMessage to handle non-ascii URIs. r?mkmelin!

Depends on D130555

Comment 5

[Mark Banner (:standard8)]

Updated try push for the additional patch that Conversations also needs: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=b1955f442da8df6d8a30f4ac5f1b603009bd1c47

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/de1e80c4a20f
Fix nsIMsgFolder.getUriForMsg, nsIMsgMessengerService.DisplayMessage and nsIMsgMessengerServices.streamMessage to handle non-ascii URIs. r=mkmelin
https://hg.mozilla.org/comm-central/rev/de73284458a3
Fix nsIMsgComposeParams.originalMsgURI to be able to handle non-ascii URIs. r=mkmelin
https://hg.mozilla.org/comm-central/rev/75a7e173d628
Fix nsIMsgCompose.quoteMessage to handle non-ascii URIs. r=mkmelin

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 9249579 [details]
Bug 1739784 - Fix nsIMsgFolder.getUriForMsg, nsIMsgMessengerService.DisplayMessage and nsIMsgMessengerServices.streamMessage to handle non-ascii URIs. r?mkmelin!

[Approval Request Comment]
Regression caused by (bug #): Bug 1571672
User impact if declined: Some users won't have messages restored on restart in the case where a folder contains non-ascii characters. Conversation's users won't be able to open emails in new tabs if their folders contain non-ascii characters.
Testing completed (on c-c, etc.): Landed in c-c, worked with Conversations :)
Risk to taking this patch (and alternatives if risky): Medium risk. This is touching various interfaces and Thunderbird obviously doesn't have lots of testing here. The alternative would be to back out bug 1571672 & related changes.

Comment 8

[Wayne Mery (:wsmwk)]

Comment on attachment 9249579 [details]
Bug 1739784 - Fix nsIMsgFolder.getUriForMsg, nsIMsgMessengerService.DisplayMessage and nsIMsgMessengerServices.streamMessage to handle non-ascii URIs. r?mkmelin!

[Triage Comment]
Approved for beta

Comment 9

[Wayne Mery (:wsmwk)]

Comment on attachment 9249580 [details]
Bug 1739784 - Fix nsIMsgComposeParams.originalMsgURI to be able to handle non-ascii URIs. r?mkmelin!

[Triage Comment]
Approved for beta

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9249584 [details]
Bug 1739784 - Fix nsIMsgCompose.quoteMessage to handle non-ascii URIs. r?mkmelin!

[Triage Comment]
Approved for beta

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 95.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/a80975631b10
https://hg.mozilla.org/releases/comm-beta/rev/b517f5ef4551
https://hg.mozilla.org/releases/comm-beta/rev/8b0170105470

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9249579 [details]
Bug 1739784 - Fix nsIMsgFolder.getUriForMsg, nsIMsgMessengerService.DisplayMessage and nsIMsgMessengerServices.streamMessage to handle non-ascii URIs. r?mkmelin!

[Triage Comment]
Approved for esr91

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9249580 [details]
Bug 1739784 - Fix nsIMsgComposeParams.originalMsgURI to be able to handle non-ascii URIs. r?mkmelin!

[Triage Comment]
Approved for esr91

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9249584 [details]
Bug 1739784 - Fix nsIMsgCompose.quoteMessage to handle non-ascii URIs. r?mkmelin!

[Triage Comment]
Approved for esr91

Comment 15

[Rob Lemley [:rjl]]

Attachment: Bug1739784_esr91_fix.patch

Fix for comm-esr91 uplift.

The DisplayMessageForPrinting functions were removed in bug 1734363; they don't appear to be used in 91.x either.
nsMsgAttachmentHandler.cpp was removed in bug 1724177.

Comment 16

[Rob Lemley [:rjl]]

Thunderbird 91.3.2:
https://hg.mozilla.org/releases/comm-esr91/rev/fb2c8a58299b
https://hg.mozilla.org/releases/comm-esr91/rev/dcbc00b59af5
https://hg.mozilla.org/releases/comm-esr91/rev/080fc4b1d03a
https://hg.mozilla.org/releases/comm-esr91/rev/f5f779d87b00

Comment 17

[Rachel Martin]

Is there a reason why nsCString(aMessageURI).get() is used twice in the first patch instead of PromiseFlatCString(aMessageURI).get()?

Comment 18

[Mark Banner (:standard8)]

I probably just got those wrong. I don't think it matters significantly though. You're welcome to file a patch to fix it.

Comment 19

[Rachel Martin]

Unless we're ill-informed, the former copies the string, whereas the latter doesn't in most cases where the string already has a terminating null byte and is hence more efficient. Sadly there are more occurrences of this incorrect usage:
https://searchfox.org/comm-central/search?q=nsCString.*get%5C%28%5C%29&path=mailnews&case=true&regexp=true
In general, shouldn't the person causing an issue also fix it, ideally also applying "boy scout rules", i.e. fixing similar problems in the vicinity?

Comment 20

[Rachel Martin]

This one const char* uidl = PL_strstr(nsCString(aSpec).get(), "uidl="); is particularly troubling since if the CString is destroyed, uidl would point to deallocated memory. This should likely be replaced with PL_strstr(aSpec.BeginReading(), "uidl=");

Comment 21

[Rachel Martin]

This should likely be replaced with PL_strstr(aSpec.BeginReading(), "uidl="); That won't work if the string isn't null terminated.

Comment 22

[Mark Banner (:standard8)]

(In reply to Rachel Martin from comment #19)

In general, shouldn't the person causing an issue also fix it, ideally also applying "boy scout rules", i.e. fixing similar problems in the vicinity?

I will fix the two instances I got wrong when I get time, if no-one beats me to it. Unfortunately I have already spent too long on this regression that wasn't caused by me, and I have a busy time at the moment so its unlikely to be for a week or two. I only fixed this as I needed it for my Conversations users and wanted to make sure it was done as soon as possible.

I'm sure there are many incorrect instances of string handling over the code base that could be fixed. Ideally I would have liked to fixed the other related bugs related to this regression, and transitioned a lot of other related functions to use nsCString rather than having them swap between raw character pointers and back. There is certainly a lot of clean up that could be done. I have even started changing some of the related functions and then backed out when I realised the amount of effort it would be - probably several days. Whilst clean up work is something I do enjoy, I simply do not have the several days to spare however much I would like to.

Comment 23

[Rachel Martin]

Thanks for the offer, Mark. Yes, switching from raw string to "smart" string could have been pushed further down in some cases, but each case requires consideration (which requires time). You can assign bug 1741559 to us ;-)

Comment 24

[Rachel Martin]

Attachment: clang-format.patch

Please apply this clang-format patch to correctly format the comm-esr91 tree again. A few merge related issues were introduced here, for example:
https://hg.mozilla.org/releases/comm-esr91/rev/fb2c8a58299bfba7348802d1849e3b6abf67920a#l3.92.

Patch also includes fixes from https://hg.mozilla.org/comm-central/rev/04b7b6e88251 and likely other bugs.

Alternatively please run clang-format yourself excluding
mailnews/mapi/include/mapidbg.h
mailnews/mapi/include/mapiutil.h
You can do mach clang-format -p comm/mailnews and then revert those two.

Comment 25

[Rachel Martin]

Comment on attachment 9253071 [details] [diff] [review]
clang-format.patch

OK, thanks: https://hg.mozilla.org/releases/comm-esr91/rev/72f4ee454628ea468781ae627a372a53f1b93dc2