372786 - Keynote files sent as attachments cannot be opened by the receiver of the e-mail

Status: VERIFIED FIXED

(MailNews Core :: Attachments, defect, P2)

Description

[marco iacoboni]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Thunderbird 1.5.0.10

My daughter sent my wife a Keynote file as an attachment. My duaghter used Thunderbird to send the e-mail and my wife used Apple Mail to receive the e-mail. My wife could not open the file because it was 'damaged'. I subsequently sent myself one of my own Keynote files, which I can obviously open, using Thunderbird. The file I sent as an attachment while using Thunderbird, I cannot open it. It seems that every time one uses Thunderbid to send a Keynote file as an attachment, the file gets corrupted.

Reproducible: Always

Steps to Reproduce:
1. Attach a Keynote file to an e-mail
2. Send the e-mail
3. Receive the e-mail and try to open the Keynote file. 
Actual Results:  
The Keynote file canot be opened

Expected Results:  
The software should have delieverd an attached file that can be opened

Comment 1

[Mike Schroepfer]

Just a note that I hit this today with TBird 2.0.0.9 - I'm guessing TBird doesn't understand that keynote files are actually folders unpacked with all their contents.  Zipping up the file and attaching works just fine.

Comment 2

[Phil Ringnalda (:philor)]

schrep: what happens if you try to upload one in an <input type="file">, say in a bug on http://landfill.bugzilla.org/, with Firefox, and with Safari? 

Comment 3

[David Ascher (:davida)]

Attachment: Source of message containing "a keynote file" as attached by Tb trunk

I attached a file called MAAWG.key, sent it to my gmail account.  Attached is the message source w/o any of the headers which I don't think are relevant.

I'm also attaching the base64 decoding of the payload, generated using Python (import base64, base64.decodestring(...))

Comment 4

[David Ascher (:davida)]

Attachment: decoded base64 payload

Comment 5

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

I think that this could also be a problem for Trunk. I'll add an example keynote file.

Comment 6

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

Attachment: Testcase (gzip compressed keynote file/folder)

Comment 7

[Phil Ringnalda (:philor)]

My vague memory of the missing context of comment 2 was that Apple made the wrong choice, and decided to have Mail.app itself do the zipping when the app asks for a file and the user chooses something that's actually a directory. That means that confusingly enough it's transparently a "file" to Mail.app, but a directory listing to Safari, a mistake which we should not make. If we're going to do this, we should do it in Widget: Cocoa, and have nsFilePicker return a zip of the directory's contents when an app calls GetLocal_Files_ so we avoid the "I can't upload Keynote files in Firefox, but attaching them works fine in Thunderbird when I do the exact same thing to pick them!" issue.

Comment 8

[Håkan Waara]

I just tested, and you're right Phil. Mail.app indeed zips the file upon sending.

I'm guessing it does this with all bundles; e.g. apps (.app) keynote files (.key) etc.

I suppose we could extend nsFilePicker to allow for this, but I'm not convinced that we should make it the only mode possible.  I'm sure there are times when XULRunner apps want the actual bundle, or any of its contents.

Comment 9

[Håkan Waara]

Actually, looking at the nsIFilePicker interface, I can't see a clean way we could get a "zip if bundle" feature in there. It's not really a cross platform thing, and kind of specific to our (mail) needs.

It should be pretty easy to fix this bug by using our excellent nsIZipWriter implementation in MsgComposeCommands.js (AttachFiles) -- who wants to have a try?

Comment 10

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

Correct. Trying to upload a Keynote file let me choose this file within the file picker but when I hit the send button nothing happens. Means that this is really a core issue in handling file types which are hidden directories. The attachment is only 1kb in size after sending the attached testcase. Following content is sent as attachment:

