Severe bug with empty junk can cause deletion of inbox (If selected folder at Folder Pane is changed while folder context menu of Junk is shown, "Empty Junk" clears wrong folder)

RESOLVED FIXED in Thunderbird 33.0

Status

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: contact, Assigned: aceman)

Tracking

({dataloss})

24 Branch
Thunderbird 33.0
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

6.10 KB, patch
jsbruner
: ui-review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20140319 Firefox/24.0 Iceweasel/24.4.0 (Nightly/Aurora)
Build ID: 20140319045509

Steps to reproduce:

I selected the junk folder of an IMAP account and opened the context-menu, then selected delete junk. Somewhere between pressing OK in that dialog and the actual deletion process, the inbox got selected. 

Icedove 24.4.0


Actual results:

As a result, Thunderbird decided _to delete the entire contents of my inbox instead of the junk folder_. This is a totally unacceptable thing to happen. If the dialog asks if I want to empty the _junk-folder_, it cannot, under no circumstances or GUI glitches or whatever, decide to empty any other folder. For ****'s sake. You just ruined my day completely. Thank you.


Expected results:

It should have emptied the junk folder.
Empty Trash context menu uses oncommand="gFolderTreeController.emptyJunk();".
emptyJunk is defined at :
> http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.js#2456

Even when other folder(FolderX) is selected before right click to show context menu, "Junk folder is selected at folder pane" states is kept from context menu open to "click OK at confirmation dialog".

1. FolderX is selected
2. Right click at Junk => Junk is selected at folder pane
    => Context menu is opened
                                                                                                                <= (a) Inbox is changed to selected status at here?
3. Click "Empty Junk"
                                                                                                                <= (b) Inbox is changed to selected status at here?
4. emptyJunk: function ftc_emptyJunk(aFolder) 
                                                                                                                <= (c) Inbox is changed to selected status at here?
       let folder = aFolder || gFolderTreeView.getSelectedFolders()[0]; => Junk is saved in folder at here
        Oen confirmation dialog
        OK click                                      => 5. selection at Folder pane : back to FolderX
        => continue process
        Do "Empty Junk" on Folder saved in variable folder == Junk folder.

There is no chance to choose Inbox as target folder of "Empty Trash", because confirmation dialog is modal dialog.

> Somewhere between pressing OK in that dialog and the actual deletion process, the inbox got selected.

When was Inbox selected? (a)? (b)? (c)?
Who selected Inbox just after "click Empty Junk menu", just before "confirmation dialog open"?

If Addon can change folder selection just after "click Empty Junk menu", just before "confirmation dialog open", use of gFolderTreeView.getSelectedFolders()[0] is dangerous. "Folder which is currently used as Junk of this account" should be used for Empty Junk. This is same in Empty Trash.

Another possibility in IMAP:
   Just before trying to send "uid xxx store +Flags(\Deleted)" at connection where Junk is selected,
   the connection is hijacked by process for Inbox and "select Inbox" is issued at the connection.
I believe there is no possibility of this case, because one connection is reserved for for Inbox and Inbox is always selected at the reserved connection for Inbox, if max cached connection>=2.
max cached connection==1?
If no, connection for Inbox is lost when you tried to do "Empty Junk"?
Network or IMAP server is not stabilized in your environment?

"junk folder in Tb" is not "folder named Junk". "junk folder in Tb" is "folder defined as junk folder in Tb" by Junk settings.
Inbox is defined as "junk folder in your Tb"?
Please note that following can occur.
   (i)  Inbox of accountA == junk folder of accountA
   (ii) Junk   of accountA == junk folder of accountB
In this case, "junk icon" is shown only for (ii) because "state of Inbox" is stronger than "state of junk", and "Empty Junk" is shown for both (i) and (ii), and "selection at folder pane" may be changed to other "folder defined as junk" upon click o "Empty Junk" menu.
(Assignee)

Comment 2

