1739814 - More potential non-ascii URI breakage from bug 1571672

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Mark Banner (:standard8)]

Other APIs that are potentially broken from bug 1571672 are listed below. These could cause various breakages with folders that have non-ascii names.

Not all of these are currently used from JavaScript, though some could be used by add-on experiments.

I generated the list by looking through the results of a search for uri on mail**idl. There may be more under url, though mailnews does seem fairly consistent.

• nsIMsgMessageService: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/base/public/nsIMsgMessageService.idl#52,125,197,245
• nsIMsgFolder: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/base/public/nsIMsgFolder.idl#521-522,624
• nsIMsgIncomingServer: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/base/public/nsIMsgIncomingServer.idl#493
• nsIMsgWindowCommands: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/base/public/nsIMsgWindow.idl#21-22
• nsIMsgCompose: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/compose/public/nsIMsgCompose.idl#242,279
• nsIMsgComposeService: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/compose/public/nsIMsgComposeService.idl#133
• nsIMsgSend: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/compose/public/nsIMsgSend.idl#323
• nsIPop3Sink: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/local/public/nsIPop3Sink.idl#18-19,22
• nsIPop3URL: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/local/public/nsIPop3URL.idl#12
• nsIMimeStreamConverter: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/mime/public/nsIMimeStreamConverter.idl#91
• nsINntpService: https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/news/public/nsINntpService.idl#34,36,46

Comment 1

[Mark Banner (:standard8)]

Note, I just moved the message filtering broken APIs out to bug 1739903, as that's a more specific bug.

Comment 2

[Rachel Martin]

Thanks Mark, some of this is certainly covered in bug 1741619. That bug started by checking whether changes from https://hg.mozilla.org/comm-central/rev/de1e80c4a20f could be pushed further down and it rippled on from there.

Comment 3

[Rachel Martin]

The German newsgroup de.comm.software.mozilla.mailnews reports that multiple drafts are stored on IMAP, likely related to the fact that the drafts folder is called "Entwürfe" in German.

Comment 4

[Rachel Martin]

This is certainly related to:
https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/compose/public/nsIMsgCompose.idl#242,279
https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/compose/public/nsIMsgSend.idl#323

Comment 5

[Rachel Martin]

Attachment: 1739814-saved-draft.patch

Together with the patch from bug 1741619 this fixes most (not all) call sites from comment #0. Also needing treatment are the call sites from bug 1739903. clang-format has not been run.
We're using this on ESR 91:
https://github.com/Betterbird/thunderbird-patches/blob/main/91/bugs/1739814-saved-draft.patch
The ESR patch also patches nsMsgSend.cpp which doesn't exist on C-C any more.

This fixes the issue that multiple drafts are stored in draft folders with non-ASCII names, easily seen on Gmail with German UI or a draft folder "Drafté" on a UTF-8 supporting server like Gmail.

Comment 6

[Rachel Martin]

Comment on attachment 9251446 [details] [diff] [review]
1739814-saved-draft.patch

Patch also appears to be fixing bug 1739789.

Comment 7

[Rachel Martin]

Comment on attachment 9251446 [details] [diff] [review]
1739814-saved-draft.patch

Please apply clang-format and update the commit message to:
Change more URI APIs to from raw string to smart string. Fixes saving drafts and smart/unified/virtual folders.

Comment 8

[Magnus Melin [:mkmelin]]

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

Comment 9

[Rachel Martin]

Since you assigned this to us, looks like the expectancy is to get this fully fixed ;-) - Together with bug 1741619 the first patch fixes most but not all IDLs. We'll submit a patch for the remaining four IDL files soon:
https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/compose/public/nsIMsgComposeService.idl#133
https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/local/public/nsIPop3Sink.idl#18-19,22
https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/local/public/nsIPop3URL.idl#12
https://searchfox.org/comm-central/rev/37baa8dd18341e3dd3083347c3b91292b6cddcc9/mailnews/mime/public/nsIMimeStreamConverter.idl#91
The first part can be landed independently. Thanks for the clang-format we see on try.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9251446 [details] [diff] [review]
1739814-saved-draft.patch

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

Seems to work, r=mkmelin

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/829aaf569187
Change more URI APIs to from raw string to smart string. Fixes saving drafts and smart/unified/virtual folders. r=mkmelin

Comment 12

[Rachel Martin]

Attachment: 1739814-remaining-APIs.patch

For bonus points we fixed nsMsgCompose::CreateMessage() as well and ran clang-format.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9251959 [details] [diff] [review]
1739814-remaining-APIs.patch

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

Looks alright, r=mkmelin

Comment 14

[Rachel Martin]

Please remove "leave-open", this is the second and last patch.

Comment 15

[Rachel Martin]

And please close bug 1739789.

Comment 16

[Rachel Martin]

C-esr91 version patches more files:
https://github.com/Betterbird/thunderbird-patches/blob/main/91/bugs/1739814-remaining-APIs.patch

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ced97457c09f
Change remaining URI APIs to from raw string to smart string. r=mkmelin

Comment 18

[Rachel Martin]

Comment on attachment 9251446 [details] [diff] [review]
1739814-saved-draft.patch

[Approval Request Comment]
Regression caused by (bug #): bug 1571672
User impact if declined: Various things don't work on folders with non-ASCII names: Saves multiple drafts, virtual folders don't work, etc.
Testing completed (on c-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky):
Not risky, mostly mechanical change of raw string API to "smart" string API allowing for non-ASCII strings to be correctly passed around.
Part of a pack of four bugs which need to be uplifted in this order:
Bug 1741559, bug 1741619, bug 1739814, bug 1739903. Bug 1739784 was the first one in the series and that's already on ESR.

Request covers both patches here.

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9251446 [details] [diff] [review]
1739814-saved-draft.patch

[Triage Comment]
Approved for beta

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9251959 [details] [diff] [review]
1739814-remaining-APIs.patch

[Triage Comment]
Approved for beta

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 95.0b5:
https://hg.mozilla.org/releases/comm-beta/rev/2943e9fdc1a7
https://hg.mozilla.org/releases/comm-beta/rev/67086535ed48

Comment 22

[Wayne Mery (:wsmwk)]

Comment on attachment 9251446 [details] [diff] [review]
1739814-saved-draft.patch

[Triage Comment]
Approved for esr91

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9251959 [details] [diff] [review]
1739814-remaining-APIs.patch

[Triage Comment]
Approved for esr91

Comment 24

[Rachel Martin]

To the person doing the uplifts:
The patches for bug 1741619, bug 1739814 and bug 1739789 don't apply cleanly to esr-91. Some have additional hunks for nsMsgAttachmentHandler.cpp, nsMsgSend.cpp and around DisplayMessageForPrinting(). We have esr-91 versions of those patches here:
https://github.com/Betterbird/thunderbird-patches/tree/main/91/bugs, see:
https://github.com/Betterbird/thunderbird-patches/blob/3c43707766fc87fc5fd20759c7111edfa9ab9034/91/series#L32-L36

Comment 25

[Rob Lemley [:rjl]]

Thunderbird 91.4.1:
https://hg.mozilla.org/releases/comm-esr91/rev/efe96d27f31b
https://hg.mozilla.org/releases/comm-esr91/rev/8f2d213be76c