22090 - Can't send mac files to other users -- AppleDouble attachments not correctly encoded on send (or save)

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[pmock]

Build Date & Platform Bug Found:
 MacOS commercial seamonkey build 1999-12-17-10-m12 installed on G3/400 OS 8.6

Overview Description:
 In Communicator 4.7, I sent a message with an attached a sample quicktime movie.
When I save the attachment, I no longer can open the file attachment in the
quicktime movie player.  I open the message in Bbedit text editor and see that
the attachment was not decoded.

I will attach a sample quicktime attachment to this bug.

Steps to Reproduce:
1) In 4.7 Mac RTM, send a message to yourself with a small quicktime attachment.
   I used a Rich text message
2) In 5.0, retrieve the mail message
3) Click on the paperclick icon in the message header and select the attachment
   The save attachment dialog appears
4) Type in a name and save the attachment
5 [details]) Drag the file on top of the quicktime movie player
   Error message appears.

Actual Results:
 After saving the quicktime attachment, I drag the file on top of the movie
player application.  I receive the error message "Couldn't open the file '...'
because the file is not a movie file".

If I view the contents of the save file, I see the following base 64 data:
--------------ad74AFCB70D2079C195F8C68F4
Content-Type: application/applefile
Content-Transfer-Encoding: base64

AAUWBwACAAAAAAAAAAAAAAAAAAAAAAAAAAUAAAADAAAAVgAAAAwAAAAJAAAAYgAAACAAAAAI
....
....
Qm8AEkJvABBCbwAOIGsAGiBQMBBTQGcOU0BnEFNAZxIMU2FtcGxlIE1vdmlldGFsbC5zcmNp


--------------ad74AFCB70D2079C195F8C68F4
Content-Type: video/quicktime; name="Sample Movie"
Content-Transfer-Encoding: base64
Content-Disposition: inline; filename="Sample Movie"

AAAI5G1vb3YAAAjcY21vdgAAAAxkY29temxpYgAACMhjbXZkAAAPa3ictVd7mBtVFT+T7
...
...
--------------ad74AFCB70D2079C195F8C68F4--

Expected Results:
 I expect the quicktime movie attachment to be decoded in its native format.


Additional Builds and Platforms Tested On:


Additional Information:
 Bug found while testing bug 2895
 This problem does not occur with Microsoft Word documents created on the
macintosh.

Comment 1

[pmock]

Changing qa assigned to [e-mail]

Comment 2

[pmock]

Attachment: Sample Quicktime movie that comes with System 8.5

Comment 3

[pmock]

fyi,
I get the same results saving from IMAP and POP3.

Comment 4

[rhp (gone)]

You did "Save As..." didn't you? If not, just bounce it back.

- rhp

Comment 5

[jefft]

Sounds like a soft of applesingle/appledouble fun problem. <not>

Comment 6

[jefft]

We probably don't know how to deal with applesingle/appledouble in 5.0. This
soulds like a M15 kind of tasks.

Comment 7

[pmock]

Rich,

The attachment Save As dialog on the Mac is entitled "Save". Jeff stop by and
found that Communicator 4.7 sent the attachment as apple single. If I send the
attachment from 5.0, then it encoded as base 64 and we decode it correctly.
Jeff thinks we may not support yet decoding apple single attachment.

Comment 8

[rhp (gone)]

Oh, ok...this makes sense. Thanks. I have code in the back end to deal with
appledouble files. Everything should be ok, except for one call that was a Mac
specific call that gave some info on files. I have to get that ported and I bet
this might work....maybe :-)

I'll take this one back.

- rhp

Comment 9

[rhp (gone)]

I know what has to be done here...just have to get the time to do it.

- rhp

Comment 10

[rhp (gone)]

This should be a beta issue.

Comment 11

[Phil Peterson]

PDT thinks we'd give PDT+ to reading, but we'd ship beta1 without being able to 
send appledouble

Comment 12

[rhp (gone)]

I really think this is important, but I was the only one before so I am moving 
to M17.

- rhp

Comment 13

[rhp (gone)]

Hi Steve,
Here is the bug for you to enjoy ;-)

- rhp

Comment 14

[selmer (gone)]

