Open Bug 1597891 Opened 2 months ago Updated 22 days ago

Fix or remove MimeObject ResetChannelCharset().

Categories

(MailNews Core :: General, defect)

defect
Not set

Tracking

(Not tracked)

REOPENED

People

(Reporter: benc, Assigned: benc)

References

Details

(Keywords: leave-open)

Attachments

(1 file, 1 obsolete file)

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

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

Assignee: nobody → benc
Attachment #9110160 - Flags: review?(jorgk)
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?
Attachment #9110160 - Flags: review?(jorgk-bmo)

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

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

Attachment #9112741 - Flags: review+
Attachment #9112741 - Flags: feedback?(benc)

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

Attachment #9112741 - Flags: feedback?(benc) → feedback+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/fc1c18cda24b
Remove superfluous infinite loop in MimeObject ResetChannelCharset(). r=jorgk

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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.

Flags: needinfo?(benc)
Target Milestone: Thunderbird 72.0 → ---

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

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

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.

Flags: needinfo?(benc)
You need to log in before you can comment on or make changes to this bug.