Closed Bug 1739784 Opened 3 months ago Closed 3 months ago

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

Categories

(MailNews Core :: Networking, defect)

defect

Tracking

(thunderbird_esr91+ fixed, thunderbird95 fixed)

RESOLVED FIXED
96 Branch
Tracking Status
thunderbird_esr91 + fixed
thunderbird95 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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.

See Also: → 1739789
Assignee: nobody → standard8
Status: NEW → ASSIGNED

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

Blocks: 1739814

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

Target Milestone: --- → 96 Branch

Pushed by mbanner@mozilla.com:
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

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED

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.

Attachment #9249579 - Flags: approval-comm-esr91?
Attachment #9249579 - Flags: approval-comm-beta?
Attachment #9249580 - Flags: approval-comm-esr91?
Attachment #9249580 - Flags: approval-comm-beta?
Attachment #9249584 - Flags: approval-comm-esr91?
Attachment #9249584 - Flags: approval-comm-beta?

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

Attachment #9249579 - Flags: approval-comm-beta? → approval-comm-beta+

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

Attachment #9249580 - Flags: approval-comm-beta? → approval-comm-beta+

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

[Triage Comment]
Approved for beta

Attachment #9249584 - Flags: approval-comm-beta? → approval-comm-beta+

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

Attachment #9249579 - Flags: approval-comm-esr91? → approval-comm-esr91+

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

Attachment #9249580 - Flags: approval-comm-esr91? → approval-comm-esr91+

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

[Triage Comment]
Approved for esr91

Attachment #9249584 - Flags: approval-comm-esr91? → approval-comm-esr91+

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.

Attachment #9250973 - Flags: review?(standard8)
Attachment #9250973 - Flags: review?(standard8) → review+

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

Flags: needinfo?(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.

Flags: needinfo?(standard8)

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?

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=");

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

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

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

Attached patch clang-format.patch (obsolete) — Splinter Review

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.

Flags: needinfo?(rob)
Attachment #9253071 - Attachment is obsolete: true
Flags: needinfo?(rob)
You need to log in before you can comment on or make changes to this bug.