81751 - when attaching multipart web pages to a piece of email only final part should be attached

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[Christopher Blizzard (:blizzard)]

When you attach a multipart web page to an email message it's attached as the
entire multipart message instead of just the final part.  A good way to
reproduce this is to attach a bugzilla query.  Instead of the resulting html
that is the bug list it attaches the entire query.  This isn't what 4.x does.

This is a spinoff of bug 81682.

Comment 1

[Jean-Francois Ducarroz]

yes, 4.x correctly attach the last part of the CGI response.

Comment 2

[Christopher Blizzard (:blizzard)]

What are the odds of seeing this in 0.9.1? It's been bugging the hell out of me
for a while now.

Comment 3

[Jean-Francois Ducarroz]

where can I find specs about http multiparts? What the rules? I presume should be like mime! Does the separator is 
always the same?

Comment 4

[Jean-Francois Ducarroz]

When I fetch a URL, I receive a content type, so far we are doing nothing with it. For regular web page, the content 
type is text/html but in case of a bugzilla query, it's multipart/x-mixed-replace. I searched 4.x code and looks like I 
will need to add a converter to support the following content-type:

multipart/x-mixed-replace
multipart/x-byteranges
multipart/byteranges
multipart/mixed

Comment 5

[Jean-Francois Ducarroz]

good news, netwerk already provider all those converters. All I need to do, is to refuse to handle those content-type 
in nsIURLFetcher and then figure out which part I need to save. Should be not too complex for  multipart/x-mixed-
replace but I don't have a clue about the meaning of the other one and especially how to test them.

Comment 6

[Jean-Francois Ducarroz]

I have a fix (for multipart/x-mixed-replace) that I need to cleanup a littele bit before I can post the diff. 
Nominating nsbeta1.

Comment 7

[Jean-Francois Ducarroz]

Attachment: proposed fix, v1

Comment 8

[Christopher Blizzard (:blizzard)]

+#define MULTIPART_MIXED_REPLACE   "multipart/x-mixed-replace"
+#define MULTIPART_MIXED           "multipart/mixed"
+#define MULTIPART_BYTERANGES      "multipart/byteranges"

Aren't there defines for these elsewhere?  kMimeMultiPart or something?  If not
shouldn't there be?

+  if (nsCRT::strcasecmp(aContentType, UNKNOWN_CONTENT_TYPE) == 0)
+    rv = InsertConverter(aContentType);
+  else
+  /*
+    if the content type is multipart/x-mixed-replace, we need to insert the
+    multipart converter.
+  */
+  if (nsCRT::strcasecmp(aContentType, MULTIPART_MIXED_REPLACE) == 0)
+    rv = InsertConverter(aContentType);
+  else

Can you move the else closer to the if please?  It's confusing when there's a
comment in the middle when looking at the code.

+    /* In case of multipart/x-mixed-replace, we need to truncate the file to
the current part size */
+    if (PL_strcasecmp(mConverterContentType, MULTIPART_MIXED_REPLACE) == 0)
     {
-      mContentType = contentType;
+      PRInt64 fileSize;
+      LL_I2L(fileSize, mTotalWritten);
+      mLocalFile->SetFileSize(fileSize);

Does this truncate before the final write?  I'm wondering if you're truncating
with the final file size using the trailing marker in the multi part message or not.

Comment 9

[Jean-Francois Ducarroz]

>Aren't there defines for these elsewhere?  kMimeMultiPart or something?  If not
>shouldn't there be?

No, they are not defined yet (except mixed/multipart) but I can add then to the mime type file and fix the few place 
in the Mozilla code where the hardcoded string is used.

>Can you move the else closer to the if please?  It's confusing when there's a
>comment in the middle when looking at the code.

Sure, I'll...

>Does this truncate before the final write?  I'm wondering if you're truncating
>with the final file size using the trailing marker in the multi part message or not.

This append when we are done writing the final part and we have already close the file.

I'll post soon a news patch that take care of those comments...

Comment 10

[scottputterman]

Putting in 0.9.1.  It looks like this will prevent the crash in 81682 and it
makes so you can actually read the page that was sent in the message.

Comment 11

[Christopher Blizzard (:blizzard)]

It will only prevent the crash because people might not send out messages with
it.  Wallpaper.

Comment 12

[Jean-Francois Ducarroz]

Attachment: patch 1/2, v2: Add new mime types and change mozilla code to use it

Comment 13

[Jean-Francois Ducarroz]

Attachment: patch 2/2, v2: New version of the fix for message compose

Comment 14

[scottputterman]

If the crash gets fixed (which it sounds like it will), what's the worst thing
that will happen by keeping this bug open until 0.9.2?  My understanding is that
in the 0.9.1 build the user should be able to open the attachment to see the
message.  Is that not correct?  

Comment 15

[Jean-Francois Ducarroz]

I don't know if we will be able to display correctly the page if we fixe just
the crash.

This bug affect dynamically generated page that use multipart, that should not
be too much.

Comment 16

[scottputterman]

I think we can wait until 0.9.2 for this and then get more time to play with
this change at the beginning of a milestone.

Comment 17

[scottputterman]

Putting back on 0.9.1 radar.  It looks like it's going to be hard to fix the crash.

Comment 18

[varada]

r=varada

Comment 19

[Jean-Francois Ducarroz]

The patch add support for multipart/x-mixed-replace and has been tested for it. 
But I have also added support for other type of multipart (mixed, byteranges) 
for which I don't have a test case and therefore cannot quaranty at 100% I am 
doing the right thing (that doesn't means it will crash but that maybe I am not 
generating the rigth output).

Comment 20

[Scott MacGregor]

hmmm this is a pretty hefty change in the way we do things for generating
certain attachments. The code looks okay to me but we're going to have to test
the heck out of this stuff once you check it in. I bet dougt may be able to help
you find a website that uses multi-part/byte. I vaguely remember him checking in
a fix to the multi-part converter recently to make it work with multi-part/byte
pages. that would at least give us a test case for the byte part.

sr=mscott

Comment 21

[scottputterman]

Scott, are you concerned about attachment types besides x-mixed-replace and
byteranges like all attachments in general?

If you think there's a lot of testing needed perhaps we should have a bunch of
people run with this for a few days or just wait until 0.9.2.  I'm not sure how
many pages there are out there like this but my guess is that it's not high
enough to risk attachment sending. If I misinterpreted your concerns, let me know.

dougt, do you have any examples of sites with multipart/byteranges?

Comment 22

[Doug Turner (:dougt)]

the only example that I have of multipart byterange is via plugins.  If you go
to www.adobe.com, and view a pdf from there (or any http server that supports
range requests), the plugin will request ranges from the http server.  This
content comes back from the server as multipart/byterange.

Also, you can hack TestProtocols.  Add a header that has two or more segmented
ranges.  

Does this help?

Comment 23

[Asa Dotzler [:asa]]

a= [e-mail] (on behalf of drivers)

Comment 24

[scottputterman]

I know this got the approval, but I'd like to ask if this can get into 0.9.2
instead (I know I'm flipping back and forth).  I think it would be good to have
some more time to run with this.

Comment 25

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 26

[Jean-Francois Ducarroz]

Fixed and checked in (TRUNK ONLY)

Comment 27

[esther]

Using builds for win, mac and linux dated 06-27-01 and the original scenario,
this is fixed.  Verified.   Note: we do not have tests for
miltipart/x-mixed-replace & multipart/byteranges.  Verified based on original
scenario.