Closed Bug 1555471 Opened 1 year ago Closed 5 months ago

modules/jsmime.jsm, line 50: NS_ERROR_NOT_AVAILABLE: [nsICharsetConverterManager.getCharsetAlias] in composition/test-charset-edit.js of Thunderbird

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set

Tracking

(thunderbird_esr6870+ fixed, thunderbird70 fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 70+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: aceman, Assigned: jorgk-bmo)

References

Details

Attachments

(2 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1416469 +++

There is some silent bustage in Thunderbird's compose window code.

The log at https://taskcluster-artifacts.net/Qan3wkGATQWebttXAXcK-A/0/public/logs/live_backing.log, the test
composition/test-charset-edit.js | test_wrong_reply_charset produces:
JavaScript error: resource:///modules/jsmime.jsm, line 50: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsICharsetConverterManager.getCharsetAlias]

Some discussion already happened in bug 1416469.

Summary: "gMsgCompose is null" in spellchecker observer in mozmill/composition tests of Thunderbird → modules/jsmime.jsm, line 50: NS_ERROR_NOT_AVAILABLE: [nsICharsetConverterManager.getCharsetAlias] in composition/test-charset-edit.js of Thunderbird
Attached patch 1555471.patch (obsolete) — Splinter Review

In bug 1416469 I claimed jcranmer suggested to use US-ASCII as the fallback. I changed it to windows-1252 per Jorg's suggestion.

Maybe you will remember why this is OK.

Attachment #9068522 - Flags: feedback?(Pidgeot18)
Comment on attachment 9068522 [details] [diff] [review]
1555471.patch

This is not the original code Joshua left behind. It's been tweaked many times, first we introduced the `MimeTextDecoder` to get rid of the `TextDecoder` trickery, then came the UTF-7 stuff and then charset alias stuff.

The comment says that this will throw and it does. That's desired behaviour, there is nothing to fix here.
Attachment #9068522 - Flags: feedback?(Pidgeot18) → feedback-

Why is it desired? We shouldn't be throwing unhandled errors if we can prevent it in a useful way.

I believe JS Mime throws on various occasions. If the requested charset isn't there, we won't paper over it. Somehow the throw is caught.

I'm seeing this in the 3-pane window:

NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsICharsetConverterManager.getCharsetAlias] jsmime.jsm:54

and this happens all the time. With each view of a message whose charset is somehow not liked, e.g.:

Content-Type: text/plain; charset=iso-8859-1
Content-Type: text/plain; charset=ISO-8859-1

I was sure I was triggering this with my own extension (BiDi Mail UI) and that it was a regression of mine - but I see it even when I remove my extension and with a clean profile.

