1731177 - Refactor nsMsgLocalMailFolder message copying.

Status: NEW

(MailNews Core :: Backend, task)

Description

[Ben Campbell]

There's a lot of cleanup and simplification to be done in the nsLocalMailFolder message copy code. At the moment it's really hard to follow, and very brittle.

• It uses different paths for single and multiple message copies CopyMessageTo() vs CopyMessagesTo(). These could be unified.
• The nsLocalMailCopyState is waaay too complicated and it's really unclear which bits are used by which code paths.
• The use of the nsICopyMessageListener callbacks is unclear and they seem to be invoked rather inconsistently.
• There's a lot more that needs to be picked through.

Comment 1

[Ben Campbell]

See also:
Bug 1717375 - The short-cut used for direct maildir->maildir copying was doubling up the nsIMsgCopyServicerListener callbacks. (OnStartCopy()/OnStopCopy()).
The maildir->maildir path also

• duplicates code for doing POP3 deletes (when moving)
• ignores some other things it should be doing (there's some keyword-related stuff)
• entirely ignores the nsLocalCopyState data

Comment 2

[Ben Campbell]

I think Bug 1749276 might be a symptom of the complexity of message-copying code.

Comment 3

[gene smith]

I saw a weird bug that popped up a continuous string of alerts about unable to copy something to Local Folder. I was unable to do or change anything because the message would pop right back up. The error occurred here: https://searchfox.org/comm-central/rev/1de28ff4bff691f753d7b30b2f7d7fcc9dc546a4/mailnews/local/src/nsLocalMailFolder.cpp#1976

I wasn't trying to copy anything but I found a filter I had entered, just for testing something, to match all and copy to a Local Folder, but no new messages arrived in the filter source Inbox so don't know why a copy was attempted.

I couldn't disable the filter due to the continuous popups so I killed TB and changed the alert call at line 1976 to just a printf and let it run again and this freed it up and I disabled the filter.

In the console I saw thousands of my printf's but right before they occurred there is an assert looking for the leading mbox specific "From " thing:
https://searchfox.org/comm-central/rev/1de28ff4bff691f753d7b30b2f7d7fcc9dc546a4/mailnews/local/src/nsLocalMailFolder.cpp#1966. For some reason in the past I had converted my Local Folders from mbox to maildir format and don't remember why. So at this point in the code I would assume that looking for a "From " is not really needed.

But the error alert seems to be due to a null m_fileString. Then I found the reference to this bug so reporting it here rather than in a new bug: https://searchfox.org/comm-central/rev/1de28ff4bff691f753d7b30b2f7d7fcc9dc546a4/mailnews/local/src/nsLocalMailFolder.cpp#1875
I couldn't duplicate the errors when I enabled the filter and let it copy to Local Folder again.

Comment 4

[Ben Campbell]

Attachment: messagecopy.md

I finally sat down and traced my way through the code path taken during a local folder -> local folder message copy operation.
I'm posting my notes mainly for my own reference, when I go to refactoring this stuff.
It's already been a really useful exercise to me but I suspect it'd be of limited use to anyone else... other than for entertainment and/or sheer horror ;-)

The notes cut a lot of corners to try and keep a straight narrative path through the code, and it doesn't really convey the sheer intertwinedness of everything. So - to me anyway - these notes suggest some obvious refactors that would simplify things a lot, but actually putting them into practice is tricky without breaking other aspects.

Comment 5

[Wayne Mery (:wsmwk)]

Ben, if you have an organizational strategy that makes better sense, please fix whatever I have done.

Comment 6

[ISHIKAWA, Chiaki]

(In reply to Ben Campbell from comment #4)

Created attachment 9295369 [details]
messagecopy.md

I finally sat down and traced my way through the code path taken during a local folder -> local folder message copy operation.
I'm posting my notes mainly for my own reference, when I go to refactoring this stuff.
It's already been a really useful exercise to me but I suspect it'd be of limited use to anyone else... other than for entertainment and/or sheer horror ;-)

The notes cut a lot of corners to try and keep a straight narrative path through the code, and it doesn't really convey the sheer intertwinedness of everything. So - to me anyway - these notes suggest some obvious refactors that would simplify things a lot, but actually putting them into practice is tricky without breaking other aspects.

Ben, for multiple messages copy, does the current code act sanely when an I/O error causes a message copy to fail?
Several years ago, I thought the code completely ignores the error reporting.
The async nature of the code seemed to hide the failure under the rug and the user was not told about it if my reading of the code was correct.
Now that you have written the operation up in its very rough framework, it would be nice to see if the
error during multi-message copying is noticed and handled sanely and reported to the user with a dialog or something.

Comment 7

[Ben Campbell]

I honestly couldn't say for sure either way. My money would be on errors not being handled very well, but I'd love to be proved wrong.