356808 - Thunderbird silently ignores attachments if a file using the same name exists in moz_mapi folder (sends wrong / old / stale / previous version of attachment instead!)

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[stoilis]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla Thunderbird 1.5.0.7 Build 20060909

If you are trying to indirectly attach a file outside thunderbird(using SimpleMAPI, for example: right-clicking on a file on Windows Explorer and choosing "Send To" -> "Mail Recipient") and a stale different file using the same name already exists in the thunderbird's temporary mapi attachment folder (in my computer this is "C:\Documents and Setting\myusername\Local Settings\Temp\moz_mapi") then Thunderbird silently ignores the new attachment and instead wrongly uses the one already found it moz_mapi directory.

This can cause users to send e-mail with the wrong attachment in them, if there are stale files in moz_mapi directory.

Reproducible: Always

Comment 1

[Mike Cowperthwaite]

Reproduced with TB 1.5.0.7, 2b1-1014, 3a1-1011, Win2K.

I'm not sure what conditions lead to the file being left in that temp directory -- every time I tested this, the temp file was no longer present after sending the message.

Comment 2

[stoilis]

There is a special case where stale files are created with another bug I submitted, bug 356919

Comment 3

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #1)
> I'm not sure what conditions lead to the file being left in that temp directory

At least "R" attribute of sent file will produce the phenomenon on MS Win.
See Bug 356919 Comment #3, please. 

Comment 4

[Daniel Colascione]

We worked around this bug by making moz_mapi an NTFS junction point for the parent temporary directory.

Comment 5

[Tim Retout]

This is probably a bug in HandleAttachments in mozilla/mailnews/mapi/mapihook/src/msgMapiHook.cpp: pTempFile is created with a unique filename, but leafName is not updated with the new unique value. So the original file is copied to the default leafname (which fails silently), and then the old version of the file is attached.

We need something like this at the end of the relevant if block:

pTempFile->GetLeafName (leafName);

Comment 6

[Kent Bower]

