Last Comment Bug 416284 - Detached attachments should not be able to be re-detached
: Detached attachments should not be able to be re-detached
Status: RESOLVED FIXED
: dataloss, fixed-seamonkey1.1.13, verified1.8.1.18, verified1.8.1.19
Product: MailNews Core
Classification: Components
Component: Attachments (show other bugs)
: Trunk
: All All
: P2 normal with 4 votes (vote)
: mozilla1.9.1a1
Assigned To: Magnus Melin
:
Mentors:
: 333904 438450 444791 463546 463850 (view as bug list)
Depends on: 292385 436869 466016
Blocks: 287939
  Show dependency treegraph
 
Reported: 2008-02-07 20:39 PST by Andrew Schultz
Modified: 2012-06-20 03:06 PDT (History)
19 users (show)
philringnalda: blocking‑thunderbird3.0a1-
philringnalda: blocking‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (11.37 KB, patch)
2008-07-06 12:53 PDT, Magnus Melin
no flags Details | Diff | Splinter Review
WIP (4.20 KB, patch)
2008-07-06 16:41 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
proposed fix, v2 (16.80 KB, patch)
2008-07-07 15:03 PDT, Magnus Melin
philringnalda: review+
neil: superreview+
Details | Diff | Splinter Review
proposed fix, v3 (for checkin) (16.50 KB, patch)
2008-07-21 13:33 PDT, Magnus Melin
no flags Details | Diff | Splinter Review
Branch patch for re-detachment v0.1 (Checkin: Comment 44) (16.40 KB, patch)
2008-08-25 07:58 PDT, Ian Neal
philringnalda: review+
mozilla: superreview+
dveditz: approval1.8.1.18+
kairo: approval‑seamonkey1.1.13+
Details | Diff | Splinter Review
Minimal branch fix, v.1 (2.87 KB, patch)
2008-11-04 22:26 PST, Phil Ringnalda (:philor)
dmose: review+
samuel.sidler+old: approval1.8.1.18+
samuel.sidler+old: approval1.8.1.19+
Details | Diff | Splinter Review

Description Andrew Schultz 2008-02-07 20:39:38 PST
With seamonkey build 2008-02-07-09-trunk, the attachment context menu for a detached attachment is the same as a non-detached attachment.  Most of the options are still appropriate but the attachment certainly be re-detached.  If you do redetach the attachment, it writes a 0-length file, which is particularly disastrous if you overwrite the original detached file.
Comment 1 Magnus Melin 2008-02-08 13:09:06 PST
xref bug 292385
Comment 2 Phil Ringnalda (:philor) 2008-03-27 08:52:17 PDT
Apparently we've never managed to disable a second detach attempt, but is the overwriting with a 0-byte file part new? If so, we'll need to at least relnote that.
Comment 3 Andrew Schultz 2008-03-27 09:29:54 PDT
I think the backend has always been happy to overwrite with nothing in this case.
Comment 4 Bertrand de Pommery 2008-04-06 14:47:32 PDT
Hello,
This bug is the first test I make. I test 1.1.9 US and the bug is always in the process. I wrote about it for the 1.1.2 version on the 07/07/03 (news mozilla.support.seamonkey) and for the 1.1.6 (07/11/13) & 1.1.8  version (08/02/04).
For the deleted function, you write for link "deleted"+ the attachment name (in the frensh version but that can be so). All new other function as save or detach is bloked.
The link for a detached file has the same name of the attachment file. That link  could have as name of link : "detached + attachement name (with the absolute way optionnal).
That detached-link can be detedted by seamonkey and, so, can stop the ulterior process (save or detach).
So we can't lost then attachement file alraedy detached.
Excuse my bad english. I can explain that in french if you want, and Andrew has good understood the bug.
Synserly
Bertrand 

Comment 5 Anand Venkataraman 2008-04-14 10:27:11 PDT
This is a rather serious dataloss bug.  Detached attachments present themselves for re-detatchment in email messages with no cognitively simple way of identifying them as detached.  Upon attempting a re-detachment, the originally detached message is clobbered to 0 bytes.  

I have had to embarrassingly request the sender to re-send attachments more than once due to this bug.

Thanks for looking into this.  Will be glad to provide more info if needed.

&
Comment 6 Gary Kwong [:gkw] [:nth10sd] 2008-04-30 12:36:12 PDT
Moving from wanted‑thunderbird3.0a1+ flag to wanted‑thunderbird3.0a2? flag since code for Thunderbird 3 Alpha 1 has been frozen.
Comment 7 Magnus Melin 2008-06-10 23:03:00 PDT
*** Bug 438450 has been marked as a duplicate of this bug. ***
Comment 8 Bertrand de Pommery 2008-06-16 13:08:03 PDT
Bonjour,
I teste the new version 1.1.10 for Seamonkey : the bug is always present and doublely present : one attachment gives two attachments empty !...
Please do something ! (en français : faites quelque chose !...).

