1842384 - notifications for new e-mails are not displayed correctly if they contain an umlaut

Status: RESOLVED FIXED

(Thunderbird :: OS Integration, defect)

Description

[Sören Hentzschel]

Attachment: screenshot

Notifications for new e-mails are not displayed correctly if they contain an umlaut. It used to work in Thunderbird 102 and no longer works in Thunderbird 115.

Regression range according to mozregression:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=d0092ed30b8462f73ab0b7368f234af99e22e137&tochange=5a9e1108b51af7d242104cea842091120b049ccc

It's unfortunately not uncommon for Thunderbird that a push includes more than one change so I can't bisect further. But I guess bug 1802815 is the only bug in this regression range that makes sense as regressor?

Tested on macOS 13.4.

Comment 1

[Alfred Peters [:infofrommozilla]]

Works for me with TB117a1, a correctly MIME encoded email and on Windows.

With an broken email see: Bug 1783499

Comment 2

[Alfred Peters [:infofrommozilla]]

(In reply to Alfred Peters from comment #1)

With an broken email see: Bug 1783499

That case is somewhat different.

In your example, I notice that some umlauts are quite correct.

Does the bug occur with all emails?
If not, can you upload an example?

Comment 3

[Sören Hentzschel]

Attachment: Neue Nachricht über das Kontaktformular - camp-firefox.de <[e-mail]> - 2023-07-09 1216.eml

I don't know if there are case where it works but I saw this very often in the last few months.

I attached an example e-mail.

Comment 4

[Alfred Peters [:infofrommozilla]]

(In reply to Sören Hentzschel from comment #3)

Created attachment 9342936 [details]

I attached an example e-mail.

Thanks for that. Unfortunately, I can't reproduce it with that either.
Tested on Windows with TB117a1, TB115b6 and TB115b6 with German settings.

Comment 5

[Sören Hentzschel]

Since it involves native OS notifications I can't rule out that it is macOS specific. I also don't know how to test the problem with the downloaded email, since opening this file in Thunderbird does not generate a notification.

Comment 6

[Alfred Peters [:infofrommozilla]]

I dropped the email marked as unread into the INBOX using a 2nd IMAP client (a 2nd TB instance with its own profile).

Comment 8

[ncf]

I filed a duplicate report with more information in https://bugzilla.mozilla.org/show_bug.cgi?id=1845080. This is not OS-specific.

Comment 9

[Francesco]

Looks like the strings are passed incorrectly to the OS notification, both Windows and Linux. You said in bug 1845080 (quote): "UTF-8 encoding of 🙂 as ISO-8859-1". That's likely not the case. It's likely more a case of confusing UTF-8 and UTF-16. If the strings are happening in the subject, maybe a fall-out from https://hg.mozilla.org/comm-central/rev/327f82032ed2#l7.12

Comment 10

[ncf]

You're right, all we can say is that UTF-8 bytes are being treated as codepoints somehow.

The commit you link does look suspicious, but it is not in the regression range in the OP, and the bug actually only happens with the body of the email (the preview property), not the subject, for me. I think you can also see that in OP, where ü is fine in the subject but „ is garbled in the body.

Comment 11

[Francesco]

Oh, yes, the preview property:
https://searchfox.org/comm-central/search?q=hdr.*property.*preview&path=&case=false&regexp=true
And here we have the bug:
https://searchfox.org/comm-central/rev/32cc2360c91edecd5553b76a6237e4c25b51450c/mailnews/base/src/MailNotificationManager.jsm#320
It's retrieved and not decoded from UTF-8 as stored in the DB to UTF-16 as used in a JS string. The author of the regressor bug is already NI'ed here.

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: Bug 1842384 - Make setStringProperty work properly for non-ascii values. r=leftmostcat

The idl needs to use a modern string type. Non-ascii doesn't survive passed through XPCOM when the type is string.

Comment 13

[betterbird.project+9]

Some drive-by comments re. better string processing:

char storeToken[100];
PR_snprintf(storeToken, sizeof(storeToken), "%lld", m_startOfNewMsg);
msgHdr->SetStringProperty("storeToken", nsDependentCString(storeToken));

This could be replaced with:

nsCString storeToken = nsPrintfCString("%lld", m_startOfNewMsg);
msgHdr->SetStringProperty("storeToken", storeToken);

There are four occurrences of this throughout the file.

---

*aResult = strdup(key.get()); Please use: ToNewCString(key);

---

  return m_mdb->SetProperty(m_mdbRow, propertyName,
                            PromiseFlatCString(propertyValue).get());

PromiseFlatCString shouldn't be necessary. Doesn't it compile without it?

Comment 14

[Magnus Melin [:mkmelin]]

PromiseFlatCString shouldn't be necessary. Doesn't it compile without it?

Yeah, needed to compile.

Comment 15

[betterbird.project+9]

Yes, of course.

As for Sean's comments: nsIMsgDatabase.setStringPropertyByHdr() doesn't need to be converted since it's not used in JS. The string is just considered raw (UTF-8) bytes. The changes to the format specifiers appear to be copy/paste errors. Different things compile on different platforms, so better not to change them.

Comment 16

[betterbird.project+9]

Actually, should that be using PRIu64 or PRId64?

Comment 17

[betterbird.project+9]

And there is a copy/paste issue here:

-    char storeToken[100];
-    PR_snprintf(storeToken, sizeof(storeToken), "%lld", m_startOfNewMsg);
-    newMsgHdr->SetStringProperty("storeToken", storeToken);
+    nsCString storeToken = nsPrintfCString(PRIu64, m_startOfNewMsg);
+    msgHdr->SetStringProperty("storeToken", storeToken);

Wrong header variable. We're trying PRIu64.

Comment 18

[betterbird.project+9]

That hunk again:

   m_curSrcHdr = nullptr;
   if (newMsgHdr) {
-    char storeToken[100];
-    PR_snprintf(storeToken, sizeof(storeToken), "%lld", m_startOfNewMsg);
+    nsCString storeToken = nsPrintfCString("%" PRIu64, m_startOfNewMsg);
     newMsgHdr->SetStringProperty("storeToken", storeToken);
     newMsgHdr->SetMessageOffset(m_startOfNewMsg);

Comment 19

[betterbird.project+9]

More issues. Before the raw UTF-8 bytes were returned from the database. You need to eliminate any "cheap" conversion from raw UTF-8 to UTF-16 (JS string). We found two here:
https://searchfox.org/comm-central/search?q=%2F%2F+Get+the+preview+text+as+a+UTF-8+encoded+string&path=&case=false&regexp=false

Comment 20

[betterbird.project+9]

https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1842384-additional-changes.patch
We also restored the error handling in nsMsgHdr.cpp.

Comment 21

[betterbird.project+9]

Magnus, please don't use "%lu" and "%ld" as the don't compile on Windows. Please use "%" PRId64 and "%" PRIu64.

Sample code:

  int64_t i64 = 89955556338558;
  uint64_t u64 = 89955556338558;
  char storeTokeni[100];
  char storeTokenu[100];
  PR_snprintf(storeTokeni, sizeof(storeTokeni), "%lld", i64);
  PR_snprintf(storeTokenu, sizeof(storeTokenu), "%lld", u64);
  nsCString storeTokenii = nsPrintfCString("%" PRId64, i64);
  nsCString storeTokenuu = nsPrintfCString("%" PRIu64, u64);
  nsCString storeTokeniix = nsPrintfCString("%lld", i64);
  nsCString storeTokenuux = nsPrintfCString("%lld", u64);
  nsCString storeTokeniiy = nsPrintfCString("%ld", i64);
  nsCString storeTokenuuy = nsPrintfCString("%lu", u64);
  printf("=== %lld %s %s %s %s\n", i64, storeTokeni, storeTokenii.get(), storeTokeniix.get(), storeTokeniiy.get());
  printf("=== %lld %s %s %s %s\n", u64, storeTokenu, storeTokenuu.get(), storeTokenuux.get(), storeTokeniiy.get());

 0:14.05 C:/mozilla-source/mozilla-central/comm/ (some sample file): error: format specifies type 'long' but the argument has type 'int64_t' (aka 'long long') [-Werror,-Wformat]
 0:14.05   nsCString storeTokeniiy = nsPrintfCString("%ld", i64);
 0:14.05                                              ~~~   ^~~
 0:14.05                                              %lld

Comment 22

[betterbird.project+9]

Another correction:

      mDatabase->SetStringProperty(msgKey, "junkscore",
                                   PromiseFlatCString(junkScore));

Here the PromiseFlatCString() isn't needed any more.

Comment 23

[betterbird.project+9]

And while you have the file open, can you please change
https://searchfox.org/comm-central/search?q=nsCString%28aProperty%29&path=&case=false&regexp=false
to use nsDependentCString instead of nsCString().

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c1ee102f05f7
Make setStringProperty work properly for non-ascii values. r=leftmostcat

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9348884 [details]
Bug 1842384 - Make setStringProperty work properly for non-ascii values. r=leftmostcat

[Approval Request Comment]
Regression caused by (bug #): bug 1802815
User impact if declined: notifications may display garbled text
Testing completed (on c-c, etc.): c-c, beta
Risk to taking this patch (and alternatives if risky): should be safe

Comment 27

[Wayne Mery (:wsmwk)]

Comment on attachment 9348884 [details]
Bug 1842384 - Make setStringProperty work properly for non-ascii values. r=leftmostcat

[Triage Comment]
Approved for esr115

Comment 28

[Daniel Darnell [:dandarnell]]

Thunderbird 115.2.3:
https://hg.mozilla.org/releases/comm-esr115/rev/998ca1d3a171