File: testcase.key      Col 0              378 bytes                                                                          100%
300: file:///Users/henrik/Desktop/testcase.key/                                                                                   
200: filename content-length last-modified file-type                                                                              
201: .typeAttributes.dict 0 Thu,%2006%20Mar%202008%2020:52:56%20GMT FILE                                                          
201: Contents 0 Thu,%2006%20Mar%202008%2020:52:56%20GMT DIRECTORY                                                                 
201: index.apxl.gz 83487 Thu,%2006%20Mar%202008%2020:52:56%20GMT FILE                                                             
201: thumbs 0 Thu,%2006%20Mar%202008%2020:52:56%20GMT DIRECTORY

Mconnor, should the Firefox issue be filed as a separate bug?

Comment 11

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

In case of Thunderbird this is a dataloss bug.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

I think the core issue (this one) really is the dataloss.  I suspect it belongs in toolkit because it effects both Thunderbird & Firefox, though it is possible that both callers are forgetting something or doing something wrong.

The "extend Thunderbird and/or toolkit to magically autozip stuff" sounds like a separate enhancement bug.

Comment 14

[Karsten Düsterloh]

See also bug 127284.

Comment 15

[Håkan Waara]

Well, sure, but what is the proposed fix for the "core issue" then? I don't see a obvious way to resolve it at the widget level, given how generic and platform agnostic nsIFilePicker is at the moment.

One way would be to create a nsIFilePickerMac sub-interface of nsIFilePicker which had this feature. You could then try QI-ing to nsIFilePickerMac and use it if the QI was successful.   That's sorta how nsILocalFileMac and friends work.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

nsIFilePickerMac would be one possibility.  Another option would be adding a mode or filter to disallow directories being picked.  Or even "bundles", if there's some reasonably generic way to frame that.

Comment 17

[Håkan Waara]

FWIW, the more I think about this, the more I feel it's not really a widget/ issue, so if anything it would probablt belong in toolkit/ as something reusable.

Technically, I think it's trivial; it's basically just nsIFilePicker + nsIZIPWriter, so maybe making it a standalone component is even overkill?

Comment 18

[David Ascher (:davida)]

Anyone can take this.

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Moving from wanted‑thunderbird3.0a1+ flag to wanted‑thunderbird3.0a2? flag since code for Thunderbird 3 Alpha 1 has been frozen.

Comment 20

[Håkan Waara]

I have a proof of concept for this, where bundles would be silently zipped before added. It's not that much work left until it should work.

Do we want attachments that are zipped to just like the bundle you're attaching in the attachments pane, or should we maybe indicate its compressed state somehow? (Even show the zipped file instead?)

Comment 21

[Håkan Waara]

Correction: Do we want attachments that are zipped to just look like the bundle you're attaching in the attachments pane, or should we maybe indicate its compressed state somehow? (Even show the zipped file instead?)

Comment 22

[Patrick]

maybe this behavior is also the underlying cause for Safari not allowing us to mail the contents of a web page, see bug 317862?

Comment 23

[Håkan Waara]

(In reply to comment #22)
> maybe this behavior is also the underlying cause for Safari not allowing us to
> mail the contents of a web page, see bug 317862?

No, I think that is because of another cause. I'll comment in the other bug as to what.

Comment 24

[David Ascher (:davida)]

It would(In reply to comment #20)
> I have a proof of concept for this, where bundles would be silently zipped
> before added. It's not that much work left until it should work.

Cool!
 
> Do we want attachments that are zipped to just like the bundle you're attaching
> in the attachments pane, or should we maybe indicate its compressed state
> somehow? (Even show the zipped file instead?)

1) what does mail.app do w/ random .zip attachments?

2) we can probably ask the security group how safe unzipping is.

Comment 25

[Håkan Waara]

Attachment: Patch v1

Here's a patch that works exactly the same as Mail.app. The only difference is #2 below.

I'm not usually hacking a lot of XPCOM in JS, so comments and suggestions are appreciated!

Details:

1. When a bundle is attached, it's zipped first, so Foobar.app becomes Foobar.app.zip

2. The only difference here is that Mail.app doesn't show that the attachment is zipped -- we could change that too, but if we want to do that, let's create a new bug.