Thunderbird should not throw in this case. At most, an informational message about an invalid charset could be logged. But - either getCharsetAlias() is usable and gets used, or if it's not usable - that call mustn't be made. Do I really have to say this? :-(

The exception/error spam is super-annoying, please fix this ASAP.

and this happens all the time. With each view of a message whose charset is somehow not liked, e.g.:

Sorry, that is simply not true. The error will happen for any character set which is not recognised and hence invalid, certainly not for iso-8859-1.

Please supply a message to reproduce this.

JorgK:

Well, I will entertain your request, but that's not the point. I mean, regardless of the conditions when this is triggered - same argument I made above. This just shouldn't happen. We have a getCharsetAlias() in nsICharsetConverterManager - why should it not be available?

Anyway, I'll attach a message.

Attached file msg.eml (obsolete) —

Note that the body doesn't actually agree with the stated charset.

This code is available and returns NS_ERROR_NOT_AVAILABLE for an invalid/unknown/unavailable charset.

Attachment #9097180 - Attachment mime type: message/rfc822 → text/plain

No error displayed for me with this message. Sure, it displays garbled.

(In reply to Jorg K (GMT+2) from comment #9)

This code is available and returns NS_ERROR_NOT_AVAILABLE for an invalid/unknown/unavailable charset.

But what is the point of showing that as an unhandled exception? What is the user supposed to do? Report the error in bugzilla, as I did? What will we do with it? If we do not want to implement the charset (as it is invalid), then there is no point to bother anyone with it. The code knows to handle the invalid message, just show it garbled. As Eyal has said, we could at most show some information message in the console, but not an exception that actually breaks execution of the function code.

If you say the MIME library is supposed to report errors using exceptions, then OK, that means we need to catch the exception somewhere higher in the call stack, e.g. at the caller of MimeTextDecoder().

OK, provide a message that shows the error, and we'll look at it further. Using charset=total-nonsense for a body part doesn't throw there, I only get:

[1576, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file c:/mozilla-source/comm-central/comm/mailnews/intl/nsCharsetConverterManager.cpp, line 111

I have javascript.options.strict and javascript.options.strict.debug set, what else do I need?

Actually, JS Mime is not used for message bodies, so we're barking up the totally wrong tree. Maybe you can trigger it with an invalid charset in a RFC-2047-encoded header.

BTW, we see this error in automation:
[task 2019-09-28T22:42:18.466Z] 22:42:18 INFO - TEST-START | /Users/cltbld/tasks/task_1569697649/build/tests/mozmill/composition/test-charset-edit.js | test_wrong_reply_charset
[task 2019-09-28T22:42:18.466Z] 22:42:18 INFO - JavaScript error: resource:///modules/jsmime.jsm, line 54: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsICharsetConverterManager.getCharsetAlias]

I've added some debug, see patch, and this is the call stack:
== JS stack> resource:///modules/jsmime.jsm (58)
== JS stack> resource:///modules/jsmime/jsmime.js (2658)
== JS stack> resource:///modules/jsmime/jsmime.js (2348)
== JS stack> resource:///modules/jsmime/jsmime.js (2174)
== JS stack> resource:///modules/mimeParser.jsm (122)
== JS stack> chrome://mozmill/content/modules/utils.jsm (429)
== JS stack> file:///c:/mozilla-source/comm-central/comm/mail/test/mozmill/composition/test-charset-edit.js (103)
== JS stack> file:///c:/mozilla-source/comm-central/comm/mail/test/mozmill/composition/test-charset-edit.js (123)

It comes from mimeParser.jsm (122) which is makeStreamListenerParser() which the test calls. The other call site is in parseAsync() which I don't see used.

Second attempt:

Using this header with JS Mime will decode
From: =?total-nonsense?Q?someone?= <some@one.com>
I get:

[6536, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file c:/mozilla-source/comm-central/comm/mailnews/intl/nsCharsetConverterManager.cpp, line 111
== JS stack> resource:///modules/jsmime.jsm (58)
== JS stack> resource:///modules/jsmime/jsmime.js (776)
== JS stack> resource:///modules/jsmime/jsmime.js (819)
== JS stack> resource:///modules/mimeParser.jsm (246)
== JS stack> file:///C:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/bin/components/mimeJSComponents.js (536)

and the header is displayed as is. I also get a heap of errors in the console from my on Cu.reportError(e);

BTW, the catch you wanted to add, is already there:
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/mailnews/mime/jsmime/jsmime.js#776

Without the patch, there is nothing in the error console (since the system obviously doesn't log every throw that occurs).

So please, can someone show me the issue here before I blow away another sunny Sunday morning in front of the screen with this investigation of a NON-ISSUE when I actually wanted to do something else :-( :-( :-(

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → INVALID

Or by all means, reopen this and fix the test, I gave you the JS call stack. Add your try/catch at jsmime.js (2658) and use the else block.

Attached patch 1555471-add-try-catch.patch (obsolete) — Splinter Review

Untested.

Attachment #9097194 - Flags: feedback?(acelists)
Comment on attachment 9097194 [details] [diff] [review]
1555471-add-try-catch.patch

OK, the test still passes, linter is happy, but is Aceman?
Attachment #9097194 - Flags: feedback?(acelists) → review?(acelists)

OK, provide a message that shows the error, and we'll look at it further.

You've already had two people confirming.

This code is available and returns NS_ERROR_NOT_AVAILABLE for an invalid/unknown/unavailable charset.

Returns, or throws?

BTW, the catch you wanted to add, is already there

The jsmime.jsm file in TB 68.1.1 (which I'm using) does not even have the function you linked to ( decode2047Token()) ... if you're saying this has been resolved in later code, then - I will be satisfied.

Actually, JS Mime is not used for message bodies, so we're barking up the totally wrong tree. Maybe you can trigger it with an invalid charset in a RFC-2047-encoded header.

Will try, and attach a folder. I got my "problematic" message with a folder containing many messages with charset issues, some of them could have them in the header.

In short: Nothing has been confirmed here. Aceman filed the bug for an issue he saw in automation, that absolutely all. Also, you're confusing jsmime.js and jsmime.jsm.

Best to file another bug with your problematic message, since this one here is about the one failure we're seeing in automation.

Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: INVALID → ---
Attachment #9097180 - Attachment is obsolete: true
Assignee: acelists → jorgk
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

@JorgK and @aceman: Ok, opened separate bug 1584822.

Added commit message.

Attachment #9068522 - Attachment is obsolete: true
Attachment #9097194 - Attachment is obsolete: true
Attachment #9097194 - Flags: review?(acelists)
Attachment #9097217 - Flags: review?(acelists)
Comment on attachment 9097217 [details] [diff] [review]
1555471-add-try-catch.patch

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

Yes, it silences the exception produced in the test-charset-edit.js, so I am happy:) Thank you.
Attachment #9097217 - Flags: review?(acelists) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bba4379257e4
add try/catch around MimeTextDecoder() in MimeParser._startBody(). r=aceman

Status: ASSIGNED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Attachment #9097217 - Flags: approval-comm-esr68+
Attachment #9097217 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.