Closed
Bug 416284
Opened 17 years ago
Closed 17 years ago
Detached attachments should not be able to be re-detached
Categories
(MailNews Core :: Attachments, defect, P2)
MailNews Core
Attachments
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.1a1
People
(Reporter: ajschult784, Assigned: mkmelin)
References
Details
(4 keywords)
Attachments
(3 files, 3 obsolete files)
16.50 KB,
patch
|
Details | Diff | Splinter Review | |
16.40 KB,
patch
|
philor
:
review+
Bienvenu
:
superreview+
dveditz
:
approval1.8.1.18+
kairo
:
approval-seamonkey1.1.13+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
dmosedale
:
review+
samuel.sidler+old
:
approval1.8.1.18+
samuel.sidler+old
:
approval1.8.1.19+
|
Details | Diff | Splinter Review |
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 2•17 years ago
|
||
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.
Reporter | ||
Comment 3•17 years ago
|
||
I think the backend has always been happy to overwrite with nothing in this case.
Comment 4•17 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•17 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.
&
![]() |
||
Comment 6•17 years ago
|
||
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?
Comment 8•17 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 | ||
Updated•17 years ago
|
Priority: -- → P2
Assignee | ||
Comment 10•17 years ago
|
||
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•17 years ago
|
Status: NEW → ASSIGNED
Comment 11•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
Right now I'm too tired to work out whether this patch is correct or not.
Assignee | ||
Comment 14•17 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•17 years ago
|
Attachment #328268 -
Attachment is obsolete: true
Attachment #328268 -
Flags: superreview?(neil)
Attachment #328268 -
Flags: review?(philringnalda)
Assignee | ||
Updated•17 years ago
|
Attachment #328278 -
Attachment is obsolete: true
Comment 15•17 years ago
|
||
(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•17 years ago
|
||
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•17 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•17 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 19•17 years ago
|
||
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•17 years ago
|
||
Yes, I could change that order I guess.
Comment 21•17 years ago
|
||
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 22•17 years ago
|
||
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•17 years ago
|
||
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•17 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
Closed: 17 years ago
Flags: wanted-thunderbird3.0a2?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Comment 26•17 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•17 years ago
|
||
The fix is only for tb3/seamonkey2.
Updated•17 years ago
|
Product: Core → MailNews Core
Comment 28•16 years ago
|
||
That is a pity !
When seamonkey 2 is comming ?
sincerly
Comment 29•16 years ago
|
||
(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•16 years ago
|
||
Thaks a lot.
Quel risque réel prend-on ?
What the risques with the pre-alpha version ?
Bertrand
Comment 31•16 years ago
|
||
(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•16 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
Comment 33•16 years ago
|
||
"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•16 years ago
|
||
Hello
Yes : release is "délivrance" or "lancement" in my dictionary.
By, I am looking the olympic games.
Bertrand
Comment 35•16 years ago
|
||
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 36•16 years ago
|
||
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 37•16 years ago
|
||
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•16 years ago
|
Attachment #335356 -
Flags: superreview?(bienvenu) → superreview+
Comment 38•16 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?
Attachment #335356 -
Flags: approval1.8.1.18? → approval-seamonkey1.1.13?
Comment 39•16 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...
Comment 40•16 years ago
|
||
(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 41•16 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•16 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+
Comment 43•16 years ago
|
||
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 44•16 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
Keywords: fixed-seamonkey1.1.13,
fixed1.8.1.18
Comment 45•16 years ago
|
||
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•16 years ago
|
||
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
Comment 47•16 years ago
|
||
We're taking bugs for 1.8.1.19!
Comment 48•16 years ago
|
||
Mmm, this means that we will now open a deleted attachment. Are we taking omfg patches, or backouts, for .18?
Comment 49•16 years ago
|
||
Well, since this is TB and we haven't built it yet, it is possible.
Cc'ing Dan and Sam.
Comment 50•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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 53•16 years ago
|
||
Comment on attachment 346404 [details] [diff] [review]
Minimal branch fix, v.1
r=dmose
Attachment #346404 -
Flags: review?(dmose) → review+
Comment 54•16 years ago
|
||
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+
Updated•16 years ago
|
Attachment #346404 -
Flags: approval1.8.1.19+
Comment 55•16 years ago
|
||
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•16 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.
Comment 59•16 years ago
|
||
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
Comment 60•16 years ago
|
||
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
Comment 62•16 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
Updated•15 years ago
|
Attachment #328397 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•