User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.1) Gecko/20040707 Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.1) Gecko/20040707 When viewing a mail with the following MIME structure: Content-Type: multipart/alternative Content-Type: text/plain Content-Type: multipart/related Content-Type: text/html Content-Type: image/jpeg Content-Type: image/gif Content-Type: image/jpeg ... Content-Type: image/gif as "plain text", the existing text/plain part is ignored completely. Instead, the text/html part is displayed, converted to plain text. Reproducible: Always Steps to Reproduce: 1. Extract the attached sample MBOX file to Local Folders as file "plaintextbug" 2. Open the "plaintextbug" folder in Mailnews 3. Read the contained mail message using "View" / "Message Body as" / "Plain Text" Actual Results: The text/html part of the message (beginning with "30/2004 Shopping <link> Günstige Charterflüge") is displayed as plain text. Expected Results: The text/plain part of the message (beginning with "+++ GMX Magazin 30/2004 Liebes GMX Mitglied" should be displayed. This might be related to Bug 138333 where nothing at all was displayed under the conditions given here. Possibly the patch still did not fix it completely, picking the wrong message part to display.

MBOX file exposing the bug

Yep, I see this is in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a3) Gecko/20040726

The behaviour is correct per spec, although it may not be ideal. multipart/alternative means that the client can pick *any one* of the parts, preferrably the later one. The later part is the multipart/relative container, which contains an HTML doc with embedded images. HTML is disallowed, so it's being downconverted. You can say that it's better to show the plaintext part that the sender sent instead of downconverting HTML to plaintext. I tried to do that, but I had to hardcode a set of known unwanted mimetypes due to the implementation (it would have been much harder to prefer text/plain than it was to discourage picking HTML). The result should be still OK. Reading the code <http://lxr.mozilla.org/seamonkey/source/mailnews/mime/src/mimemalt.cpp#226> it seems that you hit an old (probably 4.x or earlier) ceveat, i.e. no implementation for these edge cases, but still strictly conforming to spec and appearantly not a real problem so far.

You are partially right. RFC 2046 says "the best choice is the LAST part of a type supported by the recipient system's local environment". BUT if the user explicitly selects "view as plain text" I would not consider HTML as being "supported by the recipient", but restrict the acceptable MIME types to text/plain only. Btw, the attachment is NOT message/rfc822 because it has the "From - <date>" line at the beginning which distinguishes MBOX format from *.eml format. I am not changing the MIME type back though, because I don't want to start a war :)

I forgot to mention that there is no need to descend into multipart/related MIME parts to determine their base type, since they are required to have a "type=..." parameter (RFC 2112). If this parameter is missing, this MIME part is broken and can (or should?) be ignored. On the other hand multipart/related can always be treated as an unwanted MIME type in plaintext mode, since plaintext cannot have related parts at all.

