Closed Bug 1493143 Opened 6 years ago Closed 6 years ago

Allow folder move/copies to fail gracefully, doesn't notify

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr6064+ fixed, thunderbird63 wontfix, thunderbird64 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 64.0
Tracking Flags:
Tracking Status
thunderbird_esr60 64+ fixed
thunderbird63 --- wontfix
thunderbird64 --- fixed

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file)

Proposed patch
2.72 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr60+
Details | Diff | Splinter Review 
If a folder move/copy operation fails:

* IMAP tries and fails to find the new copy, so just doesn't bother to notify.

* POP3/Local Folders tries to notify for the failure with the destination folder.

Unfortunately the copy service only accepts a notification with the new folder, which is impossible in the case of a failure.
Attached patch Proposed patch — — Splinter Review 
Allow the copy service to accept either style of notification in the case of a folder move/copy, and change IMAP to always notify using the new parent. (POP3/Local Folders was too awkward to change.)
Attachment #9010927 - Flags: review?(jorgk)
Comment on attachment 9010927 [details] [diff] [review]
Proposed patch

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

Since the departure of Kent and Joshua the reviewer situation in mailnews/ is a big gloomy, so at times you need to educate/train the junior reviewer a bit ;-)

Can you please explain your change. What is the connection between the copyService->NotifyCompletion() call and the nsMsgCopyService::FindRequest() function? Why is moving the |copyRequest->m_srcSupport.get() == aSupport && ...| block up required.

Furthermore, I've checked copyService->NotifyCompletion()
https://dxr.mozilla.org/comm-central/search?q=copyService-%3ENotifyCompletion+path%3Acomm%2F&redirect=false
The other caller in nsImapMailFolder.cpp already passes 'this', and so does the caller nsLocalMailFolder.cpp. What does 'this' represent in those cases and why is it too awkward to change in the local folder case?

