Closed Bug 671440 Opened 13 years ago Closed 12 years ago

Cannot inline ANY image in ANY way (since nsmail-N.tmp files pile up until nsmail-9999.tmp)

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 16.0

People

(Reporter: Bug.Reports, Assigned: Bienvenu)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Hg diff of the proposed correction
(deleted), patch
Bienvenu
: review-
Details | Diff | Splinter Review
cleanup compose temp files
(deleted), patch
standard8
: review+
Details | Diff | Splinter Review
fix for sr
(deleted), patch
Bienvenu
: review+
standard8
: superreview+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

Tried to inline an image into a new message OR a reply, by drag-and-dropping a file from Windows Explorer OR via the Insert -> Image menu option OR by pasting an image previously copid in Paint.
Then tried to save the message in Drafts, either directly or by trying to send it, or by autosave of the draft.

Running TB 5.0 OR TB 5.0b2 OR TB 3.1.17
Win7 64bit Home Premium Service Pack 1



Actual results:

TB reported "Save Draft Error": "Unable to save your message as draft. There was an error attaching [random_name].gif. Please check you have access to the file", and a never ending progress bar behind says "Saving Messages - Test".


Expected results:

The message should have been saved (and sent, if appropriate).
I should add that the problem happens with both IMAP and POP account setting
OS: Other → Windows 7
Hardware: All → x86_64
Sorry, just changed the importance to "Critical", because this bug makes TB UNUSABLE for me: I had to buy Outlook 2010 (which is horrendous)! Really hope you can help me out with this. Thanks!!

Also, I should add that the failure to save the message (or attach the image, if you wish) does NOT add anything to the Error Console.
Severity: normal → critical
Apologies, yet two more precisions:
- I'm using Drafts in Local Folders
- it doesn't make a difference whether the image is referenced as file, data or URL...
Also, I AM ready to try whatever you suggest in order to help you trace this down (Outlook gives me shivers!).
Did you check permissions in your profile for your draft folder ?

Does it work in -safe-mode (see http://support.mozillamessaging.com/en-US/kb/Safe-Mode) ?

Anything in Tools -> Error console ?
Checked the permissions (even tried to relocate the mail folder in the past) - same problem. Note that if I simply drop the IMAGE from the message, I can save it ok into Drafts.

Started in safe mode - same problem.

The Error console contains just two warnings:

Warning: Expected color but found 'menuitem'.  Error in parsing value for 'background-color'.  Declaration dropped.
Source File: chrome://messenger/skin/mailWindow1.css
Line: 470

Warning: Expected color but found 'menuitem'.  Error in parsing value for 'background-color'.  Declaration dropped.
Source File: chrome://messenger/skin/mailWindow1.css
Line: 489
One more info... I tried to run TB "as Administrator" (the new option in Win7) to avoid any permission issues. I can't drag-and-drop images in this mode (don't know why), but I pasted one copied from Paint - same problem (can't attach image).

Thanks for your help!
Still more info (trying to put you on the right track):
When I drag-and-drop a small image into the mail and double-click it, the Image Location contains "data:image/gif;base64,R0lGODlhEwATAHAAACH5BAEAAN...", i.e. the image is inlined as raw data.
When I UNcheck the "Attach this image to the message", and check "Don't use alternate text", I CAN save the message in Drafts. I can even exit TB and restart: the image is still in the message ok... so it clearly WAS saved in Drafts.
It looks then that the actual saving of the image in Drafts is not a problem: it's the "attaching" of the image (whatever THAT means when the image is inlined as raw data!).

But of course the above is not a solution, because when the message is really sent, the image is missing (the receiver does not have the image in the message).
The difference between these two mechanisms is that unchecking the "Attach" box will leave the image as data: URL in the code, thus it's not even attempted to attach the image as "real" multipart/related attachment before saving or sending. In this regard, the part "Please check you have access to the file" of the error message is misleading. The image actually no longer touches the disk after this mechanism was redesigned in bug 490879 and bug 609632, with bug 578104 adding the random name for the attachment, but it appears that somewhat goes wrong in that process of data:-URI to MIME-part conversion.

The error console output refers to a style sheet, so that may not be directly related to this problem. Note that Insert > Image doesn't follow that redesign, thus if it shows the same error, the issue may be more general in the actual attaching or base64-encoding (as a wild guess).
Sorry for the delay, vacations... but still so much hopeful you could help out (Outlook really IS a horrendous ****).

I just tried to Insert > Image from a file. After selecting the image (a SMALL smiley), the Image Properties shows
file:///C:/Text/PERSONAL/Smileys/Heart.gif
in the Image Location, and the image is correctly shown in the Preview (just to make sure TB DOES have access to the file!). I filled Alternate text with "Heart" and tried to save... and AGAIN got the
"Unable to save... There was an error attaching Heart.gif. Please check...etc."
message, with an endless
"Attaching Heart.gif"
progress window behind.

Then I tried something else, namely attaching that same file as an outright attachment, i.e. not INLINED in the text: no problem with that!!

What gives??
More: I tried a complete and CLEAN reinstall. Uninstalled TB, rebooted (just to make sure!), renamed any folder that might have been used by Thunderbird, downloaded the 5.0 installer again, run it in the default (not Custom) install, did NOT import anything, then selected the IMAP mode. All settings were found automatically on the basis of my address (excellent job, guys!!!).
With this, Drafts are on my server, not local, i.e. TB shouldn't even try to write anything locally when saving a draft. Well, saving a pure text draft works ok (i.e. the remote folder IS accessible for writing)... but as soon as I inline an image, it's AGAIN the same "...error attaching..."!!!
Still more: when I try to Save > File, TB correctly saves the message (WITH the image inlined!) as .html... which (IMHO!) means the "Problems attaching..." should not be in the construction of the message itself, but after that. Any ideas, PLEASE?

Just to complete the information already supplied, my machine is an HP Envy - does any of you run on that machine?

P.S. I also tried to run TB as an Administrator (running out of ideas!)... but same thing. :-(
What kind of anti-virus are you running on your machine ?
None (or if you want, MS Security Essentials).

Thank you for following up!
More...
I tried to install the 64bit version (Miramar) instead. Once again into a clean install (no import of anything) and this time I set up the account as POP3 (rather than IMAP)... SAME problem!

Then I selected all in the message with the inlined image, copied and pasted it into another "new" message. Everything was pasted as it should, but this time the image was inlined as raw data (as it should, I presume). But... STILL the same problem trying to save the Draft ("cannot attach image")!

What exactly happens during the save of the Draft AFTER the message was clearly created ok? Where could the "Cannot attach" message come from, given that there is NOTHING to be attached (the image is inlined as raw data)? It is THERE that the bug must be, IMHO.

Thanks for following up!
(In reply to Emanuel from comment #14)
> None (or if you want, MS Security Essentials).

What happens if you disable it !
(In reply to Ludovic Hirlimann [:Usul] from comment #16)
> (In reply to Emanuel from comment #14)
> > None (or if you want, MS Security Essentials).
> 
> What happens if you disable it !

No change (still unable to save an inlined image to Drafts).
OK I don't tell you to switch to linux. I'm just saying here that it does work with Thunderbird 6.0 on Linux Mint 11 x64.

This is to say that there's something up that is different between the windows and linux versions of TB...
I have had this problem for many versions of Thunderbird over the past few YEARS, including the latest and greatest release. Pasting an image in to a new message appears to work - the image shows up, but it is stripped before the recipient gets it.

I bet it has something to do with filenames, since it apparently works on Linux and I use Windows.

Inline images can be received but not sent, except forwarding a message with inline messages does work, provided you don't edit the message too much!
I'm under Linux Ubuntu 11.10, Thunderbird 10.0.2, Gmail IMAP.
My Gmail under Thunderbird is 100% IMAP, nothing in local (all folders in the setings of the account are IMAP, no copy, no save ...)

Same problem with images in HTML mode.

If i create a mail, i send it directly : no problem
If the mail is from the DRAFTS imap folder, the link is break.

Thunderbird is working ... no crash ... but this bug is CRITICAL and i find some messages about this bug here since 2009 ... :/  VERY CRITICAL ! cant use images with IMAP in 2012 (only with gmail?) !
I have now installed TB on a brand new machine (Win7 Pro-64), and it works flawlessly... so far: if I remember well, it also worked for some time on this machine as well, and then stopped inlining images for reasons unknown.

Now I must say that TB is a REMARKABLE mailer, much better than anything else I've seen, so I would really like to have it working again on this machine - so much so that I'm willing to contribute by rebuilding the whole thing and catching the bug.
I have a perfectly working MinGW C/C++, and a functional GDB - can anybody confirm that with that, I should be able to build TB from sources (I've downloaded thunderbird-9.0b5.source.tar.bz2)? And, if so, would somebody be willing to help me out with it, given that I'm very much a Win developer?
Many thanks in advance!
(In reply to Emanuel from comment #21)

> I have a perfectly working MinGW C/C++, and a functional GDB - can anybody
> confirm that with that, I should be able to build TB from sources (I've
> downloaded thunderbird-9.0b5.source.tar.bz2)? And, if so, would somebody be
> willing to help me out with it, given that I'm very much a Win developer?
> Many thanks in advance!
Sorry for the dealy in replyin I'm a bit under a load of bugmail :-)

So the source code is available via HG, our instructions for building Thunderbird are at https://developer.mozilla.org/en/Simple_Thunderbird_build - With that you should be able to setup a working build environment in windows and do all your debugging in windows.

There are two place were you can find  seasoned developers to help, one is quicker than the other and it's on irc on #maildev (ie this link will bring you there : https://chat.mibbit.com/?server=irc.mozilla.org&channel=%23maildev). Ask Bienvenu NeilAway or Standard8.

The other one is the newsgroup/mailing list mozilla.dev.apps.Thunderbird (see http://www.mozilla.org/about/forums/).

If you need any help setting things up just email me directly , answer will be faster.
Dear Ludovic,

So many thanks for your input! I'll give it a try as soon as I'll have some time (me too I'm submerged!).

Now a really dumb one for you (and others, I guess): it may sound stupid, but I actually have no idea what "HG" means (it suggests "Human Growth" or @High Ground@... "Head Guarters" to me! :-D ). I realize that there is really a world apart between the "Win morons" like me, and the rest of you in the Linux/GNU Country. But I do appreciate your efforts, and am willing to learn, so wouldn't it be a great idea to compile a sort of DICTIONARY for the dumb ones here that would define all the abbreviations that seem so obvious in your Universe? It might make a bestseller, actually! As a byproduct, it could make the "Win morons" here even more appreciative of your efforts, which I guess would be a positive outcome for you.
As far as I'm concerned, I have already been really STUNNED by the performance of code built in GNU (MinGW), in particular the 64-bit port. Being in what you might call "High Performance Computing", a 30% speedup gained by a simple rebuild of the same code means A LOT to me. I spent countless hours porting my Borland sources to GNU (no easy feat, if you count in threading in particular) just to have the 64 bits, but since then, whatever I write withOUT VLC (32 or 64) will be built in GNU. And Thunderbird is another example of your great work: clearly the best mailer I know.

Btw: I'm not on IRC (sorry for that: no time for it!), so I'm afraid you might hear from me again... but I'll try hard to be as self-sufficient as possible.

Thanks again,

Emanuel
:-) sorry for the delay again. It's the way I go thru email :-) HG stands for Mercurial it's where our source code is.

If you read the wiki link we have detailed instructions on how to work on windows we even provide a download to make things easy.
Hello,

At last trying to build TB... Followed Ludovic's advice to download the sources, install all the MSVC stuff and MozillaBuild, as well as pymake. Also created .mozconfig with

ac_add_options --enable-debug in it.

The installation appears to be ok.

But when trying to build, I'm getting compile errors of "No such file or directory" on headers, which I was painstakingly trying to correct by copying the required headers where they could be found by the compiler.
All of the errors however appear to be due to the fact that the build is using MSVC:

C:\mozilla-build>start-msvc9.bat
"Mozilla tools directory: C:\mozilla-build\"
Visual C++ 9 Express directory: C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\
Windows SDK directory: C:\Program Files\Microsoft SDKs\Windows\v6.0A\
Windows SDK version: 6.0A
Setting environment for using Microsoft Visual Studio 2008 x86 tools.
Mozilla build environment: MSVC version 9.

I have a fully functional MinGW32 installed on this machine (in C:\MinGW), and would like the build to use that environment instead of MSVC (to eliminate the above errors and for later debug with gdb) - how can I make the build use MinGW instead of MSVC?

Many thanks in advance for your help!
BUG FOUND (as well as a workaround): PLEASE CORRECT IT!


It took me three days(!) to build TB from sources, but the effort was worth every penny: I managed to identify the bug (took just about two hours to find it), and it clearly lingers in TB since years over its successive versions... and the "funny" part is that even completely reinstalling TB does NOT help! Here it is:

To handle inline images,
nsMsgAttachmentHandler::SnarfAttachment(nsMsgCompFields *compFields) in nsMsgAttachmentHandler.cpp
calls
nsMsgCreateTempFile("nsmail.tmp", getter_AddRefs(tmpFile))
at one point, which FAILS on one of my machines. Here is WHY:

nsMsgCreateTempFile(const char *tFileName, nsIFile **tFile) in nsMsgCompUtils.cpp
tries to create a unique file in C:\Users\[WinUser]\AppData\Local\Temp, based on "nsmail.tmp".
To do so, it calls
nsLocalFile::CreateUnique(PRUint32 type, PRUint32 attributes) in nsLocalFileCommon.cpp
to produce a file name that doesn't exist yet... which loops up to 10000 times to find a new filename.

BUT the temp directory (C:\Users\[WinUser]\AppData\Local\Temp) already contains the files
nsmail.tmp
nsmail-1.tmp
...and up to...
nsmail-9999.tmp!

Consequently, nsMsgCreateTempFile fails (reporting "disk full"!)... and everything goes into boonies, with "Cannot save as draft" as end-result.


So here is my modest contribution to the TB endeavor:

- The Workaround:
Trivial: delete all nsmail*.tmp from C:\Users\[WinUser]\AppData\Local\Temp

- The Correction:
That will clearly be a bit trickier: I thought SnarfAttachment simply forgot to delete the temp file after usage, but that doesn't seem to be the case, for even on the machine where the 10000 nsmail*.tmp files did accumulate somehow, there was NO nsmail*.tmp when I closed TB. Clearly, in SOME conditions (but not always?), the delete fails... with the obvious consequences down the road. Clearly, the best would be to correct the delete failure, or at least warn the user when it fails(!) so that they would clear their temp manually.
But IMHO there is one efficient solution to the accumulation of temp files, namely
- create all temps in a dedicated SUBDIR of the temp, say C:\Users\[WinUser]\AppData\Local\Temp\thunderbird
- clear up completely that dedicated temp on each TB start.
This would also solve other problems I see coming already: the temp already contains HUNDREDS of nscopy*.tmp and nsemail*.eml files - it's just a question of time before something else fails lamentably.

I'll leave the correction to the Accredited Developers, not willing to fiddle with the code in ways that might not be conform to the established standards - many thanks in advance!


I would like to give a big THANKS GUYS! to Ludovic Hirliman and, especially, Siddharth Agarwal for their great help while building TB - if there is some reward system in place, please do give them both five stars from me!
Summary: Cannot inline ANY image in ANY way → Cannot inline ANY image in ANY way (since nsmail-N.tmp files pile up until nsmail-9999.tmp)
FYI.

(In reply to Emanuel from comment #26)
> BUT the temp directory (C:\Users\[WinUser]\AppData\Local\Temp) already
> contains the files
> nsmail.tmp
> nsmail-1.tmp
> ...and up to...
> nsmail-9999.tmp!
> Consequently, nsMsgCreateTempFile fails (reporting "disk full"!)... and
> everything goes into boonies, with "Cannot save as draft" as end-result.

It's bug 673703.
See bug 389132 for CPU hog when TMP is not filled by "-1" to "-9999" yet.
See bug 235432 for issue of remained garbage of -NNNN.tmp.
Duplicate of bug 673703 per Emanuel's (reporter's) comment 26, and Wada's comment 27.

(In reply to Emanuel from comment #26)
> BUG FOUND (as well as a workaround): PLEASE CORRECT IT!
> I'll leave the correction to the Accredited Developers, not willing to
> fiddle with the code in ways that might not be conform to the established
> standards - many thanks in advance!

Emanuel, being a volunteer contributor myself, I believe devs will appreciate any help that you can provide to improve the code. The truth is, we have hundreds of clearly documented bugs and RFEs, but the Mozilla TB team is too small to fix them. So in your own interest, if you don't want this to languish for more years, please do feel encouraged to fix it yourself! Developers will be more than happy to support you with reviews of your code. And it sounds like you absolutely have the skills required to do a very good job here!
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Attached file Patched to delete temp-file after usage (obsolete) (deleted) — 
Makes sure temp-files used for attachments are deleted after use. Only the SnarfAttachment function was modified, the two modifications are clearly marked with "Emanuel Falkenauer, 14MAY12" should they be removed again for some reason.
Ok, I had a look at how to correct the bug, and it turns out that SnarfAttachment actually does forget to delete the temp-file it creates, an act of mild negligence IMHO.
I have attached above a proposal for correction of the issue, but I have no idea how to submit the thing for possible inclusion in future TB releases - would you help out, please?
I have tested the correction and it appears to be ok: the .tmp is deleted as required. However, I noticed that it only is delete when the calling thread finishes, so I don't know what happens if TB crashes. In any case, my correction is clearly marked should it for some reason turn out to be bad in any way.
That being said and done, it's clear that the above corrects just ONE place where temp-files are created and left behind - there are possibly others. I do believe that the only way of solving this (persistent!) issue of temps once and for all, is to clear ALL temps at TB startup (see Comment 26).
Such a cleanup should take place the first thing after TB determined that no other TB copy is running... but I have to say I have no time nor the means of writing that (I'm just running MSVC Express - I don't even know where the startup code is). Please have a look at that, it would make TB much more robust.

Thanks,

Emanuel
I don't know how you pulled the source code, but if it was with mercurial, you can do an hg diff of the file and just attach the diff/patch file for review.
Just asking for review of the proposed correction
Attachment #623799 - Attachment is obsolete: true
Attachment #623877 - Flags: review?(Bug.Reports)
Hi David,

Ok, I tried my best (see Comment 33), but please understand that I just arrived to bugzilla - in particular, I have no idea what flags to put on the attachment. I put a "?" in Review, hope it's ok.
Let's reopen this given that the proposed patch works around bug 673703 for mailnews specifically by not letting the nsmail-*.tmp files pile up.
Assignee: nobody → Bug.Reports
Status: RESOLVED → REOPENED
Component: Message Compose Window → Composition
Ever confirmed: true
Product: Thunderbird → MailNews Core
QA Contact: message-compose → composition
Resolution: DUPLICATE → ---
Comment on attachment 623877 [details] [diff] [review]
Hg diff of the proposed correction

While basically correct, you have requested review from yourself. I'm changing this to David instead, using the e-mail address from comment #32.

Most likely, the #if preprocessor statements (and the #else part) should go, but you can wait for any other comments before posting a new patch: 

> +#if 1  // Emanuel Falkenauer, 14MAY12
> +#else
>    return fetcher->FireURLRequest(mURL, mTmpFile, mOutFile, FetcherURLDoneCallback, this);
> +#endif
Attachment #623877 - Flags: review?(Bug.Reports) → review?(dbienvenu)
OK, thanks!
Comment on attachment 623877 [details] [diff] [review]
Hg diff of the proposed correction

thx for the patch - we don't use try catch in our c++ code because of the overhead.

I'm not convinced you can remove the mTmpFile right after FireUrlRequest because I believe that's asynchronous, but I'll check.
> thx for the patch - we don't use try catch in our c++ code because of the
> overhead.

Ok.

> I'm not convinced you can remove the mTmpFile right after FireUrlRequest
> because I believe that's asynchronous, but I'll check.

Ok - if FireUrlRequest IS asynchronous, then it will have to be instructed to delete mTmpFile itself when finished, because once SnarfAttachment exits, there is no way left to access it as it's local to that function (and the temp-file would stay behind as it does now).
Comment on attachment 623877 [details] [diff] [review]
Hg diff of the proposed correction

Yeah, FireURLRequest is asynchronous, and we need the tmp file to send the message, so you can't remove the temp file there. The tmp file gets removed when the attachment handler gets deleted, if mDeleteFile is true. If we have a reproducible test case, then we should figure out why the code in the nsMsgAttachmentHandler destructor isn't getting called.
Attachment #623877 - Flags: review?(dbienvenu) → review-
I'd like to see code that cleans up the temp files. Doing this on startup is probably safest and most reliable, so we don't have to worry about some code actually using the temp file. We could limit this to once per day or something if it turns out to be hard to write code to cleanup temp files w/o pinging the disk a little.
> Yeah, FireURLRequest is asynchronous, and we need the tmp file to send the
> message, so you can't remove the temp file there.

Ok, that follows.

> The tmp file gets removed
> when the attachment handler gets deleted, if mDeleteFile is true.

I revisited and, indeed, the tmp file is normally removed in the destructor. But then I tried to see the detail of the removal and saw strange things happening. Now PLEASE forgive me if I'm being completely wrong (I'm just trying to help) - the code is pretty complex and I'm not even sure the debugger shows me things correctly. But anyway, I find the following strange indeed:

- nsMsgAttachmentHandler::~nsMsgAttachmentHandler() indeed calls
mTmpFile->Remove(false)
- entering nsLocalFile::Remove(bool recursive), I can see that this->mWorkingPath contains the correct absolute path of the file to be removed. BUT
- on return from the call to
rv = IsSymlink(&isLink);
the value of this->mWorkingPath is CHANGED (why would it change on a simple test whether the path is a symbolic link??), to garbage most of the time
- and then the test
if (NS_FAILED(rv))
right after IsSymlink is sometimes satisfied, with
RemoveDirectoryW(mWorkingPath.get())
never being reached.

If the above can help you nail down the problem, then I'd be quite happy. To me, it looks like a nasty stack corruption in IsSymlink, but as I said, I might be wrong completely. To be sure, I tried to find out WHY IsSymlink would change the mWorkingPath but couldn't figure it out (which is why I believe it might well be a stack corruption).

> If we have
> a reproducible test case, then we should figure out why the code in the
> nsMsgAttachmentHandler destructor isn't getting called.

From my tests above, I believe that it IS called as many times as there are temps to be removed, but sometimes it fails to remove the file.
(In reply to David :Bienvenu from comment #41)
> I'd like to see code that cleans up the temp files. Doing this on startup is
> probably safest and most reliable, so we don't have to worry about some code
> actually using the temp file. We could limit this to once per day or
> something if it turns out to be hard to write code to cleanup temp files w/o
> pinging the disk a little.

Excellent suggestion, IMHO: nsMsgAttachmentHandler may not be the only place (or even one of the places) where temp files are created yet never removed, and a startup-time cleanup would solve ALL those problems once and for all (see my suggestion in Comment 31).
mWorkingPath can change when the path is "resolved", iirc. Does this bug recreate for you whenever you send a message with attachments, so that the tmp files are left around? What does the garbage look like?
Bug fully REPRODUCIBLE at last!

0. Close TB, delete by hand all ns*.* in Temp, start TB
1. Create a New message (Ctrl-N) with FOUR inlined images, two of which are immediately one after the other (e.g. a repeat of a smiley-gif)
2. Save the message with Ctrl-S:
   nsmail.tmp
   nsmail-1.tmp
   nsmail-2.tmp
   nscopy.tmp
   nsemail.eml
are created in the Temp. Note that only THREE nsmail*.tmp's are created: the two consecutive images must have been merged into a single temp-file
3. Close the New Message window: the  temps above are deleted ok, after some delay

BUT then

4. Open TB's Drafts folder: the message is in the list
5. Double-click the message for re-edition (but don't change anything)
6. Save the message with Ctrl-S again:
   nsmail.tmp
   nsmail-1.tmp
   nsmail-2.tmp
   nscopy.tmp
   nsemail.eml
are again created in the Temp
7. Close the New Message window: the temps are NOT DELETED.

And to make sure

8. Open TB's Drafts folder again: the message is in the list
9. Double-click the message for another re-edition (but don't change anything)
10.Save the message with Ctrl-S again:
   nsmail-3.tmp
   nsmail-4.tmp
   nsmail-5.tmp
   nscopy-1.tmp
   nsemail-1.eml
are created in the Temp on top of the previous ones still lingering there
11.Close the New Message window: the new temps are NOT DELETED either... leaving at this point ten temp-files in the Temp.

Additional notes:
- closing TB completely after either step 7. or 11. still doesn't make the temps disappear from the Temp
- sending the message (rather than just closing the window) at step still doesn't remove the temp-files.


Now repeating the above with a message containing just THREE inlined images SEPARATED by text does NOT yield the same outcome: in that case, the temps ARE deleted in steps 7. and 11., as they should.

So IMHO, this is what is happening: during REedition (but not a new composition), CONSECUTIVE inlined images are not properly handled for removal of the temp-files used to inline the images.
thx for finding STR's. yeah, every draft save in this situation leaks temp files. I'll poke around a bit,
The fix for bug 299655 might solve this issue too.
A modest hint, perhaps:

With correctly handled inline images, this eventually removes the temps:
- CALLED: nsMsgCompose::~nsMsgCompose()
- CALLED: nsMsgComposeAndSend::Release()
- CALLED: nsMsgComposeAndSend::~nsMsgComposeAndSend()  -> removes the temps

With badly handled inline images, this eventually happens:
- CALLED: nsMsgCompose::~nsMsgCompose()
- NEVER CALLED: nsMsgComposeAndSend::Release()
and so nsMsgComposeAndSend::~nsMsgComposeAndSend() is never called (you were right, David!).

It looks to me like RE-editing a message does not create the data that are created (and subsequently destroyed by nsMsgCompose::~nsMsgCompose(), thereby calling the Release) when a NEW message is composed, but that's just a guess.

Unfortunately, the spot where nsMsgComposeAndSend::Release() is called in the OK case and should be (but is not) called in the KO case, is impenetrable for somebody not intimate with the code, so I can't go further than that. Hope you catch the nasty one!
Unfortunately, patch for bug 299655 does not seem to help here. Though maybe it's necessary but not sufficient? I'll poke around.
Attached patch cleanup dup multipart related attachments (obsolete) (deleted) — — Splinter Review 
Hiro, what do you think of this patch? This is similar to your patch for bug 299655, in that it breaks the cycle by clearing the mMsgSend reference, but it has the advantage of clearing it when the draft save/send is done, which means if we crash sometime later before the window is closed, we won't leave temp files around.  This patch also handles the case where we have duplicate mime related parts, and leak temp files. The reason we were leaking is that the attachment handler holds a reference to the msgSend object, but that reference never gets cleared if we don't try to fetch the url, which we don't do for dups. The fix has some redundancy built-in: only one of the SetMimeDeliveryState(nsnull) is really needed to fix the test case, but the code is complicated enough that I'm not sure of all the code paths, so the redundancy is probably safer (it's certainly harmless).

I had to change your unit test for bug 299655 because the temp files are cleared immediately after the draft is saved, so we can't check that they still exist.
Assignee: Bug.Reports → dbienvenu
Attachment #626597 - Flags: review?(hiikezoe)
I'd still like to figure out how to cleanup temp files in an efficient way, preferably one that doesn't iterate over all the temp files, which could be expensive. I could check for the existence of the temp files to be leaked, and if I find one, keep incrementing the counter on the temp file name until the temp file doesn't exist. Maybe I'll try to code that up.
Comment on attachment 626597 [details] [diff] [review]
cleanup dup multipart related attachments

Review of attachment 626597 [details] [diff] [review]:
-----------------------------------------------------------------

Basically this patch makes sense to me.

I am just anxious about accessing mMsgSend directly (and also setter).

::: mailnews/compose/public/nsIMsgCompose.idl
@@ +213,5 @@
>    */
>    boolean checkCharsetConversion(in nsIMsgIdentity identity, out string fallbackCharset);
>  
> +  /* the message send object */
> +  attribute nsIMsgSend messageSend;

I'd prefer to add a method named 'finishMsgSend' or 'clearMsgSend' and set mMsgSend null in the method like clearEditor does. So it is bit clearer in the call side rather than setting null.

::: mailnews/compose/test/unit/test_temporaryFilesRemoved.js
@@ +24,5 @@
> +    if (iid.equals(Ci.nsIWebProgressListener) ||
> +        iid.equals(Ci.nsISupportsWeakReference) ||
> +        iid.equals(Ci.nsISupports))
> +      return this;
> +

Though this code is written by me, it should be written with XPCOMUtils.generateQI?
Attachment #626597 - Flags: review?(hiikezoe) → review+
Attached patch cleanup compose temp files (deleted) — — Splinter Review 
this makes the compose service cleanup temp files as best it can, and adds a unit test for it.  This patch is still a bit intertwined with the prev patch (in particular, in xpcshell.ini) but we can unwind that later.
Comment on attachment 626648 [details] [diff] [review]
cleanup compose temp files

cleanup temp files used by compose window on compose service startup. This will add three extra checks for file existence on the first compose window startup, but for users with extra temp files, it will be a big speedup because they might end up doing hundreds of stats for every attachment to find a free file name.
Attachment #626648 - Flags: review?(mbanner)
Attached patch fix for sr (deleted) — — Splinter Review 
this addresses hiro's comments, carrying forward r+, asking for sr for minor interface change.
Attachment #626597 - Attachment is obsolete: true
Attachment #626996 - Flags: superreview?(mbanner)
Attachment #626996 - Flags: review+
Comment on attachment 626648 [details] [diff] [review]
cleanup compose temp files

Review of attachment 626648 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just one minor nit.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +1398,5 @@
> +nsresult MsgCleanupTempFiles(const char *fileName, const char *extension)
> +{
> +  nsCOMPtr<nsIFile> tmpFile;
> +  nsCString rootName(fileName);
> +  rootName.Append(".");

nit: use single quotes as this is just a character
Attachment #626648 - Flags: review?(mbanner) → review+
Attachment #626996 - Flags: superreview?(mbanner) → superreview+
http://hg.mozilla.org/comm-central/rev/dda57271e770
http://hg.mozilla.org/comm-central/rev/017015de681d
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Blocks: 389132
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: