Closed Bug 1510028 Opened 9 months ago Closed 9 months ago

TB crashes editing a .EML file without a 'Content-Type:' header @ nsTDependentString<T>::nsTDependentString<T> via CreateCompositionFields - mime_parse_stream_complete

Categories

(MailNews Core :: MIME, defect, critical)

defect
Not set
critical

Tracking

(thunderbird_esr6064+ fixed, thunderbird64 fixed, thunderbird65 fixed)

RESOLVED FIXED
Thunderbird 65.0
Tracking Status
thunderbird_esr60 64+ fixed
thunderbird64 --- fixed
thunderbird65 --- fixed

People

(Reporter: timrude, Assigned: jorgk)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 1 obsolete file)

TB 60.3.0 and 60.3.1 on Windows 7

To reproduce:

Create a .eml file using another app (or edit a .eml file saved by TB) and omit/remove the 'Content-Type:' header line.

Open the .eml file in TB and it opens correctly in View mode. But then press Ctrl-E to switch to Edit mode and TB crashes.

In pre-60 versions of TB, if the 'Content-Type:' header line was missing, the content type was assumed to be text/plain and both View mode and Edit mode worked with no problem.
Please post your crash ids in text form into this bug report
see https://support.mozilla.org/en-US/kb/mozilla-crash-reporter-tb#w_viewing-crash-reports
Flags: needinfo?(timrude)
Crash ID: b62557a7-bead-48bb-8f6e-ad6f70181127
Flags: needinfo?(timrude)
bp-b62557a7-bead-48bb-8f6e-ad6f70181127 is @ nsTDependentString<T>::nsTDependentString<T>

0 	xul.dll 	nsTDependentString<char>::nsTDependentString<char>(char const*) 	xpcom/string/nsTDependentString.h:80
1 	xul.dll 	CreateCompositionFields(char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char const*, char*, nsIMsgCompFields**) 	comm/mailnews/mime/src/mimedrft.cpp:352
2 	xul.dll 	mime_parse_stream_complete 	comm/mailnews/mime/src/mimedrft.cpp:1393
3 	xul.dll 	nsStreamConverter::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	comm/mailnews/mime/src/nsStreamConverter.cpp:1053
4 	xul.dll 	nsMsgProtocol::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	comm/mailnews/base/util/nsMsgProtocol.cpp:389
5 	xul.dll 	nsMailboxProtocol::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	comm/mailnews/local/src/nsMailboxProtocol.cpp:416
6 	xul.dll 	nsInputStreamPump::OnStateStop() 	netwerk/base/nsInputStreamPump.cpp:706

Based on source lines I suspect these other signatures are related 
* strlen | CreateCompositionFields  bp-66077973-621f-44cd-a7c4-246c80181124
* CreateCompositionFields  bp-cdb88f7b-41c6-44c9-a28e-7dc0c0181126

nsTDependentString<T>::nsTDependentString<T> is the most common signature but not all stacks for that signature relate to this bug.

All crashes are version 60, so let's assume regression
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsTDependentString<T>::nsTDependentString<T> ] [@ strlen | CreateCompositionFields ] [@ CreateCompositionFields ]
Component: General → MIME
Ever confirmed: true
Flags: needinfo?(jorgk)
Keywords: crash, regression
Product: Thunderbird → MailNews Core
Summary: TB crashes when trying to edit a .EML file without a 'Content-Type:' header → TB crashes editing a .EML file without a 'Content-Type:' header @ nsTDependentString<T>::nsTDependentString<T> via CreateCompositionFields - mime_parse_stream_complete
(In reply to timrude from comment #0)
> Create a .eml file using another app (or edit a .eml file saved by TB) and
> omit/remove the 'Content-Type:' header line.
Right. Now you don't have a charset and will crash here:

  if (from) {
    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(from),
                                    nsDependentCString(charset),  <===
                                    outString);
    cFields->SetFrom(outString);
  }

Patch coming.
Flags: needinfo?(jorgk)
BTW, the change is from here: https://hg.mozilla.org/comm-central/rev/bc4dff04bede#l17.62
Blocks: 1410973
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #9027859 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9027859 [details] [diff] [review]
1510028-no-null-in-nsDependentCString.patch

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

LGTM, r=mkmelin
Attachment #9027859 - Flags: review?(mkmelin+mozilla) → review+
I'm afraid that's not enough. I really need to go through https://hg.mozilla.org/comm-central/rev/bc4dff04bede and check all the call sites where nsDependentCString() was added. The argument may be null. Sigh.
I went through bc4dff04bede with a fine comb looking for
  ^\+.*nsDependentCString