(In reply to comment #4) > Btw, the attachment is NOT message/rfc822 because it has the "From - <date>" > line at the beginning which distinguishes MBOX format from *.eml format. I am > not changing the MIME type back though, because I don't want to start a war :) The advantage to having it typed as message/rfc822 is: when loading the attachment in the browser, the text is formatted as mail; and the browser doesn't care if the From line is there or not.

How do you switch between "View as plain text" and "View as HTML" when viewing the mail in the browser?

Ah, I see, the browser view honors the setting in Mailnews. Interesting, I did not expect that. Ignore my Comment #7 :)

Bug 348045 is about the opposite problem to this: if a message is multipart/alternative but has only a text/html (no text/plain, and not multipart/related), the HTML is not downconverted on View As | Plain.

sorry for the spam. making bugzilla reflect reality as I'm not working on these bugs. filter on FOOBARCHEESE to remove these in bulk.

I do not know if any work has been done on this, but I want to add something to comment #3. If it is as you say and the behaviour is correct per spec, why it DOES NOT behave the same with a message written in this format: Content-Type: multipart/alternative Content-Type: text/plain Content-Type: text/html With such an email (without embedded images, but with html and with text), the "View as plain text" option prints the text/plain part and NOT the simplified text/html part. (this is of course the behavior I want) In addition, there IS an Option "view simplified html" - so there is no reason, why not to print the plain/text part, if the user selects "view as plain text". Therefore, what is described in this bugreport is a bug and needs to be fixed.

I think I said it all in comment 3. Yes, it we could just use the sender's plaintext part instead of downconverting the HTML part. I tried to do that, but had to do it by blacklisting certain content types, that's why it works for normal HTML, but not HTML with embedded images. So, I agree this bug is valid (otherwise I'd have closed it), but too hard to implement, considering the current code, to be worth it.

In our application, we need to send mail in multipart/alternative type, and we allow HTML to have embedded images. The structure reported in this bug is the only way I found to work. But unfortunately I found TB can't display the text/plain part as I expected. The bat! and Apple Mail both can correctly show it. I think converting HTML to plain text should be the last choice. If text/plain part is present, "View Message Body As Plain" should give it higher priority. Or at least, TB provide similar function as Apple Mail to let user navigate among parts. I hope this bug can be fixed as soon as possible.

Will check this out if I ever get back to fixing similar bugs like Bug 551698 and family.

Why not whitelist text/plain instead of blacklisting text/html. That way, if prefer_plaintext is true and if in a multipart/alternate, there is a text/plain part present, _always_ pick that one, no matter what the other part is? Or if the current code only supports blacklists (why would that be?), at least add multipart/related to the blacklist

> Why not whitelist text/plain instead of blacklisting text/html. I very much wanted and tried to do that, but I couldn't do it given the code organization. See comment 3.

Could you post some code (or pseudo-code) of the relevant section, maybe somebody has an idea how to restructure this in order to achieve the goal? I suppose you have something like foreach s in alternatives { if(s.type not in blacklist) { return s; } if( s is last) { return s; } } Couldn't this be rewritten as: foreach s in alternatives { if(s.type in whitelist) { return s; } if( s is last) { return s; } } Or is the problem that you are trying whether the multipart/related section actually contains text/html or a text/plain section within it? If that's the case, is it even possible that multipart/related contains a main text/plain section? As text/plain doesn't have any embedded elements (pictures, style sheets), there would be no reason to put that into a multipart/related, or would it?

> Could you post some code (or pseudo-code) of the relevant section I already did. See comment 3!

The fundamental problem is that it's line-based stream processing. We do not buffer the whole email. The (most preferred) HTML part comes first, but at that time we do not know what's following. It's solvable, e.g. by converting both parts to display and only in the end decide which one to take, but that's double processing and not trivial to implement either, because you'd need to changed entirely how the multipart/alternative code works. I guess the simple fix here would be to blacklist multipart/related - if that doesn't break anything.

> I already did. See comment 3! Oops, sorry missed that comment. In there you have: 247 if (prefer_plaintext 248 && self->options->format_out != nsMimeOutput::nsMimeMessageSaveAs 249 && (!PL_strncasecmp(ct, "text/html", 9) || 250 !PL_strncasecmp(ct, "text/enriched", 13) || 251 !PL_strncasecmp(ct, "text/richtext", 13)) 252 ) 253 // if the user prefers plaintext and this is the "rich" (e.g. HTML) part... 254 { 255 return PR_FALSE; 256 } Couldn't that be changed into: 247 if (prefer_plaintext 248 && self->options->format_out != nsMimeOutput::nsMimeMessageSaveAs 249 && (PL_strncasecmp(ct, "text/plain", 10) && 250 PL_strncasecmp(ct, "text/otherplain", 15) && 251 ...) 252 ) 253 // if the user prefers plaintext and this is the "rich" (e.g. HTML) part... 254 { 255 return PR_FALSE; 256 } But of course, just adding multipart/related to the blacklist would probably work just as well (at least, for this particular message... until somebody else discovers another "HTML-like" Mime-type...) > The fundamental problem is that it's line-based stream processing. We do not > buffer the whole email. The (most preferred) HTML part comes first, but at > that time we do not know what's following. But exactly how does that make a whitelist more difficult than a blacklist? The bug seems not to be related to the order, as text followed by html works fine, but text followed by multipart/related doesn't. I.e. in both cases the "good" part precedes the "bad". The only case where line-based parsing hurts is in situations where you need to go back (multipart composed _only_ of HTML): you at first eliminate the html part, then noticed that there is no other part, and then you've got to go back. But as far as I understand, that scenario is handled all right. > It's solvable, e.g. by converting > both parts to display and only in the end decide which one to take, but > that's double processing and not trivial to implement either, because you'd > need to changed entirely how the multipart/alternative code works. Wouldn't it be easier to instead buffer the inputs (i.e. _store_ both parts, then decide which one to take, and only in the very end convert the chosen part to display?). Well you'd still incur a memory penalty, but with your approach you'd incur a memory- and a conversion penalty. Or is the unprocessed text so much larger than the one converted to display? (other than base64 overhead?) ... and if there is a "real" (i.e. non-HTML) attachment in addition to the HTML part, then usually the multipart-mixed (to attach the non-HTML document) is at the root, containing the multipart-alternative under it. So, probability of large "real" movie and pps attachment gooing up the works would be quite low (because these would already have been handled _before_ the multipart/alternate processing starts...)

> Wouldn't it be easier to instead buffer the inputs Most of the network and conversion code is designed to avoid exactly that, for memory reasons (msgs can be big, and you do not know what's coming, could be a 40 MB attachment). > Couldn't that be changed into: No, for the reasons explained. Please do not guess or just ask about code, that's not helpful. You're welcome to create a patch and test whether your ideas work (in normal cases and exceptional ones like this bug).

This patch fixes the issue described, by using a whitelist of text/plain instead of a blacklist of text/html, text/enrichted, text/richtext

This patch fixes the issue described, by using a whitelist of text/plain instead of a blacklist of text/html, text/enrichted, text/richtext

Did you test it, whether it works in both setups, with prefered HTML and preferred plaintext? I very much doubt that it does, because I wrote this code originally, and that's the obvious way to do it, and I *wanted* to do it like that, but it didn't work. And I made a comment in the source code saying exactly that.

Yes, it does work in both setups (prefer_plaintext=true and false), both with HTML and with plain-text messages. What exactly happened in your case? Do you have a testcase where it breaks?

No, I can't remember why I didn't do it like that, I just know there was a reason, because I wanted to do it like this, and tried hard. Did you try a mail with attachments?

I just did, and it still works fine (both with and without prefer_plaintext).

Uwe this might have been fixed by bug 351224. Could you take a few minutes and download the latest nightly ( http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-central/ ), backup your profile and test and let us know if this is fixed or not ?

I just tried Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101018 Thunderbird/3.3a1pre from http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-central/ , and it still doesn't work (tested with my test case from bug 592819 comment 5). Whoever fixed it: did they test it themselves? Did it work now? Or what did happen?

> Whoever fixed it Ludo just asked whether it was fixed by coincidence. Apparently it was not. Thanks for testing.

Is there any hope of a resolution for this? This is more than an annoyance for me. I generate mail messages this way, which is per the MIME spec - that is, multipart/alternative with text/plain version first, then the multipart/related bundle of html & images. The real text/plain version is important for recipients who read their e-mail with a screen reader - and is both optimized for those people and has specific content for them (e.g. text-format tables and links to screen-reader friendly versions of web pages - that don't have FLASH, etc). The html down-conversion isn't particularly friendly to screen readers, but more importantly, recipients don't get the different content that is in the text/plain version of the message. So my recipients can't use TB for these e-mails. And I have to validate these messages using another mail client. (Squirrelmail handles this nicely, BTW.) I believe Ben (the developer) when he says that the code somehow makes this a difficult fix - though I'm at a loss to follow the explanations of why. Yes, I've read comment #3. And 21. And 1-34. And I don't see that anyone identified an issue with the patch proposed in .25. Although #21 indicates that the difficulty is that the html version comes first, it does not. At least, not on the wire. And not in the problem statement. The MIME spec was written to make this straightforward. The reader is supposed to take the last alternative *that it (has been told to) understand(s)*. Which means that the linear, line-by line processing is supposed to be able to say "I only understand text/plain; this alternative is text/plain; display it". IIRC, VMSmail adopted this strategy c.a. 1993ish. Or, if it's REALLY dumb (TB is not), the plaintext will display first (with some headers), followed by cybercrud. But the plaintext gets through. It's a nice bonus that thunderbird will down-convert HTML if the user selected text/plain, but no text/plain alternative is present. But if we have to choose, TB should do what the sender and recipient explicitly arranged for. If the user has selected view->message body->as plain text, it is known what is accceptable when the message display process starts. The text/plain version comes first. (Not the hmtl version as indicated in #21.) So there is no buffering required to make the decision for text/plain. (But there is for html, as if no html is present, backtracking to the text/plain section is necessary. Again, the MIME spec tries to make this easy in that the simpler versions come first, and so should require less memory to buffer than the complex ones.) If plaintext is required by the user: o When a text/plain section is encountered, display it. Optionally, make any following sections available as attachments. o If any other type is encountered, error: "This message was not sent with a plaintext version" - and (optionally) do the down-conversion, or display the html. If html version is acceptable: o When a non-html alternative is encountered, discard any previously-buffered alternative, and buffer the new one. o When an html alternative is ecnountered, display it. o If no html alternative is present, display the buffered alternative (or the message body is empty.) And as indicated in comment #5, in this mode, multipart/related bundles can simply be ignored (or turned into an attachment, which is what Squirrelmail does when the plaintext option is selected.) Real life is slightly more complicated if the message is signed/encrypted. But that wrapper is logically removed first. (Though to avoid buffering, it may be implemented in parallel.) So my bottom line: This is actually important. The proposed patch hasn't been acted on - though no hard reason was given. The explanation for why a proper fix is hard (buffering) doesn't match the message structure. Perhaps with the passage of time and fresh eyes, this can finally be fixed? I, and those who receive e-mail from me would appreciate it. Thanks.

Why is this not fixed yet? It even has a working fix in comment 25. See https://bugzilla.mozilla.org/attachment.cgi?id=471497

I have made a new diff that solves this bug once and for all. Please review it and commit. I am sorry that I was not able to test it my self, because my computer had too little RAM to build thunderbird.

Comment on attachment 714770 [details] [diff] [review] Patch to fix bug 253830 Ok, the patch compiles and builds fine. It seems to work on the attached testcase mbox file, according to the requested results in comment 0. Without the patch, TB is showing the downconverted HTML as originally claimed. Is the patch adressing BenB's issues from comment 26, or is it just the original patch refreshed? Cleaning up requests: I think BenB is not a reviewer, no idea who is jik.

Yes, this patch is addressing all the issues. When I looked at the code I saw that the original patch would not work, so I did some rewrite of the code in the file mimemalt.cpp. I think this patch will also fix bug 348045

jik is Jonathan Kamens, the one who last did a major rewrite of mailnews/mime/src/mimemalt.cpp

I added a comment in this patch on how the code can be improved. Other than the comment change, it is the same as the previous patch.

Sorry about the repeated replacement of patches, but the comment I had added turned out to be a misleading when I thought about it, so I had to update the comment. Nothing is changed in this patch apart from the comment.

Comment on attachment 714895 [details] [diff] [review] New patch with updated comment Review of attachment 714895 [details] [diff] [review]: ----------------------------------------------------------------- This is missing tests. I realize that our MIME-to-HTML converter is not easy to test in general, but if you're just trying to select which alternative is presented, that is easily tested by giving text/plain and text/html very different part bodies and checking which contents can be found. The test should also be exercising the code under various preference options. Also, I am reminded at how much simpler this was to do in jsmime. :-) ::: mailnews/mime/src/mimemalt.cpp @@ +74,5 @@ > - Jonathan Kamens, 2010-07-23 > + > + When the option prefer_plaintext is on, the last plain text part > + should be preferred over any other part that can be displayed. But > + if no plain-text part is found, then the algoritm should go as Nits: Use text/plain instead of "plain text" or "plain-text". "algorithm" has an h in it.. @@ +186,3 @@ > { > /* > + The cache should always have at the head the part with highest priority Nit: Sentences in comments should always end in punctuation. @@ +235,5 @@ > + /* Not finished */ > + if (in_buffer_priority > next_priority) { > + /* Keep the buffer and continue to next part (Case 5) */ > + do_flush = false; > + } /* else Case 4 and 6 */ Please keep the logic like it originally was. It is easier to follow the control flow that way. @@ +301,5 @@ > if (! malt->part_buffers) > return MIME_OUT_OF_MEMORY; > } > + if (malt->pending_parts > malt->pri_buff_len) { > + malt->pri_buff_len += PRI_MALLOC_BLOCK; You shouldn't need an array for buffer priorities anyways, but if you did, you should have used sizeof(int32_t) here instead of an obscured hard-coded 4. @@ +391,2 @@ > { > + priority = 12; Why did you pick 12 here? And 5 later? Move these numbers to #defines near the top of the file so you can give them descriptive names. @@ +400,5 @@ > + (A text/plain part inside a multipart is more likely to be > + only a part of the massage, but if there are multiple multipart > + sub-parts then one that contains a true text/plain should be chosen > + over a multipart that does not contain a true text/plain.) > + But I do not know how to test for this. - Terje Bråten. Under the current architecture of libmime, this is impossible. And if you were to propose a patch to make it possible, I would r- it immediately on the basis that it would be an extremely invasive change and that all of this code is a year or so away from being completely ripped out. ::: mailnews/mime/src/mimemalt.h @@ +29,4 @@ > MimeHeaders **buffered_hdrs; /* The headers of pending parts */ > MimePartBufferData **part_buffers; /* The data of pending parts > (see mimepbuf.h) */ > + int32_t *buffered_priority; /* Priorities of pending parts */ Why do you need an array of buffered priorities? You only ever use buffered_priority[0]. Also, use uint32_t here.

(In reply to Joshua Cranmer [:jcranmer] from comment #43) [snip] > > This is missing tests. I realize that our MIME-to-HTML converter is not easy > to test in general, but if you're just trying to select which alternative is > presented, that is easily tested by giving text/plain and text/html very > different part bodies and checking which contents can be found. The test > should also be exercising the code under various preference options. Yes, I know it is missing tests. But since I am unable to link thunderbird on my computer, I cannot run the code. So I am afraid I have pass the deveolpment of tests to some one else. Ok, changed all your nits. [snip] > > @@ +235,5 @@ > > + /* Not finished */ > > + if (in_buffer_priority > next_priority) { > > + /* Keep the buffer and continue to next part (Case 5) */ > > + do_flush = false; > > + } /* else Case 4 and 6 */ > > Please keep the logic like it originally was. It is easier to follow the > control flow that way. I find it much easier to follow when it is like this. I thought is was a major improvement and simplification of the logic. > > @@ +301,5 @@ > > if (! malt->part_buffers) > > return MIME_OUT_OF_MEMORY; > > } > > + if (malt->pending_parts > malt->pri_buff_len) { > > + malt->pri_buff_len += PRI_MALLOC_BLOCK; > > You shouldn't need an array for buffer priorities anyways, but if you did, > you should have used sizeof(int32_t) here instead of an obscured hard-coded > 4. No, here you misunderstand. It increases the buffer with 4 integers at a time. This is because I think it is a waste to increase buffer size with only 1 integer a time. The number 4 is randomly chosen, and could as well be any low single digit number. Since few mails have more than 4 parts I thought it would be a good number to use. > > @@ +391,2 @@ > > { > > + priority = 12; > > Why did you pick 12 here? And 5 later? Just random picked. I could have used 1 and 2, but then it would be harder to fit in other priorities if that was needed later. > > Move these numbers to #defines near the top of the file so you can give them > descriptive names. Is it not more useful to have priority as a number? I find that to be a lot more descriptive than a random name, since priorities are compared to each other. > > @@ +400,5 @@ > > + (A text/plain part inside a multipart is more likely to be > > + only a part of the massage, but if there are multiple multipart > > + sub-parts then one that contains a true text/plain should be chosen > > + over a multipart that does not contain a true text/plain.) > > + But I do not know how to test for this. - Terje Bråten. > > Under the current architecture of libmime, this is impossible. And if you > were to propose a patch to make it possible, I would r- it immediately on > the basis that it would be an extremely invasive change and that all of this > code is a year or so away from being completely ripped out. Ok. So I take it you want this comment to be gone then? > > ::: mailnews/mime/src/mimemalt.h > @@ +29,4 @@ > > MimeHeaders **buffered_hdrs; /* The headers of pending parts */ > > MimePartBufferData **part_buffers; /* The data of pending parts > > (see mimepbuf.h) */ > > + int32_t *buffered_priority; /* Priorities of pending parts */ > > Why do you need an array of buffered priorities? You only ever use > buffered_priority[0]. What is in position 0 of the buffer changes after the buffer is flushed. Do you want more complicated logic to avoid storing all the priorities in the buffer? > > Also, use uint32_t here. I used int32_t becuse it was used for the other varibles, like pending_parts etc. Since it is count of mime parts that is increased with 4 at a time, it does not make a difference if it is int32_t or uint32_t. (But if you insist I can make the change.)

(In reply to Terje Bråten from comment #44) > (In reply to Joshua Cranmer [:jcranmer] from comment #43) > > @@ +235,5 @@ > > > + /* Not finished */ > > > + if (in_buffer_priority > next_priority) { > > > + /* Keep the buffer and continue to next part (Case 5) */ > > > + do_flush = false; > > > + } /* else Case 4 and 6 */ > > > > Please keep the logic like it originally was. It is easier to follow the > > control flow that way. > > I find it much easier to follow when it is like this. I thought is was a > major improvement and simplification of the logic. There is a big comment at the beginning of the method that breaks the input down into 6 cases. It's much easier to visually verify correctness when the six cases are a cascading if-else rather than requiring me to parse every control flow path for a case individually. > > > > Move these numbers to #defines near the top of the file so you can give them > > descriptive names. > > Is it not more useful to have priority as a number? I find that to be a lot > more descriptive than a random name, since priorities are compared to each > other. You appear to misunderstand me. What I was asking for was something like this: /// The normal priority for a part #define PRIORITY_NORMAL 10 /// The priority for a non-text/plain part if we are preferring text/plain #define PRIORITY_DOWNGRADED 5 (Perhaps also add in a PRIORITY_UNDISPLAYABLE?) Actually, on second thought, this really ought to be an enum instead of an integer. > > ::: mailnews/mime/src/mimemalt.h > > @@ +29,4 @@ > > > MimeHeaders **buffered_hdrs; /* The headers of pending parts */ > > > MimePartBufferData **part_buffers; /* The data of pending parts > > > (see mimepbuf.h) */ > > > + int32_t *buffered_priority; /* Priorities of pending parts */ > > > > Why do you need an array of buffered priorities? You only ever use > > buffered_priority[0]. > What is in position 0 of the buffer changes after the buffer is flushed. > Do you want more complicated logic to avoid storing all the priorities in > the buffer? The buffered_priority array is never touched other than a realloc when more pending_parts are added, and it is never accessed accept via buffered_priority[0]. Since the design of the code is such that only the first part in the buffer is considered as a candidate for choice of actual display, there is no reason to keep track of the priorities of the later parts in the buffer.

(In reply to Joshua Cranmer [:jcranmer] from comment #45) > (In reply to Terje Bråten from comment #44) > > (In reply to Joshua Cranmer [:jcranmer] from comment #43) [snip] > > > Please keep the logic like it originally was. It is easier to follow the > > > control flow that way. > > > > I find it much easier to follow when it is like this. I thought is was a > > major improvement and simplification of the logic. > > There is a big comment at the beginning of the method that breaks the input > down into 6 cases. It's much easier to visually verify correctness when the > six cases are a cascading if-else rather than requiring me to parse every > control flow path for a case individually. So what you are saying is that if it where not for the break down in the comment at the beginning, you would agree that it was a simplification of the logic? > > > > > > > Move these numbers to #defines near the top of the file so you can give them > > > descriptive names. > > > > Is it not more useful to have priority as a number? I find that to be a lot > > more descriptive than a random name, since priorities are compared to each > > other. > > You appear to misunderstand me. What I was asking for was something like > this: > > /// The normal priority for a part > #define PRIORITY_NORMAL 10 > /// The priority for a non-text/plain part if we are preferring text/plain > #define PRIORITY_DOWNGRADED 5 > > (Perhaps also add in a PRIORITY_UNDISPLAYABLE?) Yes, but then if you have to do an extra lookup to see what is the actual number of a priority if you want to compare the priorites when you read the code. But if you want names for the priorites I guess I can add that. > > Actually, on second thought, this really ought to be an enum instead of an > integer. Ok, I can make it an enum then. > > The buffered_priority array is never touched other than a realloc when more > pending_parts are added, and it is never accessed accept via > buffered_priority[0]. Since the design of the code is such that only the > first part in the buffer is considered as a candidate for choice of actual > display, there is no reason to keep track of the priorities of the later > parts in the buffer. Ok, I agree.

Here is a new patch

New patch with the logic even more simplified. (Just 2 ifs)

An even more streamlined patch. Removed an uneccesesary local variable.

Comment on attachment 715204 [details] [diff] [review] Fixing bug 253830 and bug 348045 Review of attachment 715204 [details] [diff] [review]: ----------------------------------------------------------------- Next time you make a patch, could you please add a -p option? It makes it much easier to review. Better, but, as I mentioned earlier, I would still like to see a test before I can give this r+. ::: mailnews/mime/src/mimemalt.cpp @@ +224,5 @@ > + do_display = true; > + } else { > + /* Keep the buffer and continue to next part (Case 5) */ > + do_flush = false; > + } As mentioned in my previous review, please restore the logic here to the original code.

(In reply to Joshua Cranmer [:jcranmer] from comment #50) > Comment on attachment 715204 [details] [diff] [review] > Fixing bug 253830 and bug 348045 > > Review of attachment 715204 [details] [diff] [review]: > ----------------------------------------------------------------- > > Next time you make a patch, could you please add a -p option? It makes it > much easier to review. > > Better, but, as I mentioned earlier, I would still like to see a test before > I can give this r+. You have to give this bug to someone else then. I do not have enough RAM on my machine to continue this development. > > ::: mailnews/mime/src/mimemalt.cpp > @@ +224,5 @@ > > + do_display = true; > > + } else { > > + /* Keep the buffer and continue to next part (Case 5) */ > > + do_flush = false; > > + } > > As mentioned in my previous review, please restore the logic here to the > original code. Why on earth do you want 5 ifs instead of just 2? Any way, this is the logic that had to be rewritten to fix this bug. I cannot just simply reinstall the original ifs, because then the code would not be correct again, I would have to change it to implement my change in logic. I guess you can rewrite it yourself, I have moved on to other things.

I'll take a look to see whether we can use the patch.

Next I will address Joshua's review issue from comment #50. I will also add a test. Tested with attachment 154853 [details]. When displaying the message as plain text, I now see the text/plain part *and* the HTML part forced to plain text. Is this intentional?

As far as I can see, this is functionally equivalent to the original patch, yet it has the review issue from comment #50 addressed. Terje, can you please look at the interdiff between this patch and the previous one and tell me whether this is OK. I would then add some tests and submit it for review.

This is the interdiff I'd like you to look at: https://bugzilla.mozilla.org/attachment.cgi?oldid=8757007&action=interdiff&newid=8757371&headers=1 I know you weren't so enthused about restoring the old code (comment #51: "Why on earth do you want 5 ifs instead of just 2?"), but the reviewer insisted on it ;-( Either we follow what the reviewer asked for or your good work will be lost. Let's not lose it ;-) It's been sitting there for three year with no action, so let's get this done. I'll write the tests in a second patch. This way your work will get landed in your name, and the test in mine. How does this sound?

In mailnews/mime/src/mimemalt.cpp at line 214 you have: have_displayable = MimeMultipartAlternative_display_part_p(obj, malt->buffered_hdrs[0]); This needs to be changed to: have_displayable = (malt->buffered_priority > next_priority); Kind regards Terje B.

Also the extra condition on line 234 should be deleted.

My main point is that the result of the call MimeMultipartAlternative_display_part_p(obj, malt->buffered_hdrs[0]) will always be available in the parameter next_priority. So it will be a waste of processor time to do that function call again.

Thanks, with have_displayable = (malt->buffered_priority > next_priority); the extra condition on line 227 needs to go, too, right? 226 else if (! finished && have_displayable && 227 ! (malt->buffered_priority > next_priority)) { since this will always come out false. I did this, but it doesn't work any more. I now get the HTML part forced to plain text instead of the text/plain part. I really don't know much about this, I'm just trying to pick up the pieces here so the work doesn't go to waste and our users get the benefit.

Line 226 must be: else if (! finished && !have_displayable) { It is a duplication of case 3, but if that is what they want ...

OK, this works now. (In reply to Terje Bråten from comment #60) > Line 226 must be: > else if (! finished && !have_displayable) { > It is a duplication of case 3, but if that is what they want ... This line 226 was case 4, changing it as you said made it become identical with case 6. I hope I got this right. It works nicely, so with an added test, that should pass review after three years ;-) - Thanks for your help. If you don't mind, please click "Details" on the attachment and give it f+, so it has your seal of approval.

Comment on attachment 8757450 [details] [diff] [review] Terje Bråten's unbitrotted patch with original logic restored as per comment #50 (v2). Review of attachment 8757450 [details] [diff] [review]: ----------------------------------------------------------------- r=jcranmer according to comment #50 since we addressed the issues and rs=jorgk (hey, I have review power here now according to https://wiki.mozilla.org/User:Rkent/Proposed_MailnewsCoreModules#MIME_Parser). r+ conditional on adding a test which I will do myself. Coming up. Terje, thanks a lot for your work and your patience. Much appreciated!! BTW, I'm the clean-up guy here, I find patches which have been lingering for a while and get them landed. This is not the first one. If I have very little involvement, like here, I leave the original author. It it's more work for me, I add my own name.

Thank you for getting it in.

Try run here, no idea what could be affected, so I'm running the lot ;-) https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c61efcfaa599 (In reply to Terje Bråten from comment #63) > Thank you for getting it in. Not quite there yet. First, we run it on our try servers to see whether anything breaks. And then I need to add the test. That's pretty simple though. We have an easy framework. I can just import pre-canned messages with the right MIME parts (also defective ones for bug 348045), then switch the view to either plain or HTML and check that I get the right thing. A few hours work, but easier than your part which was working through our glorious MIME code, which, BTW, will one day be replaced. But that days hasn't come yet, so we need to land some C++ patches here and there. The advantage of having extra tests is that these tests can be used to check that the re-write hasn't regressed anything.

The try run didn't show any errors introduced by this change. Especially mozmake SOLO_TEST=composition/test-forward-utf8.js mozmill-one didn't break. This test will be the boilerplate for the new test. I'll have the following cases: - multipart/alternative with text/plain and text/html, selecting plain and rich display. - multipart/alternative with text/html only, selecting plain and rich display (bug 348045). - multipart/alternative with text/html and text/plain wrong order, selecting plain and rich display (bug 574989). - multipart/alternative with text/plain and inner multipart/related containing HTML and image, content-utf8-alt-rel.eml. - incorrect multipart/related where one part is multipart/alternative with text/plain and text/HTML and the related part is an image, content-utf8-rel-alt.eml. This should give us the necessary test coverage.

Here are some test cases. I noticed that in the case where we have this structure: multipart/alternative text/plain multipart/related text/html image we see the text/plain *and* the text/html part downgraded to plain text at the same time. This can also be observed with the message from attachment 154853 [details]. Terje, I already asked at the bottom of comment #53 (quote): === Tested with attachment 154853 [details]. When displaying the message as plain text, I now see the text/plain part *and* the HTML part forced to plain text. Is this intentional? === I'd expect only the text/plain part to show. If I reply to the message, both parts are included in the reply.

Hi That is surprising to me too. I do not know why you get both parts. When the mimemalt.cpp file has done its job, the second part (the downgraded HTML message) should have been marked as do not display. May be you are able to add some logging to see if that is really the case? This is just speculation, but may be some other part of the code do not respect the "do not display" marking when it discovers that is a HTML part, or something? I really do not know, sorry. Terje B.

(In reply to Terje Bråten from comment #67) > That is surprising to me too. I do not know why you get both parts. As I said in comment #53: That already happened with your original patch on the original test case. I'll look into it.

In case the debug isn't self explanatory, I'm attaching the patch that generates it. I have some general questions: - What is buffered_priority and why is that set for i==0? - And what is next_priority? - Why does MimeMultipartAlternative_display_cached_part get called with do_display && (i == 0)? This will only ever be true for i==0, so the first (cached?) part? Sorry, I was hoping this is 100% working and I could just rs (rubber stamp) this given that our expert Joshua already looked at it in detail. But now that I have to debug it myself, I need to understand it better. Here comes the debug in the case of preferring text/plain. On a message with multipart/alternative text/plain text/html I see: ==== set highest on text/plain ==== set normal on text/html ==== finished 0, buffered prio 2, next prio 1 ==== case 5 ==== finished 1, buffered prio 2, next prio 0 ==== case 2 ==== display part text/plain: 1 ==== display part text/html: 0 On a message with multipart/alternative text/plain multipart/related text/html image I see: ==== set highest on text/plain ==== set normal on multipart/related ==== finished 0, buffered prio 2, next prio 1 ==== case 5 ==== finished 1, buffered prio 2, next prio 0 ==== case 2 ==== display part text/plain: 1 ==== display part multipart/related: 0 Now this clearly says that the second alternative "multipart/related" shouldn't be displayed. You already speculated (quote): 'but may be some other part of the code do not respect the "do not display" marking'. So this may well be it. Looking at MimeMultipartAlternative_display_cached_part, how would that suppress the display of the second alternative "multipart/related"? Of course this is new processing. Before, the second alternative "multipart/related" was always displayed, even when forced to plain text.

I've dug through it a bit. When we have a HTML part with is not displayed, then this call https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemalt.cpp#493 status = body->clazz->parse_eof(body, false); finally gets here: https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimethpl.cpp#81 and decides that there is nothing to display. thpl stands for TextHtmlAsPlaintext. Now, if at the same spot we process the multipart/related, we get here instead: https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemrel.cpp#991 And that busily pumps out the HTML, even if it's not desired. Sigh, this MIME stuff is really terrible, that's why we normally don't touch it.

Comment on attachment 8757450 [details] [diff] [review] Terje Bråten's unbitrotted patch with original logic restored as per comment #50 (v2). Sorry, I have to clear the r+ for now since the condition was that we get tests which I was going to supply. Sadly while working on the tests, I realised that a major case (as already noted in comment #53) isn't working. I've come to the conclusion that another challenge of this bug is to "educate" the related part processing in mimemrel.cpp that it shouldn't create any output if the controlling multipart/alternative parent has correctly decided that it doesn't want any. This function doesn't appear to be in the box, since (quoting from the last paragraph of comment #69) "... the "multipart/related" was always displayed, even when forced to plain text". How shall we proceed here? Without fixing this aspect, obviously the patch as it is cannot land. Having multipart/alternative with embedded multipart/related is the most common case of sending "rich" text e-mail, so that must work.

Hi. Sorry I have been busy all day. It was my birthday today. Ok, to answer your questions: - What is buffered_priority and why is that set for i==0? This is the priority of part that is currently on the top (the beginning) of the buffer. It is set for i==0 because it is supposed to only reflect the priority of the part that is first in the buffer. - And what is next_priority? That is the priority of the next part that is about to be appended to the bottom (the end) of the buffer. - Why does MimeMultipartAlternative_display_cached_part get called with do_display && (i == 0)? This will only ever be true for i==0, so the first (cached?) part? Yes, that is right. When the buffer (cache) is flushed the first part in the buffer (if the logic has worked) should be the latest one with the highest priority, and that is the one to be displayed. All the other parts in the buffer should not be displayed. And as you say the debug output shows that the logic in that part of the code works, the multipart/related is marked to not be displayed. MimeMultipartAlternative_display_cached_part is suppressing the display by setting body->output_p = false.

MimeMultipartAlternative_display_cached_part is also trying to suppress the display by setting body->options->output_fn = nullptr; This seems not to be working in MimeMultipartRelated_parse_eof in the file mimemrel.cpp, since there the output_fn is set to something else. This patch was working for me 3 years ago, I remember that. The "parse_eof" functions must have changed since then. I guess the only way out it to raise a new bug, one that will block this one, about that the "parse_eof" output functions needs to respect the output_p flag.

Happy Birthday!! Yes, I can see the body->output_p = do_display; and the /* parse_begin resets output_p. This is quite annoying. To convince it that we mean business, we set output_fn to null if we don't want output. */ if (! do_display) body->options->output_fn = nullptr; If you're saying that this worked three years ago I'll do some digging through the history. However, there is very little movement in those files: https://hg.mozilla.org/comm-central/log/tip/mailnews/mime/src/mimemalt.cpp https://hg.mozilla.org/comm-central/log/tip/mailnews/mime/src/mimemrel.cpp

Hi. Now that I think more about it, I am not so sure any more that it even really worked 3 years ago. I only had a small ASUS computer at home at that time, and it ran out of memory when trying to compile thunderbird. So I was relying on other people to help test it for me. Any way, I guess the test case(s) you have written will be a big help. Nothing is like a but report that already has failing tests written.

Correction: "but report" --> "bug report"

(In reply to Terje Bråten from comment #75) > Now that I think more about it, I am not so sure any more that it even > really worked 3 years ago. I doubt that it ever worked. All the relevant files that I looked at have no relevant changes in the last three years: https://hg.mozilla.org/comm-central/log/tip/mailnews/mime/src/mimemalt.cpp https://hg.mozilla.org/comm-central/log/tip/mailnews/mime/src/mimemrel.cpp https://hg.mozilla.org/comm-central/log/tip/mailnews/mime/src/mimemult.cpp https://hg.mozilla.org/comm-central/log/tip/mailnews/mime/src/mimeobj.cpp The only file that gets changed more regularly is https://hg.mozilla.org/comm-central/log/tip/mailnews/mime/src/mimedrft.cpp since this is the interface to drafts and composition. > I only had a small ASUS computer at home at that > time, and it ran out of memory when trying to compile thunderbird. I know the feeling. I had to upgrade all my machines to 8 GB. (In reply to Terje Bråten from comment #73) > I guess the only way out it to raise a new bug, one that will block this > one, about that the "parse_eof" output functions needs to respect the > output_p flag. Yes, but that won't get fixed either so this bug here won't make progress.

Just a shot in the dark; if you at line 492 in mimemalt.cpp (just before it calls body->clazz->parse_eof) insert the line body->output_p = do_display; will it work then? (Just to rule out that the bug is that the value has been altered before that point.)

No, that doesn't help since output_p is simply not read in mimemrel.cpp at all. What does help is repeating if (! do_display) body->options->output_fn = nullptr; before the call to body->clazz->parse_eof(body, false); At first that leads to a crash, but when not executing relobj->real_output_fn(buf, size, closure); in mimemrel.cpp when the function pointer is null, gets me closer. The HTML part of the multipart/related is not output, but the related image now shows as untyped attachment (Part 1.2.2). As I said, mimemrel.cpp needs to be fixed, there is no doubt. It wasn't written to not output anything.

With a little tweak in mimemrel.cpp I can suppress the HTML from the multipart/related. The horizontal ruler still shows and so does the image as attachment Part 1.2.2

While looking for the spot where the mimeAttachmentHeader gets added: https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimei.cpp#1702 I came across this code: https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeobj.cpp#193 While obj->output_p was in fact passed in as false, it got reset to true here: https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeobj.cpp#202 Stack: MimeObject_parse_begin(MimeObject * obj) Line 202 C++ MimeLeaf_parse_begin(MimeObject * obj) Line 124 C++ MimeInlineTextPlain_parse_begin(MimeObject * obj) Line 97 C++ MimeInlineTextHTMLAsPlaintext_parse_begin(MimeObject * obj) Line 56 C++ MimeMultipartRelated_parse_eof(MimeObject * obj, bool abort_p) Line 1076 C++ If I reset obj->output_p to false manually, then at least the attachment separator isn't shown. The Part 1.2.2 attachment still shows. So whilst the parent (multipart/relative) may be set to "don't display", the children parts seem to have their own life. Conclusion: It will require a significant amount of investigation to make the multipart/relative not display.

Added a case with a real attachment, so we get multipart/mixed. This behaves the same as the multipart/alternative with embedded multipart/related.

I have made this patch that add an option "no_output_p" to the mime display options. I hope this will make it easier to find a solution.

Comment on attachment 8757703 [details] [diff] [review] Terje's new patch with a new option Nice idea. Did you actually try it? Unless I've missed something, this destroys the message display completely, nothing is displayed any more :-( Does 'bool no_output_p' get initialised anywhere? Also, if you're submitting more patches, please change the header to: # User Terje Braten <bugzilla@braten.be> (sorry, only ASCII in the user name) Bug 253830 - "View as plain text" should use text/plain part. r=jcranmer r=jorgk Also, somewhere you have the word "massage" instead of "message". I'd prefer not to make these changes repeatedly ;-)

I am sorry, I still have not everything set up to be able to compile thunderbird. Was going to do that next, but I ran into some dependencies problems. Anyway here is take 2 on my new patch. (BTW, how do you auto-generate those patch headers? I just copied what you had.)

Take 3, I had accidentally removed something in an unrelated file.

(In reply to Terje Bråten from comment #85) > (BTW, how do you auto-generate those patch headers? I just copied what you > had.) We use Mercurial queues: hg qnew -m "Commit comment" xxx.patch does the trick. Later you use hg qrefresh hg qpop hg qpush hg qdelete. There is also a graphical interface: TortoiseHg.

Comment on attachment 8757710 [details] [diff] [review] Terje's new patch with a new option Promising ;-) So all that was missing from the previous patch was the initialisation. Now this case (included in the test cases): multipart/alternative text/plain multipart/related text/html image now shows the plain text part, no mimeAttachmentHeader. Only problem: The image still shows as attachment Part 1.2.2.

How to handle that the image is not shown as attachment is a part of the code I have not looked at. I am not sure if I can figure out how to do that. Would it be blocking for this bugfix? I mean, it is not ideal, but at least now the display at plaintext works.

Hi. I have been able to compile it and test this myself now. It only shows the attachment if you have "Display Attachments Inline" ticked in the view menu. Is that not expected behaviour then?

(In reply to Terje Bråten from comment #89) > How to handle that the image is not shown as attachment is a part of the > code I have not looked at. Neither have I nor anyone on left on the team :-( > I am not sure if I can figure out how to do that. That's a challenge then ;-) The old code managed to force the HTML to plain text and not show any extra attachments. > Would it be blocking for this bugfix? > I mean, it is not ideal, but at least now the display at plaintext works. Well, we wouldn't want to half fix something. This is really quite confusing. Take: multipart/mixed multipart/alternative text/plain multipart/related text/html image 1 image 2 (attachment). Showing the plain text part, the user gets to see two attachments, one phantom one that comes from the related part and one real one. Give that this will not land before TB 52, we have 3x 6 weeks = 18 weeks = more than four months time to get it right. I guess you came up with the no_output_p because I pointed you to https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeobj.cpp#193 So I guess it's just some more digging around with the debugger to find where the attachments are added.

(In reply to Terje Bråten from comment #90) > I have been able to compile it and test this myself now. Welcome to the team ;-) > It only shows the attachment if you have "Display Attachments Inline" ticked > in the view menu. Is that not expected behaviour then? Yes and no. multipart/alternative text/plain multipart/related text/html image shows an attachment called Part 1.2.2 and that's not supposed to be shown. At least the current version doesn't do that.

Comment on attachment 8757710 [details] [diff] [review] Terje's new patch with a new option Review of attachment 8757710 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/mime/src/mimemalt.cpp @@ -418,5 @@ > mime_free(body); > return status; > } > - /* We need to muck around with the options to prevent output when > - do_display is false. More about this below. */ Would be good to see this stuff go. @@ +445,5 @@ > /* add_child assigns body->options from obj->options, but that's > just a pointer so if we muck with it in the child it'll modify > the parent as well, which we definitely don't want. Therefore we > + need to make a copy of the old value and restore it later. */ > + old_options_no_output_p = obj->options->no_output_p; Are you sure that this is all we need. Before we copied the entire options, now we just save one field. Do we know how much "mucking" goes on? @@ -441,2 @@ > if (! do_display) > - body->options->output_fn = nullptr; Same here. Would be good to see this dirty business go.

Please ignore comment #92 and #93. I got confused with the "display attachments inline" option. If you switch "display attachments inline" off, the current TB also displays the funny Part 1.2.2 attachments. So that behaviour hasn't change with your patch. The only thing that has changed is that we now get the plain text part if requested. So kudos for that. Would you mind answering the question from comment #93. Here is another try run, so if this comes out good, this is good to go, I'll do a format review (I spotted a few so-called 'nits') and I'll write the tests. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=66c7b876697b

Comment on attachment 8757710 [details] [diff] [review] Terje's new patch with a new option Review of attachment 8757710 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, sorry about picking nits. ::: mailnews/mime/src/mimemalt.cpp @@ +506,5 @@ > if (status < 0) return status; > } > #endif /* MIME_DRAFTS */ > > + /* restore options to what parent classes expects */ Nit: Upper case and full stop. ::: mailnews/mime/src/mimeobj.cpp @@ +190,5 @@ > } > } > > /* Decide whether this object should be output or not... */ > + if (!obj->options || obj->options->no_output_p || !obj->options->output_fn Nit: We have the old FORTRAN rules of limiting lines to about 80 characters. ::: mailnews/mime/src/modlmime.h @@ +165,5 @@ > > + bool no_output_p; /* Will never write output when this is true. (When false, output or not may depend on other things.) > + This needs to be in the options, because the method MimeObject_parse_begin > + is controlling the property "output_p" (on the MimeObject) so we need a way for > + other functions to override it and tell that we do not want output */ Same here.

Comment on attachment 8757710 [details] [diff] [review] Terje's new patch with a new option Review of attachment 8757710 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/mime/src/mimemalt.cpp @@ -418,5 @@ > mime_free(body); > return status; > } > - /* We need to muck around with the options to prevent output when > - do_display is false. More about this below. */ Well, those lines are removed. @@ +445,5 @@ > /* add_child assigns body->options from obj->options, but that's > just a pointer so if we muck with it in the child it'll modify > the parent as well, which we definitely don't want. Therefore we > + need to make a copy of the old value and restore it later. */ > + old_options_no_output_p = obj->options->no_output_p; Yes, I have read through this whole function, and know for sure how much "mucking" goes on here. Since "no_output_p" now is the only thing we change in the options, it is also the only thing that needs to be restored. @@ -441,2 @@ > if (! do_display) > - body->options->output_fn = nullptr; I am not sure what you mean? This is the line that makes it work that output is suppressed in all cases.

Comment on attachment 8757710 [details] [diff] [review] Terje's new patch with a new option Review of attachment 8757710 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/mime/src/mimeobj.cpp @@ +190,5 @@ > } > } > > /* Decide whether this object should be output or not... */ > + if (!obj->options || obj->options->no_output_p || !obj->options->output_fn This line is 77 chars long. Is that not ok? The comment below (which I have not changed) is longer.

Ok, here is my new revised patch.

Forgot a full stop in the comment.

(In reply to Terje Bråten from comment #96) > Well, those lines are removed. > I am not sure what you mean? This is the line that makes it work that output > is suppressed in all cases. Sorry, I wasn't clear. I should have said: I'm glad to see this old dirty business go, referring to -body->options->output_fn = nullptr; > @@ +445,5 @@ > > /* add_child assigns body->options from obj->options, but that's > > just a pointer so if we muck with it in the child it'll modify > > the parent as well, which we definitely don't want. Therefore we > > + need to make a copy of the old value and restore it later. */ > > + old_options_no_output_p = obj->options->no_output_p; > > Yes, I have read through this whole function, and know for sure how much > "mucking" goes on here. Since "no_output_p" now is the only thing we change > in the options, it is also the only thing that needs to be restored. I'm not 100% convinced. Before we had: status = body->clazz->parse_begin(body); if (status < 0) return status; - /* Now that parse_begin is done mucking with output_p, we can put - body->options back to what it's supposed to be. Avoids a memory - leak. */ - delete body->options; - body->options = obj->options; Sure, you read through the function to look at what mucking goes on, but do you know what the callees do? The comment even suggests that parse_begin mucks with parts of the body. I think we would be on the safer side to restore the original copy. Or am I missing something?

The parse_begin "mucking" that is referred to here is what you found yourself (in comment #81) https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeobj.cpp#202 The original code got around it by setting body->options->output_fn = nullptr; Instead of saving the old value of body->options->output_fn, the original code saved (made a copy of) the value of the whole body->options struct. This was restored (and the copy deleted) as early as possible in order to avoid a memory leak if we have an early exit. Since you do not need to allocate memory on the heap for a bool variable, my solution avoids that problem, and we can do the restore at the end of the function instead. And most importantly the value is restored after the call to body->clazz->parse_eof.

I'm still not 100% convinced that the "mucking" I found is the only mucking that takes place. Why would the original code copy the whole thing, if it just wanted to preserve obj->output_p? Besides, that your code currently doesn't restore. You only restore no_output_p. As you saw, this MIME code is twisted and complicated, so to be on the safe side, we should do minimal change here. I appreciate your clean-up effort, but it's too risky. Now having said all that, I can even prove it. Please take a look at the try run at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=66c7b876697b&selectedJob=18717 Apart from already known xpcshell failures, the orange X, there are now more: TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_mimemaltdetach.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_mimemaltdetach.js | testDetach - [testDetach : 70] false == true TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_mimemaltdetach.js | _async_driver - TypeError: ex.stack is undefined at ../../../resources/asyncTestUtils.js:169 Here is the test: https://dxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_mimemaltdetach.js#6 So sadly your patch in the current form can't land ;-) So may I suggest that: 1) you also restore output_p. 2) you leave the original code in place and just add your no_output_p tweak. Before submitting a new patch, please make sure the test passes by running it locally: mozilla/mach xpcshell-test mailnews/base/test/unit/test_mimemaltdetach.js I've run it locally and it fails as expected. Thanks for all your work on this. I know, it's a steep learning curve and you need a lot of patience. Your advantage is that I answer mostly straight away. Other contributors (myself included) wait for answers or reviews for days or weeks. But that's very discouraging, so we strike the iron while it's hot ;-)

Isn't there more mucking here? https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeobj.cpp#229

(In reply to Jorg K (PTO during summer, NI me) from comment #102) > So may I suggest that: > 1) you also restore output_p. > 2) you leave the original code in place and just add your no_output_p tweak. Now I think option 2) is the only way to be on the safe side. So the only change we do is (of course together with declaring and initialising it): - if (!obj->options || !obj->options->output_fn + if (!obj->options || obj->options->no_output_p || !obj->options->output_fn if (! do_display) - body->options->output_fn = nullptr; + body->options->no_output_p = true; What do you think?

Hi Aceman, I'm trying to put together a test. What I want to do is - load a message from a file (done) - switch the view to plain text and HTML (not working) - check the content (missing). To switch the view, I looked at test-drafts.js and adapted the cwc.window.OutputFormatMenuSelect(cwc.e("format_both")); to msgc.window.MsgBodyAsPlaintext(); With this line, the test passes, so I can call the function like this. Sadly, the view doesn't switch to plain text. What am I doing wrong, Mr. Mozmill? Or if you have any better idea or know where this sort of thing is already done, please let me know.

How about this? No fancy switching. Just setting the preferences before loading the message. This works. So unless someone has a better ides, I'd go with this.

(In reply to Jorg K (PTO during summer, NI me) from comment #105) > To switch the view, I looked at test-drafts.js and adapted the > cwc.window.OutputFormatMenuSelect(cwc.e("format_both")); > to > msgc.window.MsgBodyAsPlaintext(); > > With this line, the test passes, so I can call the function like this. > Sadly, the view doesn't switch to plain text. What am I doing wrong, Mr. > Mozmill? What does "view doesn't switch" mean? The function itself only changes prefs. Then it tries to reload the message? Is the rendered message wrong? Do you expect some other UI changes to be seen (e.g. the checkboxes on the plain/HTML menuitems?) Can you check if ReloadMessage and gMessageDisplay.displayExternalMessage(window.arguments[0].spec); is being called? Then, you need to find out if the reload is sync or async and you need to wait for it to finish. Also, I'm not sure wait_for_window_focused(msgc.window) does anything here. The window probably is focused already from the opening of it (the same call is part of open_message_from_file()).

(In reply to :aceman from comment #107) > What does "view doesn't switch" mean? The function itself only changes > prefs. Then it tries to reload the message? Is the rendered message wrong? Yes, what is displayed doesn't change. > Then, you need to find out if the reload is sync or async and you > need to wait for it to finish. All too hard. Let's do the sledge hammer approach in the other patch. > Also, I'm not sure wait_for_window_focused(msgc.window) does anything here. That was a desperate attempt ;-(

(In reply to Jorg K (PTO during summer, NI me) from comment #106) > Created attachment 8757800 [details] [diff] [review] > WIP: test (v2) - sledge hammer approach. > > How about this? No fancy switching. Just setting the preferences before > loading the message. This works. So unless someone has a better ides, I'd go > with this. Seems fine enough.

(In reply to Jorg K (PTO during summer, NI me) from comment #102) > I'm still not 100% convinced that the "mucking" I found is the only mucking > that takes place. Why would the original code copy the whole thing, if it > just wanted to preserve obj->output_p? I think you got a bit confused here. It is obj->options that is preserved (because that is a pointer that is copied to child objects, and has possibly been copied from a parent object). All the other properties (directly on "obj") can be changed without worry about parent or child objects. > > Besides, that your code currently doesn't restore. You only restore > no_output_p. And no_output_p is the only option in obj->options that is changed. > > As you saw, this MIME code is twisted and complicated, so to be on the safe > side, we should do minimal change here. I appreciate your clean-up effort, > but it's too risky. > > Now having said all that, I can even prove it. Please take a look at the try > run at > https://treeherder.mozilla.org/#/jobs?repo=try-comm- > central&revision=66c7b876697b&selectedJob=18717 > > Apart from already known xpcshell failures, the orange X, there are now more: > TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_mimemaltdetach.js | > xpcshell return code: 0 > TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_mimemaltdetach.js | > testDetach - [testDetach : 70] false == true > TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_mimemaltdetach.js | > _async_driver - TypeError: ex.stack is undefined at > ../../../resources/asyncTestUtils.js:169 > > Here is the test: > https://dxr.mozilla.org/comm-central/source/mailnews/base/test/unit/ > test_mimemaltdetach.js#6 I am not sure what all that is about. Will have to look into it. > > So sadly your patch in the current form can't land ;-) > > So may I suggest that: > 1) you also restore output_p. Restoring that was not done in the original code, (and will have no effect because it is a property on obj that is not copied around). > 2) you leave the original code in place and just add your no_output_p tweak. This will not work, since no_ouput_p cannot cannot be restored before body->clazz->parse_eof is called. > > Before submitting a new patch, please make sure the test passes by running > it locally: > mozilla/mach xpcshell-test mailnews/base/test/unit/test_mimemaltdetach.js > > I've run it locally and it fails as expected. > Will get back to you about that test later. Abot mucking in: https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeobj.cpp#229 That line is checking obj->dontShowAsAttachment, and is as expected. (And it was not restored in the original code either.) > Now I think option 2) is the only way to be on the safe side. > > So the only change we do is (of course together with declaring and initialising it): > - if (!obj->options || !obj->options->output_fn > + if (!obj->options || obj->options->no_output_p || !obj->options->output_fn > > if (! do_display) > - body->options->output_fn = nullptr; > + body->options->no_output_p = true; That will not work. body->options->no_output_p will then be restored to false before the call to body->clazz->parse_eof at the end of the function, and the output will not be prevented. And if I move the 'delete' and the restoration of obj->options to the end of the function, we will introduce a memory leak in the cases when the function exits early.

OK, I'm happy to admit confusion. Right, there are the object and its options. I think you contributed to the confusion by saying that https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeobj.cpp#202 constitutes "mucking", but it only changes obj->output_p = true; and not any options. Or am I still confused? Don't you hate reviewers who ask many silly questions? (I've fought many battles with my reviewers who just didn't get what I wanted to do and changed their minds many times, sigh.) However, the test failures are real ;-)

Sorry to trouble you again, what am I doing wrong. There is no innerHTML printed. How do I get what I see on the screen. I've looked in the DOM inspector. The test runs through OK, so all I'm accessing is OK.

(In reply to Jorg K (PTO during summer, NI me) from comment #111) > OK, I'm happy to admit confusion. Right, there are the object and its > options. > I think you contributed to the confusion by saying that > https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeobj.cpp#202 > constitutes "mucking", but it only changes obj->output_p = true; and not any > options. > Or am I still confused? Don't you hate reviewers who ask many silly > questions? (I've fought many battles with my reviewers who just didn't get > what I wanted to do and changed their minds many times, sigh.) > The mucking the original code complained about was that "parse_begin" did not leave obj->output_p alone, yes. And that could not be easily remedied by changing only "obj>ouptut_p" again afterwards, since "parse_begin" also gets called on all the child objects. The key here is to look at line 194 in "parse_begin": https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeobj.cpp#194 The "if" statement depends on "!obj->options->output_fn" being true, and that is why the "body->options->output_fn = nullptr" hack worked. By adding the property "no_output_p" to "obj->options" we now have more direct method of forcing that if branch, and we can leave "obj->options->output_fn" alone. > However, the test failures are real ;-) The test failures seems to be something else. Still trying to understand that test.

(In reply to Jorg K (PTO during summer, NI me) from comment #112) > Created attachment 8757837 [details] [diff] [review] > WIP: test (v3). > > Sorry to trouble you again, what am I doing wrong. There is no innerHTML > printed. How do I get what I see on the screen. I've looked in the DOM > inspector. Have you tried document.getElementById("messagepane").contentWindow? Taken from http://mxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#1034 . Is this the messagepane you are using: http://mxr.mozilla.org/comm-central/source/mail/base/content/messageWindow.xul#153 ? It is a <browser> so may not have .innerHTML directly. You may need to fetch the document it contains first. Maybe the .contentWindow or .contentDocument.

mozilla/mach xpcshell-test mailnews/base/test/unit/test_mimemaltdetach.js gives the exact same error on the code without my patches. So that test failure is unrelated.

(In reply to Terje Bråten from comment #115) > So that test failure is unrelated. I'm sorry, but I don't agree. I removed your patch and ran mozilla/mach xpcshell-test mailnews/base/test/unit/test_mimemaltdetach.js Ran 5 tests (1 parents, 4 subtests) Expected results: 5 Unexpected results: 0 With your patch: Ran 4 tests (1 parents, 3 subtests) Expected results: 1 Unexpected results: 3 (FAIL: 3) We're working on the "alt" bit, so given that the test that fails is called test_mimemaltdetach.js, it's pretty self-evident. If it were an unrelated failure, we would see it here: https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=38527 but we don't.

(In reply to :aceman from comment #114) > Maybe the .contentWindow or .contentDocument. Aceman, you're a champion. Thanks!!! This works: let messagePane = aWindow.document.getElementById("messagepane"); let h = messagePane.contentDocument.firstChild.innerHTML; See at https://dxr.mozilla.org/comm-central/source/mail/test/mozmill/composition/test-forward-utf8.js#79 And yes, it's a <browser>. I'll upload my test in the next few hours. Looks like the test will be ready before the code ;-) But don't worry, the test will pass, we only have to look into some additional xpcshell test failures.

Sorry, I have been working with php for too long. I forgot to recompile the code before I ran the test again.

This patch proves that it is not the changes around lines 445-480 (where the "mucking" code is) that is causing the test to fail. This patch is the same as my previous patch, but with the difference of line 497 in mimemalt.cpp that restores obj->options->no_output_p before calling body->clazz->parse_eof The good news it that this fixes the test mailnews/base/test/unit/test_mimemaltdetach.js The bad news is that it brings back the failure showing the not wanted downgraded HTML in the alternative/related part.

Here is a patch that works. It was a faulty test, I had to change mailnews/test/data/multipartmalt-detach It relied on the buggy behaviour that the multipart/related was always displayed, even it it had a displayable part coming after it. I hope all should be good now.

Comment on attachment 8757939 [details] [diff] [review] Test (v1). Review of attachment 8757939 [details] [diff] [review]: ----------------------------------------------------------------- Nice job bashing on this code guys :) ::: mail/test/mozmill/message-window/test-view-plaintext.js @@ +10,5 @@ > + > +var MODULE_NAME = "test-view-plaintext"; > + > +var RELATIVE_ROOT = "../shared-modules"; > +var MODULE_REQUIRES = ["folder-display-helpers", "compose-helpers", "window-helpers"]; Are compose helpers needed in this test? @@ +15,5 @@ > + > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource:///modules/mailServices.js"); > +var elib = {}; > +Cu.import('resource://mozmill/modules/elementslib.js', elib); Mailservices and elib seems unused in this test. @@ +25,5 @@ > + collector.getModule(lib).installInto(module); > + } > +} > + > +function check_content(aWindow, aExpected, aDontWantToSee) { We use to comment function and its arguments with the syntax: /** * Function purpose... * * @param aWindow Arg description... */ @@ +29,5 @@ > +function check_content(aWindow, aExpected, aDontWantToSee) { > + let messagePane = aWindow.document.getElementById("messagepane"); > + let messageHTML = messagePane.contentDocument.firstChild.innerHTML; > + if (aExpected != aDontWantToSee) { > + assert_true(messageHTML.includes(aExpected), "Didn't find expected content"); If you only compare the text part of the document (and ignore html tags), maybe you could compare messagePane.contentDocument.firstChild.textContent == aExpected? This would also catch cases when the content would be there twice.

(In reply to Terje Bråten from comment #120) > It was a faulty test, I had to change mailnews/test/data/multipartmalt-detach Yes, at times we have to change tests, but at first we need to understand what's going on. This test https://hg.mozilla.org/comm-central/log/tip/mailnews/test/data/multipartmalt-detach was written for bug 351224. If you look at the structure we have: multipart/alternative text/plain multipart/relative text/html text/plain - head_update.txt (without an e in txt). text/plain - smurf_update_neu.txt text/plain - head_update.text (with an e in text) So there are three attachments shown. With your new version, the last part is now displayed, where before the multipart/relative was displayed. That doesn't seem right, since as you said, in multipart/alternative the last part is the preferred part. The test seems to detach the head_update.txt at the end. Very unfortunate that these have the same name. Now, what you've done is remove the last part, which is the one that the test should remove. Instead it will now remove the part from the multipart/relative (which makes little sense). I've just looked at attachment 236615 [details] from bug 351224 again. It's a simple multipart/alternative with embedded multipart/relative and images. So perhaps they did want to test that they can remove the images from the multipart/related?? That doesn't make sense. And the test case surely doesn't reflect that, with your correction it does. === What we should consider doing in another bug is fixing the test completely. If I read bug 351224 correctly, the complaint was that "Could not delete/detach attachments from multipart/related messages". Well, the parts of the multipart/related, which are typically images, we shouldn't delete. I think we should replace the test with something useful like: multipart/mixed multipart/alternative text/plain multipart/related text/html image 1 image 2 (attachment). Then remove the image 2 attachment. Fortunately we already have a message of this structure in my test cases, so to change test_mimemaltdetach.js to do something useful shouldn't be hard. What do you think?

Comment on attachment 8757934 [details] [diff] [review] Terje's latest patch on this issue Review of attachment 8757934 [details] [diff] [review]: ----------------------------------------------------------------- OK, let's go with this ;-) You got rid of one line too many. And you beat me to it: Code is ready before the test ;-) ::: mailnews/test/data/multipartmalt-detach @@ -50,5 @@ > -Content-Type: text/plain > -Content-Disposition: attachment; filename="head_update.txt" > - > -headUpdate.text > ---gmxboundary=-1156956072-29266-top-- You need that last line ;-)

Thanks. (In reply to :aceman from comment #122) > If you only compare the text part of the document (and ignore html tags), > maybe you could compare messagePane.contentDocument.firstChild.textContent > == aExpected? This would also catch cases when the content would be there > twice. Well, that also returns the title of the document and a heap of newlines, so == doesn't work.

Added back last line in test email

Comment on attachment 8757985 [details] [diff] [review] Terje's latest patch on this issue If you already have r+, you can carry it forward if you're only addressing a minor issue. You typically put: Carrying forward xxxx's r+. Now, you're joining the team? I have another bug for you: Bug 574989. There is a test here already, you just need to change the parameters.

That bug was already next on my list. ;-) BTW, how can I run the tests you made?

Easy: cd obj* mozmake SOLO_TEST=message-window/test-view-plaintext.js mozmill-one Of course you have to apply my patch first.

Comment on attachment 8757983 [details] [diff] [review] Test (v2). Review of attachment 8757983 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks. Would it work to put the .eml file into a data subdir under message-window? It is already done in some of the test directories.

Thanks. (In reply to :aceman from comment #131) > Would it work to put the .eml file into a data subdir under message-window? Maybe. But the directory is small and there are already two loose .eml files in it. So I don't see the need.

https://hg.mozilla.org/comm-central/rev/a1892093b04d https://hg.mozilla.org/comm-central/rev/2b0a97915f70

Looks like we caused another test failure here. Can we prepare and follow up patch and land it quickly? mozilla/mach xpcshell-test mailnews/mime/test/unit/test_parser.js Fails locally and here: https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=38632 Log: http://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1464638391/comm-central_win7_ix_test-xpcshell-bm110-tests1-windows-build16.txt.gz says: TEST-PASS | mailnews/mime/test/unit/test_parser.js | compare_objects - [compare_objects : 28] "\\"head_update.txt\\"" == "\\"head_update.txt\\"" TEST-PASS | mailnews/mime/test/unit/test_parser.js | emitter_endPart - [emitter_endPart : 191] "2.2" == "2.2" TEST-PASS | mailnews/mime/test/unit/test_parser.js | emitter_partData - [emitter_partData : 169] "2.3" == "2.3" TEST-PASS | mailnews/mime/test/unit/test_parser.js | emitter_endPart - [emitter_endPart : 186] "2.3" == "2.3" TEST-PASS | mailnews/mime/test/unit/test_parser.js | compare_objects - [compare_objects : 28] "\\"smurf_update_neu.txt\\"" == "\\"smurf_update_neu.txt\\"" TEST-PASS | mailnews/mime/test/unit/test_parser.js | emitter_endPart - [emitter_endPart : 191] "2.3" == "2.3" TEST-PASS | mailnews/mime/test/unit/test_parser.js | emitter_endPart - [emitter_endPart : 191] "2" == "2" TEST-PASS | mailnews/mime/test/unit/test_parser.js | emitter_endPart - [emitter_endPart : 191] "" == "" TEST-PASS | mailnews/mime/test/unit/test_parser.js | emitter_endMsg - [emitter_endMsg : 156] 0 == 0 TEST-PASS | mailnews/mime/test/unit/test_parser.js | test_parser - [test_parser : 196] 2 == 2 TEST-UNEXPECTED-FAIL | mailnews/mime/test/unit/test_parser.js | test_parser - [test_parser : 198] 4 == 5 Looks like this uses the test data we know already, and given that we killed the last part, we now only have four instead of five? Just guessing. Aleth, please don't back it out, we're at it!!

Done: https://hg.mozilla.org/comm-central/rev/336c1a3c70ad

Changed other test based on the same eml file

Looks like you beat me to it. :-)

Comment on attachment 8758059 [details] [diff] [review] Fix failed test Review of attachment 8758059 [details] [diff] [review]: ----------------------------------------------------------------- Great, I wouldn't have done it any different ;-) Actually, better like this, so we have two people look at it.

Note for the future: "r=test-only" doesn't exist ("a=test-only" does in some cases). But I see from the comments here that two people did agree on the fix ;)

Well, I took my inspiration from here: https://hg.mozilla.org/comm-central/rev/05fa88baaef3 The author there didn't even open a bug for the work; I associated the fix with bug 1269658 comment #3 later.

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #140) > Well, I took my inspiration from here: > https://hg.mozilla.org/comm-central/rev/05fa88baaef3 > > The author there didn't even open a bug for the work; I associated the fix > with bug 1269658 comment #3 later. Right. I would have complained about that as well if I had noticed at the time it happened.