3. From my brief testing, it doesn't seem that Mail.app auto-unzips zipped bundles, so maybe we don't have to worry but that. But more testing would be appreciated. That's also a new bug, in that case.

Comment 26

[Håkan Waara]

I realized that ideally this zipping would be done on send-time (and not attach-time). The most important reason for that is this: if the temporary zipped attachment goes away from the TemporaryItems/ folder, Send Later will not work. I'm not sure how often (if ever) that would happen though...

I briefly looked over our attachment handling and it gives me the feeling this bug is gonna be a lot harder to fix; with the attachment handling moving into libmime-territory, for one...

Not sure how to proceed. The upside with this patch is that it's very simple and is 98% there. The downside is of course if Send Later will not work in this special case.  Would a follow-up bug for this case suffice, as it is a much less severe bug than the current state?

Comment 27

[David :Bienvenu]

First of all, thanks for tackling this!

My first reaction was that I think I would do this at send time, not at compose time. I don't think libmime would be involved but scary mailnews/compose code like nsMsgSend.cpp's GatherMimeAttachments or PreProcessPart might be. The drag is you'd probably need to create the zip file synchronously and block the UI during that part of the send process, but I think your patch blocks the UI during the attach process anyway.

I'm not really clear on when the temp files might be cleared up on OS/X, and whether it's the app or the OS that handles that. But if SendLater doesn't work, if you shutdown the app and restart, I think that would be an issue. It would also be an issue if the temp files were never cleaned up. So if you could do some tests with the problem scenario and let me know the results, that would be very helpful.

Comment 28

[Håkan Waara]

Thanks for the comments David,

(In reply to comment #27)
> First of all, thanks for tackling this!
> 
> My first reaction was that I think I would do this at send time, not at compose
> time. I don't think libmime would be involved but scary mailnews/compose code
> like nsMsgSend.cpp's GatherMimeAttachments or PreProcessPart might be. The drag
> is you'd probably need to create the zip file synchronously and block the UI
> during that part of the send process, but I think your patch blocks the UI
> during the attach process anyway.

Yes, I don't think blocking during send is a big deal. I'll see just how scary digging in mailnews/compose is. It looked really scary, but I'll try again. :)

Once the code is run during send-time, it will also be easier to include this in general send progress and make it async if we'd like to.

> 
> I'm not really clear on when the temp files might be cleared up on OS/X, and
> whether it's the app or the OS that handles that. But if SendLater doesn't
> work, if you shutdown the app and restart, I think that would be an issue. It
> would also be an issue if the temp files were never cleaned up. So if you could
> do some tests with the problem scenario and let me know the results, that would
> be very helpful.
> 

The temp folder that you get when you ask the directory service provider for 'TmpD' (which is how we always get the temp folder) is actually not /tmp (which I would expect on OS X / UNIX) but ~/Library/Caches/TemporaryItems/ which is a folder I've never seen before. 

I guess that the reason XPCOM returns this folder is legacy; our OS X support is sometimes still doing this like if it was MacOS 9... Maybe you know more?

I'm not sure who is responsible for cleaning that temp folder up, so I agree with you that cleaning up is something that needs to be taken care of from our side...

Comment 29

[David :Bienvenu]

I'm not sure why we have our own temp dir, but I believe we do the same thing on windows, so I don't think it's because of Mac OS 9 (but that's just speculation on my part)

Comment 30

[David :Bienvenu]

Hakan, this is where I would investigate putting your code:

http://mxr.mozilla.org/mozilla/source/mailnews/compose/src/nsMsgAttachmentHandler.cpp#684

The logic would be something like:

check if it's a bundle and needs to be zipped
zip up a temp file
set the attachment content type to the appropriate zip type
run the url to fetch your zipped up temp file

Let me know if that does or doesn't help...

Comment 31

[Håkan Waara]

Attachment: WIP patch 1

I'm very close to have this working. Right now it's zipping any bundle attachment correctly, and I've verified that the Zip file works great locally, but sending it over the wire seems to corrupt it. I think my attachment object has the wrong content type or is munged by MIME or something. Looking into it...

