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

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[laurel]

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).

Comment 1

[David :Bienvenu]

Jeff works on move/copy - there must be some error status he can check when stop 
is hit.

Comment 2

[selmer (gone)]

Mass moving to M16 to get these off the M15 radar.  Please let me know if this
is really an M15 stopper.

Comment 3

[selmer (gone)]

Mail review recommends nsbeta2.

Comment 4

[selmer (gone)]

Mass moving M16 to M17 - look for nsbeta2 before anything else.

Comment 5

[leger]

Putting on [nsbeta2-] radar. Not critical to beta2.

Comment 6

[laurel]

general mention item ("stop doesn't work in some situations") for b2 relnotes

Comment 7

[selmer (gone)]

David, this is pretty important.  Can you take this one?

Comment 8

[David :Bienvenu]

I'll take it but I can't promise anything. This will be a royal pain to fix, I'm
sure.

Comment 9

[David :Bienvenu]

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.

Comment 10

[David :Bienvenu]

Attachment: part of fix?

Comment 11

[David :Bienvenu]

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.

Comment 12

[David :Bienvenu]

fix in hand

Comment 13

[David :Bienvenu]

fix checked in.

Comment 14

[laurel]

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.

Comment 15

[David :Bienvenu]

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.

Comment 16

[laurel]

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.

Comment 17

[David :Bienvenu]

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.

Comment 18

[laurel]

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...

Comment 19

[David :Bienvenu]

giving back to its rightful owner :-)

Comment 20

[jefft]

Accepting and clear the fix in hand status ....

Comment 21

[laurel]

Welcome back, Jeff.

Comment 22

[selmer (gone)]

nsbeta3-

Comment 23

[scottputterman]

sorry for the extra email. Removing mail2 keyword.

Comment 24

[scottputterman]

reassigning jefft's bugs to naving

Comment 25

[Navin Gupta]

nominating for 0.9.2 

Comment 26

[scottputterman]

Navin was in the process of working on this, so I'll move it back.

Comment 27

[Navin Gupta]

Attachment: proposed fix

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..

Comment 28

[Navin Gupta]

cc bienvenu for review

Comment 29

[David :Bienvenu]

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?

Comment 30

[Navin Gupta]

Attachment: fix for the assertion when copying local msg to imap

I was not sure about IMPL_THREADSAFE. I believe this is the correct fix.

Comment 31

[David :Bienvenu]

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.

Comment 32

[Navin Gupta]

logged separate bug to fix the assertion in the correct way. 

Comment 33

[David :Bienvenu]

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?

Comment 34

[Navin Gupta]

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 35

[David :Bienvenu]

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

d'oh, right. cool, r=bienvenu

Comment 36

[David :Bienvenu]

but please make sure you revert the nsMsgHdr change in your tree locally before
you accidentally check it in, thanks!

Comment 37

[Navin Gupta]

fixed

Comment 38

[laurel]

I'll take this back since sheela will be out for awhile.

Comment 39

[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.

Comment 40

[laurel]

OK, logged bug 128546 as one separate issue to Stop IMAP-->IMAP move/copy.

Comment 41

[Dawn Endico]

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.