Closed Bug 416284 Opened 16 years ago Closed 16 years ago

Detached attachments should not be able to be re-detached

Categories

(MailNews Core :: Attachments, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: ajschult784, Assigned: mkmelin)

References

Details

(4 keywords)

Attachments

(3 files, 3 obsolete files)

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.
xref bug 292385
OS: Linux → All
Hardware: PC → All
Flags: blocking-thunderbird3.0a1?
Keywords: dataloss
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.
Depends on: 292385
Flags: wanted-thunderbird3.0a1+
Flags: blocking-thunderbird3.0a1?
Flags: blocking-thunderbird3.0a1-
Flags: blocking-thunderbird3+
Keywords: relnote
I think the backend has always been happy to overwrite with nothing in this case.
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 

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.

&
Moving from wanted‑thunderbird3.0a1+ flag to wanted‑thunderbird3.0a2? flag since code for Thunderbird 3 Alpha 1 has been frozen.
Flags: wanted-thunderbird3.0a1+ → wanted-thunderbird3.0a2?
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
Taking.
Assignee: nobody → mkmelin+mozilla
Priority: -- → P2
Attached patch proposed fix (obsolete) — Splinter Review
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)
Attachment #328268 - Flags: superreview?(neil)
Attachment #328268 - Flags: review?(philringnalda)
Status: NEW → ASSIGNED
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().
I realise that canOpen should have been canSave, but it still took me some time to work out what deletedAll and detachedAll really meant.
Attached patch WIP (obsolete) — Splinter Review
Right now I'm too tired to work out whether this patch is correct or not.
(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. 

Attachment #328268 - Attachment is obsolete: true
Attachment #328268 - Flags: superreview?(neil)
Attachment #328268 - Flags: review?(philringnalda)
Attachment #328278 - Attachment is obsolete: true
(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.)
Attached patch proposed fix, v2 (obsolete) — Splinter Review
Better patch. I now disable also the "delete all" and "detach all" entries if any attachment is deleted or detached.
Attachment #328397 - Flags: superreview?(neil)
Attachment #328397 - Flags: review?(philringnalda)
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
"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 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 ?
Yes, I could change that order I guess.
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.
Attachment #328397 - Flags: superreview?(neil) → superreview+
Comment on attachment 328397 [details] [diff] [review]
proposed fix, v2

Wups, bet I was supposed to r as a reminder to re-sr.
Attachment #328397 - Flags: review?(philringnalda) → review+
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.
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: wanted-thunderbird3.0a2?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Hello
This bug is still in the 1.1.11 version. Does can be resolved in the 1.1.12 version ?
Thanks
The fix is only for tb3/seamonkey2.
Product: Core → MailNews Core
That is a pity !
When seamonkey 2 is comming ?
sincerly
(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.
Thaks a lot.
Quel risque réel prend-on ?
What the risques with the pre-alpha version ?
Bertrand
(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).
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
"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.
Hello
Yes : release is "délivrance" or "lancement" in my dictionary.
By, I am looking the olympic games.
Bertrand
This patch:
* Does the equivalent of trunk patch for branch
* Good dataloss fix for branch
Attachment #335356 - Flags: superreview?(bienvenu)
Attachment #335356 - Flags: review?(bugzilla)
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.
Attachment #335356 - Flags: review?(bugzilla) → review?(philringnalda)
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).
Attachment #335356 - Flags: review?(philringnalda) → review+
Attachment #335356 - Flags: superreview?(bienvenu) → superreview+
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
Attachment #335356 - Flags: approval1.8.1.18?
Attachment #335356 - Flags: approval1.8.1.18? → approval-seamonkey1.1.13?
Comment on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

Just need flags for approval-tb now...
(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.
Attachment #335356 - Flags: approval1.8.1.18?
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 on attachment 335356 [details] [diff] [review]
Branch patch for re-detachment v0.1 (Checkin: Comment 44)

a=me for 1.1.13, thanks!
Attachment #335356 - Flags: approval-seamonkey1.1.13? → approval-seamonkey1.1.13+
Blocks: 287939
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
Attachment #335356 - Flags: approval1.8.1.18? → approval1.8.1.18+
Attachment #335356 - Attachment description: Branch patch for re-detachment v0.1 → Branch patch for re-detachment v0.1 (Checkin: Comment 44)
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
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.
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.
Keywords: fixed1.8.1.18
We're taking bugs for 1.8.1.19!
Mmm, this means that we will now open a deleted attachment. Are we taking omfg patches, or backouts, for .18?
Well, since this is TB and we haven't built it yet, it is possible. 

Cc'ing Dan and Sam.
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.
I'd expect SeaMonkey to be fine, both by not having either of the Tbird issues and by being what Ian actually uses.
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).
Attachment #346404 - Flags: review?(dmose)
Attachment #346404 - Flags: approval1.8.1.18?
Comment on attachment 346404 [details] [diff] [review]
Minimal branch fix, v.1

r=dmose
Attachment #346404 - Flags: review?(dmose) → review+
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.
Attachment #346404 - Flags: approval1.8.1.18? → approval1.8.1.18+
Attachment #346404 - Flags: approval1.8.1.19+
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 :)
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.
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.
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.
Depends on: 436869
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
Attachment #328397 - Attachment is obsolete: true
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.