Even though the patch may seem large, 70% of it simply moving the code that today is used for AppleSingle/AppleDouble encoding to its own method.

Comment 32

[Håkan Waara]

I've found the problem. The first few bytes of the zip file are overwritten by something in the send process. I think it has to do with the complex tempfile handling in this process.

Bienvenu, do you know what nsIURLFetcher is about? Is it needed for local files?

Comment 33

[David :Bienvenu]

nsIURLFetcher is used for local files, I believe, to attach the files asynchronously (i.e., avoid blocking the whole UI).

Comment 34

[David Ascher (:davida)]

It'd be great to see this in a2.

Comment 35

[Håkan Waara]

Attachment: Patch v1

I finally got it to work! They last remaining issue was that I didn't properly edit the mURL of the nsAttachmentHandler to point at the new zipped file, which messed things up.

It was almost impossible to not do some improving to the code, except for the zipping. Here are some notes on those changes:

* I've moved the code the appledouble/applesingle encodes attachments into its own function. Ideally I would like that functions in-parameters to be improved and so on, but that requires more work than I have time right now.

* apple-encoded and zipped attachments use the same member variable for its working file. This file is automatically removed when the attachment handler goes away.

* I've removed redundant code in nsMsgSend that did work that the attachment handler's destructor already did.

There is a *lot* more that can be done, but I didn't want to drag too much into this patch, to make it reviewable.

Comment 36

[Håkan Waara]

Here is some more review aid, before I go to sleep:

* Basically the structure is now as follows:

  Decide whether we should apple-encode or zip.

    a) Apple-encoding means going through the same flow as earlier (with some of my touch-ups, but they're minor).
    b) Go through the zip flow - this code is all new.

* As for the changes that involve freeing strings and removal of the temporary file: these should be the same statements as before, so no functional differences expected.

Comment 37

[David :Bienvenu]

Comment on attachment 326589 [details] [diff] [review]
Patch v1

Thx for doing this! It all looks reasonable at first glance. Some nits:

+    if (isBundle) {
+      rv = ConvertToZipFile(macFile);
     }

don't need the braces. Same here:

+  if (isDirectory) {
+    containerPath.AppendLiteral("/");
+  }

I'm a little iffy on putting #pragma mark - in the code

Is there any reason (currently) to not #ifdef MAC_OSX the nsSimpleZipper implementation? If we need it for other platforms, we can always take out the #ifdef.

You can use PR_Free here - we don't need to null out m_type or m_realName since you're assigning to them immediately after (PR_Free checks for null just like PR_FREEIF; the actual difference is that PR_FREEIF nulls the result)
+  PR_FREEIF(m_type);
+  m_type = PL_strdup(APPLICATION_ZIP);
+  PR_FREEIF(m_real_name);
+  m_real_name = PL_strdup(zippedName.get());

Now that you've removed the printf here, you could get rid of the braces and move the comment to above the if:

+      if (sendResourceFork)
+      {
+        // before deciding to send the resource fork along the data fork, check if we have one,
+        // else don't need to use apple double.
+        sendResourceFork = HasResourceFork(&fsSpec);
+      }

No need for braces here:

+    separator = mime_make_separator("ad");
+    if (!separator)
+    {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }

Ugh, I could make a lot more comments about the code that was moved, but I'll restrain myself since you restrained yourself from cleaning it up in order to get through the review quicker :-)

I'll try running with this tomorrow - it would be great if you could attach a patch tomorrow morning addressing these comments, and then I could apply that patch...

Comment 38

[Håkan Waara]

Attachment: Patch v2

Thanks, all comments addressed in this patch. As well as:

* Change the ctor to use initializers, and remove some nsCOMPtrs from there because they're set to null by default.
* Changed all PR_FREEIF (in this file) that was immediately reset to PR_Free

Comment 39

[David :Bienvenu]

Comment on attachment 326653 [details] [diff] [review]
Patch v2

