1597891 - Fix or remove MimeObject ResetChannelCharset().

Status: RESOLVED FIXED

(MailNews Core :: General, defect)

Description

[Ben Campbell]

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

ResetChannelCharset(), in comm/mailnews/mime/src/mimemoz2.cpp has a code path which ends up in an infinite loop. Since it doesn't seem to have ever been an issue, it's probably safe to assume that the code is never invoked.

• ResetChannelCharset() is only called from one place - in MimeObject_init_output().
• Running the comm/mailnews/mime unit tests don't invoke ResetChannelCharset() at all (when running under gdb).

Would be nice to figure out exactly what it's trying to do, and if it's even needed any more.

Comment 1

[Ben Campbell]

Attachment: 1597891-resetchannelcharset-fix-1.patch

This should at least stop it crashing if it gets there... (and adds a MOZ_DIAGNOSTIC_ASSERT referencing this bug if it does).

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9110160 [details] [diff] [review]
1597891-resetchannelcharset-fix-1.patch

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

::: mailnews/mime/src/mimemoz2.cpp
@@ +1774,5 @@
>        obj->options->default_charset && obj->headers) {
> +    // The previous version of this code would have entered an infinite
> +    // loop. But it never showed up, so it's not clear that it is ever
> +    // called...
> +    // See Bug #1584473.

Which bug number do you want here? I suggest we use bug 1597891.

@@ +1777,5 @@
> +    // called...
> +    // See Bug #1584473.
> +    MOZ_DIAGNOSTIC_ASSERT(false, "Should never get here (see Bug 1584473)");
> +
> +    mime_stream_data *msd = GetMSD(obj->options);

No need to use that function, IMHO, obj->options was checked in the if clause above. We might as well revert this change to avoid unnecessary churn.

@@ +1782,3 @@
>      char *ct = MimeHeaders_get(obj->headers, HEADER_CONTENT_TYPE, false, false);
>      if ((ct) && (msd) && (msd->channel)) {
> +      char *cSet = MimeHeaders_get_parameter(ct, "charset", nullptr, nullptr);

OK, so we use a function to extract the charset instead of hand-crafting it. Seems like a good move.

@@ +1787,5 @@
>          msd->channel->SetContentType(nsDependentCString(ct));
>  
>          // Second, if this is a Save As operation, then we need to convert
> +        // to override the output charset.
> +        if (msd->format_out == nsMimeOutput::nsMimeMessageSaveAs) {

Can't we move that condition up? Why get the charset if we're not nsMimeOutput::nsMimeMessageSaveAs?

Comment 3

[Jorg K (CEST = GMT+2)]

Sorry, I missed the msd->channel->SetContentType(nsDependentCString(ct)); which is done unconditionally.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1597891-resetchannelcharset-fix-1.patch - JK

Sorry about my wrong comment. I fixed the nits. If you agree, we can land it like this. So now we have better dead code ;-)

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=9110160&action=interdiff&newid=9112741&headers=1

Comment 5

[Ben Campbell]

Thanks Jorg, that all looks great.
I like dead code. It never crashes. We should definitely add more! :-)

Anyway, I shall park this one and revisit it next time I end up doing a deep dive into the mime code (and channel implementations).

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fc1c18cda24b
Remove superfluous infinite loop in MimeObject ResetChannelCharset(). r=jorgk

Comment 7

[Magnus Melin [:mkmelin]]

Should have made a try build. This creates oranges across the board, so the code does run: https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=279022667&revision=fc1c18cda24b0a157d4b6d71c401bf4b5146b17e

Comment 8

[Magnus Melin [:mkmelin]]

Backout: https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=9c5ad9180e9133d9268073251fe2a574cefd7716

Comment 9

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/9c5ad9180e9133d9268073251fe2a574cefd7716
Please paste the backout changeset in the future.

Sorry about the bustage, at least we get some idea how to trigger what we thought was dead code, and it must partly be, otherwise it would hang in an endless loop. I guess the diagnostic assert was in the wrong spot, should have been further down in an if-block.

It might be prudent to re-run the Dailies since the diagnostic assert, as you can see, also crashes opt builds.

Comment 10

[Magnus Melin [:mkmelin]]

Indeed, looks like reply to a bugzilla mail causes a crash.
Jörg, would you mind sending me the instructions?

Comment 11

[Jorg K (CEST = GMT+2)]

You have to be authorised with the right TC permissions. I'll do it.

Comment 12

[Jorg K (CEST = GMT+2)]

Used to be at
https://tools.taskcluster.net/hooks/project-comm/releng%2Fnightly%2Fdesktop%2Fcomm-central
but that no longer exists, it all moved to https://firefox-ci-tc.services.mozilla.com.
Now seems to be at:
https://firefox-ci-tc.services.mozilla.com/hooks/project-releng/cron-task-comm-central%2Fnightly-desktop

And it still worked, yay!

Comment 13

[Ben Campbell]

Doh! Sorry about that. My fault for a) not running a try build and b) being insufficiently careful about where the ASSERT went.
But like Jörg says, at least we know that the function is being called, and we've got a repeatable test case. So we have a clue as to what issues the function is supposed to be fixing.
I'll work out a local STR and sort out a fix for the fix.