This is broken for me as well (I've got TB3beta1 and 3beta2 both installed)
This seems to work with 3b1 but is broken in 3b2

Comment 7

[Kent Bower]

(In reply to comment #6)
> This is broken for me as well (I've got TB3beta1 and 3beta2 both installed)
> This seems to work with 3b1 but is broken in 3b2

Sorry, in my case, 3b2 simply ignores the SimpleMAPI altogether - it isn't a case of it attaching the wrong file.

Comment 8

[Michel Dessaintes]

This very annoying bug is still in TB3b2.

Comment 9

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

This might be dataloss if the wrong file was inadvertently attached and/or sent.

Comment 11

[Daniel Colascione]

Indeed, it was data loss for us. Customers ended up getting the wrong quote letters a few times. (Man, was that embarrassing!)

Using an NTFS junction point to make the moz_mapi folder an alias for the profile temporary folder fixes the issue in the short term, but it's a nasty hack.

Comment 12

[Darescu Ionut]

when is this bug going to be fixed?

Comment 13

[Robert Sander]

We are currently experiencing this bug with our users.

It is very annoying as they try to send out different versions of documents from our DMS (i.e. the same filename but different content) and end up with the same version everytime.

The sending user does not see any difference when composing the email.

Comment 14

[Thomas D. (:thomas8)]

I'm aware mapi is microsoft-related, but does this bug only affect windows systems? Otherwise platform/OS should be changed accordingly.

Comment 16

[David Ascher (:davida)]

Attachment: tested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name)

This is just a patch version of comment #5, but I neither know this code nor know how to repro it. If someone can repro this bug easily with nightly builds, we could do a try-server build.  (which doesn't mean we'd take the patch in the tree at this point, as our test coverage in this area is bad, I fear)

Comment 17

[Phil Lacy]

http://mxr.mozilla.org/comm-central/source/mailnews/mapi/mapihook/src/msgMapiHook.cpp#560
557              {
558                rv = pTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0777);
559                NS_ENSURE_SUCCESS(rv, rv);
560                pTempFile->Remove(PR_FALSE); // remove so we can copy over it.
561              }
562 
I think the intent of this was to rename the file with name we want and then delete the original but it is only deleting the new named old file so the later CopyTo() fails because it still exists.

I don't know why we want to keep temp files anyway.  If someone is using this the old file then it won't rename anyway I don't think.
lets just delete the old file and copy the new one in.

Comment 18

[Wayne Mery (:wsmwk)]

bug 77810 related?

this may help alleviate Bug 389132 -  100% CPU for long time when forwarding multiple messages as attachment, if many garbages of nsmail-N.tmp remain in \Temp or /tmp directory

Comment 19

[Phil Lacy]

Attachment: Using original file for attachment

This eliminates 'send to' creating a temp file which, BTW, never gets deleted.

If you create a message compose and drag and drop or add attachment any other way it uses the original file so why not this method as well.

FWIW, OExp uses the original file also in its 'send to'.

Comment 20

[[e-mail]]

My understanding was that we made a copy of the file because "Send To" is a synchronous API and we don't own the file after the function returns; OExp is able to create a compose window modally to work around the problem.

Comment 21

[Phil Lacy]

(In reply to comment #20)
> My understanding was that we made a copy of the file because "Send To" is a
> synchronous API and we don't own the file after the function returns; OExp is
> able to create a compose window modally to work around the problem.

Can we lock the file while under control of composer?

If not then I can do a patch to fix the bug and keep the temp file.

Comment 22

[[e-mail]]

(In reply to comment #21)
> (In reply to comment #20)
> > My understanding was that we made a copy of the file because "Send To" is a
> > synchronous API and we don't own the file after the function returns; OExp is
> > able to create a compose window modally to work around the problem.
> Can we lock the file while under control of composer?
Not as far as I know.

> If not then I can do a patch to fix the bug and keep the temp file.
Please do.

Comment 23

[[e-mail]]

Comment on attachment 401576 [details] [diff] [review]
Using original file for attachment

At least with Outlook 2000 (best test I could come up with, sorry), if you use Explorer to Send To, then the window that's opened doesn't let you open other windows; various errors appear when you try. I assume this is so that the MAPI call doesn't return until the window is closed (either via sending, saving or cancelling).

Comment 24

[Phil Lacy]

I'll do a patch 'deleting the old file and renaming the new file'.
note in meantime in case you have comments on these (not necessary):
1. nothing in temp file is deleted, ever (need new bug);
2. this mapi send to procedure will be only one that uses temp files for attachments, drag and drop and adding attachments thru menu both uses original file. (haven't tried but if we can open file on front end with limited shared access and close file when compose window closes, we will be locking file while composer in use). Maybe this would be new bug too.

Comment 25

[Phil Lacy]

Attachment: delete existing if possible or else use new name

tested it to work with windows. 
Does delete existing file with same name in temp directory.pass
In case of file in use: did a python Open( ,'r+) an existing temp file with same name before 'send to' and it returned an error and the 'send to' file was copied to temp dir under unique name. pass

Note, we don't do this with other methods of adding attachments to composer. Only use original files and don't lock them.
Also this code does not delete files in directory when done.

Comment 26

[[e-mail]]

(In reply to comment #25)
> Created an attachment (id=404768)
> Note, we don't do this with other methods of adding attachments to composer.
> Only use original files and don't lock them.
Hmm, so if I use MAPI to send two different files with the same name without closing the compose window in between, then things will break?

Comment 27

[[e-mail]]

Comment on attachment 404768 [details] [diff] [review]
delete existing if possible or else use new name

>-               pTempFile->Remove(PR_FALSE); // remove so we can copy over it.
Don't we need to keep this? By my reading, CopyTo will fail it we don't remove.

>+               pTempFile->GetLeafName(leafName);
So, this is the actual fix, isn't it! If you prefer, I'll gladly r+ a patch with just this fix in it and we can decide what to do about deleting separately.

Comment 28

[Thomas D. (:thomas8)]

(In reply to comment #27)
> (From update of attachment 404768 [details] [diff] [review])
> >-               pTempFile->Remove(PR_FALSE); // remove so we can copy over it.

Can someone explain to me what exactly is removed in this line? Is it deleting an actual existing temp file? Or are we just emptying a path variable for the file name? (If it's a file, where does it come from? If we haven't created it in the same run, this will have similar effect as bug 344034?)

I think we should NOT be deleting any files that we haven't created in the same run.

Comment 29

[Thomas D. (:thomas8)]

I think the patch for this problem needs to meet at least following requirements, if we want to avoid trouble like in bug 344034.

When user attaches file "filename.ext" via mapi:
- try to create immediate snapshot copy of "filename.ext" in moz_mapi/, but
- do not delete any existing temp files of that name (-> bug 344034)
- do not rename any existing temp files of that name (-> bug 344034)
-> make sure to use unique file name that doesn't exist yet

- The unique file name should include original "filename" as substring, e.g. "filename-1.ext" or "filename.847565.ext". It should not be all cryptic like "nsmail1.ext". Watch out for bug 407577 (Thunderbird incorrectly handles Simple MAPI attachments if they have a '-' in the file name).

Comment 30

[Phil Lacy]

(In reply to comment #26)
> Hmm, so if I use MAPI to send two different files with the same name without
> closing the compose window in between, then things will break?

yes, why I say stick with attaching original

regarding other comments questions about code I modified the block dealing with existing temp files of same name. It now deletes the file, if that fails it creates new unique file and whichever filename is successful that is the one used for the copyto.

Comment 31

[Phil Lacy]

(In reply to comment #29)
> - do not delete any existing temp files of that name (-> bug 344034)

good luck.  Our temp files are never deleted. Maybe on TB first use we can empty our directory.  OEXP and IE seem to have notice problem here as they never seem to empty their directories, but just expect user to 'clean disk' with those directories in the cue so to speak.

Comment 32

[Wayne Mery (:wsmwk)]

(In reply to comment #31)
> (In reply to comment #29)
> > - do not delete any existing temp files of that name (-> bug 344034)
> 
> good luck.  Our temp files are never deleted. Maybe on TB first use we can
> empty our directory.  

it is also a security issue. see Bug 58979 comment 13  store all compose temp files in directory under /tmp, and remove that directory on quit and  Bug 377630 -  Attachment filename disclosure in /tmp 

OT perhaps, but the wider cluster of these bugs (which needs some cleanup) is 
 Bug 235432 Mailnews leaves unused nsqmail.tmp (nsqmail-*.tmp) files in temporary folder after quit
 Bug 389132 -  100% CPU for long time when forwarding multiple messages as attachment, if many garbages of nsmail-N.tmp remain in \Temp or /tmp directory
 Bug 368380 -  lots of temp files left around
 Bug 74047 -  use a subdirectory under the temp directory to hold all the compose temp files
 Bug 220179 -  Temporary files in mail folders on hard drive are not removed if compact interruped 
 Bug 299655 -  /tmp files left behind, some world-readable
 Bug 392592 -  Temporary files used to save mail attachments are not deleted when canceling in file picker
 Bug 58580 -  temp files from sending drafts or posting news are create with bad permissions


> OEXP and IE seem to have notice problem here as they
> never seem to empty their directories, but just expect user to 'clean disk'
> with those directories in the cue so to speak.

indeed, but a bit optimistic to assume users will do that :)

Comment 33

[Phil Lacy]

I use sendto of same file name on daily basis and can't live with bug in TB3. This should be +block TB3. And least problematic would be send-original-file patch.

Most windows users with business application of TB will be in same boat and that means dataloss at *best*, and at worse, scenarios of unintentional sending of confidential files with generic names. report.pdf or employeereview.txt. etc.
Problems similar to previous comments will be popping up all over if something isn't in TB3.
Sorry for bug spam but we have a -TB3blocking.

Comment 34

[[e-mail]]

Comment on attachment 404768 [details] [diff] [review]
delete existing if possible or else use new name

Call me a coward, but I think we should only delete our new temporary file for now.

Comment 35

[Thomas D. (:thomas8)]

(In reply to comment #30)
> (In reply to comment #26)
> > Hmm, so if I use MAPI to send two different files with the same name without
> > closing the compose window in between, then things will break?
> 
> yes, why I say stick with attaching original

Phil, even attaching the original file (i. e. "attach original later when sending") will not solve the current problem of your patch. It will still break the scenario you describe in comment #33, when user wants to send 2 versions of same file.

Scenario/Steps, why "attach later when sending" doesn't work:

- via mapi (e.g. from within winword or explorer) user attaches c:\test.txt with content "hello world" -> 1st compose window
-> In 1st compose win, TB only has a link to c:\test.txt and does not keep a tmp copy
-> in the time before sending, a lot of things can happen so that file cannot be retrieved when sending (renamed, removed, deleted etc. see list below).
- user does changes to C:\test.txt, content is now "modified text"
- via mapi, user sends same file again -> 2nd compose window
-> In 2nd compose win, TB only has a link to c:\test.txt and does not keep a tmp copy
- user now sends both mails
-> both mails will have original "C:\test.txt" attached upon sending, i.e. both mails will have "modified text"

So from user's point of view, two *different* versions of file have been attached, but in reality TB has two mails with the same link to the original file, and will attach only the second/last version to both mails upon sending
-> dataloss, unwanted leak of information (breach of privacy)

Generally speaking, attaching the original file (i. e. "attach original source file later when sending") has a lot of problems. To name but a few:
- source file might have been renamed by the time you actually send
- source file might have been deleted
- source file might have been on removable media which is gone
- source file might have changed and user might send sensitive data without knowing, because the changes weren't there when the file was attached
- can't compose with two different versions of same file simultaneously

Therefore, after long discussion in bug 378046, developers have requested to use "attach snapshot immediately" as default (see bug 378046, comment #38). This is what TB currently does for mapi attachments.

> regarding other comments questions about code I modified the block dealing with
> existing temp files of same name. It now deletes the file, if that fails it
> creates new unique file and whichever filename is successful that is the one
> used for the copyto.

As pointed out by Neil's and my comments, deleting existing files that you haven't just temporarily created yourself in the same process will always break things. I think the desired patch just needs very few lines, and few changes compared to your current patch, as per Neil's comment #27 and comment #34.

Comment 36

[Thomas D. (:thomas8)]

Comment on attachment 404768 [details] [diff] [review]
delete existing if possible or else use new name

Phil, it would be great if you could change your current patch like this:
(please let me know if I overlooked sth or suggest sth. wrong)

>-            // rename or copy the existing temp file with the real file
> name

Please re-include this line ("this needs to stay"), although I'm not sure if this is accurate description of what actually happens

>+            // delete any existing temp file with the same file name
>+            // if error on delete, create unique filename

Please remove this change ("this needs to go.")

>-               rv = pTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE,
> 0777);

this needs to stay

>+               rv = pTempFile->Remove(PR_FALSE);
>+               if (NS_FAILED(rv))
>+                 rv = pTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE,
> 0777);

these lines need to go

>-               pTempFile->Remove(PR_FALSE); // remove so we can copy
> over it.

this needs to stay

>+               pTempFile->GetLeafName(leafName);

this is the patch

Could you try if this fixes the problem? I'd be curious to know because I don't have testing environment to test myself.

Comment 37

[Thomas D. (:thomas8)]

Comment on attachment 401481 [details] [diff] [review]
tested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name)

(In reply to comment #36)

Well, so basically that leaves us with the one-line change proposed bei Neil in comment #27. Which is basically the one-line change proposed by David Ascher in attachment 401481 [details] [diff] [review]. Has this ever been tested?

Comment 38

[Thomas D. (:thomas8)]

Comment on attachment 404768 [details] [diff] [review]
delete existing if possible or else use new name

As per comment #26 to comment #30, this patch will break when using mapi to compose two mails with different versions of same source file. It's also review- per Neil's comment #34.

Comment 39

[Phil Lacy]

Neil
The first untested patch is ok to go in. We will now have to deal with bugs asking why attachments using sendto have "-1" or some number appended to their filenames and why the other methods of attaching files do not.
But this patch does fix the bug so better this then nothing.

Comment 40

[Thomas D. (:thomas8)]

(In reply to comment #39)
> Neil
> The first untested patch is ok to go in. We will now have to deal with bugs
> asking why attachments using sendto have "-1" or some number appended to their
> filenames and why the other methods of attaching files do not.

Phil, you mean that not only the temp file in moz_mapi, but also the display name of attachments in attachment panel will show the "-1"? I thought that tmp-filname and display name are two separate things, now that we can use context menu to rename attachments in Tb3?

Comment 41

[Phil Lacy]

(In reply to comment #40)
> Phil, you mean that not only the temp file in moz_mapi, but also the display
> name of attachments in attachment panel will show the "-1"? I thought that
yes that is what happens. There would have to be another bug for handling the naming of the attachment file in the front end.

Comment 42

[Phil Lacy]

sorry but I omitted in my comment 39 that I did test that patch and it fixes this bug

Comment 43

[stoilis]

Considering how the name change may disturb many people, myself included, how about creating a new directory of a random name inside the temporary moz_mapi directory, that will contain the filename unchanged?

I'am not sure how this will complicate things though, any ideas?

Comment 44

[Thomas D. (:thomas8)]

Phil, thanks for your feedback.
(In reply to comment #41)
> (In reply to comment #40)
> > Phil, you mean that not only the temp file in moz_mapi, but also the display
> > name of attachments in attachment panel will show the "-1"? I thought that
> yes that is what happens. There would have to be another bug for handling the
> naming of the attachment file in the front end.

Could you actually *test* if our unique tmp filename (file-1.ext) really gets promoted into the front end file name?
If yes, could you post an mxr link where the display names are set, and a few notes how our temp file name gets there?

What I find is that renaming the attached file in the front-end does NOT change the tmp file name in moz_mapi. So we definitely have two separate entities (tmp file name vs. display file name), perhaps you can find out how to keep them separate from the start and promote only the original file name to the front end? That would be great.

(In reply to comment #43)
> Considering how the name change may disturb many people, myself included, how
> about creating a new directory of a random name inside the temporary moz_mapi
> directory, that will contain the filename unchanged?
> I'am not sure how this will complicate things though, any ideas?

It would work, but surely that's not what we want. Considering that we are not yet cleaning up the files, one extra tmp subfolder for each attachment would add a lot of clutter to the mess.

Comment 45

[Thomas D. (:thomas8)]

Tim Retout, you seem to know this code (comment #5). Your comment fixes the main bug, but now there is a new problem since our unique tmp file name (like "attachment-1.doc") gets promoted to the front end, but what we want is just "attachment.doc". However, user can rename file from frontend, so there must be a separate entitiy for the display name. Any ideas where to look or how to fix this remaining problem?

Comment 46

[Phil Lacy]

We should have this patch blessed and landed in order to cure the more serious problem of dataloss. Another bug can be filed for the names. I think that will be in front end code anyway, not mapi code.

Comment 47

[Thomas D. (:thomas8)]

Agreed. Let's get the basic patch in to fix dataloss, and privacy issue which is probably worse, when wrong version with sensitive data gets sent to wrong recipient.

Neil's comments from last review have been addressed.

Neil, could you review this?

Comment 48

[Thomas D. (:thomas8)]

Comment on attachment 401481 [details] [diff] [review]
tested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name)

Neil, could you review the first patch provided by David Ascher?

The patch fixes the dataloss and privacy issue where wrong versions of files get sent; however, in these cases the filename of the attachment in the attachment pane will be changed to sth. like attachment-1.doc. If User doesn't like that, he can rename the file from attachment pane, back to attachment.doc, without breaking anything data-wise.

I think we should stamp out the dataloss and privacy issue for 3.0 and accept the display name issue until 3.1.

Comment 49

[Thomas D. (:thomas8)]

For judging the severity of this bug, please consider that we will /always/ send the wrong file whenever user composes, via mapi, two different mails to which he attaches two different files of the same name (one each, even from different locations), or two different versions of the same file (probably, yet not confirmed, only in cases where the second mail incl. attachment is composed before first mail is sent).

Comment 50

[[e-mail]]

Comment on attachment 401481 [details] [diff] [review]
tested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name)

>+               pTempFile->GetLeafName (leafName);
Nit: unnecessary space before (

Comment 51

[[e-mail]]

(Sorry for the delay but my email server is down and it's not so easy to work out from bug queries which bugs to look at.)

Comment 52

[Phil Lacy]

Attachment: fixes name

this should fix the name but my static build is screwed up so I'll test later and reset flags if this doesn't get checked in first.

Comment 53

[Phil Lacy]

Comment on attachment 416332 [details] [diff] [review]
fixes name

verified this fixes the same name problem.

Comment 54

[[e-mail]]

Comment on attachment 416332 [details] [diff] [review]
fixes name

Some nits:

>+            nsCOMPtr<nsIMsgAttachment> attachment = do_CreateInstance(NS_MSGATTACHMENT_CONTRACTID, &rv);
>+            NS_ENSURE_SUCCESS(rv, rv);
>+            attachment->SetName(leafName);
Can you add a blank line after here please?

>              nsCOMPtr <nsIFile> pTempFile;
>              rv = pTempDir->Clone(getter_AddRefs(pTempFile));
>              if (NS_FAILED(rv) || (!pTempFile) )
>                return rv;
> 
>              pTempFile->Append(leafName);
>              pTempFile->Exists(&bExist);
>              if (bExist)
>              {
>                rv = pTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0777);
>                NS_ENSURE_SUCCESS(rv, rv);
>                pTempFile->Remove(PR_FALSE); // remove so we can copy over it.
>+               pTempFile->GetLeafName (leafName);
This still doesn't need a space before the (

>              }
A blank line after here would be nice.
Better still, outdent these 13 lines to match the lines before and after.

Comment 55

[Phil Lacy]

Attachment: nit fixes to whitespace on patched function only

forward r+=neil

Comment 56

[[:Aureliano Buendía]]

checkin-needed?

Comment 57

[Phil Lacy]

Attachment: fix bug plus fixed 'some' existing whitespace

r+=Neil

Comment 59

[Phil Lacy]

Comment on attachment 416489 [details] [diff] [review]
fix bug plus fixed 'some' existing whitespace

this function needs reformatting of the all the lines. and the file isn't much better. I'll do it if ok'd

Comment 60

[David :Bienvenu]

Comment on attachment 416489 [details] [diff] [review]
fix bug plus fixed 'some' existing whitespace

Thx for the patch. I know this is just moved code, but can you lose the space between nsCOMPtr and <nIFile, and lose the () around !pTempFile, and the space after the close paren.


+            nsCOMPtr <nsIFile> pTempFile;
+            rv = pTempDir->Clone(getter_AddRefs(pTempFile));
+            if (NS_FAILED(rv) || (!pTempFile) )

Comment 61

[Phil Lacy]

(In reply to comment #60)
> (From update of attachment 416489 [details] [diff] [review])
> Thx for the patch. I know this is just moved code, but can you lose the space
> between nsCOMPtr and <nIFile, and lose the () around !pTempFile, and the space
> after the close paren.
> 
> 
> +            nsCOMPtr <nsIFile> pTempFile;
> +            rv = pTempDir->Clone(getter_AddRefs(pTempFile));
> +            if (NS_FAILED(rv) || (!pTempFile) )

David, it didn't match the rest of the function formatting, should I fix all the format nits in this function (see previous patch), or just this one.

Comment 62

[Phil Lacy]

Attachment: updated per sr for checkin

whitespace etc. 
r+=neil
sr+=bienvenu

Comment 63

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/fa4c75b79bf6

Comment 66

[Phil Lacy]

Comment on attachment 417400 [details] [diff] [review]
updated per sr for checkin

bug 356808 comment 33 is reasoning for maintenance fix. 
This is not data loss it is data misrepresentation.
We would be better off breaking mapihook then letting the bug hang around. 
I have users emailing me asking for a way to patch.

Comment 67

[Phil Lacy]

sorry, meant to add.
This patch will apply as is to 1.9.1 and nsIMsgAttachment that is referenced in this patch has not changed

Comment 70

[Mark Banner (:standard8)]

Checked into 1.9.1: http://hg.mozilla.org/releases/comm-1.9.1/rev/a6b19b17aeae

Comment 73

[Thomas D. (:thomas8)]

This bug should have its own automated testcase (to be created).

IIRC, there was considerable uncertainty about the code and the patches, and it's about dataloss / breach of privacy, so it's really worth the work of creating the test.