The first link said that there is a problem and create an empty file with the same name that the attachment name, the second link said as in the 1.1.x (x=9 to 2) and create the empty file as discribe.
sincerly
Bertrand
Comment 9 Magnus Melin 2008-07-02 14:26:48 PDT
Taking.
Comment 10 Magnus Melin 2008-07-06 12:53:48 PDT
Created attachment 328268 [details] [diff] [review]
proposed fix

I assume the isExternalAttachment applies to two attachment type: detached attachments and rss enclosures.

Detaching deleted attachments doesn't work, nor detaching deleted attachments. This patch disables the menus according to that. (Fix for thunderbird+seamonkey)
Comment 11 neil@parkwaycc.co.uk 2008-07-06 15:51:57 PDT
Comment on attachment 328268 [details] [diff] [review]
proposed fix

>-  if (("content-type" in currentHeaderData) &&
>-      /application\/x-pkcs7-(mime|signature)/
>-      .test(currentHeaderData["content-type"].headerValue))

>+  return contentType.indexOf("application/pkcs7-mime") >= 0 ||
>+         contentType.indexOf("application/pkcs7-signature") >= 0 ||
>+         contentType.indexOf("application/x-pkcs7-mime") >= 0 ||
>+         contentType.indexOf("application/x-pkcs7-signature") >= 0;
Why did you replace that lovely regexp with that horrible index?

>   var canDetach = CanDetachAttachments();

>+  var canDetach = CanDetachAttachments();
Oops.

>-  if (canOpen)
>-  {
>-    saveMenu.removeAttribute('disabled');
>-  }
>-  else
>-  {
>-    saveMenu.setAttribute('disabled', true);
>-  }
What was wrong with the save code, apart from the bad choice of variable name?

I think you also need to patch FillAttachmentListPopup().
Comment 12 neil@parkwaycc.co.uk 2008-07-06 16:12:14 PDT
I realise that canOpen should have been canSave, but it still took me some time to work out what deletedAll and detachedAll really meant.
Comment 13 neil@parkwaycc.co.uk 2008-07-06 16:41:21 PDT
Created attachment 328278 [details] [diff] [review]
WIP