That gave 29 hits:
Line 94: +      nsMsgI18NConvertToUnicode(nsDependentCString(charset), OK
Line 238: +    if (NS_SUCCEEDED(nsMsgI18NConvertFromUnicode(nsDependentCString(charset), FIXED
Line 528: +    rv = nsMsgI18NConvertFromUnicode(nsDependentCString(m_compFields->GetCharacterSet()),
Line 635: +    rv = nsMsgI18NConvertFromUnicode(nsDependentCString(aCharset), OK
Line 658: +      rv = nsMsgI18NConvertFromUnicode(nsDependentCString(aCharset), OK
Line 702: +                                   nsDependentCString(m_attachment1_body), OK
Line 791: +    nsMsgI18NConvertToUnicode(nsDependentCString(fallbackCharset), FIXED
Line 815: +  nsMsgI18NConvertFromUnicode(nsDependentCString(msg.GetBodyCharset()), bodyW, bodyA); FIXED
Line 860: +                                              nsDependentCString((char *) aFiles[i].lpszFileName), OK
Line 883: +                                   nsDependentCString((char *) aMessage->lpszSubject), OK
Line 902: +                                   nsDependentCString((char *) aMessage->lpszNoteText), OK
Line 946: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(from), OK
Line 947: +                                    nsDependentCString(charset), FIXED
Line 959: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(reply_to), OK
Line 960: +                                    nsDependentCString(charset), FIXED
Line 967: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(to), OK 
Line 968: +                                    nsDependentCString(charset), FIXED
Line 975: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(cc), OK 
Line 976: +                                    nsDependentCString(charset), FIXED
Line 983: +    nsMsgI18NConvertRawBytesToUTF16(nsDependentCString(bcc), OK 
Line 984: +                                    nsDependentCString(charset), FIXED
Line 1004: +            rv = nsMsgI18NConvertToUnicode(nsDependentCString(bodyCharset), OK
Line 1005: +                                           nsDependentCString(body), OK
Line 1029: +                                   nsDependentCString(opt->default_charset), FIXED
Line 1049: +      if (NS_SUCCEEDED(nsMsgI18NConvertFromUnicode(nsDependentCString(charset), OK
Line 1094: +          rv = nsMsgI18NConvertToUnicode(nsDependentCString(mailCharset), OK
Line 1115: +      rv = nsMsgI18NConvertFromUnicode(nsDependentCString(mailCharset), OK
Line 1139: +        rv = nsMsgI18NConvertToUnicode(nsDependentCString(mailCharset), OK
Line 1161: +        rv = nsMsgI18NConvertFromUnicode(nsDependentCString(mailCharset), OK

which I checked individually. OK means it's clear from the context the the argument can't be null.

Five fixes were in the previous patch, here I'm adding four more to make it nine. Most likely those additional four are not necessary, but better be safe than sorry. Not worth a crash.
Attachment #9027859 - Attachment is obsolete: true
Attachment #9027871 - Flags: review?(mkmelin+mozilla)
Oops, miscounted that, five additional fixes, this one here
Line 528: +    rv = nsMsgI18NConvertFromUnicode(nsDependentCString(m_compFields->GetCharacterSet()),
is also a FIXED.
Comment on attachment 9027871 [details] [diff] [review]
1510028-no-null-in-nsDependentCString.patch (v2)

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

::: mailnews/mime/src/mimehdrs.cpp
@@ +41,5 @@
>      nsAutoCString output;
>      nsMsgI18NConvertRawBytesToUTF8(value,
> +                                   opt->default_charset
> +                                     ? nsDependentCString(opt->default_charset)
> +                                     : EmptyCString(),

odd ? placement, usually it's on the previous line
Attachment #9027871 - Flags: review?(mkmelin+mozilla) → review+
Note that passing null to nsMsgI18NConvertFromUnicode() when it still had a char* argument would have crashed here:
https://hg.mozilla.org/comm-central/rev/bc4dff04bede#l5.24 or here
https://hg.mozilla.org/comm-central/rev/bc4dff04bede#l5.53

Before switching to encoding_rs the code was like this (line in TB 52):
https://dxr.mozilla.org/comm-esr52/source/mailnews/base/util/nsMsgI18N.cpp#119
and
https://dxr.mozilla.org/comm-esr52/source/mailnews/base/util/nsMsgI18N.cpp#128
would also have crashed on an empty charset on !*aCharset dereferencing null.

So I suspect, the crash in mimedrft.cpp is new, before it would have crashed in nsMsgI18N.cpp in nsMsgI18NConvertToUnicode() or nsMsgI18NConvertFromUnicode() when those received and empty charset.

Were they any crashes of that nature that have disappeared?

Surely doing those charset conversions with an empty charset will lead to "interesting" results ;-(
(In reply to Magnus Melin [:mkmelin] from comment #11)
> odd ? placement, usually it's on the previous line
Actually, no, I'm following our linting effort now where the ? goes to start of the line. Looking in MsgComposeCommands.js there are both types now, so I'm confused. I'll do some checking before landing this, but I'm pretty sure lately I've been doing them all like this.
(In reply to Jorg K (GMT+1) from comment #12)
> ...
> So I suspect, the crash in mimedrft.cpp is new, before it would have crashed
> in nsMsgI18N.cpp in nsMsgI18NConvertToUnicode() or
> nsMsgI18NConvertFromUnicode() when those received and empty charset.
> 
> Were they any crashes of that nature that have disappeared?

We have several low volume crashes (that don't have bug report) that don't happen in version 60 from https://crash-stats.mozilla.com/search/?signature=~nsMsgI18NConvertToUnicode&product=Thunderbird&date=%3E%3D2018-08-27T09%3A22%3A44.000Z&date=%3C2018-11-27T08%3A22%3A44.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature 

OOM | large | NS_ABORT_OOM | AppendUTF8toUTF16 | CopyUTF8toUTF16 | nsMsgI18NConvertToUnicode   
OOM | large | NS_ABORT_OOM | AppendASCIItoUTF16 | CopyASCIItoUTF16 | nsMsgI18NConvertToUnicode   
shutdownhang | ConvertUTF8toUTF16::write | AppendUTF8toUTF16 | AppendUTF8toUTF16 | CopyUTF8toUTF16 | nsMsgI18NConvertToUnicode  
shutdownhang | memcpy | nsAString_internal::Assign | nsAString_internal::Assign | nsAString_internal::Assign | nsMsgI18NConvertToUnicode   
shutdownhang | nsMsgI18NConvertToUnicode   

And https://crash-stats.mozilla.com/signature/?product=Thunderbird&_sort=-date&signature=OOM%20%7C%20large%20%7C%20NS_ABORT_OOM%20%7C%20CopyUTF16toUTF8%20%7C%20nsMsgI18NConvertFromUnicode&date=%3E%3D2018-08-27T09%3A22%3A44.000Z&date=%3C2018-11-27T08%3A22%3A44.000Z
Dunno which linting efforts you refer to. That is an odd style. *All* the eslint examples place ? on the first line - https://eslint.org/docs/rules/multiline-ternary
(In reply to Wayne Mery (:wsmwk) from comment #14)
> We have several low volume crashes (that don't have bug report) that don't
> happen in version 60 from
... yes, in nsMsgI18NConvertToUnicode and nsMsgI18NConvertFromUnicode as predicted.

(In reply to Magnus Melin [:mkmelin] from comment #15)
> Dunno which linting efforts you refer to.
Linting in mail/ and mailnews/. Anyway, I was a friend of

x ?
  y :
  z

Doing this in Calendar, where I must have coded something, gives
2357:80  error  ':' should be placed at the beginning of the line.  operator-linebreak (eslint)

So I assumed our linting rules mandated ? and : a the front since surely you don't want to place ? at the end and : at the front.

Looking at the freshly linted MsgComposeCommands.js I see:
  let msgCompFormat = (aEvent && aEvent.shiftKey) ?
    Ci.nsIMsgCompFormat.OppositeOfDefault :
    Ci.nsIMsgCompFormat.Default;
and
      var recommAction = (convert == Ci.nsIMsgCompConvertible.No)
                          ? Ci.nsIMsgCompSendFormat.AskUser
                          : Ci.nsIMsgCompSendFormat.PlainText;

I also added some at the front here recently:
https://hg.mozilla.org/comm-central/rev/b0459a76e420#l3.25 (and two more in that changeset).

So how would you like it, I'm thoroughly confused now :-( - and wasted too much time of this nit-ty issue.
Flags: needinfo?(mkmelin+mozilla)
We also had ? and : at the beginning of lines, I even took some out:
https://hg.mozilla.org/comm-central/rev/01215911a27e#l1.411
Looks for |   : | in the changeset.

I think

abcdefghijk
  ? xxx
  : yyy;

looks OK and so does

abcdefghijk ?
  xxx : yyy;
We definitely want operator-linebreak after, that's questionmarks at the end of line

So this is correct:

  let msgCompFormat = (aEvent && aEvent.shiftKey) ?
    Ci.nsIMsgCompFormat.OppositeOfDefault :
    Ci.nsIMsgCompFormat.Default;
Flags: needinfo?(mkmelin+mozilla)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/096fefc1934f
Add null check to avoid crash due to nsDependentCString(null). r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Magnus, I went with you style although mailnews/mime/src/mimehdrs.cpp is full of the ? and : at the front.
Target Milestone: --- → Thunderbird 65.0
Attachment #9027871 - Flags: approval-comm-esr60+
Attachment #9027871 - Flags: approval-comm-beta+
Thanks for the fix!
You need to log in before you can comment on or make changes to this bug.