Comment 14

[Jorg K (CEST = GMT+2)]

No further action here?

Comment 15

[Ben Campbell]

Attachment: 1597891-resetchannelcharset-fix-2.patch

So, we've now got a way to exercise this code. Yay mochitests!

The mochitests which hit the ASSERT in the previous patch (ie, by calling ResetChannelCharset()) are:

comm/mail/test/browser/cloudfile/browser_attachmentUrls.js
comm/mail/test/browser/composition/browser_charsetEdit.js
comm/mail/test/browser/content-policy/browser_dnsPrefetch.js
comm/mail/test/browser/folder-display/browser_messageCommandsOnMsgstore.js
comm/mail/test/browser/message-header/browser_replyIdentity.js

I've just been looking at browser_charsetEdit.js.
The test can be run headless (phew), and it lets you attach a debugger, for example:

$ ./mach mochitest --debugger gdb --headless comm/mail/test/browser/composition/browser_charsetEdit.js

Single-stepping though, it all looks like it's doing what it should, as far as I can tell.
(the only bit I've not single-stepped through is the nsMimeMessageSaveAs override-the-output charset bit, but that looks pretty sane).

So, in this patch I've just removed the comment and diagnostic ASSERT. If the try build comes back clean, I think we can call it done.

Comment 16

[Jorg K (CEST = GMT+2)]

I don't get it. Did we not agree that the old code would enter an endless loop? And the conclusion was that we'd never get there? So now that we found a test that exercises the code, can we please check why it didn't hang, or maybe the if (ptr) { was never true?

Comment 17

[Ben Campbell]

Sorry, yes - the infinite loop in the old code only occurs when the stream output format is nsMimeOutput::nsMimeMessageSaveAs, and this isn't exercised by the current tests.
The old version was a little misleading, as about half the function (including the infinite loop) was in this execution path. In the new version it's about 3 or 4 lines.
The old code inside this path had some brittle bespoke parsing to detect and extract the charset from a content-type header. It was the extraction that had the infinite loop. In the new version, I used an existing mime function to parse the content-type, so we've already got the charset by the time it enters the nsMimeMessageSaveAs branch.
So we've still got 4 lines of uncovered code, which isn't ideal, but that code now looks sane at least.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9121786 [details] [diff] [review]
1597891-resetchannelcharset-fix-2.patch

> So we've still got 4 lines of uncovered code, which isn't ideal, but that code now looks sane at least.

IMHO, that where the diagnostic ASSERT must do. The whole aim here is to eventually remove dead code.

Comment 19

[Magnus Melin [:mkmelin]]

So what's left is to check if the msd->format_out == nsMimeOutput::nsMimeMessageSaveAs code is needed (by exercising a manual save as test) and depending on whether it works without it or not, keep or remove?

Comment 20

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #19)

So what's left is to check if the msd->format_out == nsMimeOutput::nsMimeMessageSaveAs code is needed (by exercising a manual save as test) and depending on whether it works without it or not, keep or remove?

I've just been trying to trigger it with a manual "save as", but no joy - it doesn't invoke ResertChannelCharset().

There are test emails in comm/mailnews/test/data/ with a "charset=..." content-type. iso-2022-jp-not-qp.eml is a nice simple one.
I was runnging in gdb, breakpoint on ResetChannelCharset(), drag the .eml file from an outside file browser into a folder, then click "save as..." on the resulting email. Saved it flawlessly, without triggering the breakpoint. Also tried displaying the message with a different char encoding and then clicking "save as", but got the same behaviour (as I'd expect - displaying a message shouldn't influence "save as").

I can't rule out that there's some esoteric path that the nsMimeMessageSaveAs path in ResetChannelCharset() is invoked, but I wouldn't know where to start looking.

Comment 21

[Ben Campbell]

Attachment: 1597891-resetchannelcharset-fix-3.patch

This one leaves in the possibly-unused code, but adds back the diagnostic assert Jorg suggested so anyone who does happen to hit it can find their way back here and let us know!

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 9121922 [details] [diff] [review]
1597891-resetchannelcharset-fix-3.patch

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

Yes, that what I meant.

::: mailnews/mime/src/mimemoz2.cpp
@@ -1775,1 @@
>      mime_stream_data *msd = (mime_stream_data *)(obj->options->stream_closure);

So funny they got this twice, here ...

@@ -1781,5 @@
>          msd->channel->SetContentType(nsDependentCString(ct));
>  
>          // Second, if this is a Save As operation, then we need to convert
> -        // to override the output charset!
> -        mime_stream_data *msd = GetMSD(obj->options);

... and here.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c5d3c34a34c9
Remove superfluous infinite loop in MimeObject ResetChannelCharset(). r=mkmelin,jorgk DONTBUILD