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!)

VERIFIED FIXED in Thunderbird 3.1a1

Status

--
major
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: giannis, Assigned: philbaseless-firefox)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dataloss, helpwanted, privacy})

Thunderbird 3.1a1
dataloss, helpwanted, privacy
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 -
in-testsuite ?

Thunderbird Tracking Flags

(thunderbird3.0 .2-fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Updated

12 years ago
Version: unspecified → 1.5

Comment 1

12 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

12 years ago
There is a special case where stale files are created with another bug I submitted, bug 356919
(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. 
Blocks: 344034
No longer depends on: 344034
Blocks: 356919, 405717
No longer depends on: 356919, 405717
Assignee: mscott → nobody

Comment 4

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

Comment 5

10 years ago
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

10 years ago
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

10 years ago
(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

10 years ago
This very annoying bug is still in TB3b2.
This might be dataloss if the wrong file was inadvertently attached and/or sent.
Flags: blocking-thunderbird3?
Keywords: dataloss
Duplicate of this bug: 508052

Comment 11

10 years ago
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

10 years ago
when is this bug going to be fixed?

Comment 13

10 years ago
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.
Flags: blocking-thunderbird3? → blocking-thunderbird3-
Keywords: helpwanted
Whiteboard: [student-project]

Updated

10 years ago
Flags: wanted-thunderbird3?
I'm aware mapi is microsoft-related, but does this bug only affect windows systems? Otherwise platform/OS should be changed accordingly.
Severity: normal → major
Keywords: privacy
Version: 1.5 → Trunk
Duplicate of this bug: 405717
Summary: Thunderbird silently ignores attachments if a file using the same name exists in moz_mapi folder → Thunderbird silently ignores attachments if a file using the same name exists in moz_mapi folder (sends wrong / old / stale / previous version of attachment intead!)
Created 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)

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)
(Assignee)

Comment 17

9 years ago
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.
Assignee: nobody → philbaseless-firefox
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
Blocks: 389132
No longer blocks: 405717
(Assignee)

Comment 19

9 years ago
Created attachment 401576 [details] [diff] [review]
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'.
Attachment #401576 - Flags: review?(neil)
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.
(Assignee)

Comment 21

9 years ago
(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.
(Reporter)

Updated

9 years ago
Summary: Thunderbird silently ignores attachments if a file using the same name exists in moz_mapi folder (sends wrong / old / stale / previous version of attachment intead!) → 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!)
(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.
Attachment #401576 - Flags: review?(neil) → review-
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).
(Assignee)

Comment 24

9 years ago
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.
(Assignee)

Comment 25

9 years ago
Created attachment 404768 [details] [diff] [review]
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.
Attachment #401481 - Attachment is obsolete: true
Attachment #401576 - Attachment is obsolete: true
Attachment #404768 - Flags: review?(neil)
(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 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.
(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.
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).
(Assignee)

Comment 30

9 years ago
(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.
(Assignee)

Comment 31

9 years ago
(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.
(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 :)
(Assignee)

Comment 33

9 years ago
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 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.
Attachment #404768 - Flags: review?(neil) → review-
(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 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 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?
Attachment #401481 - Attachment description: untested patch → untested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name)
Attachment #401481 - Attachment is obsolete: false
Attachment #401481 - Flags: review?(neil)
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.
Attachment #404768 - Attachment is obsolete: true
(Assignee)

Comment 39

9 years ago
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.
(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?
(Assignee)

Comment 41

9 years ago
(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.
(Assignee)

Comment 42

9 years ago
sorry but I omitted in my comment 39 that I did test that patch and it fixes this bug
(Reporter)

Comment 43

9 years ago
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?
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.
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?
(Assignee)

Comment 46

9 years ago
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.
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?
OS: Windows XP → All
Hardware: x86 → All
Version: Trunk → 3.0
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.
Attachment #401481 - Attachment description: untested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name) → tested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name)
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 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 (
Attachment #401481 - Flags: review?(neil) → review+
(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.)
(Assignee)

Updated

9 years ago
Assignee: philbaseless-firefox → david.ascher
(Assignee)

Comment 52

9 years ago
Created attachment 416332 [details] [diff] [review]
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.
(Assignee)

Comment 53

9 years ago
Comment on attachment 416332 [details] [diff] [review]
fixes name

verified this fixes the same name problem.
Attachment #416332 - Flags: review?(neil)
(Assignee)

Updated

9 years ago
Attachment #401481 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Assignee: david.ascher → philbaseless-firefox
Attachment #416332 - Flags: review?(neil) → review+
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.
(Assignee)

Comment 55

9 years ago
Created attachment 416374 [details] [diff] [review]
nit fixes to whitespace on patched function only

forward r+=neil
Attachment #416332 - Attachment is obsolete: true
Attachment #416374 - Flags: review+
checkin-needed?
(Assignee)

Updated

9 years ago
Attachment #416374 - Flags: review+
(Assignee)

Comment 57

9 years ago
Created attachment 416489 [details] [diff] [review]
fix bug plus fixed 'some' existing whitespace

r+=Neil
Attachment #416374 - Attachment is obsolete: true
Attachment #416489 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Duplicate of this bug: 533933
(Assignee)

Updated

9 years ago
Attachment #416489 - Flags: superreview?(bienvenu)
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 59

9 years ago
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

9 years ago
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) )
Attachment #416489 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 61

9 years ago
(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.
(Assignee)

Comment 62

9 years ago
Created attachment 417400 [details] [diff] [review]
updated per sr for checkin

whitespace etc. 
r+=neil
sr+=bienvenu
Attachment #416489 - Attachment is obsolete: true
Attachment #417400 - Flags: superreview+
Attachment #417400 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [student-project]
Checked in: http://hg.mozilla.org/comm-central/rev/fa4c75b79bf6
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: wanted-thunderbird3?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1a1
Duplicate of this bug: 539902
Duplicate of this bug: 539905
(Assignee)

Comment 66

9 years ago
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.
Attachment #417400 - Flags: approval-thunderbird3.0.1?
(Assignee)

Comment 67

9 years ago
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
(Assignee)

Updated

9 years ago
Attachment #417400 - Flags: approval-thunderbird3.0.1? → approval-thunderbird3.0.2?
Duplicate of this bug: 540149

Updated

9 years ago
Duplicate of this bug: 543011
Attachment #417400 - Flags: approval-thunderbird3.0.2? → approval-thunderbird3.0.2+
Checked into 1.9.1: http://hg.mozilla.org/releases/comm-1.9.1/rev/a6b19b17aeae
status-thunderbird3.0: --- → .2-fixed
Duplicate of this bug: 543700
Duplicate of this bug: 537826
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.
Flags: in-testsuite?
Duplicate of this bug: 545108
Status: RESOLVED → VERIFIED
Keywords: verified-thunderbird3.0
Duplicate of this bug: 546875
Duplicate of this bug: 548095
Duplicate of this bug: 548350
Duplicate of this bug: 548528
Duplicate of this bug: 545205
Duplicate of this bug: 397482
Duplicate of this bug: 319714
Duplicate of this bug: 554644
Duplicate of this bug: 554644
You need to log in before you can comment on or make changes to this bug.