Thx again for doing this. I tried this out and it worked. Some nits:

+  if (isDirectory) {
+    // now add all its contents recursively
+    rv = AddDirectoryContentsToZip(zipWriter, containerPath, aInputFile);
+  }

don't need these braces, if you move the comment above the if ()

and just a note - you also don't need PR_FREEIF if you're freeing a local var buffer and returning right away, e.g.,

+    if (NS_FAILED(rv) || !obj->fileStream)
+    {
+      PR_FREEIF(separator);
+      delete obj;

but I'm not saying you have to fix those :-)

I'm going to ask Neil for an sr since this is a big enough patch that someone else should look at it...

Comment 40

[[e-mail]]

Comment on attachment 326653 [details] [diff] [review]
Patch v2

>+    containerPath.AppendLiteral("/");
General nit: use .Append('/'); for single characters.

Your zipping logic seems somewhat confused. The main issue is that ::Zip always calls AddEntryFile whatever the type of the file is. Fixing this would mean copying more code from ::AddDirectoryContentsToZip. Instead, I would suggest that ::Zip should just open and close the zip, and then you write a method ::AddFileToZip which a) checks for a file or directory b) in the case of the directory, enumerate all the files and call itself recursively.

Comment 41

[Håkan Waara]

(In reply to comment #40)
> (From update of attachment 326653 [details] [diff] [review])
> >+    containerPath.AppendLiteral("/");
> General nit: use .Append('/'); for single characters.
> 
> Your zipping logic seems somewhat confused. The main issue is that ::Zip always
> calls AddEntryFile whatever the type of the file is. Fixing this would mean
> copying more code from ::AddDirectoryContentsToZip. Instead, I would suggest
> that ::Zip should just open and close the zip, and then you write a method
> ::AddFileToZip which a) checks for a file or directory b) in the case of the
> directory, enumerate all the files and call itself recursively.

Actually, it's not confused; addEntryFile does the right thing for directories, which is why I used it: http://developer.mozilla.org/en/docs/nsIZipWriter#addEntryFile.28.29

Comment 42

[[e-mail]]

Comment on attachment 326653 [details] [diff] [review]
Patch v2

>nsresult nsSimpleZipper::Zip(nsIFile *aInputFile, nsIFile *aOutputFile)
>{
>  // create zipwriter object
>  nsresult rv;
>  nsCOMPtr<nsIZipWriter> zipWriter(do_CreateInstance("@mozilla.org/zipwriter;1", &rv));
>  NS_ENSURE_SUCCESS(rv, rv);
>  
>  rv = zipWriter->Open(aOutputFile, PR_RDWR | PR_CREATE_FILE | PR_TRUNCATE);
>  NS_ENSURE_SUCCESS(rv, rv);
>
>  rv = AddToZip(zipWriter, aInputFile, EmptyCString());
>  NS_ENSURE_SUCCESS(rv, rv);
>  
>  // we're done.
>  return zipWriter->Close();
>}