5 years ago
(In reply to WADA from comment #1)
> Empty Trash context menu uses oncommand="gFolderTreeController.emptyJunk();".
> emptyJunk is defined at :
> > http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.js#2456
> 
> Even when other folder(FolderX) is selected before right click to show
> context menu, "Junk folder is selected at folder pane" states is kept from
> context menu open to "click OK at confirmation dialog".

It appears to me this function emptyJunk will happing clear out any folder passed to it. If no folder is passed, as in the oncommand you quote, it will choose the first currently selected folder.

So if there is some sudden change of selection BEFORE clicking OK to the question about "really delete all messages in Trash", it may delete an unexpected folder.

I see a problem that the message has "Trash" hardcoded and doesn't really say which folder was selected. So we can add that as a preventive measure.

But I couldn't reproduce the bug as described.
Assignee: nobody → acelists
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 3

5 years ago
I cannot comment on how the selection changed exactly, as all this must have happened very fast. Also, I didn't suspect any malbehaviour until I realised that the "Empty-Junk" action took really long, and then it was too late. I am managing two accounts, but all has happening only in account A. This has one Inbox and one two Junk folders, one called "Junk", the other called "Spam". Thunderbird/Icedove only uses the "Junk" folder, although both "Junk" and "Spam" use the 'fire' like icon. (I believe "Spam" is an obsolete folder created by my old e-mail client - Apple Mail which I had used until I changed to Thunderbird on OS X back in the middle of last year (?); the Linux version of Thunderbird was used from February or March this year.)
(Assignee)

Comment 4

5 years ago
You can see in Account settings -> account -> Junk setting what folder is TB using to store Junk messages.
(Reporter)

Comment 5

5 years ago
Yes, the "Junk" folder is the designated folder. Strangely, when I try to delete the "Spam" folder, I get an error message: "The current command did not succeed. The mail server for account ... responded: [ALREADYEXISTS] Mailbox does already exist." - of course it exists, that's why I'm trying to delete it?!
(Assignee)

Comment 6

5 years ago
Posted patch patch (obsolete) β€” β€” Splinter Review
So at least improve the message to contain the folder to be deleted. Hopefully the user notices if there is something wrong.
Attachment #8434283 - Flags: ui-review?(josiah)
Attachment #8434283 - Flags: review?(mkmelin+mozilla)
Ouch. Can we do better than hoping the user notices, like pull the original folder name at time of first click, and compare to the folder name being acted upon?
Severity: normal → critical
Component: Untriaged → Folder and Message Lists
Keywords: dataloss
(Assignee)

Comment 8

5 years ago
Sure, if we find out where the gap is.

Clicking in the context menu item calls emptyTrash from oncommand. emptyTrash() calls gFolderTreeView.getSelectedFolders()[0] . Is it possible that the selection changes between the click in the menuitem and call to .getSelectedFolders() ? Does the code of emptyTrash run in parallel to the folder tree handler that would accept mouse clicks and change the selection while emptyTrash is running?
Or must the folder tree wait until the oncommand from the context menu finishes?
I don't know. But if it is parallel (oncommand code execution is async) then we have this same problem in all of the context menu commands.

I tried it on Windows and POP3 account with clicking the Empty Trash and trying to quickly select other folder before the question dialog comes up. But I didn't succeed. And the dialog is modal. But maybe the case is different for IMAP.
(Reporter)

Comment 9

5 years ago
I found the solution of how it could happen: I am using an Apple Mouse which under Linux has the side-buttons configured as 'go back'; I have wanted to deactivate them since long, but haven't found an easy way to do that in the GNOME Shell and haven't had time to figure out how to do it in some Xorg config files. Because my thumb and little finger grab the mouse on its sides, I often accidentally cause this side-click when actually using the Mouse 1 click.

The following is what is being sent then:

ButtonPress event, serial 36, synthetic NO, window 0x3200001,
    root 0x283, subw 0x3200002, time 150944932, (37,43), root:(39,148),
    state 0x0, button 8, same_screen YES

EnterNotify event, serial 36, synthetic NO, window 0x3200001,
    root 0x283, subw 0x0, time 150944840, (37,43), root:(39,148),
    mode NotifyGrab, detail NotifyInferior, same_screen YES,
    focus YES, state 0

KeymapNotify event, serial 36, synthetic NO, window 0x0,
    keys:  4294967171 0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   
           0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   

ButtonRelease event, serial 36, synthetic NO, window 0x3200001,
    root 0x283, subw 0x3200002, time 150945092, (37,43), root:(39,148),
    state 0x0, button 8, same_screen YES

LeaveNotify event, serial 36, synthetic NO, window 0x3200001,
    root 0x283, subw 0x0, time 150944975, (37,43), root:(39,148),
    mode NotifyUngrab, detail NotifyInferior, same_screen YES,
    focus YES, state 0

Now: When I select the Junk folder and choose 'Empty Junk' from the context menu, I get the 'model' Dialog. But when perform this accidental 'double-click' (both the side buttons and the top button), the selected folder jumps back to whatever was previously selected, e.g. the Inbox, before the 'Yes' button is pressed.

So the design is fundamentally flawed. I have no idea about how the Thunderbird code base is structured, but you certainly cannot read the selected-folder state at the time the dialog is closed. This is really dirty. You can only erase the junk folder, there is no context menu item for erase folder when any other folder than the Junk is selected. So this action must be intimately tied to this one particular folder. Instead of this horrible `gFolderTreeView.getSelectedFolders()[0]` you should just have a function `gFolderTreeView.getJunkFolder` and this must be unambigous.
(Reporter)

Comment 10

5 years ago
Typo:

> When I select the Junk folder and choose 'Empty Junk' from the context menu, I get the MODAL Dialog.

(which is not modal at all then)
(Assignee)

Comment 11

5 years ago
If you see at
http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.js#2463 then gFolderTreeView.getSelectedFolders()[0] is run BEFORE the dialog comes up (it is the this._checkConfirmationPrompt("emptyJunk") line). So there is a very small window.

(In reply to Sciss from comment #10)
> Typo:
> 
> > When I select the Junk folder and choose 'Empty Junk' from the context menu, I get the MODAL Dialog.
> 
> (which is not modal at all then)
Maybe the situation in Linux is different. I'll check that.
(Reporter)

Comment 12

5 years ago
Correction - the `KeymapNotify` is not actually doing anything. It is the button 8 press which causes the view to go back to the previous selection (despite the modal dialog being open). Since aceman says, the selected folder is determined before the dialog opens, perhaps the jump back was performed straight after selecting from the context menu and before the event got dispatched/processed.
(Assignee)

Comment 13

5 years ago
So can we put the steps right?
1. You left click e.g. Inbox.
2. then right click Junk
3. choose Empty Junk but double click it?

I see that right clicking on currently not selected folder seems visually problematic. The folder rightclicked now has the highlight, however the previous folder has the focus ring. If *seems* operations are done on the highlighted folder (it is returned as gFolderTreeView.getSelectedFolders()[0]), but is it 100% reliable?
(Reporter)

Comment 14

5 years ago
Yes that may have been the steps. Obviously I cannot vouch for that, and I don't really want to experiment with deleting the inbox once more. I can see, however, that if I right-click on Junk, the focus ring is still on Inbox, and when the dialog closes, highlight jumps back to Inbox.

Since each account is associated with exactly one Junk folder, in my reading the safe solution is to pass exactly that folder into the empty-junk function and not query the selected folders at all.
(Assignee)

Comment 15

5 years ago
Yes, but the EmptyJunk and EmptyTrash function as coded so that they happily clear any folder passed to them (or the selected folder) not just Junk or Trash. It is up to the caller (in this case the menu) to send the right folder. I don't know why that was done and if that functionality is still needed.

I now tested it and this behaviour or switching back to the previously selected folder also happens in Outlook. But it does not e.g. in Windows explorer. There right-click also properly selects the folder as if left-clicked. This also happens in e.g. Firefox's bookmarks/history window. So it seems there are more approaches and we just need to decide whom to follow. I would also consider this approach safer as it would clearly be defined where the focus+selection really is.

But the current state seems to be done intentionally, see http://mxr.mozilla.org/comm-central/source/mail/base/content/commandglue.js#340 .
(Assignee)

Comment 16

5 years ago
OK, that code comment only says that someone also noticed this behaviour and worked around it. But it does not say where the behaviour originates.
(Assignee)

Comment 17

5 years ago
It looks like the behaviour originates somewhere around here: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/tree.xml#1173
but I do not see where the check for the "UNLESS it was a right-click".
It originates in bug 30560. Also, for me, Windows 7 Explorer allows me to right-click a folder in the tree without permanently changing the selection, but it's possible that I've changed some of the default settings.

When you right-click and select the Empty Junk menuitem, the selection should register at that point and get passed to the back end, and I'm not exactly clear as to how the selection is changing.

Checking for the junk folder flag does not seem unreasonable.

(I wonder whether "Undo" would have worked in this case?)
(In reply to neil@parkwaycc.co.uk from comment #18)
> Checking for the junk folder flag does not seem unreasonable.

"Checking for the junk folder flag" is not sufficient, because multiple folders of an account can be used as "Junk of multiple accounts", and "1. select JunkX of other account, 2, right click JunkA of this account, 3. selection goes back to JunkX, 4. Empty Trash" can occur.
If Junk folder, "folder defined as Junk of this account" should be checked in addition to "Junk folder flag", if  folder's attribute check is needed.
If Trash, because Tb currently sets "Trash flag" for only one folder upon folder re-discovery, even when "Trash flag" is set in multiple folders of an account by Script, "Checking Trash flag upon execution of Empty Trash" is currently sufficient.

If "n-th mouse button click while context menu is opened for a folder" can select other folder at folder pane(reardless of single folder or multiple folders), any context menu action which relies on gFolderTreeView.getSelectedFolders()[0] or Currently_Selected_Folder_At_Folder_Pane will be affected.
Dangerous one is "Delete Folder". Even though Confirmation Dialog is shown with folder name, user can click "Delete Folder" without looking shown folder name.

> When you right-click and select the Empty Junk menuitem,
> the selection should register at that point and get passed to the back end

Is it possible in any context menu of a folder? "Save folder name in an XUL element upon right click" is sufficient?
How about "Context menu at Thread Pane with single or multiple mail selection?
(In reply to Sciss from comment #5)
> Strangely, when I try to delete the "Spam" folder, I get an error message: "The current command did not succeed.
> The mail server for account ... responded: [ALREADYEXISTS]

If IMAP delete model="move to trash"(Server Settings), "delete xxx/Spam" = "rename xxx/Spam Trash/Spam".
Do you alredy have Spam under Trash? If yes, do "Empty Trash"  before "delete xxx/Spam".
Comment on attachment 8434283 [details] [diff] [review]
patch

Review of attachment 8434283 [details] [diff] [review]:
-----------------------------------------------------------------

I'm clearing the ui-r flag until a better approach is implemented, as noted in the comments.
Attachment #8434283 - Flags: ui-review?(josiah)
"Empty Trash" doesn't have problem of this bug.
> emptyTrash() which is called from context menu.
>    http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.js#2436
> Currently selected folder is obtained by getSelectedFolders, as done in Empty Junk case.
>   2437     let folder = aFolder || gFolderTreeView.getSelectedFolders()[0];
>   If "empty trash on exit", search trash folder of all accounts, and call TrashFolder.emptyTrash().
>   If "empty trash via menu", call folder.emptyTrash().
> However, actual emptyTrash is executed by msgFolder.emptyTrash().
>    http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#643
>    http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#1450
> Both emptyTrash routine of nsXXXMailFolder.cpp searches folder which has Trash folder flag.

I believe "Empty Junk" should search "Junk folder of this account". "Empty Trash" shouldn't use "curently selected folder at folder pane" any time, in any case. "File/Empty Junk menu" is not implemented yet, but "Empty Junk" function should work even when called from "File/Empty Junk menu".
Can "let folder = aFolder || gFolderTreeView.getSelectedFolders()[0];" of "Empty Junk" be replaced by code like next?
   let folder = aFolder || GetJunkOfFolderOwnerAccountOfThisFolder( gFolderTreeView.getSelectedFolders()[0] );

Is following possible and useful?
(1) msgFolder.emptySpecialFolder(...,...,TargetFolder,TargetFolderFlag) which is similar to msgFolder.emptyTrash().
      - TargetFolderFlag==Trash : find fist folder which has "Trash folder flag".   same as current emptyTrash().
      - TargetFolderFlag==Junk  : If(!TargetFolder), find all folders which has "Junk folder flag", choose Junk for this account.
            mail.server.server#.spamActionTargetFolder can be used if non-"Local Folders" of Folder View=All.
            But it's not usable if folder of Local Folders or folder at Folder View=Unified.
      - TargetFolderFlag==XXX   : XXX=Archive, Sent, Draft, etc. which is set via "Copies&Folders".
            If(!TargetFolder), find all folders which has "XXX folder flag", choose XXX folder used by identities of this account.
(2) When "Empty Junk" is requested via menu,
      oncommand of the menu finally calls msgFolder.emptySpecialFolder(...,...,TragetFolder,JunkFlag).
This is excessive, but I think "File/Empty Junk of all accounts", "Empty Junk upon exit" like feature can be easily implemented by pretty small Addon, if feature like above is available.
    Search folder which has "Junk folder flag", call folder.emptySpecialFolder(...,...,folder,JunkFlag) for each Junk folder.
Summary: Severe bug with empty junk can cause deletion of inbox → Severe bug with empty junk can cause deletion of inbox (If selected folder at Folder Pane is changed while folder context menu of Junk is shown, "Empty Junk" clears wrong folder)
Another possible alternative would be to close the context menu if the selection changes while it's open, thus preventing you from invoking any command on the wrong folder.
(Assignee)

Comment 24

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #23)
> Another possible alternative would be to close the context menu if the
> selection changes while it's open, thus preventing you from invoking any
> command on the wrong folder.

Yes, but we do not understand yet, how the selection can change since showing the context menu. On Windows, any click outside the menu (as an attempt to selection another folder) just closes the menu and does not select the new folder.

But OK, I can check if the folder to be processed has the necessary nsMsgFolderFlags.Junk or nsMsgFolderFlags.Trash flag.

Just decide what to do in that situation. Just do nothing? I would vote for this as this is a rare situation. The user could understand that he just mis-clicked and will try again and that will probably work.
Flags: needinfo?(josiah)
(In reply to :aceman from comment #24)
> Just decide what to do in that situation. Just do nothing?
> I would vote for this as this is a rare situation.

I vote.
I think code like next is practically sufficient, because it's very rare case.
  var isJunkFolder = folder.isSpecialFolder(nsMsgFolderFlags.Junk) ;
  var isDefinedAsJunk = folder.URI is registered in a mail.server.server#.spamActionTargetFolder entry ;
  if(!isJunkFolder || !isDefinedAsJunk ){ Show message of "please retry"; return; }
  Because "Junk folder flag" is set/removed by only "Junk Settings"  unless Addon or user's script code alters "Junk folder flag", 
  "if(!isJunkFolder)" is perhaps sufficient.




The user could understand that he just
> mis-clicked and will try again and that will probably work.
(Assignee)

Comment 26

5 years ago
As we show the menuitem on just the existence of the flag (folder.isSpecialFolder(nsMsgFolderFlags.Junk)) I would only check for the flag in the function. No iterating over mail.server.server#.spamActionTargetFolder prefs. The prefs code should be responsible for setting the flags correctly. We also show the special folder icon if it has the flag so having the flags wrong would be visible.
(Assignee)

Comment 27

5 years ago
Posted patch patch v2 (obsolete) β€” β€” Splinter Review
Ok, what about this?
Attachment #8434283 - Attachment is obsolete: true
Attachment #8434283 - Flags: review?(mkmelin+mozilla)
Attachment #8437143 - Flags: ui-review?(josiah)
Attachment #8437143 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8437143 [details] [diff] [review]
patch v2

Review of attachment 8437143 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +538,5 @@
>  mdnBarMessageAddressDiffers=%1$S has asked to be notified (on %2$S) when you read this message.
>  
>  # mailCommands.js
>  emptyJunkTitle=Confirm
> +emptyJunkFolderMessage=Are you sure you want to permanently delete all messages and subfolders in the Junk folder "%S"?

This does risk sounding very repetitive. 
Maybe the emptyJunkTitle should be "Empty Junk?" and then we don't have to repeat in the text that it's "the Junk folder Junk". 

Same thing with Trash of course.
Attachment #8437143 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 29

5 years ago
OK, let's see if Josiah also agrees to that. Not sure if it is common to have question mark in the title. It could be "Confirm Junk deletion".
I think it should be something like:

emptyJunkTitle=Empty Junk
emptyJunkFolderMessage=Delete all messages and subfolders in the Junk folder "%S"?

No need for a question mark after "Empty Junk"
Flags: needinfo?(josiah)
Actually, you know what, better yet:

emptyJunkTitle=Empty "%S"
emptyJunkFolderMessage=Delete all messages and subfolders in the Junk folder?

That way the folder message won't ever display duplicated text (E.G. "Delete all messages and subfolders in the Junk folder "Junk"). It would instead look like:

*------------------------------------*
| Delete all messages and subfolders |
| in the Junk folder?                |
|                                    |
|            [Cancel] [Empty "Junk"] |
*------------------------------------*
(Assignee)

Comment 32

5 years ago
Just that people like me would not read the title of the dialog :) It should not contain critical information.
Well people probably won't read the text either. In fact, since the message text is so long, people will more likely notice: "Empty 'Inbox'" than "Delete all messages and subfolders in the Junk folder 'Inbox'"?
Comment on attachment 8437143 [details] [diff] [review]
patch v2

Review of attachment 8437143 [details] [diff] [review]:
-----------------------------------------------------------------

ui-r+ if you change the two strings to my suggestion. :)

Or we can debate it more, which ever.
Attachment #8437143 - Flags: ui-review?(josiah) → ui-review+
(Assignee)

Comment 35

5 years ago
Posted patch patch v2.1 β€” β€” Splinter Review
OK.
Attachment #8437143 - Attachment is obsolete: true
Attachment #8448218 - Flags: ui-review?(josiah)
Attachment #8448218 - Flags: ui-review?(josiah) → ui-review+
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a708826c1bf8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
You need to log in before you can comment on or make changes to this bug.