Adding plus and putting in M21 bucket for sustaining to help.

Thanks Steve!!

Comment 15

[rhp (gone)]

*** Bug 53120 has been marked as a duplicate of this bug. ***

Comment 16

[Phil Peterson]

Not holding PR3 for this; marking nsbeta3-. Adding rtm keyword since it's data
corruption

Comment 17

[selmer (gone)]

Steve, what's progress on this bug?  The bar moves up every day and there's not
any kind of patch in the bug yet...

Comment 18

[Jean-Francois Ducarroz]

Rich, what do we still have to do? Can I help?

Comment 19

[rhp (gone)]

The modules to do the AppleDouble encoding are in the compose back end and 
pretty well named. I got them pretty much ported into the new world order and 
the code is actually in the back end for handling these files (its in ifdefs 
since its pretty mac specific). My speed on Mac development is just hampered, 
but I could help you out on what needs to be done.

- rhp

Comment 20

[selmer (gone)]

*** Bug 56294 has been marked as a duplicate of this bug. ***

Comment 21

[(not reading, please use [e-mail] instead)]

saving these AppleDouble attachments also fail.

is this a dogfood issue for mac users?

Comment 22

[Simon Fraser [no longer active]]

Yes. I send Mac files by email, either to myself or others, several times per 
week.

Comment 23

[lchiang]

selmer talked to sdagley today - this won't get fixed, I think, for rtm. 

Comment 24

[Simon Fraser [no longer active]]

In that case we need to relnote, and put in some kind of defensive dialog/silent 
failure to avoid Mac users trying to send corrupted files all over the net.

Comment 25

[kmurray1115]

Cool. Can we RelNote this and get it off the rtm radar?

Comment 26

[(not reading, please use [e-mail] instead)]

triaging.  not going to be done for rtm.

my apologies to the mac users.

Comment 27

[selmer (gone)]

Adding relnotertm keyword.  Simon, did you open a new bug for any changes you
want in lieu of this fix?

Comment 28

[rhp (gone)]

*** Bug 57348 has been marked as a duplicate of this bug. ***

Comment 29

[Simon Fraser [no longer active]]

I didn't open another bug. Do people think that some kind of warning dialog, or 
silent refusal to attach, is justified?

Comment 30

[Simon Fraser [no longer active]]

Mozilla 0.9 this puppy. This is a dogfood issue for mac users.

Comment 31

[Mike Pinkerton (not reading bugmail)]

not being able to send files to people is a fairly fundamental dogfood issue for
me, and most users, I would think.

why hasn't this gotten any attention? I have to go back to 4.x to send files to
people. Why should I ever then go back to ns6?

Comment 32

[selmer (gone)]

rtm-, just not making it without even a patch.  UI freeze is past.

Jean-Francois, this was given to the wrong owner, are you interested in this one?

Comment 33

[Jean-Francois Ducarroz]

Sure, accepting

Comment 34

[scottputterman]

nominating nsbeta1.

Comment 35

[(not reading, please use [e-mail] instead)]

samir told me that he's got some code that we may be able to use for this.

samir, comments?

Comment 36

[scottputterman]

changing milestone to unknown.  It will get changed back when we figure out what
milestone to put this bug in.

Comment 37

[Samir Gehani]

The AppleSingle encoder and decoder classes I mentioned live at:
mozilla/xpinstall/packager/mac/ASEncoder/src/nsAppleSingleEncoder.[h,cpp]
mozilla/xpinstall/src/nsAppleSingleDecoder.[h,cpp]

You can use these classes to encode a file if it has a resource fork (Boolean
nsAppleSingleEncoder::HasResourceFork(FSSpec *)) and decode a file if it is in
AppleSingle format (Boolean nsAppleSingleDecoder::IsAppleSingleFile(FSSepc *)).

You can pull the native FSSpec out of an nsILocalFileMac.

Comment 38

[scottputterman]

moving to mozilla0.8 and nsbeta1+

Comment 39

[fenella]

Change QA-assign to [e-mail]

Comment 40

[Jean-Francois Ducarroz]

Attachment: New file to add to the tree

Comment 41

[Jean-Francois Ducarroz]

Attachment: mozilla/mailnews/base/src/nsDecodeAppleFile.cpp

