Detached attachments should not be able to be re-detached

RESOLVED FIXED in mozilla1.9.1a1

Status

MailNews Core
Attachments
P2
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Andrew Schultz, Assigned: Magnus Melin)

Tracking

(4 keywords)

Trunk
mozilla1.9.1a1
dataloss, fixed-seamonkey1.1.13, verified1.8.1.18, verified1.8.1.19
Dependency tree / graph
Bug Flags:
blocking-thunderbird3.0a1 -
blocking-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Comment 3

9 years ago
I think the backend has always been happy to overwrite with nothing in this case.

Comment 4

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

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

Updated

9 years ago
Duplicate of this bug: 438450

Comment 8

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

Comment 9

9 years ago
Taking.
Assignee: nobody → mkmelin+mozilla
(Assignee)

Updated

9 years ago
Priority: -- → P2
(Assignee)

Comment 10

9 years ago
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)
Attachment #328268 - Flags: superreview?(neil)
Attachment #328268 - Flags: review?(philringnalda)
(Assignee)

Updated

9 years ago
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.
Created attachment 328278 [details] [diff] [review]
WIP

Right now I'm too tired to work out whether this patch is correct or not.
(Assignee)

Comment 14

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

(Assignee)

Updated

9 years ago
Attachment #328268 - Attachment is obsolete: true
Attachment #328268 - Flags: superreview?(neil)
Attachment #328268 - Flags: review?(philringnalda)
(Assignee)

Updated

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

Comment 16

9 years ago
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.
Attachment #328397 - Flags: superreview?(neil)
Attachment #328397 - Flags: review?(philringnalda)

Comment 17

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

Comment 18

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

Comment 20

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

Comment 23

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

Comment 24

9 years ago
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
Last Resolved: 9 years ago
Flags: wanted-thunderbird3.0a2?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
(Assignee)

Updated

9 years ago
Duplicate of this bug: 333904

Comment 26

9 years ago
Hello
This bug is still in the 1.1.11 version. Does can be resolved in the 1.1.12 version ?
Thanks
(Assignee)

Comment 27

9 years ago
The fix is only for tb3/seamonkey2.
Product: Core → MailNews Core

Comment 28

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

Comment 30

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

Comment 32

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

Comment 34

9 years ago
Hello
Yes : release is "délivrance" or "lancement" in my dictionary.
By, I am looking the olympic games.
Bertrand

Comment 35

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

Updated

9 years ago
Attachment #335356 - Flags: superreview?(bienvenu) → superreview+

Comment 38

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

Updated

9 years ago
Attachment #335356 - Flags: approval1.8.1.18? → approval-seamonkey1.1.13?

Comment 39

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

Updated

9 years ago
Attachment #335356 - Flags: approval1.8.1.18?

Comment 41

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

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

Updated

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

Updated

9 years ago
Attachment #335356 - Attachment description: Branch patch for re-detachment v0.1 → Branch patch for re-detachment v0.1 (Checkin: Comment 44)

Comment 44

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

Updated

9 years ago
Keywords: fixed-seamonkey1.1.13, fixed1.8.1.18
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.
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).
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 :)
Keywords: fixed1.8.1.18, fixed1.8.1.19

Comment 56

9 years ago
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.
Duplicate of this bug: 463546
Duplicate of this bug: 463850
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.
Keywords: fixed1.8.1.18 → verified1.8.1.18
Depends on: 466016
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.
Keywords: fixed1.8.1.19 → verified1.8.1.19

Updated

9 years ago
Duplicate of this bug: 444791
Depends on: 436869

Comment 62

8 years ago
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.