Right now I'm too tired to work out whether this patch is correct or not.
Comment 14 Magnus Melin 2008-07-07 13:36:34 PDT
(In reply to comment #11)
> >-  if (canOpen)
> >-  {
> >-    saveMenu.removeAttribute('disabled');
> >-  }
> >-  else
> >-  {
> >-    saveMenu.setAttribute('disabled', true);
> >-  }
> What was wrong with the save code, apart from the bad choice of variable name?

Copying code from thunderbird, and a one liner is easier to read. 

Comment 15 Phil Ringnalda (:philor) 2008-07-07 13:55:30 PDT
(In reply to comment #11)
> Why did you replace that lovely regexp with that horrible index?

Ooh, ooh, I know this one! That unlovely stack of toothpicks regexp (which now needs a (x-)? since we recently noticed that those mime-types were registered in 1998) is just like artful splitting of pref names and property names in the middle: saving one line of code by making a string literal difficult to search for isn't a great win unless only two or three people who are intimately familiar with all of the codebase are the only ones to ever do anything to it. (I think mxr will give it to you if you somehow know to only search for "pkcs7" but I'm not sure, since that gives you more than a thousand hits - otherwise, you have to know that you are searching for a regexp that matches any of the four types to find it as a use of the types.)
Comment 16 Magnus Melin 2008-07-07 15:03:47 PDT
Created attachment 328397 [details] [diff] [review]
proposed fix, v2

Better patch. I now disable also the "delete all" and "detach all" entries if any attachment is deleted or detached.
Comment 17 Mitra Ardron 2008-07-07 16:56:56 PDT
I don't think you should remove "Delete all" if one has been deleted or detached, since the desired effect is achieved whether or not a previous attachment has been deleted or detached, i.e. they are removed from the message. 

I agree on removing "detach all" - since the desired effect (moving attachments out of message into a location) is dependent on them all being in the message when you start.

On a related topic - wouldn't it be useful if the icons showing hte attachements indicated detached one - the reason I hit this bug was not being able to tell whether a detachment had already been detached, and trying to detach it again being the only way to find out.

- Mitra
Comment 18 Magnus Melin 2008-07-07 22:58:45 PDT
"Detach all" is disabled so we can't have the redetach issue this bug is/was primarily about. "Delete all" does, kind of work - it doesn't do anything an attachment which is already detached/deleted, so the result in that case is a bit odd. But if people want, it could be left enabled I guess.

The visual indication is bug bug 292385 - hoping to get to that sometimes later. 
Comment 19 Serge Gautherie (:sgautherie) 2008-07-11 17:41:02 PDT
Comment on attachment 328397 [details] [diff] [review]
proposed fix, v2

>RCS file: /cvsroot/mozilla/mailnews/base/resources/content/msgHdrViewOverlay.js,v
>retrieving revision 1.162
>@@ -1105,81 +1105,94 @@ createNewAttachmentInfo.prototype.detach
>   messenger.detachAttachment(this.contentType,
>                              this.url,
>                              encodeURIComponent(this.displayName),
>                              this.uri,
>                              true);
> }
> 
> function CanDetachAttachments()
> {
>-  if (("content-type" in currentHeaderData) &&
>-      /application\/x-pkcs7-(mime|signature)/
>-      .test(currentHeaderData["content-type"].headerValue))
>-    return false;
>-

Why not keep the early return, and synchronize the /mail file instead ?
Comment 20 Magnus Melin 2008-07-12 14:24:44 PDT
Yes, I could change that order I guess.
Comment 21 neil@parkwaycc.co.uk 2008-07-17 13:28:03 PDT
Comment on attachment 328397 [details] [diff] [review]
proposed fix, v2

Sorry for the delay in rereview.

>+  saveMenu.setAttribute('disabled', deletedAmongSelected);
>+  detachMenu.setAttribute('disabled', !canDetach || deletedAmongSelected
>+                                      || detachedAmongSelected);
>+  deleteMenu.setAttribute('disabled', !canDetach || deletedAmongSelected
>+                                      || detachedAmongSelected);
>+  saveAllMenu.setAttribute('disabled', deletedAll);
Unless you can provide evidence to the contrary, I suggest that since you can't save a selection of attachments if any one of them is deleted, then it seems to me that you can't save all attachments if one of them is already deleted, so that this should use anyDeleted instead (and deletedAll can be removed). sr=me with this fixed, if my analysis is correct.
Comment 22 Phil Ringnalda (:philor) 2008-07-17 14:24:58 PDT
Comment on attachment 328397 [details] [diff] [review]
proposed fix, v2

Wups, bet I was supposed to r as a reminder to re-sr.
Comment 23 Magnus Melin 2008-07-21 13:33:17 PDT
Created attachment 330640 [details] [diff] [review]
proposed fix, v3 (for checkin)

Updated patch, for the deletedAll stuff - the analysis was correct. Didn't do comment 20 as i needed to touch it anyway, and it didn't really matter.
Comment 24 Magnus Melin 2008-07-21 13:40:22 PDT
Checking in mail/base/content/msgHdrViewOverlay.js;
/cvsroot/mozilla/mail/base/content/msgHdrViewOverlay.js,v  <--  msgHdrViewOverlay.js
new revision: 1.109; previous revision: 1.108
done
Checking in mailnews/base/resources/content/msgHdrViewOverlay.js;
/cvsroot/mozilla/mailnews/base/resources/content/msgHdrViewOverlay.js,v  <--  msgHdrViewOverlay.js
new revision: 1.163; previous revision: 1.162
done

->FIXED
Comment 25 Magnus Melin 2008-07-22 12:44:50 PDT
*** Bug 333904 has been marked as a duplicate of this bug. ***
Comment 26 Bertrand de Pommery 2008-07-30 14:00:01 PDT
Hello
This bug is still in the 1.1.11 version. Does can be resolved in the 1.1.12 version ?
Thanks
Comment 27 Magnus Melin 2008-07-30 15:57:11 PDT
The fix is only for tb3/seamonkey2.
Comment 28 Bertrand de Pommery 2008-08-10 14:09:35 PDT
That is a pity !
When seamonkey 2 is comming ?
sincerly
Comment 29 Tony Mechelynck [:tonymec] 2008-08-10 20:01:42 PDT
(In reply to comment #28)
> That is a pity !
> When seamonkey 2 is comming ?
> sincerly
> 

Une version pré-alpha est disponible (à tes risques et périls), c'est celle que j'utilise mais YMMV (votre consommation peut varier). Si ça t'intéresse: http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-trunk/ . Ne pas utiliser pour lancer une fusée Ariane ni pour toute application qu'un bogue risquerait de compromettre dangereusement. Sans garantie, pas de remboursement en cas de malfonction; peut faire atterrir votre disque dur, battre votre chien à mort, mettre le feu à votre appart' et partir avec votre femme. Vous êtres prévenus.
Comment 30 Bertrand de Pommery 2008-08-12 06:20:49 PDT
Thaks a lot.
Quel risque réel prend-on ?
What the risques with the pre-alpha version ?
Bertrand
Comment 31 Tony Mechelynck [:tonymec] 2008-08-12 13:32:27 PDT
(In reply to comment #30)
> Thaks a lot.
> Quel risque réel prend-on ?
> What the risques with the pre-alpha version ?
> Bertrand
> 

Les risques qu'on prend sont tous les risques associés à une version qui contient les tous derniers changements, très peu testés, et qui donc risque de contenir bien plus de bogues que les versions dites stables faisant l'objet d'un (quel est le mot français?) "release". De plus, ces versions, quoique utilisables, ne sont pas prêtes pour la consommation générale: par exemple, le menu "Préférences" est partagé en deux, à savoir (en anglais) "Edit=>Preferences" (préférences reprogrammées sur la nouvelle "boîte à outils") et "Edit=>(Legacy Prefwindow)" (celles qui ne l'ont pas encore été)

The risks you run with a pre-alpha version are all the risks associated with a "bleeding-edge development version", very little tested, and thus likely to include much more bugs than the so-called "stable" versions branded as "released". In addition, these pre-alpha versions, though usable, aren't ready for prime-time yet: for instance, the "Preferences" menu is split in two, namely "Edit=>Preferences" (for the pages already migrated to the "new toolkit" backend) and "Edit=>(Legacy Prefwindow)" (those which are still to be migrated).
Comment 32 Bertrand de Pommery 2008-08-14 11:54:09 PDT
Hello
"release" is "révision" or "mise à jour" but probably not a "version".
Thanks for your explain in frensh, I understand better of course...
Merci de cette explication claire.
Bertrand
Comment 33 Tony Mechelynck [:tonymec] 2008-08-14 14:28:31 PDT
"révision" or "mise à jour" is "update" or "upgrade", that's not what I meant. By "this version is released" I meant, "cette version reçoit un statut officiel" or something like that.
Comment 34 Bertrand de Pommery 2008-08-15 07:09:37 PDT
Hello
Yes : release is "délivrance" or "lancement" in my dictionary.
By, I am looking the olympic games.
Bertrand
Comment 35 Ian Neal 2008-08-25 07:58:22 PDT
Created attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

This patch:
* Does the equivalent of trunk patch for branch
* Good dataloss fix for branch
Comment 36 Mark Banner (:standard8) 2008-08-26 13:12:46 PDT
Comment on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

I think it would be better for Phil to review this as he looked at the original patch.
Comment 37 Phil Ringnalda (:philor) 2008-09-02 22:13:16 PDT
Comment on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

r=me, looks like a good port of the trunk patch, though I'm on the fence about whether or not it's an appropriate patch for a .18 release (which fortunately isn't my call).
Comment 38 Ian Neal 2008-09-05 16:24:46 PDT
Comment on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

Requesting approval for branch landing - this is a good dataloss fix
Comment 39 Ian Neal 2008-09-19 04:57:44 PDT
Comment on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

Just need flags for approval-tb now...
Comment 40 Mark Banner (:standard8) 2008-09-19 05:03:54 PDT
(In reply to comment #39)
> (From update of attachment 335356 [details] [diff] [review])
> Just need flags for approval-tb now...

Thunderbird 2 security/stability releases have been using the same flags as the gecko 1.8 releases - MoCo still manage those releases, and therefore I think it makes sense to keep them on the approval1.8.1.* flags.
Comment 41 Ian Neal 2008-09-19 05:11:00 PDT
Comment on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

Ok, adding back flag for TB then :)
Comment 42 Robert Kaiser 2008-09-19 05:18:20 PDT
Comment on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

a=me for 1.1.13, thanks!
Comment 43 Daniel Veditz [:dveditz] 2008-10-01 16:09:47 PDT
Comment on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

Approved for 1.8.1.18 per bienvenu, a=dveditz
Comment 44 Ian Neal 2008-10-02 16:54:08 PDT
Comment on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

Checking in to 1.8 branch
mail/base/content/msgHdrViewOverlay.js; new revision: 1.56.2.34; previous revision: 1.56.2.33
mailnews/base/resources/content/msgHdrViewOverlay.js; new revision: 1.141.2.15; previous revision: 1.141.2.14
done
Comment 45 Al Billings [:abillings] 2008-11-04 16:17:51 PST
Shouldn't this show up in the nightly TB 1.8.1.18 builds? I'm using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18pre) Gecko/2008110403 Thunderbird/2.0.0.18pre. If I detach an attachment, the context menu when I right click on the still visible attachment a second time is no different in TB 2.0.0.18. It allows me to attempt to detach a second time but will write a zero byte file and give an error message. As I recall, this has been the behavior for a long time so I see no change here.
Comment 46 Phil Ringnalda (:philor) 2008-11-04 16:55:10 PST
That would be (not surprisingly) my fault: somehow I failed to notice that on 1.8, Tb doesn't have one separator with an id, it has two separators without ids, and the code currently doesn't have a |var saveAllMenu = ...|, so we have another error in reserve, even after we try to setAttribute on the undefined menuSeparator.
Comment 47 Al Billings [:abillings] 2008-11-04 16:58:43 PST
We're taking bugs for 1.8.1.19!
Comment 48 Phil Ringnalda (:philor) 2008-11-04 17:09:24 PST
Mmm, this means that we will now open a deleted attachment. Are we taking omfg patches, or backouts, for .18?
Comment 49 Al Billings [:abillings] 2008-11-04 18:02:05 PST
Well, since this is TB and we haven't built it yet, it is possible. 

Cc'ing Dan and Sam.
Comment 50 Samuel Sidler (old account; do not CC) 2008-11-04 18:13:23 PST
I'm okay with this landing on the relbranch since it's Thunderbird/SeaMonkey-only code. It needs to land in both places though (and use both keywords).

SeaMonkey might want to know so they can rebuild if it affects them.
Comment 51 Phil Ringnalda (:philor) 2008-11-04 21:55:21 PST
I'd expect SeaMonkey to be fine, both by not having either of the Tbird issues and by being what Ian actually uses.
Comment 52 Phil Ringnalda (:philor) 2008-11-04 22:26:36 PST
Created attachment 346404 [details] [diff] [review]
Minimal branch fix, v.1

Not quite the literal minimal fix, which is just the |var saveAllMenu| plus removing the hide-the-separator line, but that looks like ass, with one or two separators with nothing to separate (and, really, this is less change from what 1.8 was doing, and it's not like I'm changing stuff that anyone at all has ever tested since it landed).
Comment 53 Dan Mosedale (:dmose) 2008-11-05 14:41:27 PST
Comment on attachment 346404 [details] [diff] [review]
Minimal branch fix, v.1

r=dmose
Comment 54 Samuel Sidler (old account; do not CC) 2008-11-05 14:46:15 PST
Comment on attachment 346404 [details] [diff] [review]
Minimal branch fix, v.1

Approved for 1.8.1.18. a=ss

Please land ASAP on the MOZILLA_1_8_BRANCH *and* the GECKO181_20081030_RELBRANCH and use both the fixed1.8.1.18 and fixed1.8.1.19 keywords.
Comment 55 Phil Ringnalda (:philor) 2008-11-05 15:25:56 PST
1.8.1: mail/base/content/msgHdrViewOverlay.js 1.56.2.35
20081030_RELBRANCH: mail/base/content/msgHdrViewOverlay.js 1.56.2.34.2.1

Thanks for holding the elevator for me :)
Comment 56 Ian Neal 2008-11-05 15:39:53 PST
Thanks for picking that up Phil whilst I had a day of no internet :(
I usually do test on TB, not sure what happened this time round though.
Comment 57 Phil Ringnalda (:philor) 2008-11-06 18:41:21 PST
*** Bug 463546 has been marked as a duplicate of this bug. ***
Comment 58 Phil Ringnalda (:philor) 2008-11-08 15:55:31 PST
*** Bug 463850 has been marked as a duplicate of this bug. ***
Comment 59 Al Billings [:abillings] 2008-11-08 23:03:06 PST
This is verified for TB 2.0.0.18 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18) Gecko/2008110519 Thunderbird/2.0.0.18.
Comment 60 Al Billings [:abillings] 2008-12-01 14:18:14 PST
Verified in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19pre) Gecko/2008120103 Thunderbird/2.0.0.19pre as well.
Comment 61 Philip Chee 2008-12-03 03:25:48 PST
*** Bug 444791 has been marked as a duplicate of this bug. ***
Comment 62 Bertrand de Pommery 2009-01-04 14:32:06 PST
Hello
for seamonkey 1.1.1x and futurs for windows 9x and Milenium : If the bug isn't resolve, could you kill this bad "detached" function only.
A "save" function whith "deleted" function that is good and sufficient.
good new year !
Bertrand

Note You need to log in before you can comment on or make changes to this bug.