507137 - attachment of .eml file can't be read by stream because of wrong size information

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Tobias Koenig]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6) Gecko/2009020409 Iceweasel/3.0.11 (Debian-3.0.11-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.2pre) Gecko/20090729 Shredder/3.0b4pre

When trying to access the attachment of an external .eml file by any stream class (e.g. from within java script), the size of the attachment is always 0 bytes.
The reason for that is, that nsDummyMsgHeader is used which has its messageSize property always initialized with 0.

This report provides a patch that fixes it by passing the url of the .eml file to the appropriated message header sink, which setup a nsDummyMsgHeader with the file size as messageSize.

Reproducible: Always

Steps to Reproduce:
1. Open an external .eml file that contains attachments
2. Try to open a stream on the attachment and read from it

Actual Results:  
The reported size of the attachment is 0 bytes, although the attachment is not empty.

Expected Results:  
If the attachment is not empty, the content should be readable.

Comment 1

[Tobias Koenig]

Attachment: Patch that sets the correct attachment size

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 391339 [details] [diff] [review]
Patch that sets the correct attachment size

roc has not yet seen the light and come over to the glamorous world of Thunderbird development from layout and what not.  Bienvenu is probably a better reviewer for this ;)

Comment 3

[David :Bienvenu]

Sid, how does this relate to your idea of making the dummy message header a separate component?

Comment 4

[Rain [:rain1] (inactive)]

One step at a time ;) -- though as I mentioned on IRC, it would be nice to have tests for this, and that practically necessitates a separate component. Do you think tests are necessary here before the patch gets checked in?

Comment 5

[David :Bienvenu]

No, whatever feature ends up needing this can have mozmill tests for now...

Comment 6

[David :Bienvenu]

Comment on attachment 391339 [details] [diff] [review]
Patch that sets the correct attachment size

thx for the patch, this looks reasonable, but you should add doxygen style comments to the method you added to nsIMsgHeaderSink, and explain what the url parameter is (specifically that it should be a file url).

Also, can you use let instead of var here:

+      var msgHdr = new nsDummyMsgHeader();
+  
+      var io = Components.classes["@mozilla.org/network/io-service;1"]
+                         .getService(Components.interfaces.nsIIOService);
+
+      var url = io.newURI(pathUrl, null, null);
+      var fileUrl = url.QueryInterface(Components.interfaces.nsIFileURL);

Or, you could do this all in one statement, without the temp vars.

Comment 7

[Tobias Koenig]

Attachment: Updated patch that addresses previous remarks

Changed the js code to use 'let' instead of 'var' and add documentation to the new method getDummyMsgHeader in the IDL file

Comment 8

[Tobias Koenig]

Attachment: Do not overload the method name

Renamed GetDummyMsgHeader to CreateDummyMsgHeaderFromURL to avoid conflicts during binding generation for the interface.

Comment 9

[David :Bienvenu]

Comment on attachment 392515 [details] [diff] [review]
Do not overload the method name

thx, this looks OK, with a few nits:

any reason not to use ACString in the idl instead of string?

Also, this comment should be of the 
/**
 * @param url ...
 *
 * @returns
 */

form, of which there are a lot of examples in the existing code...

+  // returns a dummy message header that has its messageSize property set to the size
+  // of the .eml file that the passed url points to

Comment 10

[Tobias Koenig]

Attachment: Adapt to review comments

Changed comments to doxygen style and make use of ACString instead of string as input parameter type.

Comment 11

[David :Bienvenu]

Comment on attachment 393481 [details] [diff] [review]
Adapt to review comments

either this is the wrong patch, or I'm the wrong reviewer, since I'm not a toolkit peer.

Comment 12

[Tobias Koenig]

Attachment: Adapt to review comments

Fixed documentation and passing ACString instead of string in IDL file.

(This time the correct patch ;))

Comment 13

[David :Bienvenu]

Comment on attachment 395009 [details] [diff] [review]
Adapt to review comments

looks good, thanks.

Comment 14

[Ludovic Hirlimann [:Usul]]

David does this patch needs sr too ?

Comment 15

[David :Bienvenu]

technically, yes. And it also needs the corresponding suite change to suite/mailnews/msgHdrViewOverlay.js, now that I think of it. For that, you'd probably want to ask for sr from [e-mail], or standard8, who can review the suite change and give you sr at the same time.l

Comment 16

[Ludovic Hirlimann [:Usul]]

Comment on attachment 395009 [details] [diff] [review]
Adapt to review comments

Choosing neil as standard8 is swamped in triagging

Comment 17

[[e-mail]]

Do we really need to create a new message header each time?

Comment 18

[David :Bienvenu]

This should only happen when opening a .eml file, which shouldn't be that common...

Comment 19

[[e-mail]]

Comment on attachment 395009 [details] [diff] [review]
Adapt to review comments

>         if (headerSink)
>-          return headerSink->GetDummyMsgHeader(aMsgHdr);
>+        {
>+          nsCAutoString pathName;
>+          m_filePath->GetNativePath(pathName);
>+          pathName.Insert("file://", 0);
>+
>+          return headerSink->CreateDummyMsgHeaderFromURL(pathName, aMsgHdr);
In which case rather than this bogus URI creation code I would just get the dummy msg header, get the size of m_filePath and set the size on the header.

Comment 20

[Tobias Koenig]

(In reply to comment #19)
Hej Neil,

> In which case rather than this bogus URI creation code I would just get the
> dummy msg header, get the size of m_filePath and set the size on the header.

Well, sid0 suggested to have this encapsulated inside the dummy message header class... So what way to go?

Comment 21

[Tobias Koenig]

Attachment: Simplified patch

The new version of the patch moves the code to set the message size of the dummy header from the callee site to the caller site as requested by the super review.

Comment 22

[[e-mail]]

Comment on attachment 396186 [details] [diff] [review]
Simplified patch

>+
>+          return rv;
>+        }
>       }
>     }
>   }
>   else
>     rv = NS_ERROR_NULL_POINTER;
> 
>   return rv;
> }
[It's probably safe to fall through to the end of the function to return rv.]

Comment 23

[David :Bienvenu]

Comment on attachment 396186 [details] [diff] [review]
Simplified patch

I fixed Neil's nit, and a couple formatting issues, and landed this. Thx very much for the patch!