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

RESOLVED FIXED in Thunderbird 3.3a1

Status

MailNews Core
Attachments
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Alexander Kluss, Assigned: Jonathan Kamens)

Tracking

(Blocks: 1 bug, {qawanted})

Trunk
Thunderbird 3.3a1
qawanted
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tracking bug 505172 needs review])

Attachments

(9 attachments, 5 obsolete attachments)

44.71 KB, message/rfc822
Details
336.90 KB, message/rfc822
Details
4.30 KB, text/plain
Bienvenu
: review+
Details
300 bytes, message/rfc822
Details
1.82 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
50.24 KB, message/rfc822
Details
1.63 KB, message/rfc822
Details
8.24 KB, patch
standard8
: review+
Details | Diff | Splinter Review
38.06 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 236615 [details]
this is a eml file of a GMX newsletter

Comment 2

11 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → Windows 2000
(Reporter)

Comment 3

11 years ago
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?
Seems that bug 351224 could be a dupe of this one.

Comment 5

10 years ago
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.
Product: Core → MailNews Core
Benoit, could you provide a small testcase in the mbox format and attach it to this bug? That would be very helpful.
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.
Joel, if even you could attach a small testcase it would be fantastic.
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.
Created attachment 338766 [details]
test case as per my previous comments.
Blocks: 505172
(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.
Summary: Could not delete/detach attachments from multipart/related messages → Could not delete/detach attachments from multipart/related messages (not-used/not-shown part in multipart/related is deltable/detachable?)
(Assignee)

Comment 12

7 years ago
(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.
(Assignee)

Comment 13

7 years ago
(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.
Summary: Could not delete/detach attachments from multipart/related messages (not-used/not-shown part in multipart/related is deltable/detachable?) → Could not delete/detach attachments from multipart/related messages (not-used/not-shown part in multipart/related is deletable/detachable?)
(Assignee)

Comment 14

7 years ago
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

7 years ago
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.
OS: Windows 2000 → All
Hardware: x86 → All
(Assignee)

Comment 16

7 years ago
Created attachment 459751 [details] [diff] [review]
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.
(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.
(Assignee)

Comment 18

7 years ago
(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.
(Assignee)

Comment 19

7 years ago
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

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

Comment 21

7 years ago
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.
Attachment #459751 - Flags: superreview?(bienvenu)
Attachment #459751 - Flags: review?(bugmail)
(Assignee)

Updated

7 years ago
Duplicate of this bug: 555691
(Assignee)

Comment 23

7 years ago
Created attachment 460144 [details] [diff] [review]
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).
Attachment #459751 - Attachment is obsolete: true
Attachment #459751 - Flags: superreview?(bienvenu)
Attachment #459751 - Flags: review?(bugmail)

Comment 24

7 years ago
(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

7 years ago
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

7 years ago
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.
(Assignee)

Comment 27

7 years ago
Created attachment 461089 [details] [diff] [review]
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
Attachment #460144 - Attachment is obsolete: true
(Assignee)

Comment 28

7 years ago
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!
(Assignee)

Comment 29

7 years ago
Created attachment 461150 [details] [diff] [review]
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.
Attachment #461089 - Attachment is obsolete: true
(Assignee)

Comment 30

7 years ago
Created attachment 461151 [details]
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.
(Assignee)

Updated

7 years ago
Attachment #461151 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 31

7 years ago
Created attachment 462352 [details] [diff] [review]
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);
@
Attachment #461150 - Attachment is obsolete: true

Comment 32

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

Comment 33

7 years ago
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

7 years ago
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 :-)
(Assignee)

Comment 35

7 years ago
(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

7 years ago
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++
(Assignee)

Comment 37

7 years ago
(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
(Assignee)

Comment 38

7 years ago
Created attachment 462863 [details]
email message which demonstrates View > Message Body As > Plain Text behavior

Comment 39

7 years ago
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.
(Assignee)

Comment 40

7 years ago
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

7 years ago
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

7 years ago
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.
(Assignee)

Comment 43

7 years ago
Created attachment 462919 [details] [diff] [review]
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.
Attachment #462919 - Flags: review?
Duplicate of this bug: 417646
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...
Attachment #462919 - Flags: review? → review?(bienvenu)

Comment 46

7 years ago
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.
Attachment #462919 - Flags: review?(bienvenu) → review+

Updated

7 years ago
Attachment #462919 - Attachment description: patch to fix assertion which is actually independent of my changes → patch to fix assertion which is actually independent of my changes - checked in
(Assignee)

Comment 47

7 years ago
Status on this?
(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.
(Assignee)

Comment 49

7 years ago
(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.
(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
Assignee: nobody → jik
(Assignee)

Comment 51

7 years ago
(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.
(Assignee)

Comment 52

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #461151 - Flags: review?(bienvenu)
(Assignee)

Updated

7 years ago
Attachment #462352 - Flags: review?(bienvenu)
(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

7 years ago
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...
(Assignee)

Comment 55

7 years ago
Created attachment 479458 [details]
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

7 years ago
Created attachment 480234 [details]
sanitized test case

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

Comment 57

7 years ago
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.
Attachment #461151 - Flags: review?(bienvenu) → review+

Comment 58

7 years ago
Created attachment 480300 [details] [diff] [review]
[checked in] unit test that fails w/o the patch, and succeeds with it.
Attachment #480300 - Flags: review?(bugzilla)

Comment 59

7 years ago
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

7 years ago
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...
Attachment #462352 - Flags: review?(bienvenu) → review-
(Assignee)

Comment 61

7 years ago
Created attachment 480555 [details] [diff] [review]
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).
Attachment #462352 - Attachment is obsolete: true
Attachment #480555 - Flags: review?(bienvenu)

Comment 62

7 years ago
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.
Attachment #480555 - Flags: review?(bienvenu) → review+
(Assignee)

Comment 63

7 years ago
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

7 years ago
PR_FREEIF does null out the original pointer, afaik.

Comment 65

7 years ago
in fact, PR_FREEIF does: if (ptr) PR_DELETE(ptr) :-)
(Assignee)

Comment 66

7 years ago
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

7 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: qawanted
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1
(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 ?)
Flags: in-testsuite?

Comment 69

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

Updated

7 years ago
Depends on: 602718
Whiteboard: [tracking bug 505172 needs review]
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.
Attachment #480300 - Flags: review?(bugzilla) → review+
Keywords: checkin-needed
Which patches need checkin here, is it just the unit test?

Comment 72

7 years ago
yes, just the unit test, thx. I can land it if/when the tree is open again...
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
Attachment #480300 - Attachment description: unit test that fails w/o the patch, and succeeds with it. → [checked in] unit test that fails w/o the patch, and succeeds with it.
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Duplicate of this bug: 606659
(Assignee)

Updated

6 years ago
Depends on: 678244
Depends on: 674473
Depends on: 685072
No longer depends on: 685072

Updated

6 years ago
Depends on: 697702

Updated

6 years ago
Depends on: 702569

Updated

6 years ago
No longer depends on: 702569

Updated

6 years ago
Depends on: 712885
You need to log in before you can comment on or make changes to this bug.