167579 - Make copyService more robust to handle failure cases.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Navin Gupta]

We talked about some cases over aim.

Comment 1

[Navin Gupta]

Attachment: proposed fix

I have addressed error handling cases in CopyMessages and CopyFileMessage for
both local and imap. Basically I am passing srcSupport to OnCopyCompleted so
that it can be called when we haven't even inited a copyState object. Also I
removed some of null check for arguments because copyService does that and we
are using copyService for all copies (inlcluding patches I have, but not
checked in). In copyService I made it so that we queue request only if
destination is same and checked it works because we mark copySource processed
once we are issuing a copy so there is no chance of an infinite loop!

David, can you review ? thx.

Comment 2

[Navin Gupta]

still looking may come up w/ better patch. 

Comment 3

[David :Bienvenu]

OK, while you're coming up with another patch:

     if (m_copyState)
-    {
-        nsCOMPtr<nsISupports> srcSupport =
do_QueryInterface(m_copyState->m_srcSupport);
         m_copyState = nsnull;
this can just be
m_copyState = nsnull; right? no need to check if it's not null before.

+nsMsgCopyService::QueueRequest(nsCopyRequest* aRequest, PRBool *aResult)
could we make the variable name aResult have a more meaningful?

Comment 4

[Navin Gupta]

No, I thought about it doesn't look to be possible. yes, I have changed those
two lines in my tree. Can you do a full review because I have lot of changes in
my tree. 

Comment 5

[David :Bienvenu]

+    nsresult QueueRequest(nsCopyRequest* aRequest, PRBool *aResult);
this line should change too (more descriptive var name)

this comment should change, right? To something about "if there wasn't another
request for this dest folder..." or something like that.
//if only one request, then go ahead and do the copy, otherwise the request is
queued

fix (remove) the comment here:
+          return OnCopyCompleted(srcSupport, PR_FALSE); // I think we want to
return ok here


Here:
all of the early returns here will leave us with a copy request left hanging
around, right? We don't want that. Either all these control paths should call
::OnCopyCompleted or we could have some way in the copy service of knowing that
the copy never got started (checking the rv, as I suggested before, is one
possibility). Another possibility is to have some sort of accessor on
nsIMsgFolder that tells you whether a copy state is attached or not. We really
should strive to not have returns that leave the copy request hanging around,
unless we're going to check for an error return in the copy service.
@@ -1630,24 +1630,19 @@
 		return NS_MSG_FOLDER_BUSY;
 
 	rv = GetPath(getter_AddRefs(pathSpec));
-	if (NS_FAILED(rv)) goto done;
+	NS_ENSURE_SUCCESS(rv, rv);
 
 	rv = pathSpec->GetFileSpec(&path);
-  if (NS_FAILED(rv)) goto done;
+  NS_ENSURE_SUCCESS(rv, rv);
 

Comment 6

[Navin Gupta]

I have fixes for those minor nits. Those early returns are ok because they are
in InitCopyState which if fails is caught in CopyMessages or CopyFileMessage
where we call OnCopyCompleted. 

Comment 7

[Navin Gupta]

David, just a reminder to complete the review.

Comment 8

[David :Bienvenu]

sorry, I have to apply this diff to see the routines in context, but the
attachment is not an applyable diff, so I have to do it by hand. Bear with me,
please.

Comment 9

[David :Bienvenu]

nsresult nsImapMailFolder::CopyMessagesOffline(nsIMsgFolder* srcFolder,

at the end of this function, we should use your new mechanism of calling
OnCopyCompleted() now that it works w/o a copy state.

nsMsgLocalMailFolder::OnCopyCompleted(nsISupports *srcSupport, PRBool
moveCopySucceeded)
{
  if (mCopyState)
  {
    delete mCopyState;
    mCopyState = nsnull;
  }
as in the imap case, no need to check null mCopyState here - delete is a noop
with a null pointer.

Other than that, it seems fine, modulo a lot of testing.

Comment 10

[David :Bienvenu]

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

sr=bienvenu, but since there are about 5 outstanding changes, it would be nice
to have a new patch for whoever's r='ing. What I do when I have lots of changes
in my tree is do a whole diff, and edit out the files that aren't part of the
patch.	It makes life easier if we need to try to take the patch to another
branch, for example.

Comment 11

[Navin Gupta]

I understand but I have changes for other bug in the same file. I'll try to
attach a new patch. 

Comment 12

[David :Bienvenu]

unless the changes actually overlap on a line basis, you can remove diff
segments from the diffs in a particular file (if you're careful :-) )

Comment 13

[Navin Gupta]

that is what I did for last one.

Comment 14

[Navin Gupta]

Attachment: patch w/ those minor changes

Cavin, can yor r=? this patch. I have played w/ imap(cross-server,
offline),local, news and search for sometime now. everything looks ok.

Comment 15

[Cavin Song]

Comment on attachment 98625 [details] [diff] [review]
patch w/ those minor changes

r=cavin.

Comment 16

[Navin Gupta]

Comment on attachment 98625 [details] [diff] [review]
patch w/ those minor changes

carrying fwd sr=bienvenu

Comment 17

[Navin Gupta]

tested compose also -> save as draft/template send copy and send later copying
to unsent messages for local and imap. 

Comment 18

[Navin Gupta]

fix checked in. This fix could not make 1.2a so look out for dups. 

Comment 19

[Navin Gupta]

*** Bug 167590 has been marked as a duplicate of this bug. ***

Comment 20

[grylchan]

Navin or David,
Not sure what i should be testing? I see Navin's test
comment 17.

I assume testing cases that would generate an error mesg,
When dong a save or copy?

Comment 21

[Navin Gupta]

yes, testing more error-specific cases than normal ones.