::: mailnews/base/src/nsMsgCopyService.cpp
@@ +381,5 @@
>    {
>      copyRequest = m_copyRequests.ElementAt(i);
> +    if (copyRequest->m_srcSupport.get() == aSupport &&
> +        copyRequest->m_dstFolder.get() == dstFolder)
> +        break;

Nit: Indented too far.
Comment on attachment 9010927 [details] [diff] [review]
Proposed patch

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

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +5457,5 @@
>            NS_ENSURE_SUCCESS(rv, rv);
>            nsCOMPtr<nsIMsgFolder> srcFolder = do_QueryInterface(m_copyState->m_srcSupport);
>            if (srcFolder)
>            {
> +            copyService->NotifyCompletion(m_copyState->m_srcSupport, this, aExitCode);

Does this notify with the original folder instead of the destination? Then it could use a comment.
Summary: Allow folder move/copies to fail gracefully → Allow folder move/copies to fail gracefully, doesn't notify
(In reply to Jorg K from comment #2)
> Can you please explain your change. What is the connection between the
> copyService->NotifyCompletion() call and the nsMsgCopyService::FindRequest()
> function? Why is moving the |copyRequest->m_srcSupport.get() == aSupport &&
> ...| block up required.
> 
> Furthermore, I've checked copyService->NotifyCompletion()
> https://dxr.mozilla.org/comm-central/search?q=copyService-
> %3ENotifyCompletion+path%3Acomm%2F&redirect=false
> The other caller in nsImapMailFolder.cpp already passes 'this', and so does
> the caller nsLocalMailFolder.cpp. What does 'this' represent in those cases
> and why is it too awkward to change in the local folder case?

The copy service wants to avoid two copies happening to the same folder at the same time. To achieve this, it queues the requests, and if a request collides with one in the queue, it knows not to start it copying yet, but to wait for the completion of the previous request. When a request completes it needs to be removed from the queue.

In the case of a message copy this is reasonably straightforward: the destination is a folder, and when the message copy finishes, that folder then notifies the copy service with itself as the destination.

In the case of a folder copy however the original design cheated. In particular, a folder copy creates a new folder (with hierarchy where applicable) and usually includes a message copy of the messages in the folder. (If there are no messages, the folder actually generates a fake notification; this was the bit where I couldn't work out whether to notify using the destination rather than the newly created folder.) The message copy operation then goes and notifies the copy service when it's done, as if the original operation had been a message copy. However, this notification doesn't know that this was really a folder copy, so it notifies using the newly created folder. The folder copy service therefore assumes that if it's dequeuing a folder copy that the notification will arrive using the newly created folder. This only works if the copy actually got as far as creating the new folder, of course.

The change to the copy service means that it allows the copy to be dequeued using either the destination parent folder or the newly created folder (for folder copies). Since dequeuing using the destination parent is the desired behaviour for all other copy operations anyway, it was easier to have that first, as the other block contains continue statements which would have had to have been adapted.

As :aceman mentioned, since the copy service now allows either destination folder, I simplified the IMAP code as it only needs to pass the parent destination folder.

> ::: mailnews/base/src/nsMsgCopyService.cpp
> @@ +381,5 @@
> >    {
> >      copyRequest = m_copyRequests.ElementAt(i);
> > +    if (copyRequest->m_srcSupport.get() == aSupport &&
> > +        copyRequest->m_dstFolder.get() == dstFolder)
> > +        break;
> 
> Nit: Indented too far.

Hey, it was just copy/pasted indentation...
(In reply to aceman from comment #3)
> > +            copyService->NotifyCompletion(m_copyState->m_srcSupport, this, aExitCode);
> 
> Does this notify with the original folder instead of the destination? Then
> it could use a comment.

What do you mean by original folder? The original operation transfers from `m_srcSupport` to `this`. If successful, that includes creation of a new subfolder of this folder. The old notification always used that new subfolder. The new notification allows either that subfolder or this folder.
Comment on attachment 9010927 [details] [diff] [review]
Proposed patch

OK, this is much clearer now.

You haven't answered my questions re. your (lapsed?) Level 3 access. Since I haven't heard any protest, I've landed your patches at my leisure. In the future it would be great to provide a full HG patch with
  # User Neil Rashbrook <neil@parkwaycc.co.uk>
and a decent commit message. The bug summary isn't always the best choice. Since I'm landing most patches here, I always have to correct commit messages.
Attachment #9010927 - Flags: review?(jorgk) → review+
Target Milestone: --- → Thunderbird 64.0
https://hg.mozilla.org/comm-central/rev/32f0ea906e38c2916eb4f45f92babe129d71dc08
Allow copy service to find request by dstFolder for folder copy/move; change IMAP notify accordingly. r=jorgk
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Jorg K from comment #6)
> You haven't answered my questions re. your (lapsed?) Level 3 access.
Sorry, I haven't actually checked yet.

> Since I haven't heard any protest, I've landed your patches at my leisure.
> In the future it would be great to provide a full HG patch
Unfortunately that means committing changes which isn't my preferred workflow (mq doesn't help).

> and a decent commit message. The bug summary isn't always the best choice.
> Since I'm landing most patches here, I always have to correct commit messages.
Sorry for making so much work for you.
Comment on attachment 9010927 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Regression caused by (bug #): Time immemorial, probably.
User impact if declined: Potential for moves/copies to get stuck if a folder move/copy fails.
Testing completed (on c-c, etc.): Baked on c-c for a couple of weeks
Risk to taking this patch (and alternatives if risky): Low, only affects folder move/copy.
Attachment #9010927 - Flags: approval-comm-esr60?
Comment on attachment 9010927 [details] [diff] [review]
Proposed patch

Let's put it into a beta first (if we're doing another 63 one).
Attachment #9010927 - Flags: approval-comm-beta+
... by which I mean, that while there were test failures, they either already had bugs in Bugzilla, or they appeared intermittently across several recent Try pushes, and didn't appear to be in relevant code anyway.
@Jörg: Can we push this to beta?
Yes, planned for later today.
Here we have a problem. Our policy is to have any uplift (except theme/CSS changes and "obvious" regression fixes) be on a beta version for two weeks or so before getting into the ESR. By the looks of it TB 63.0b2 was delayed so much that it got canned altogether. After the 22nd of October we can already ship a TB 64 beta 1. I'm not sure about the timing of TB 60.3 ESR, but without any beta exposure I'm a little reluctant to accept this right now.
FYI, we don't really need this urgently for Owl. This was a TB bug that we found and fixed. It affects TB just as much as it affects Owl. This can wait for a later TB 60 release or not be backported at all, depending on what you think is best.
I usually follow the requests. A beta request wouldn't make sense since it is in TB 64 and will go to beta next week. An ESR request would get this into TB 60.4 in about six weeks time. I've never seen the issue, so I'll leave it to you to request ;-)
Sorry, TB 63b2 never happened.
Comment on attachment 9010927 [details] [diff] [review]
Proposed patch

Good for 60.4.
Attachment #9010927 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: