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

VERIFIED FIXED in mozilla0.9.9

Status

P2
normal
VERIFIED FIXED
19 years ago
11 years ago

People

(Reporter: laurel, Assigned: naving)

Tracking

Trunk
mozilla0.9.9

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

19 years ago
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).
(Reporter)

Updated

19 years ago
QA Contact: lchiang → laurel

Comment 1

19 years ago
Jeff works on move/copy - there must be some error status he can check when stop 
is hit.
Assignee: bienvenu → jefft

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M15

Comment 2

19 years ago
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

Comment 3

19 years ago
Mail review recommends nsbeta2.
Keywords: nsbeta2

Comment 4

19 years ago
Mass moving M16 to M17 - look for nsbeta2 before anything else.
Target Milestone: M16 → M17

Comment 5

19 years ago
Putting on [nsbeta2-] radar. Not critical to beta2.
Keywords: nsbeta3
Whiteboard: [nsbeta2-]
(Reporter)

Comment 6

19 years ago
general mention item ("stop doesn't work in some situations") for b2 relnotes
Keywords: relnote2

Updated

19 years ago
Keywords: mail2
Target Milestone: M17 → M18

Updated

19 years ago
Assignee: jefft → bienvenu
Status: ASSIGNED → NEW
Whiteboard: [nsbeta2-] → [nsbeta2-][nsbeta3+]

Comment 7

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

Comment 8

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

Comment 9

19 years ago
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

19 years ago
Posted patch part of fix? (obsolete) — Splinter Review

Comment 11

19 years ago
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

Comment 12

19 years ago
fix in hand
Keywords: nsbeta2
Whiteboard: [nsbeta2-][nsbeta3+] → [nsbeta3+]fix in hand

Comment 13

19 years ago
fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Reporter)

Comment 14

19 years ago
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

19 years ago
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.
(Reporter)

Comment 16

19 years ago
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

19 years ago
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.
(Reporter)

Comment 18

19 years ago
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 → ---

Comment 19

19 years ago
giving back to its rightful owner :-)
Assignee: bienvenu → jefft
Status: REOPENED → NEW

Comment 20

19 years ago
Accepting and clear the fix in hand status ....
Status: NEW → ASSIGNED
Whiteboard: [nsbeta3+]fix in hand → [nsbeta3+]
(Reporter)

Comment 21

19 years ago
Welcome back, Jeff.

Comment 22

19 years ago
nsbeta3-
Whiteboard: [nsbeta3+] → [nsbeta3-][cut 9/11]
Target Milestone: M18 → Future
(Reporter)

Updated

19 years ago
Keywords: relnote3, relnoteRTM

Comment 23

19 years ago
sorry for the extra email. Removing mail2 keyword.
Keywords: mail2
Keywords: relnote2, relnote3
Whiteboard: [nsbeta3-][cut 9/11] → [nsbeta3-][cut 9/11] relnote-user

Comment 24

19 years ago
reassigning jefft's bugs to naving
Assignee: jefft → naving
Status: ASSIGNED → NEW
(Assignee)

Comment 25

18 years ago
nominating for 0.9.2 
(Assignee)

Updated

18 years ago
Keywords: nsbeta1
(Reporter)

Updated

18 years ago
QA Contact: laurel → sheelar
(Assignee)

Updated

18 years ago
Target Milestone: Future → mozilla0.9.9

Updated

18 years ago
Keywords: nsbeta1, nsbeta3 → nsbeta1+

Updated

18 years ago
Priority: P3 → P2

Updated

17 years ago
Blocks: 122274
Status: NEW → ASSIGNED
Keywords: nsbeta1+ → nsbeta1-
Whiteboard: [nsbeta3-][cut 9/11] relnote-user → relnote-user
Target Milestone: mozilla0.9.9 → mozilla1.2

Comment 26

17 years ago
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
(Assignee)

Comment 27

17 years ago
Posted 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
(Assignee)

Comment 28

17 years ago
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

Comment 29

17 years ago
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?
(Assignee)

Comment 30

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

Comment 31

17 years ago
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.
(Assignee)

Comment 32

17 years ago
logged separate bug to fix the assertion in the correct way. 

Comment 33

17 years ago
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?
(Assignee)

Comment 34

17 years ago
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

17 years ago
Comment on attachment 68425 [details] [diff] [review]
proposed fix

d'oh, right. cool, r=bienvenu
Attachment #68425 - Flags: review+

Comment 36

17 years ago
but please make sure you revert the nsMsgHdr change in your tree locally before
you accidentally check it in, thanks!
(Assignee)

Comment 37

17 years ago
fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 38

17 years ago
I'll take this back since sheela will be out for awhile.
QA Contact: sheelar → laurel
(Reporter)

Comment 39

17 years ago
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
(Reporter)

Comment 40

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

Comment 41

17 years ago
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.