Closed Bug 28129 Opened 22 years ago Closed 20 years ago

IMAP: Stop doesn't work on multiple Move/Copy messages.

Categories

(MailNews Core :: Backend, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: laurel, Assigned: naving)

Details

Attachments

(2 files, 1 obsolete file)

Using 2000-02-16-08m14 mozilla builds on NT 4.0 and linux rh6.0

Stop doesn't seem to work on disengaging multiple selection move/copy messages. 
From what I've seen, it doesn't work for stopping IMAP->IMAP, IMAP->local or
NNTP->local copy/move of large multiple selections.  (POP multiple select and
move/copy seems to be having separate problems).

1.  From mail window open and login to IMAP account.
2.  Open a large folder, multiple select a lot of messages (I tried ranges of
100-500 and up to a couple thousand (painfully slow)).
3.  Message|Move Message or Copy Message.  Select a fairly empty local folder as
the target/destination folder.
4.  Click the Stop button as soon as you can. (I clicked it a few times to make
sure the click registered).
5.  Note the move/copy appears to still be going on. Note the progress doesn't
stop until messages are done.  
6.  Check the destination folder and see (eventually) all the messages were
copied/moved.

Note:  when copying NNTP to local, there seems to be more trouble involved.  In
some cases I never came out of the progress state.

Actual result:  Stop doesn't stop process.

Expected result:  Stop should Stop (now or soon).
QA Contact: lchiang → laurel
Jeff works on move/copy - there must be some error status he can check when stop 
is hit.
Assignee: bienvenu → jefft
Status: NEW → ASSIGNED
Target Milestone: M15
Mass moving to M16 to get these off the M15 radar.  Please let me know if this
is really an M15 stopper.
Target Milestone: M15 → M16
Mail review recommends nsbeta2.
Keywords: nsbeta2
Mass moving M16 to M17 - look for nsbeta2 before anything else.
Target Milestone: M16 → M17
Putting on [nsbeta2-] radar. Not critical to beta2.
Keywords: nsbeta3
Whiteboard: [nsbeta2-]
general mention item ("stop doesn't work in some situations") for b2 relnotes
Keywords: relnote2
Keywords: mail2
Target Milestone: M17 → M18
Assignee: jefft → bienvenu
Status: ASSIGNED → NEW
Whiteboard: [nsbeta2-] → [nsbeta2-][nsbeta3+]
David, this is pretty important.  Can you take this one?
I'll take it but I can't promise anything. This will be a royal pain to fix, I'm
sure.
I can't get stop to work on any imap operations, e.g., message loading - the
mock channel is never getting cancelled, which makes it hard to implement any
stop handling, or even tell where the actual problem is, if any.
Attached patch part of fix? (obsolete) — Splinter Review
Scott, could you look at the attached patch and let me know if it makes sense?
The reason stop doesn't work anymore in general is that we're cancelling the
wrong load group. It seems like urls are run through the doc  shell now, so we
need to get the doc shell. With this change, I can stop message loads. Message
move/copies still aren't stoppable because we're not passing the msg window into
the copy message call.
Status: NEW → ASSIGNED
fix in hand
Keywords: nsbeta2
Whiteboard: [nsbeta2-][nsbeta3+] → [nsbeta3+]fix in hand
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Using aug25 commercial build on Linux, mac and NT:
I don't see that this is working. Even on my very slow machine, when I click
Stop in the midst of a 200+ message move I see the barber pole progress meter
and throbber stop, then I can't select or do anything for a minute and whoosh --
all the messages which were being moved are indeed moved from the original
folder.

It seems the Stop only stops display/foreground activity, not background
activity... Will keep trying to see it really work.
are you talking about imap or local? For imap, once we've sent the protocol to
the server, it can't really be stopped, since only one command is sent to the
imap server to do the whole move. Local/imap to local move we can stop in
between messages.
Well, local --> local I've finally gotten to stop, but the Stop button appears
unresponsive after issueing the File/Move/Copy command... I'm clicking it
anyway.  It takes the ui an awful long time to show any response in the display,
not just the button.  This is awful.

I'll keep verifying that the stop does work in local->imap, but I think I may
log a ui bug about the button not appearing to work.
We do seem to lock up the ui process the move/copy operation. Perhaps Jeff or
Scott might know why - I think it might have to do with the way we stream the
messages.
Using sep6 commercial build, I have not been able to interrupt a local-->imap
copy or move.  The Stop button doesn't enable (bug 50365) and if I click it, the
Stop button will keep enabling and disabling (flashing on/off) it seems with
each message being copied/moved.  All messages wind up at destination. Cannot
stop the process.

I'm going to reopen this, since I've successfully only stopped local-->local
moves/copies when using the "disabled" stop button (Bug 50365 ).

Comments...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
giving back to its rightful owner :-)
Assignee: bienvenu → jefft
Status: REOPENED → NEW
Accepting and clear the fix in hand status ....
Status: NEW → ASSIGNED
Whiteboard: [nsbeta3+]fix in hand → [nsbeta3+]
Welcome back, Jeff.
nsbeta3-
Whiteboard: [nsbeta3+] → [nsbeta3-][cut 9/11]
Target Milestone: M18 → Future
Keywords: relnote3, relnoteRTM
sorry for the extra email. Removing mail2 keyword.
Keywords: mail2
Keywords: relnote2, relnote3
Whiteboard: [nsbeta3-][cut 9/11] → [nsbeta3-][cut 9/11] relnote-user
reassigning jefft's bugs to naving
Assignee: jefft → naving
Status: ASSIGNED → NEW
nominating for 0.9.2 
Keywords: nsbeta1
QA Contact: laurel → sheelar
Target Milestone: Future → mozilla0.9.9
Keywords: nsbeta1, nsbeta3nsbeta1+
Priority: P3 → P2
Blocks: 122274
Status: NEW → ASSIGNED
Keywords: nsbeta1+nsbeta1-
Whiteboard: [nsbeta3-][cut 9/11] relnote-user → relnote-user
Target Milestone: mozilla0.9.9 → mozilla1.2
Navin was in the process of working on this, so I'll move it back.
No longer blocks: 122274
Keywords: nsbeta1-nsbeta1+
Target Milestone: mozilla1.2 → mozilla0.9.9
Attached patch proposed fixSplinter Review
Stop button works for local and we are deleting failed local move/copies
correctly.

For imap the fix is to AddChannelToLoadGroup() for onlineMove/copy or
AppendMsgFromFile (local -> imap). this will allow the stop button to
work and then ofcourse we have to handle the failure cases by sending
failed notification and not doing undo etc..
Attachment #12448 - Attachment is obsolete: true
cc bienvenu for review
Keywords: relnoteRTM
Summary: Stop doesn't work on multiple Move/Copy messages. → IMAP: Stop doesn't work on multiple Move/Copy messages.
Whiteboard: relnote-user
off the top of my head, nsIMsgHdrs should not be referred to from other threads;
they're not threadsafe, so using IMPL_THREADSAFE is just hiding the problem. Is
the imap thread now referring to msg hdrs?
I was not sure about IMPL_THREADSAFE. I believe this is the correct fix.
that just removes the assertion; it's not the correct fix. This code is broken,
and has been broken for a long time. This code
nsImapProtocol::OnAppendMsgFromFile, is running on the imap thread, and poking
at the db objects, which is absolutely wrong - the db objects are not
thread-safe and should not be referred to from the imap thread. They should only
be referred to from the ui thread. We go to an enormous amount of trouble to
make sure we're thread-safe, but this one slipped in.  The fix is to proxy over
to the ui thread to find out info about the msg hdr object. To do this, we'd
need one of the sinks (folder, server, miscellaneous, or extensions - I'd
probably go with the message sink, and add an interface to it something like 

boolean isCurMoveCopyMessageRead(in nsIImapUrl runningUrl);

and then add code to nsImapMailFolder like this:

NS_IMETHODIMP nsIMapMailFolder::IsCurMoveCopyMessageRead(nsIImapUrl *runningUrl,
PRBool *aREsult)
{
          nsCOMPtr <nsISupports> copyState;
          runningUrl->GetCopyState(getter_AddRefs(copyState));
          if (copyState)
          {
            nsCOMPtr<nsImapMailCopyState> mailCopyState =
do_QueryInterface(copyState, &rv);
            if (mailCopyState)
            {
              nsCOMPtr <nsIMsgDBHdr> curMsg = mailCopyState->m_message;
              PRUint32 flags;

              if (curMsg)
              {
                curMsg->GetFlags(&flags);
                *result =(flags & MSG_FLAG_READ) != 0;
               }
           }
}

anyway, you get the general idea.
logged separate bug to fix the assertion in the correct way. 
It looks basically OK, as near as I can tell.

how come we can remove these lines?

-            }
-            else if (m_copyState->m_isMove && !m_copyState->m_isCrossServerOp
&& srcFolder)
-               srcFolder->EnableNotifications(allMessageCountNotifications,
PR_TRUE);  //enable message count notification even if we failed.

I don't see that we're doing the same thing later on and I couldn't find any
other code that would do that for us. Where does that end up getting called now?
basically cvs diff -uw is to blame. It is done right above if !NS_SUCCEEDED(rv)

+              else
+              {
+                srcFolder->EnableNotifications(allMessageCountNotifications, 
PR_TRUE);
+                srcFolder->NotifyFolderEvent(mDeleteOrMoveMsgFailedAtom);  
+              }
+                
+            }

Comment on attachment 68425 [details] [diff] [review]
proposed fix

d'oh, right. cool, r=bienvenu
Attachment #68425 - Flags: review+
but please make sure you revert the nsMsgHdr change in your tree locally before
you accidentally check it in, thanks!
fixed
Status: ASSIGNED → RESOLVED
Closed: 22 years ago20 years ago
Resolution: --- → FIXED
I'll take this back since sheela will be out for awhile.
QA Contact: sheelar → laurel
Using feb28-mar01 commercial trunk: linux rh6.2, win98, mac OS 10.1

Well, the act of using the Stop button works in all the cases I've tried,
IMAP-->IMAP, IMAP --> local. That was the initial main problem, user being able
to stop, to get out of the activity.

However, I'm seeing some results which don't make inherent sense to me.
Particularly in a move IMAP to IMAP, I'm stopping, but then find the second
opening of the destination reveals the messages' move picks back up again.  Will
log my issues separately.

Marking this verified based on the Stop does stop immediate activity and allows
user to do something else.
Status: RESOLVED → VERIFIED
OK, logged bug 128546 as one separate issue to Stop IMAP-->IMAP move/copy.
removed the item for this bug from the release notes for 0.9.9 and beyond,
because the bug is fixed now. Let me know if you think the item should be 
re-added.
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.