Comment 42

[Jean-Francois Ducarroz]

Attachment: mozilla/mailnews/base/public/nsISaveMsgListener.idl

Comment 43

[Jean-Francois Ducarroz]

Attachment: patch for module mozilla/mailnews/mime

Comment 44

[Jean-Francois Ducarroz]

Attachment: patch for module mozilla/mailnews/mime/emitters

Comment 45

[Jean-Francois Ducarroz]

Attachment: patch for module mozilla/mailnews/compose

Comment 46

[Jean-Francois Ducarroz]

Attachment: patch for module mozilla/mailnews/base

Comment 47

[Jean-Francois Ducarroz]

The fix is about sending AppleDouble attachment from Macintosh
(mailnews/compose) and receiving Apple double attachment from any platform
(mailnews/mime, mailnews/mime/emitters and mailnews/base). The fix also include
some emitters' cleanup.

Comment 48

[(not reading, please use [e-mail] instead)]

it all look good to me, r=sspitzer

but I'd like a mac expert (like sfraser) to review it, since the bulk of the
code is all mac code.

nsISaveMsgListener is mac specific, but you've added it to all three builds,
even though the implementation is mostly stubbed out with XP_MAC.  will
nsISaveMsgListener ever be extended to do stuff for the other platforms? 
Having it on all three platforms does make the code cleaner, but will we pay for
it on win32 or linux at run time or at download time (bloat)? 

one minor thing, use the stack based nsAutoString and not nsString in 
nsMsgIsMacFile().

excellent work ducarroz, mac users must have been dying with out this.

Comment 49

[Jean-Francois Ducarroz]

nsISaveMsgListener is also used by other platform for the base class
(nsIStreamListener). This class use to be called nsSaveAsListener but didn't
have an IDL. I have added the IDL in order to have a clean access to it from
mime and have just added those 3 Mac specific functions.

Sure I will une an nsAutoString in nsMsgIsMacFile(). Thanks

Comment 50

[David :Bienvenu]

shouldn't we be using an nsCOMPtr here?

 nsISaveMsgListener *saveListener = nsnull;
+          outputListener->QueryInterface(NS_GET_IID(nsISaveMsgListener), (void
**)&saveListener);
+          if (saveListener)
+

looks like this is a ref-cnt leak here because saveListener is never released.

Comment 51

[Jean-Francois Ducarroz]

good catch. I have replaced this piece of code in both place it was used by:

nsCOMPtr<nsISaveMsgListener> saveListener = do_QueryInterface(outputListener);
if (saveListener)
{
...
}

Comment 52

[David :Bienvenu]

Should nsMimeBaseEmitter::GetOutputListener
 addref the return variable?

Comment 53

[Simon Fraser [no longer active]]

nsDecodeAppleFile.h
-------------------
Somewhat non-standard include guards (__nsDecodeAppleFile_h). The spec disallows
leading double underscores, I believe. Would prefer as nsDecodeAppleFile_h__.

Should avoid use of nsFileSpec in new code, preferring nsI(Local)File.

Should have a #pragma options align=reset after the applefile struct definitions.


nsDecodeAppleFile.cpp
---------------------
Bad tabbing in this file.
In ProcessAppleDoubleResourceFork::Write(), you assume that you'll get all the
AppleDouble headers in the first chunk. I don't think the order of entries
in the AppleSingle data is guaraneteed, so this seems fragile (esp. when you
may be decoding files sent by other mailers). What if a large resource
fork entry comes first?

Also, you may want to delay the PBSetCatInfoSync(&cipbr); with the file creator/
type
information until the file has been totally written to disk. You don't want
to have an application file lying around with only half of its resource
fork for any period of time.

nsISaveMsgListener.idl
----------------------
Where are the comments?

mimeeobj.cpp
------------
Why not just removed the commented out XP_MAC stuff?

mimei.cpp
---------
More bad tabbing here.

mimemoz2.cpp
------------
Leaking here:
+    nsIStreamListener *outputListener = nsnull;
+    msd->output_emitter->GetOutputListener(&outputListener);
and here:
+      nsISaveMsgListener *saveListener = nsnull;
+      outputListener->QueryInterface(NS_GET_IID(nsISaveMsgListener), (void **)&
saveListener);
(not sure that you fixed both).

