Holding down the A key for repeated archiving results in lots of extra blank messages in Archive, and corruption of Undo (POP3 only)
Categories
(Thunderbird :: Folder and Message Lists, defect, P2)
Tracking
(thunderbird_esr91+ fixed, thunderbird92 affected)
People
(Reporter: johnkw, Assigned: benc)
References
(Blocks 2 open bugs)
Details
(Keywords: access, ux-error-prevention)
User Story
In this bug, user starts on a single message and then holds down the A-key to archive an uncontrollable number of messages. Using this method is not recommended (and not really supported...) Workaround: First select the messages you want to archive (click first, shift+click last, or Ctrl+A, or some such), then press A key once to archive them.
Attachments
(3 files, 1 obsolete file)
bug.png
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:87.0) Gecko/20100101 Firefox/87.0
Steps to reproduce:
Click and hold down "A" in a folder. Messages start getting created in whatever the appropriate Archives folder would have been. But the messages are completely blank (no attribute of any kind), with the date shown as "1969-12-31, 6:00 PM" and marked unread.
After a pile of these get created, Undo does not function properly. The Undo action instead moves the fake blank message back to the original folder, and seemingly leaves the moved message in the Archives folder.
The attached screenshot of the Archive folder sorted by "Order Received" shows some sense of the timeline of this bug. It looks like each real email is moved over, and then 2 or 3 fake blank emails are created after it. Bizarrely, perhaps this is dependent on the keyboard repeat rate for the "a" key?
I'm not 100% certain, but it doesn't seem to result in data loss, strictly speaking. However it's easy to THINK it resulted in data loss, because "Undo" brings back blank emails, and your real emails are left behind in the Archive folder.
Actual results:
Many bizarre things.
Expected results:
Emails moved over to Archive folder normally.
It may be that the UNDOs are still in there, but you just have to do many more of them. Ie, if 5 real emails were moved, clicking undo ~15 times may get things back?
|
||
Comment 2•4 years ago
|
||
I was not able to reproduce this (creating rogue messages with "A") using beta on Mac.
Does it also happen if you have started in safe mode with Help > restart with add-ons disabled
The behavior in Safe Mode was even worse. Does Safe Mode limit the number of Undos you're allowed? Only the first 5(?) Undos worked and then I had to manually correct the rest.
Have only tried on Windows 10. Version 78.9.1 (32-bit).
|
||
Comment 4•4 years ago
|
||
wfm on win10, 78.9.1 (32-bit) on gmx imap.
The results of holding down A key are somewhat unpredictable because the function is probably asynchroneous. I was under the impression that I had to let go and repress A long to succeed on all messages. Undo on 60+ messages holding/repeating Ctrl+Z left 2 behind before the command was disabled. Undo buffer is unlikely to be unlimited, but correct procedure as below will help.
Holding down A-key to archive an uncontrollable number of messages is not recommended (and not really supported -> invalid).
You first need to select the messages you want to archive (click first, shift+click last, or Ctrl+A, or some such), then press A key once.
Sorry John, nothing we could do here. Pls use controlled operation.
This bug report is not INVALID. It is easy to reproduce and the number archived is quite controllable by letting go of A at the correct time. I have only tested on POP3. It's possible that the behavior on IMAP is not broken in the same way.
The issue is that Thunderbird creates huge numbers of broken blank messages as per the original bug.png. If you think that broken behavior is ok, it could be resolved WONTFIX or something, but INVALID is completely inappropriate.
|
|
||
Comment 6•4 years ago
•
|
||
(In reply to johnkw from comment #5)
This bug report is not INVALID. It is easy to reproduce and the number archived is quite controllable by letting go of A at the correct time. I have only tested on POP3. It's possible that the behavior on IMAP is not broken in the same way.
Ah, POP3! That wasn't mentioned in the original report.
Indeed, on a POP3 account I can easily reproduce on 78.10.0 (32-bit), win10, gmx pop, and it does create loads of phantom blank messages around the same date as seen in reporter's screenshot (probably with a timezone offset, I'm in DE and got 01-01-1970, 01:00).
The issue is that Thunderbird creates huge numbers of broken blank messages as per the original bug.png. If you think that broken behavior is ok, it could be resolved WONTFIX or something, but INVALID is completely inappropriate.
Sorry, my mistake! Thank you for providing the missing detail and insisting!
|
|
||
Comment 7•4 years ago
|
||
Geoff?
(In reply to Thomas D. (:thomas8) from comment #6)
Indeed, on a POP3 account I can easily reproduce on 78.10.0 (32-bit), win10, gmx pop, and it does create loads of phantom blank messages around the same date as seen in reporter's screenshot (probably with a timezone offset).
Also, undo was still enabled in menu but doing nothing, so some sort of Undo corruption does seem to happen, too, as reported.
|
|
||
Updated•4 years ago
|
|
||
Comment 9•4 years ago
|
||
Someone would have to debug what goes wrong in https://searchfox.org/comm-central/source/mail/base/modules/MessageArchiver.jsm#25, or probably in the backend it calls.
|
|
||
Updated•4 years ago
|
|
Reporter | |
Comment 10•4 years ago
|
||
I would point out that if this is "not supported", then the auto-repeat should be disabled entirely (which I think would be a good resolution here).
A person with less-than-perfect manual dexterity may do this accidentally without meaning to, and so, this is a usability/accessibility issue.
|
|
||
Comment 11•4 years ago
|
||
(In reply to johnkw from comment #10)
I would point out that if this is "not supported", then the auto-repeat should be disabled entirely (which I think would be a good resolution here).
Indeed.
Surprisingly, I did not find anyduplicate bug reports, and almost no related bug reports.
|
|
||
Comment 12•4 years ago
|
||
(In reply to Wayne Mery (:wsmwk) from comment #11)
(In reply to johnkw from comment #10)
I would point out that if this is "not supported", then the auto-repeat should be disabled entirely (which I think would be a good resolution here).
Indeed.
+1. We should really prevent repeated (as in long-press) keyboard shortcuts where they don't make sense.
Surprisingly, I did not find anyduplicate bug reports, and almost no related bug reports.
Here's another one:
Bug 1675212 - By accidentally pressing the keyboard Delete key for too long, unwanted emails will be deleted.
|
|
||
Comment 13•4 years ago
|
||
Another "related" bug:
Whereas most long keypresses will just repeat the same action in an uncontrolled manner, in some cases, the long keypress might even spill over into another part of the UI:
Bug 620853 - Holding Ctrl+Enter a little too long can cause unintentional confirmation of "Send Message?" warning popup - only plain Enter (without modifier key) should confirm the prompt
|
|
||
Updated•4 years ago
|
|
|
||
Comment 14•4 years ago
|
||
I suspect this would be a back-end issue that we should fix (it could cause other problems as well).
|
Assignee | |
Comment 15•4 years ago
|
||
It's definitely a back-end issue.
My current working guess is that it's because write access to the mbox is not being properly managed. Earlier messages are being overwritten by later ones.
This is supported by the fact that it doesn't seem to be a problem if the pop3 server is maildir - there's no interference then, as every message is has its own file.
In the back of mind I had this flagged as a possible issue (Bug 1719996 touches on this area).
While it's probably not too hard to patch the messagearchiver to be more cautious, this is a sucky solution - we'd just be papering over a rather fundamental architectural problem.
It really needs to be fixed in the nsIPlugableMessageStore interface. I've got a couple of ideas:
- getNewMsgOutputStream() could return an async stream (or have a callback which is called when the stream is ready). Behind the scenes, the mbox store can make sure write requests happen in strict sequence. The most intrusive to implement, but probably the "best" solution, long term.
- It might be possible to have getNewMsgOutputStream() return streams which block when an attempt is made to write, only executing when previous messages have completed writing. This is a lot less intrusive than 1, but I could imagine possible deadlocks might be a show-stopper here...
- mbox could always use the quarantine method. That is, rather than writing directly to the mbox file, getNewMsgOutputStream() returns a temp file, which is appended to the mbox upon completion. So sequence would be enforced. Bug 1717147, in progress, is to move the quarantining into the mailstore, which would make this option trivial, if we were prepared to swallow the cost of the extra write...
I've got a few other possible solutions, but they're all awful and/or crazy.
I'll be investigating ideas 1 & 2 further.
|
|
Assignee | |
Comment 16•4 years ago
|
||
I was jumping the gun in comment #15 (although I think it's still a totally legitimate issue that will likely bite one day!).
The problem seems to be in the reading side, rather than writing.
The "copy message" setup for the source side of things occurs in nsMailboxProtocol::Initialize(). When the problem occurs, it starts picking up a dud offset within the mbox file, and a message size of zero message. Hence all the blank messages appearing in the result.
Digging into it now.
|
|
Assignee | |
Comment 17•4 years ago
|
||
|
|
Assignee | |
Comment 18•4 years ago
|
||
That patch isn't a fix, but it does remove a bunch of error output (in debug builds anyway) that occur when transactions are being composed. The transaction manager checks to see if multiple transactions can be coalesced into one. The patch just gets out operation to return "nope" instead of throwing an error, which seems a lot more polite.
|
|
Assignee | |
Comment 19•4 years ago
|
||
I was jumping the gun in comment #16 too :-)
The issue is that when you hold down the 'A' key, it repeatedly creates MessageArchiver objects containing the selected message.
Sometimes this is fine - the archiving happens very quickly, the message is moved, the folder receives a notification to say the message is gone, and the selection is updated to the next message before the next 'A' registers.
But sometimes, the 'A' registers before the archiver has finished, and so multiple archivers are set running containing the same nsIMsgDBHdr. The first one runs fine, but when the second one kicks off, the message has been moved.
By this time, the message header is all kinds of wrong. In particular, the messageOffset field is blank - this is the field which says where in the mbox file a message lives. And when the messageOffset is blank, reading it returns the messageKey instead (it totally shouldn't do this). And when that happens... all kinds of crazy stuff can happen.
So.
I think the proper fix is in the front end after all.
(that said, there are a bunch of other backend/mailstore things that I'll be tightening up in the course of the maildir work).
There should really only be one MessageArchiver active at any one time. Alternatively, you could have multiple ones going as long as they didn't overlap which messages/folders they involved, but that just sounds ultra overcomplicated.
I'll have a bit of a look into how to enforce only-one-MessageArchiver-at-a-time, but I suspect it might be better handled by someone who actually knows what they're doing in the GUI side...
|
|
||
Updated•4 years ago
|
|
|
||
Comment 20•4 years ago
|
||
Making MessageArchiver a singleton seems complicated as well, since it needs it's member variables set...
We might be able to avoid the problem by at https://searchfox.org/comm-central/rev/2372e390f3b6add0f2b3530964472fa0e2e4463c/mail/base/content/mainKeySet.inc.xhtml#71
<key id="key_archive" key="&archiveMsgCmd.key;" oncommand="if (!event.repeat) goDoCommand('cmd_archive')"/>
|
||
Comment 21•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/8c7a5f12f74c
Make nsMsgTxn::Merge a no-op rather than unimplemented. r=mkmelin
|
|
Assignee | |
Comment 22•4 years ago
|
||
|
|
Assignee | |
Comment 23•4 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #20)
<key id="key_archive" key="&archiveMsgCmd.key;" oncommand="if (!event.repeat) goDoCommand('cmd_archive')"/>
Hmm. Didn't work for me.
I did get a solution working (patch is comment #22) even if it's a bit ugly. I hacked in a global flag to indicate there is an archive operation in progress, and it just ignores subsequent requests. It uses the archiver oncomplete hook to clear the flag.
Reading through the archiver code, I can't see any circumstances where oncomplete is not called (other than passing in an empty list of messages). But if that's not the case then you could be left in a situation where archiving is left disabled...
|
|
Assignee | |
Comment 24•4 years ago
|
||
|
||
Updated•4 years ago
|
|
|
Assignee | |
Updated•4 years ago
|
|
|
||
Updated•4 years ago
|
|
|
||
Comment 25•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ea2ce7b545d4
Only allow one active MessageArchive operation at a time. r=mkmelin
|
|
||
Updated•4 years ago
|
|
|
||
Comment 26•4 years ago
|
||
Please request 91 uplift.
|
||
Comment 27•4 years ago
|
||
Comment on attachment 9236093 [details]
Bug 1705824 - Make nsMsgTxn::Merge a no-op rather than unimplemented. r?mkmelin
[Approval Request Comment]
Been on beta since TB 92.
|
|
||
Comment 28•4 years ago
|
||
Comment on attachment 9236093 [details]
Bug 1705824 - Make nsMsgTxn::Merge a no-op rather than unimplemented. r?mkmelin
[Triage Comment]
Approved for esr91
|
|
||
Comment 29•4 years ago
|
||
| bugherder uplift | ||
Description
•