>nsresult nsSimpleZipper::AddToZip(nsIZipWriter *aZipWriter, 
>                                  nsIFile *aFile)
>                                  const nsACString &aPath)
>{
>  // find out the path this file/dir should have in the zip
>  nsCString leafName;
>  aFile->GetNativeLeafName(leafName);
>  nsCString currentPath(aPath + leafName);
>  PRBool isDirectory;
>  aFile->IsDirectory(&isDirectory);
>  if (isDirectory)
>    // append slash for a directory entry
>    currentPath.AppendLiteral("/");
>
>  rv = aZipWriter->AddEntryFile(currentPath, nsIZipWriter::COMPRESSION_DEFAULT, aFile, PR_FALSE);
>  NS_ENSURE_SUCCESS(rv, rv);
>
>  if (isDirectory) {
>    nsCOMPtr<nsISimpleEnumerator> e;
>    nsresult rv = aDirectory->GetDirectoryEntries(getter_AddRefs(e));
>    NS_ENSURE_SUCCESS(rv, rv);
>
>    nsCOMPtr<nsIDirectoryEnumerator> dirEnumerator = do_QueryInterface(e, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);
>
>    nsCOMPtr<nsIFile> file;
>    while (NS_SUCCEEDED(dirEnumerator->GetNextFile(getter_AddRefs(file))) && file) {
>      rv = AddToZip(aZipWriter, file, currentPath);
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  }
>  return NS_OK;
}

Comment 43

[David :Bienvenu]

I suspect hwaara is too busy to address Neil's comments - I'm going to have a quick look at rearranging the code a little per Neil's suggestion, unless hwaara objects... 

Comment 44

[Håkan Waara]

I have a new version of the patch which addressed Neil's and bienvenu's comments. Unfortunately while testing, I noticed that keynote files are not considered to be "bundles"/"packages" by XPCOM, so I had to file and fix that bug too. See bug 442401

In order for this bug to not be blocked by that, let's do the check for package-ness locally in nsMsgAttachmentHandler for now, until the fix for the bug above is landed in CVS or mozilla-central (and Thunderbird has moved there).

Comment 45

[Håkan Waara]

Attachment: Patch v3

Here's a new version of the patch. Changes:

* bug 442401 is worked around by creating a very small file which can correctly check an nsIFile for its packageness. We can create a bug to remove this workaround once XPCOM has the proper fix.

* The zip algorithm has been adjusted to Neil's comments

* Bienvenu's nits have been taken care of

Comment 46

[Håkan Waara]

Thanks for the reviews. I'll check this in tonight.

Comment 47

[Håkan Waara]

Checked in, with an extra bustage fix to avoid using a variable on linux/windows that is only defined on OS X. 

Anyone up for verifying the fix tonight?

Comment 48

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

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2pre) Gecko/2008070803 Thunderbird/3.0a2pre ID:2008070803

Using the given attachment 307796 [details] the keynote folder content gets zipped and is received correctly as zip file. After unpacking the content everything looks identical. As I see there is one remaining issue when having a more closely look at the files:

Original keynote file:
drwxr-xr-x@ 3 henrik  staff    102  6 Mär 21:52 Contents
-rw-r--r--@ 1 henrik  staff  83487  6 Mär 21:52 index.apxl.gz
drwxr-xr-x@ 3 henrik  staff    102  6 Mär 21:52 thumbs

Received keynote file:
drwxr-xr-x@ 3 henrik  staff    102  6 Mär 20:52 Contents
-rwxr-xr-x@ 1 henrik  staff  83487  6 Mär 20:52 index.apxl.gz
drwxr-xr-x@ 3 henrik  staff    102  6 Mär 20:52 thumbs

Each file is packed with the executable flag so we have modified rights afterwards. Shall I file this as a separate bug or is it an already known issue?

Otherwise it is working perfect. Thanks Håkan!

=> Verified

Comment 49

[Håkan Waara]

(In reply to comment #48)
> Each file is packed with the executable flag so we have modified rights
> afterwards. Shall I file this as a separate bug or is it an already known
> issue?

Henrik, please file this as a new bug unless you haven't already. I think all file permissions should be preserved when zipping, unless this is a standard behavior for zipping apps (doubtful).   Thanks a lot for taking the time to verify the functionality.

Comment 50

[David Ascher (:davida)]

I tried to attach a keynote file today, and it refused to send it, complaining that I didn't have access.  Removing the executable bits on the .key directory let the file be zipped up and sent.

Feels like the attachment code is a bit too picky about modes?

Comment 51

[Håkan Waara]

(In reply to comment #50)
> I tried to attach a keynote file today, and it refused to send it, complaining
> that I didn't have access.  Removing the executable bits on the .key directory
> let the file be zipped up and sent.
> 
> Feels like the attachment code is a bit too picky about modes?

That's weird. Please file a bug for that and CC me, thanks!

Comment 52

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

I haven't seen this issue while verifying the fix. David, could you please attach the zipped keynote content to the new bug?