351224 - Could not delete/detach attachments from multipart/related messages (not-used/not-shown part in multipart/related is deletable/detachable?)

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Alexander Kluss]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060808 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060808 SeaMonkey/1.5a

If a message has the following structure, then the attachment can not be deleted. The new message contained further the attachments. If you select detach attachment, the attachment would be saved but not deleted in the message.

,----
| <1 text/plain>
| <2.* multipart/related>
| <2.1 text/html>
| <2.2 image/gif>
| <2.3 image/gif>
| <2.4 image/gif>
| <2.5 image/jpeg>
`----

Reproducible: Always

Steps to Reproduce:
e.g. with the GMX newsletter

Comment 1

[Alexander Kluss]

Attachment: this is a eml file of a GMX newsletter

Comment 2

[Mike Cowperthwaite]

Reproduced with TB 2a1-0901, Win2K.

Of course, you have to view the message As Plain to see the images listed as attachments at all.  It makes sense not to remove the attachment from the message, as this would break the HTML display of the message; but if that is the intent, the UI should make it clear -- those menu options should be disabled, at least.

Comment 3

[Alexander Kluss]

In the discussion in the german mozilla newsgroup we believe that the delete and detach function has also work fine with this attachments. If the attachments detach so could it load from the hard disk if the user delete it so should be displayed the broken image link icon.

I think the most mails with inline images are newsletter and the images are commercials. Why I should not delete the images before I archive the newsletter?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Seems that bug 351224 could be a dupe of this one.

Comment 5

[Benoît]

I'm experiencing this bug with an e-mail message that's similar in structure, but with less attachments.

-multipart/alternative
--text/plain (e-mail in plain text)
--multipart/mixed
---text/html (one line of HTML garbage)
---text/plain (the attachment that was meant to be sent)
---text/html (e-mail in HTML)

Anyone have an idea as to what is going on here? If it helps, this e-mail was sent with Apple Mail.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Benoit, could you provide a small testcase in the mbox format and attach it to this bug? That would be very helpful.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

OK, this is the right bug, not <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=292385">bug 292385</a>

I find this on Windows with Thunderbird:
 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/2008091421
Lightning/0.8 Thunderbird/2.0.0.17

If I do a delete/detach (single|all), nothing happens to the message on imap or
pop.  if I detach the files end up where I direct them to, but the message
still contains the attachments.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Joel, if even you could attach a small testcase it would be fantastic.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

I have a .eml extension that I will attach.  If you open it up, detach all you will end up in the scenario where it cannot delete them (as it tries to delete automatically after a detach). Furthermore, you can do a delete all and it will not do anything.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: test case as per my previous comments.

Comment 11

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #10)
> test case as per my previous comments.

Original problem(comment #0) is for next structure. 
  multipart/alternative
  part-1 : text/plain (<= mail_body if Message Body As = Plain Text)
  part-2 : multipart/related
           part-2-1 : text/html (<= mail_body if Message Body As = ... HTML)
           part-2-2 : image/gif (pointed by <img src="cid:..."> etc. in HTML)   
                  |
           part-2-N : image/xxx (pointed by <img src="cid:..."> etc. in HTML)
So issue of this bug is next;
   image/xxx in multipart/related is removable or not when it is displayed
   as if attachment because of Message Body As = Plain Text.

Your case is for next structure and all attachments is conceptually removable (detach/delete is possible). However, text/plain & text/html part is involved in your case. It may have relation to phenomenon when "detach/delete all".
> Content-Type: multipart/mixed; boundary="------------070700020407030507070207"
> --------------070700020407030507070207
> Content-Type: text/plain; charset=ISO-8859-1; format=flowed (<=mail_body part)
> --------------070700020407030507070207
> Content-Type: application/pdf; name="Great Keyboard Shortcuts.pdf"
> Content-Transfer-Encoding: base64
> Content-Disposition: inline; filename="Great Keyboard Shortcuts.pdf"
> --------------070700020407030507070207
> Content-Type: text/plain; name="test.txt"
> Content-Transfer-Encoding: base64
> Content-Disposition: inline; filename="test.txt"
> --------------070700020407030507070207
> Content-Type: application/octet-stream; name="test.doc"
> Content-Transfer-Encoding: base64
> Content-Disposition: attachment; filename="test.doc"
> --------------070700020407030507070207
> Content-Type: image/jpeg; name="akanyi.jpg"
> Content-Transfer-Encoding: base64
> Content-Disposition: inline; filename="akanyi.jpg"
> --------------070700020407030507070207
> Content-Type: image/gif; name="1_star.gif"
> Content-Transfer-Encoding: base64
> Content-Disposition: inline; filename="1_star.gif"
> --------------070700020407030507070207
> Content-Type: text/html; name="test.html"
> Content-Transfer-Encoding: base64
> Content-Disposition: inline; filename="test.html"
> --------------070700020407030507070207--

Joel Maher, can you reproduce your problem with newer Tb 2 or Tb trunk nightly build? If yes, please open separate bug after search B.M.O well for already opened bug, because your problem is absolutely different problem from this bug even if "detach/delete doens't work" is same.

Comment 12

[Jonathan Kamens]

(In reply to comment #9)
> I have a .eml extension that I will attach.  If you open it up, detach all you
> will end up in the scenario where it cannot delete them (as it tries to delete
> automatically after a detach). Furthermore, you can do a delete all and it will
> not do anything.

I cannot reproduce this problem with your test case with Thunderbird 3.1. I do not know yet whether the same is true of the problem with the other MIME structure described earlier in the ticket. and in Comment 11. I am continuing to investigate.

Comment 13

[Jonathan Kamens]

(In reply to comment #1)
> Created attachment 236615 [details]
> this is a eml file of a GMX newsletter

This is still a problem in Thunderbird 3.1. When the attached message is viewed as plain text and then File > Attachments > Delete All... is selected, the attachments are not deleted.

Comment 14

[Jonathan Kamens]

I spent many hours researching this today to try to determine the root cause and what can be done about it. It looks like there are several root causes.

The issues all occur in the code in mailnews/mime/src, which handles parsing and emitting MIME messages. The code is riddled with special cases, abstraction barrier violations and convoluted logic. The purpose of all these twists and turns is presumably to make the code efficient, but their side effect, in my opinion, is that the code is extremely opaque and unmaintainable.

For example, one of the causes of this particular issue that that the multipart/alternative code in mimemalt.cpp is "smart" about throwing away MIME parts that aren't actually needed when displaying a message. But if one of the children of a multipart/alternative object is thrown away, than the numbering is off for all the subsequent children of that object. This is because mime_part_address in mimei.cpp counts children to figure out what numbers to assign (which makes perfect sense). When the message is displayed, the MIME hierarchy ends up being different from when it is raw formatted with nsMimeMessageAttach, so the code in MimeMultipart_parse_line that tries to find attachments to be deleted can't actually find them.

But it's worse than that. When formatting in nsMimeMessageAttach mode, other special-case code kicks in which prevents mimemalt.cpp from parsing the children of the mime/alternative part *at all*. Note that MimeMultipartAlternative_parse_child_line always Writes the lines when in nsMimeMessageAttach mode, and then MimeMultipartAlternative_parse_eof doesn't call display_cached_part when nsMimeMessageAttach is set. This means that the grandchildren of mime/alternative, e.g., the images inside a multipart/related that's in one of the parts of the mime/alternative, never get parsed and therefore, once again, can't be located by the code in MimeMultipart_parse_line to get rid of them.

It's a big mess.

Comment 15

[George Hazlewood]

Does look to me that bug 417646 is a duplicate of this one.  It should also be noted that this bug exists in more than just x86 Windows 2000.  I am on OSX and as far as I am aware this bug is platform independant.

Comment 16

[Jonathan Kamens]

Attachment: rewrite multipart/alternative handling to fix this problem

Well, it took an absurd number of hours for me to figure everything out well enough to fix this, but I think I've done it. Attached is my proposed patch.

It's essentially a complete rewrite of mimemalt.cpp, i.e., of the code for handling multipart/alternative blocks, so if you want me to attach mimemalt.h and mimemalt.cpp in their entirety, let me know.

I've explained the approach for the new architecture I'm using in a detailed comment at the top of mimemalt.cpp. I've also commented the new code pretty heavily.

There is one related bug fix here, to mimemult.cpp, in addition to the changes to mimemalt.*. The code for handling the last fragment (with no final newline) of a multipart child was wrong because it was calling parse_buffer rather than parse_line to push out the last fragment, and parse_buffer doesn't do anything if there's no trailing newline. I changed it to use parse_line instead, so that the final fragment will be pushed out. This is relevant to the case we're discussing here, because if you've got a nested multipart, e.g., multipart/related inside multipart/alternative, the terminating boundary on the multipart/related will be dropped without this fix.

Comment 17

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #16)
> rewrite multipart/alternative handling to fix this problem

Jonathan Kamens, where can we see ATTACHMENT in this bug's case?

Parts of image/jpeg in multipart/related is usually pointed by <img src="cid:..."> in HTML source of text/html part in multipart/related in multipart/alternative.
If such image parts are deleted, images will never be renderd when view of HTML part in multipart/alternative is requested by user.
If such parts are detached, I don't know whether image/jpeg parts in multipart/related in multipart/alternative is rendered or ot.
If such situation happens when user tries to view mail in HTML mode, I'm sure that user will open many bug reports of "embed image of HTML is lost!".
Jonathan Kamens, are you ready to answer to all of such unwanted bugs at B.M.O by yourself?

If delete/detach of parts in multipart/related is supported, I think it's better to be limited to parts which are not pointed by HTML via cid: or URI.
Note:
Users open BUG at B.M.O of "ATTACHMENT is not forwarded" even for parts in multipat/related which is not pointed by mail body part of text/html in multipart/related.

Comment 18

[Jonathan Kamens]

(In reply to comment #17)
I am not sure I understand exactly what you are saying, but I *think* you are saying is that with my change, if someone is sent an email message with inline embedded images and uses "Delete all attachments" on the email message, then the inline embedded image attachments will be removed and will therefore no longer be rendered properly in the body of the email message.

This is not actually the case. Inline attachments are not deleted when the user selects "Delete all..." unless they first switch the message view to View > Message Body As > Plain Text, and if they do that, then the attachments will be visible at the bottom of the screen in the attachments bar so they'll know that they're being deleted when the select Delete all...

In short, this is not a problem.

Comment 19

[Jonathan Kamens]

And, by the way, statements such as, "Jonathan Kamens, are you ready to answer to all of such unwanted bugs at B.M.O by yourself?" are rude and obnoxious. I just spent something like 20 hours of my time learning enough about the innards of Thunderbird to rewrite a module to fix a significant bug which has been around for four years. Do you really think that's the reaction the Mozilla community should be giving to the people who are willing to devote their time to improving Thunderbird? Because brother, if I've got to put up with a steep learning curve AND taking crap from people just for daring to submit patches, then I'm not sure it's worth the effort. And I'm pretty thick-skinned, so if you come close to putting me off helping, then you can be sure there are a lot of other people whom you'd drive away entirely.
Polite treatment is called for toward the people who give their time and energy to improving free software for everyone.

Comment 20

[David :Bienvenu]

Jonathan, that's super impressive.  Thx for tackling this.

Comment 21

[David :Bienvenu]

Comment on attachment 459751 [details] [diff] [review]
rewrite multipart/alternative handling to fix this problem

The next steps are to get this reviewed, and to make sure we have unit tests for this, probably not in that order. I have a simple unit test for the detach functionality, iirc, though it's more about making sure the whole process finishes, and not so much the integrity of the resulting message. It's mailnews/base/test/unit/test_detachToFile.js, and is more complicated than you'd need for this bug because it's making sure things work with imap.

Comment 23

[Jonathan Kamens]

Attachment: rewrite of mimemalt.cpp to fix this problem

Slightly improved version of this patch to fix a problem with the last version. In the last version, Edit as New or Forward Inline produced incorrect results because a non-displayable multipart/alternative part was being fed to the engine preparing the draft composition. The new version of the patch fixes this issue (it's just a one-line change to the patch).

Comment 24

[David :Bienvenu]

(In reply to comment #9)
> I have a .eml extension that I will attach.  If you open it up, detach all you
> will end up in the scenario where it cannot delete them (as it tries to delete
> automatically after a detach). Furthermore, you can do a delete all and it will
> not do anything.

Joel, were you viewing message body as plain text? I did a detach all on your messaage and it detached/deleted all the attachments, from what I could tell.

Comment 25

[David :Bienvenu]

I've verified that the gmx message shows the bug, and the patch fixes it. Unfortunately, there are several libmime assertions when detach all runs, and assertions break unit tests, so those will need to be fixed before I can get a unit test that passes.

Comment 26

[David :Bienvenu]

And reading multipart alternative messages generates this assertion as well:

>	mime.dll!MimeMultipart_parse_line(const char * line=0x1143d6d0, int length=0x00000031, MimeObject * obj=0x0a53d880)  Line 195 + 0x25 bytes	C++
 	mime.dll!convert_and_send_buffer(char * buf=0x1143d6d0, int length=0x00000031, int convert_newlines_p=0x00000001, int (char *, unsigned int, void *)* per_line_fn=0x63df5130, void * closure=0x0a53d880)  Line 184 + 0xf bytes	C++
 	mime.dll!mime_LineBuffer(const char * net_buffer=0x08edeb18, int net_buffer_size=0x00000031, char * * bufferP=0x0a53d8a8, int * buffer_sizeP=0x0a53d8b0, unsigned int * buffer_fpP=0x0a53d8b8, int convert_newlines_p=0x00000001, int (char *, unsigned int, void *)* per_line_fn=0x63df5130, void * closure=0x0a53d880)  Line 272 + 0x1d bytes	C++
 	mime.dll!MimeObject_parse_buffer(const char * buffer=0x08edeb18, int size=0x00000031, MimeObject * obj=0x0a53d880)  Line 275 + 0x31 bytes	C++
 	mime.dll!MimeMessage_parse_line(const char * aLine=0x08edeb18, int aLength=0x00000031, MimeObject * obj=0x0c94ad80)  Line 232 + 0x16 bytes	C++
 	mime.dll!convert_and_send_buffer(char * buf=0x08edeb18, int length=0x00000031, int convert_newlines_p=0x00000001, int (char *, unsigned int, void *)* per_line_fn=0x63dfbd20, void * closure=0x0c94ad80)  Line 184 + 0xf bytes	C++
 	mime.dll!mime_LineBuffer(const char * net_buffer=0x08e856ed, int net_buffer_size=0x0000086b, char * * bufferP=0x0c94ada8, int * buffer_sizeP=0x0c94adb0, unsigned int * buffer_fpP=0x0c94adb8, int convert_newlines_p=0x00000001, int (char *, unsigned int, void *)* per_line_fn=0x63dfbd20, void * closure=0x0c94ad80)  Line 272 + 0x1d bytes	C++
 	mime.dll!MimeObject_parse_buffer(const char * buffer=0x08e84890, int size=0x000016c8, MimeObject * obj=0x0c94ad80)  Line 275 + 0x31 bytes	C++
 	mime.dll!mime_display_stream_write(_nsMIMESession * stream=0x08435040, const char * buf=0x08e84890, int size=0x000016c8)  Line 946 + 0x16 bytes	C++
 	mime.dll!nsStreamConverter::OnDataAvailable(nsIRequest * request=0x08b27b48, nsISupports * ctxt=0x00000000, nsIInputStream * aIStream=0x06cb95a8, unsigned int sourceOffset=0x00000000, unsigned int aLength=0x000016c8)  Line 979 + 0x1a bytes	C++
 	docshell.dll!nsDocumentOpenInfo::OnDataAvailable(nsIRequest * request=0x08b27b48, nsISupports * aCtxt=0x00000000, nsIInputStream * inStr=0x06cb95a8, unsigned int sourceOffset=0x00000000, unsigned int count=0x000016c8)  Line 323 + 0x30 bytes	C++
 	msgimap.dll!nsImapCacheStreamListener::OnDataAvailable(nsIRequest * request=0x1117ffc0, nsISupports * aCtxt=0x00000000, nsIInputStream * aInStream=0x06cb95a8, unsigned int aSourceOffset=0x00000000, unsigned int aCount=0x000016c8)  Line 8563	C++
 	necko.dll!nsInputStreamPump::OnStateTransfer()  Line 510 + 0x40 bytes	C++
 	necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x06cb95a8)  Line 400 + 0xb bytes	C++

So that needs to be fixed - either the assertion is no longer valid, or there's a problem with the patch.

Comment 27

[Jonathan Kamens]

Attachment: rewrite of mimemalt.cpp to fix this problem

David,

Regarding your last two comments, it's unclear to me whether you're saying that the assertion you posted there occurs when deleting attachments or viewing multipart/alternative messages, or that there are two different assertions, one (which you did not give a stacktrace for) which occurs when deleting attachments and another which occurs when viewing multipart/alternative messages.

I'm hoping that since you gave only one stacktrace, it's that same assertion that occurs in both cases, because the new version of my patch which I'm attaching here fixes that assertion when viewing multipart/alternative messages, and with this new patch in place I don't get any assertions when deleting attachments.

If there is still another assertion I'm missing, please give me some more details about it so I can address it.

Thanks for catching this issue (which, incidentally, I believe was left over from the original mimemalt.cpp, not caused by my rewrite, although I'm not 100% certain about that).

Thanks,

  jik

Comment 28

[Jonathan Kamens]

Actually, I think there's another issue I need to track down with my patch. It's showing message bodies as attachments in the attachment list at the bottom of the message when it shouldn't. I need to figure out why this is happening and fix it. So please don't spend any more time unit testing the patch until I figure it out and submit a fixed version. Thanks!

Comment 29

[Jonathan Kamens]

Attachment: changes to mimemalt.cpp and other mime/src modules to fix this problem

Updated patch attached. I'm going to also attach a README explaining my changes in more detail to assist reviewers and testers. That's coming in a minute.

Comment 30

[Jonathan Kamens]

Attachment: README file explaining the changes made to fix this issue

Please review this README for a description of what I've changed, including the changes I'm nervous about and would particularly like feedback about from reviewers.

Please search for 07/29/2010 in the README file to see what's new in the patch I just submitted.

Comment 31

[Jonathan Kamens]

Attachment: changes to mimemalt.cpp and other mime/src modules to fix this problem

Yet another updated patch.

I just noticed a particular combination of circumstances that my prior testing did not detect -- deleting an attachment that was the child of a multipart/mixed entity was inserting the deleted-attachment headers and text but then leaving the body of the attachment in place. Double plus ungood. This updated patch fixes that problem. FYI, here's what's different about this new patch:

@@ -594,6 +543,9 @@ MimeMultipart_create_child(MimeObject *o
 static PRBool
 MimeMultipart_output_child_p(MimeObject *obj, MimeObject *child)
 {
+  /* We don't output a child if we're stripping it. */
+  if (obj->options && obj->options->state && obj->options->state->strippingPart)
+    return PR_FALSE;
   /* if we are saving an apple double attachment, ignore the appledouble wrapper part */
   return (obj->options && obj->options->write_html_p) ||
           PL_strcasecmp(child->content_type, MULTIPART_APPLEDOUBLE);
@

Comment 32

[David :Bienvenu]

I'll switch to running with this patch later today.

Comment 33

[David :Bienvenu]

all mozmill and xpcshell tests pass with the most recent patch, and I don't get assertions running with it. I'll try to do an xcpshell test for this in the next day or so.

Comment 34

[David :Bienvenu]

with this patch, the gmx e-mail above doesn't show any attachments, even when viewed as plain text with attachments not inline...which may or may not be the right thing to do, but it does break my test case for detaching :-)

Comment 35

[Jonathan Kamens]

(In reply to comment #34)
> with this patch, the gmx e-mail above doesn't show any attachments, even when
> viewed as plain text with attachments not inline...which may or may not be the
> right thing to do, but it does break my test case for detaching :-)

This is a tough one.
I don't think anything I changed specifically pertains to this behavior, though obviously it's a maze of twisty little passages with all kinds of non-obvious side-effects.
From my understanding of the code, which admittedly even after all the work I've done on it is not complete, I think the behavior you're seeing now, i.e., inline images not being displayed, was actually the intended behavior of the code all along, and it just wasn't working right.
In the example message here, you've got a multipart/alternative body with one text/plain part and one multipart/related part. The images are in the multipart/related part. When the user tells Thunderbird to display the message as plaintext, that means telling Thunderbird to display the text/plain part instead of the multipart/related part, and if the latter isn't being displayed, then the attachments within it shouldn't be displayed. So the behavior is, from at least this point of view, correct.
Note also comment 17 above, where at least one user expressed the opinion that inline images should not be viewable as attachments. So there are least some users who think this is correct behavior.
By way of comparison, Outlook won't let you see inline images as attachments no matter what (as far as I could tell), and the Mac Mail client displays inline images as attachments even when you're viewing the body as HTML. So no matter what behavior we choose for Thunderbird, there's precedent. ;-)
A pretty strong argument in favor of the current behavior being correct is that quite frankly, for it to behave any other way, the code would have to be just incredibly hideous and difficult to maintain. "Multipart/alternative means pick one part to display and don't display the others, except that if one of the parts is multipart/related and it's not the part we picked to display and it has images in them that would be treated as inline images if it WERE the part we picked to display, then instead of displaying them as inline images, display them as attachments." I mean, really? Ew.
On the one hand, this a change in behavior from what's currently in the field. On the other hand, keep in mind that the change in behavior is minimal, because although the inline images could be *seen* before when displaying the body as text, they couldn't be *deleted*, because of the bug that this ticket is all about. So the best you could do was to right click on them to save them to a file, and you can right-click on images in the HTML and save them that way, so I don't see much loss in functionality.
I am not sure how we decide whether to keep the current behavior or hack the old behavior back into place (which I'd personally rather not do). How are decisions like this usually made in Mozilla?

Comment 36

[David :Bienvenu]

These decisions are usually made by the module owner or peers - see https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements , except for decisions that affect the UI non-trivially, in which case clarkbw generally makes them (for Thunderbird, at least - this bug would also affect SeaMonkey). I think this is enough of an edge case that I can decide. In particular, this is really a question of how view | message body as plain text should work, and I'm ok with it not showing you image attachments...

My one caveat with your argument above is that my understanding of view message body as plain text is that we still pick the html part, but convert the html to plain text using mimeInlineTextHTMLAsPlaintextClass, not that we just pick the plain text part to display, though the latter would seem more intuitive. But that option needs to work in the case where no plain text part is provided (e.g., a spam message).

Oh, and when I have view | message body | as plain text, I'm getting an assertion on every multipart/alternative message:

 	mime.dll!MimeInlineTextPlain_parse_line(const char * line=0x0045d46c, int length=0x00000000, MimeObject * obj=0x09d3e5b8)  Line 325 + 0x22 bytes	C++
 	mime.dll!MimeInlineTextHTMLAsPlaintext_parse_eof(MimeObject * obj=0x09d3e5b8, int abort_p=0x00000000)  Line 127 + 0x1c bytes	C++
 	mime.dll!MimeMultipartAlternative_display_cached_part(MimeObject * obj=0x14b9ac68, MimeHeaders * hdrs=0x14217008, MimePartBufferData * buffer=0x14217150, int do_display=0x00000000)  Line 562 + 0x10 bytes	C++
 	mime.dll!MimeMultipartAlternative_flush_children(MimeObject * obj=0x14b9ac68, int finished=0x00000001, int next_is_displayable=0x00000000)  Line 295 + 0x43 bytes	C++
 	mime.dll!MimeMultipartAlternative_parse_eof(MimeObject * obj=0x14b9ac68, int abort_p=0x00000000)  Line 318 + 0xd bytes	C++
 	mime.dll!MimeMultipartRelated_parse_eof(MimeObject * obj=0x14b99fa8, int abort_p=0x00000000)  Line 1166 + 0x10 bytes	C++
 	mime.dll!MimeContainer_parse_eof(MimeObject * object=0x0659a948, int abort_p=0x00000000)  Line 140 + 0x12 bytes	C++
 	mime.dll!MimeMessage_parse_eof(MimeObject * obj=0x0659a948, int abort_p=0x00000000)  Line 542 + 0xe bytes	C++
 	mime.dll!mime_display_stream_complete(_nsMIMESession * stream=0x0659aa38)  Line 969 + 0x12 bytes	C++
 	mime.dll!nsStreamConverter::OnStopRequest(nsIRequest * request=0x15210ce8, nsISupports * ctxt=0x00000000, unsigned int status=0x00000000)  Line 1090 + 0xf bytes	C++

Comment 37

[Jonathan Kamens]

(In reply to comment #36)
> My one caveat with your argument above is that my understanding of view
> message body as plain text is that we still pick the html part, but convert
> the html to plain text using mimeInlineTextHTMLAsPlaintextClass, not that we
> just pick the plain text part to display, though the latter would seem more
> intuitive.

Nope, we're actually picking the plain text part to display.

I will attach a message which demonstrates this. I tried it with Thunderbird 3.1.1 both with and without my patch and the result is the same -- when you select View > Message Body As > Plain Text, you see the stuff that's in the text/plain body part, not the stuff that's in the text/html body part converted to plain text.

> Oh, and when I have view | message body | as plain text, I'm getting an
> assertion on every multipart/alternative message:

I'll need to get back to you on this. I need to rebuild with debugging enabled and then track it down, and it'll take a while.

  jik

Comment 38

[Jonathan Kamens]

Attachment: email message which demonstrates View > Message Body As > Plain Text behavior

Comment 39

[David :Bienvenu]

I see "This is HTML" no matter how I'm viewing the message body as, which would seem to bolster my argument, unless I'm confused, or seeing something different from what you're seeing.

Comment 40

[Jonathan Kamens]

When I select "View > Message Body As > Plain Text", I see "This is plain text."

It is fascinating that you see something different ;-).

There must be a preference somewhere affecting this, although I don't see it in the preferences dialog.

I will see if I can figure it out.

Comment 41

[David :Bienvenu]

My first test was just opening the .eml file from bugzilla and it behaved as I described. But when I created a local mail folder out of it, it behaved as you described.  So there's some difference between .eml file handling and message handling (perhaps view | message body as doesn't work for .eml files).

Comment 42

[David :Bienvenu]

setting view | message body as | plain text also sets the pref "mailnews.display.prefer_plaintext", which mimemalt.cpp uses to decide it can't display the html parts. So you were right...and the new behavior of not showing the image attachments in the html part seems completely defensible.

Comment 43

[Jonathan Kamens]

Attachment: patch to fix assertion which is actually independent of my changes - checked in

(In reply to comment #36)
> Oh, and when I have view | message body | as plain text, I'm getting an
> assertion on every multipart/alternative message:

OK, so here's the story with this assertion...

As I've noted previously and in my documentary comment at the top of the new mimemalt.cpp, what's different about how I'm doing things now is that all MIME parts are created in the message, even the ones that aren't being displayed, to ensure that all the MIME paths are correct. The MIME parts that aren't being displayed end up empty because they aren't being displayed. And there's code in mimethpl.cpp which tries to parse the contents of an empty MIME part, thus triggering this assertion.

The fix is simple: Don't do that. I.e., don't try to parse an empty line.

This bug could occur in other circumstances, not just because of my changes, so I'm submitting it as a separate patch and requesting review for it. David, I think you're in a better position than I to figure out who the reviewer should be.

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 462919 [details] [diff] [review]
patch to fix assertion which is actually independent of my changes - checked in

Bugzilla has the unfortunate ability to request reviews of noone in particular, which usually end up going unnoticed.  Setting bienvenu as reviewer...

Comment 46

[David :Bienvenu]

Comment on attachment 462919 [details] [diff] [review]
patch to fix assertion which is actually independent of my changes - checked in

Thx for the patch. You probably could move the Truncate to inside the if block as well...I'll try to land this today.

Comment 47

[Jonathan Kamens]

Status on this?

Comment 48

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #30)
> README file explaining the changes made to fix this issue

You look resolving many bugs listed in dependency tree for Bug 505172 and issues relevant to "what is mail body, what is attachment".
I believe you should have started your great work in bug 253830, instead of this bug. 
> bug 253830:  "View as plain text" displays HTML part converted to plaintext
> even though text/plain part is present
> ( multipart/alternative[ text/plain + multipart/related[ text/html + image/jpeg ] ] )
I think "why bug 253830 occurs" is essential issue in multipart/alternative handling of Tb. 
Your comment #13, "This is still a problem in Thunderbird 3.1.", should have been posted to bug 253830. I misunderstood that you were trying to implement request of comment #0. It was reason of my negative response.
comment #13 and trying to propose patch to this bug without answer to question of "(not-used/not-shown part in multipart/related is deletable/detachable?)" in bug summary, usually means you are trying to make delete/detach of such parts possible.

I hope you(and David) continue your great/tough work in bug 253830 or successor of that bug and some other bugs instead of misleading this bug.
I believe this bug is to be closed as INVALID or some other codes, because this bug can't occur after your valid solution. No one can see embed image for HTML as attachhment, so no can try to delete/detach them.

Comment 49

[Jonathan Kamens]

(In reply to comment #48)
> ...

If the work I did to fix this bug also addresses other issues outlined in other bugs, that's great, but that doesn't indicate that I am "misleading" anything or anyone here or that this bug is in any way "invalid".

Speculating on which specific bugzilla ticket should have motivated me to patch Thunderbird, or which specific bugzilla ticket I should have attached my patch to, etc., etc. is in my opinion a pointless waste of time.

The issue outlined in this bug is real. The patch I've posted in this bug addresses that issue and makes Thunderbird behave better than it did before. David said in comment #46 on August 17, "I'll try to land this today," which obviously didn't happen because he got pulled away on other things. That's fine, I'm not complaining about that, I'm just saying that if we got to the point a month and a half ago where David was ready to land the patch into the Thunderbird source tree, it is pointless to now start talking about which bugs I should have attached the patch to instead.

The next step, as far as I can tell, is for David, when he has the time, to land this patch into the source tree as he said he was going to.

David, please correct me if I'm wrong here.

Comment 50

[Mark Banner (:standard8)]

(In reply to comment #49)
> The next step, as far as I can tell, is for David, when he has the time, to
> land this patch into the source tree as he said he was going to.

I believe this has already landed on trunk:

http://hg.mozilla.org/comm-central/rev/f734adc19f35

Comment 51

[Jonathan Kamens]

(In reply to comment #50)
> (In reply to comment #49)
> > The next step, as far as I can tell, is for David, when he has the time, to
> > land this patch into the source tree as he said he was going to.
> 
> I believe this has already landed on trunk:
> 
> http://hg.mozilla.org/comm-central/rev/f734adc19f35

Hmm. Something is strange here.

That patch is actually only a very small and ancillary part of the patch I submitted in this ticket, and that patch doesn't actually address the root issue of this ticket.

Furthermore, the bug ticket referenced in that changeset is different from this one. Furthermore, the bug ticket referenced in that changeset doesn't appear to have anything to do with the patch included in the changeset.

Confusion abounds.

Comment 52

[Jonathan Kamens]

Oh, wait, it's becoming a little clearer. When David said he was going to land the patch, he was referring only to that minor little patch, not to the whole thing. The question still remains of what we can do to get the big patch checked in. I guess somebody needs to review it. I don't know from whom to request a review. David's listed as one of the module owners, so I guess I'll request from him and he can reassign if he thinks someone else would be better.

Comment 53

[Mark Banner (:standard8)]

(In reply to comment #51)
> That patch is actually only a very small and ancillary part of the patch I
> submitted in this ticket, and that patch doesn't actually address the root
> issue of this ticket.

If attachment 462352 [details] [diff] [review] is the main patch that you're referring to, then that hasn't ever had review requested on it from what I can tell - so I doubt that David has reviewed it fully if that is the case.

> Furthermore, the bug ticket referenced in that changeset is different from this
> one. Furthermore, the bug ticket referenced in that changeset doesn't appear to
> have anything to do with the patch included in the changeset.

Unfortunately typos/mistakes do occur. In this case it looks like David used the number of the attachment (attachment 462919 [details] [diff] [review]) rather than the bug number.


So if the main patch is the one you need, I suggest you request review from David on it so that there's a formal stamp on the attachment.

Comment 54

[David :Bienvenu]

yes, I checked in the assertion fix. I was having trouble creating a mozmill test for the main bug because the latest version of the patch doesn't show the attachments for the test case I have, so I need a message that shows the bug in detaching, and still shows the attachments. See comment 34 and on...

Comment 55

[Jonathan Kamens]

Attachment: new test case that shows an attachment even with the patch applied

Here's a message modified from the original test case by adding an attachment after the multipart/related part. If you try to delete this attachment in Thunderbird without my patch, the message gets totally screwed up. If you try to delete this attachment in Thunderbird with my patch, everything works as it should.

Comment 56

[David :Bienvenu]

Attachment: sanitized test case

This is a sanitized test case that I'll base my xpcshell test on...

Comment 57

[David :Bienvenu]

Comment on attachment 461151 [details]
README file explaining the changes made to fix this issue

this doesn't really need review, but it looks fine, thx.

This might be something that could go on the wiki, with whatever other doc we have about libmime.

Comment 58

[David :Bienvenu]

Attachment: [checked in] unit test that fails w/o the patch, and succeeds with it.

Comment 59

[David :Bienvenu]

Comment on attachment 462352 [details] [diff] [review]
changes to mimemalt.cpp and other mime/src modules to fix this problem

Thx very much for the patch - Some nits:

MimeObjectChildIsMessageBody should just be removed, since it's not used, as well as the comment.

// Please use NS_ASSERTION instead of PR_ASSERT (e.g., NS_ASSERTION(malt->pending_parts, "should be pending parts, but there aren't");
+  PR_ASSERT(malt->pending_parts);

put return on its own line:
+  if (!malt->pending_parts) return -1;
+  PRInt32 i = malt->pending_parts - 1;

No need for the braces here:
+  if (body->options->part_to_load) {
+    body->options->part_to_load = strdup(body->options->part_to_load);
+  }
+  if (body->options->default_charset) {
+    body->options->default_charset = strdup(body->options->default_charset);
+  }

Other than that, it looks OK - I can make the changes myself if you like...

Comment 60

[David :Bienvenu]

Comment on attachment 462352 [details] [diff] [review]
changes to mimemalt.cpp and other mime/src modules to fix this problem

minusing based on some cleanup required...

Comment 61

[Jonathan Kamens]

Attachment: updated for code cleanup: changes to mimemalt.cpp and other mime/src modules to fix this problem

Made changes requested by code David, and a couple of other minor non-functional code cleanup changes (noticed a typo and some missing words in a comment, fixed compiler complaint about /* inside a comment).

Comment 62

[David :Bienvenu]

Comment on attachment 480555 [details] [diff] [review]
updated for code cleanup: changes to mimemalt.cpp and other mime/src modules to fix this problem

I think you want to use PR_FREEIF instead of PR_Delete here:

+  PR_DELETE(malt->buffered_hdrs);
+  PR_DELETE(malt->part_buffers);

Other than that, it looks OK. I can land this, with that change, if that change looks OK to you, and then we can get some trunk baking.

Comment 63

[Jonathan Kamens]

Neither PR_FREEIF nor PR_DELETE does exactly what I want.

You're right that PR_DELETE is a problem since it doesn't chedk if the pointer is null before calling PR_Free on it, and PR_Free just calls free, and free on some platforms chokes on a null argument.

On the other hand, PR_FREEIF is a problem because I want to set the pointers to null when I'm doing freeing them, just to be paranoid about potential multiple cleanup attempts, and PR_DELETE sets the pointers to null whereas PR_FREEIF doesn't.

I guess you need to change it to:

if (malt->buffered_hdrs)
  PR_DELETE(malt->buffered_hdrs);
if (malt->part_buffers)
  PR_DELETE(malt->part_buffers);

Actually, the CORRECT fix would be to add a PR_DELETEIF macro to prmem.h which combines the functionality of PR_FREEIF and PR_DELETE, but do you really want to get into a core change to fix this bug? :-)

  jik

Comment 64

[David :Bienvenu]

PR_FREEIF does null out the original pointer, afaik.

Comment 65

[David :Bienvenu]

in fact, PR_FREEIF does: if (ptr) PR_DELETE(ptr) :-)

Comment 66

[Jonathan Kamens]

Oh, yeah, you're right, I'm an idiot, it was right in front of me and I didn't see it.

Go ahead and change my PR_DELETE to PR_FREEIF (which should really be called PR_DELETEIF, but whatever :-).

Comment 67

[David :Bienvenu]

fix checked in. It would be good to go through the tracking bug 505172 and see which, if any, of those are fixed by this change, per WADA's suggestion, so, adding qawanted keyword.

Comment 68

[Ludovic Hirlimann [:Usul]]

(In reply to comment #67)
> fix checked in. It would be good to go through the tracking bug 505172 and see
> which, if any, of those are fixed by this change, per WADA's suggestion, so,
> adding qawanted keyword.

Any chances to get a few Unit test with this ? (or doesn't it make sense ?)

Comment 69

[David :Bienvenu]

There is a unit test waiting for standard8's review.

Comment 70

[Mark Banner (:standard8)]

Comment on attachment 480300 [details] [diff] [review]
[checked in] unit test that fails w/o the patch, and succeeds with it.

r=Standard8, I'll add checkin-needed in a mo so we don't forget about this.

Comment 71

[Mark Banner (:standard8)]

Which patches need checkin here, is it just the unit test?

Comment 72

[David :Bienvenu]

yes, just the unit test, thx. I can land it if/when the tree is open again...

Comment 73

[Mark Banner (:standard8)]

Comment on attachment 480300 [details] [diff] [review]
[checked in] unit test that fails w/o the patch, and succeeds with it.

Checked in: http://hg.mozilla.org/comm-central/rev/a8f88c9cb2d4