mimemult.cpp
------------
More leaks:
+        nsIStreamListener *outputListener = nsnull;
+        msd->output_emitter->GetOutputListener(&outputListener);
and
+          nsISaveMsgListener *saveListener = nsnull;
+          outputListener->QueryInterface(NS_GET_IID(nsISaveMsgListener), (void *
*)&saveListener);

These lines:
+        nsIStreamListener *outputListener = nsnull;
+        msd->output_emitter->GetOutputListener(&outputListener);
+        if (outputListener)
+        {
+          nsISaveMsgListener *saveListener = nsnull;
+          outputListener->QueryInterface(NS_GET_IID(nsISaveMsgListener), (void *
*)&saveListener);
+          if (saveListener)

are better written as:
    nsCOMPtr<nsIStreamListener>  outputListener;
    msd->output_emitter->GetOutputListener(getter_AddRefs(outputListener));
    nsCOMPtr<nsISaveMsgListener>  
saveListener(do_QueryInterface(outputListener));
    if (saveListener)

Note that it's safe to pass null into a do_QueryInterface(), so no test is needed
for a non-null outputListener.

nsMimeBaseEmitter.cpp
---------------------
nsMimeBaseEmitter::GetOutputListener() should be a good XPCOM citizen and AddRef 
its
out param.

nsMsgAppleDoubleEncode.cpp
--------------------------
+  nsString urlStr; urlStr.AssignWithConversion(aUrlString);

Slightly better to use an nsAutoString here.

+  char  *ext = nsMsgGetExtensionFromFileURL(urlStr);

Is 'ext' allocated? Where is it freed?

It also seems that nsMsgIsMacFile() has some arbitrary number of file extensions
it considers as "non-mac" files, and everything else is a Mac file. I'm not sure
what calls this routine, but feel that it would be better to use the Internet 
Config
service to tell us if the resource fork of a file is important, rather than 
guessing
based on the file extension.

nsMsgAppleEncode.cpp
--------------------
Bad tabbing.

nsMsgAttachmentHandler.cpp
--------------------------
Bad tabbing
Use nsILocalFile rather than nsFileSpec.

nsMsgCompUtils.cpp
------------------
nsMsgGetLocalFileFromURL should return a nsILocalFile.

nsMessenger.cpp
---------------
Use nsILocalFiles, not nsIFileSpecs.
Why the totally empty nsSaveAttachmentListener ?

Comment 54

[Jean-Francois Ducarroz]

>Should nsMimeBaseEmitter::GetOutputListener
> addref the return variable?

I would say no because mime doesn't own the listener. In the function
nsMimeBaseEmitter::SetOutputListener, we already don't addref it. I just try to
stay consistent with the existing code. Anyway it's a tricky question.

Comment 55

[Jean-Francois Ducarroz]

I have changed my mind. GetOutputListener
 will now addref the result and therefore I need to use an XPCOMPtr to avoid
leak when calling GetOutputListener
.

Comment 56

[Jean-Francois Ducarroz]

Appart nsFIleSpec, I have addressed all simon's points

Comment 57

[Simon Fraser [no longer active]]

r=sfraser with the changes jfd says he will do.

Comment 58

[David :Bienvenu]

sr=bienvenu, with the changes I and sfraser suggested.

Comment 59

[Jean-Francois Ducarroz]

Fixed and checked in. I will file bug for stuff to cleanup.

Comment 60

[hirata masakazu]

Is this fix related to bug 67553? If so, you need to back this out.

Comment 61

[Jean-Francois Ducarroz]

don't need to back it up as I already have a fix ready to be check in.

Comment 62

[fenella]

Cannot verify this bug until bug 67435 is fixed.

Comment 63

[fenella]

To Esther..

Comment 64

[J Luh]

The fix that added AppleDouble support seems to have introduced a regression.
See bug 70222

Comment 65

[esther]

Using build 2001-05-08 on mac and following the original scenario. I can save 
shows video/quicktime: xmac-type="4D6F6F56"; x-mac-creator="54564F44"; name=" 
123.mov".  Verified.