User Agent: Mozilla/5.0 (Windows NT 10.0; rv:56.0) Gecko/20100101 Firefox/56.0 Build ID: 20170926190823 Steps to reproduce: Received email message from certain senders with inline message attachment with small mistake in source code at line Content-Type: multipart/related;type="multipart/alternative";; Actual results: Message body is not displayed Expected results: It should throw some error message or view message body text. Other email clients like Outlook or Windows mail renders whole message and attachment correctly

Also, when I edit that EML file and delete one of the semicolons at that line message and attachment is displayed correctly. I manualy edited posted EML file and replaced original image. So, for some reason, when you try to correct that line too, image is displayed as normal attachment and not as inline.

searched for exact duplicate but found none

There is a glitch in nsMIMEHeaderParamImpl::DoParameterInternal() parsing an empty Content-Type header parameter. <https://dxr.mozilla.org/comm-central/rev/32b850fa28ae1c29039cb7ddcdfd71b324762c05/mozilla/netwerk/mime/nsMIMEHeaderParamImpl.cpp#473> | if (*str++ != '=') { | // don't accept parameters without "=" | goto increment_str; If the parameter is empty (';;') there is no '='. So the jump to "increment_str" should skip this parameter and continue with the next one. The "str++" skips over the second ';'. But this character is needed by the "increment_str" code to proceed to the next parameter. Since it is missing, the function is aborted and the remaining parameters are ignored. The fix executes the "str++" only without the jump. The patch displays the example correctly. This is mozilla-central code. Who would be possible as reviewer?

Comment on attachment 8946052 [details] [diff] [review] Bug1412213_v1.patch Content-Type MIME-header - Fix of skipping an empty parameter (In reply to Alfred Peters from comment #3) > This is mozilla-central code. Who would be possible as reviewer? Usually you refer to https://wiki.mozilla.org/Modules/Core and pick one of the peers as reviewer. I know Honza and Valentin from other bugs, so I'll add them. One review will do! ;-) As a matter of interest, how do we get into this code? I thought, header parsing is done in TB code in JS Mime. Do you have a call stack? Or do we string together a big HTML document and pass that to M-C for display?

(In reply to Jorg K (GMT+1) from comment #4) > As a matter of interest, how do we get into this code? I thought, header > parsing is done in TB code in JS Mime. Do you have a call stack? Or do we <https://dxr.mozilla.org/comm-central/rev/2b56d89a8792fc1af6ae66be5f38cd9d26ecb7e1/mailnews/mime/src/mimehdrs.cpp#500> ...this (and others) calls GetParameterInternal() from 'nsMIMEHeaderParamImpl.cpp'.

Comment on attachment 8946052 [details] [diff] [review] Bug1412213_v1.patch Content-Type MIME-header - Fix of skipping an empty parameter Review of attachment 8946052 [details] [diff] [review]: ----------------------------------------------------------------- I don't feel comfortable to review any strstr/*str++ kind of code changes these days... I would suggest to rewrite this using mozilla::Tokenizer from scratch, but might be an overkill (the parser seems complicated, but maybe it's just because it's a *str++ kind of parsing code :))). Anyway, regardless of which way is chosen here, I won't accept such a patch w/o a test for the particular case this is being fixed for. r- for lack of a test. Also, please read the https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch docs before you submit next time. Thanks! ::: netwerk/mime/nsMIMEHeaderParamImpl.cpp @@ +474,4 @@ > // don't accept parameters without "=" > goto increment_str; > } > + str++; if at least.. this really needs a very good comment why it can/has to be here

I think this is a bit harsh, Alfred made a good effort to track down the problem. Yes, a big comment would be good. As for the test, maybe test_MIME_params.js could be extended or taken as a template for a new test.

(In reply to Honza Bambas (:mayhemer) from comment #6) > Anyway, regardless of which way is chosen here, I won't accept such a patch > w/o a test for the particular case this is being fixed for. I added a test. And a second, in the case of a parameter without =. Both tests pass with the patch and fail without it. > > + str++; > > if at least.. this really needs a very good comment why it can/has to be here I did that too.

(In reply to Honza Bambas (:mayhemer) from comment #6) > I don't feel comfortable to review any strstr/*str++ kind of code changes > these days... I would suggest to rewrite this using mozilla::Tokenizer from > scratch, but might be an overkill (the parser seems complicated, but maybe Agreed, but we should do that in a new bug...

Only a few space corrections.

Comment on attachment 8946444 [details] [diff] [review] Fix of skipping a parameter without a = Review of attachment 8946444 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I've pushed to try as I tried the patch locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20823b0cbdc8b293ed26dbc5f25b552a8c0f81f0

For future reference, please try to use commit messages in your patches that summarize what the patch is actually doing, not just restating the problem being fixed. Thanks!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8174e0c380 Content-Type MIME-header - Skipping a parameter without an '='. r=mayhemer