Last Comment Bug 738651 - message move/copying issues with maildir
: message move/copying issues with maildir
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 2 votes (vote)
: Thunderbird 14.0
Assigned To: David :Bienvenu
:
Mentors:
Depends on:
Blocks: 58308 maildirblockers
  Show dependency treegraph
 
Reported: 2012-03-23 08:16 PDT by David :Bienvenu
Modified: 2015-02-20 18:18 PST (History)
7 users (show)
mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (8.65 KB, patch)
2012-03-23 08:16 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
fix addressing comments, add missing file (10.20 KB, patch)
2012-03-23 16:14 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
ugh, you're right. (10.16 KB, patch)
2012-03-23 17:18 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
fix with assertion for null undo txn (10.89 KB, patch)
2012-04-01 17:17 PDT, David :Bienvenu
neil: review+
Details | Diff | Splinter Review
fix for sr (9.47 KB, patch)
2012-04-04 13:37 PDT, David :Bienvenu
standard8: superreview+
Details | Diff | Splinter Review

Description David :Bienvenu 2012-03-23 08:16:48 PDT
Created attachment 608714 [details] [diff] [review]
proposed fix

There are several issues with maildir message move copying. Undo doesn't work, copying doesn't send the right done notification, and pop3 filters don't work with quarantining. I'm trying to work through the issues. This fixes a couple of the basic ones.

I had to add the do_QueryInterface to the srcSupports line because the code that sets up the srcSupports also does a QI (in nsMsgCopyService::CopyMessages), and if I don't, they don't match.
Comment 1 neil@parkwaycc.co.uk 2012-03-23 15:27:43 PDT
Comment on attachment 608714 [details] [diff] [review]
proposed fix

>   boolean copyMessages(in boolean isMove,
>                        in nsIArray aHdrArray,
>                        in nsIMsgFolder aDstFolder,
>-                       in nsIMsgCopyServiceListener aListener);
>+                       in nsIMsgCopyServiceListener aListener,
>+                       out nsITransaction aUndoAction);
I don't see any callers of this method in this patch?

> NS_IMETHODIMP
> nsMsgBrkMBoxStore::CopyMessages(bool isMove, nsIArray *aHdrArray,
>                                nsIMsgFolder *aDstFolder,
>                                nsIMsgCopyServiceListener *aListener,
>+                               nsITransaction **aUndoAction,
>                                bool *aCopyDone)
> {
>   NS_ENSURE_ARG_POINTER(aHdrArray);
>   NS_ENSURE_ARG_POINTER(aDstFolder);
>   NS_ENSURE_ARG_POINTER(aCopyDone);
>   *aCopyDone = false;
>   return NS_OK;
Nit: Should zero out *aUndoAction if not throwing an exception.
(Given this code and the maildir code, perhaps the failure to copy should be indicated by an exception, but I can't tell because I can't see a caller.)

>-  return NS_OK;
>+  return CallQueryInterface(msgTxn, aUndoAction);
msgTxn->forget(aUndoAction); would save an addref/release.
Comment 2 David :Bienvenu 2012-03-23 16:14:33 PDT
Created attachment 608901 [details] [diff] [review]
fix addressing comments, add missing file

msgTxn is a refptr, so .forget doesn't work (should have said that I tried that when I attached the original patch).

copying is "optional" - if the store doesn't do the copy, the backend code does it for the store, so that's not a failure per se.
Comment 3 neil@parkwaycc.co.uk 2012-03-23 17:12:52 PDT
(In reply to David Bienvenu from comment #2)
> msgTxn is a refptr, so .forget doesn't work (should have said that I tried
> that when I attached the original patch).
Strange, my nsAutoPtr.h shows a template <typename I> void forget(I** rhs)
Comment 4 David :Bienvenu 2012-03-23 17:18:04 PDT
Created attachment 608924 [details] [diff] [review]
ugh, you're right.

I must have done ->forget... or dreamt it all.
Comment 5 neil@parkwaycc.co.uk 2012-03-23 17:33:30 PDT
Comment on attachment 608924 [details] [diff] [review]
ugh, you're right.

>+  nsCOMPtr<nsITransaction> undoTxn;
>+  rv = msgStore->CopyMessages(isMove, messages, this, listener,
>+                              getter_AddRefs(undoTxn), &storeDidCopy);
>   if (storeDidCopy)
>+  {
>+    if (msgWindow)
>+    {
>+      nsCOMPtr<nsITransactionManager> txnMgr;
>+      msgWindow->GetTransactionManager(getter_AddRefs(txnMgr));
>+      if (txnMgr)
>+        txnMgr->DoTransaction(undoTxn);
>+    }
>     return rv;
>+  }
So, there must be a transaction if and only if the store did the copy?
Comment 6 David :Bienvenu 2012-03-23 17:37:11 PDT
(In reply to neil@parkwaycc.co.uk from comment #5)
> Comment on attachment 608924 [details] [diff] [review]
> ugh, you're right.
> 
> >+  nsCOMPtr<nsITransaction> undoTxn;
> >+  rv = msgStore->CopyMessages(isMove, messages, this, listener,
> >+                              getter_AddRefs(undoTxn), &storeDidCopy);
> >   if (storeDidCopy)
> >+  {
> >+    if (msgWindow)
> >+    {
> >+      nsCOMPtr<nsITransactionManager> txnMgr;
> >+      msgWindow->GetTransactionManager(getter_AddRefs(txnMgr));
> >+      if (txnMgr)
> >+        txnMgr->DoTransaction(undoTxn);
> >+    }
> >     return rv;
> >+  }
> So, there must be a transaction if and only if the store did the copy?

Yes, right.
Comment 7 neil@parkwaycc.co.uk 2012-03-28 14:00:30 PDT
(In reply to David Bienvenu from comment #6)
> (In reply to comment #5)
> > (From update of attachment 608924 [details] [diff] [review])
> > >+  nsCOMPtr<nsITransaction> undoTxn;
> > >+  rv = msgStore->CopyMessages(isMove, messages, this, listener,
> > >+                              getter_AddRefs(undoTxn), &storeDidCopy);
> > >   if (storeDidCopy)
> > >+  {
> > >+    if (msgWindow)
> > >+    {
> > >+      nsCOMPtr<nsITransactionManager> txnMgr;
> > >+      msgWindow->GetTransactionManager(getter_AddRefs(txnMgr));
> > >+      if (txnMgr)
> > >+        txnMgr->DoTransaction(undoTxn);
> > >+    }
> > >     return rv;
> > >+  }
> > So, there must be a transaction if and only if the store did the copy?
> 
> Yes, right.

(Sorry for the delay.) So, is there any point in returning both a transaction and a separate boolean that says "hey, check that I returned a transaction!"?
Comment 8 David :Bienvenu 2012-03-28 14:11:31 PDT
(In reply to neil@parkwaycc.co.uk from comment #7)

> 
> (Sorry for the delay.) So, is there any point in returning both a
> transaction and a separate boolean that says "hey, check that I returned a
> transaction!"?

Only if we wanted undo support to be optional, and I'd rather not do that. We could assert if there's no txn returned, though. Or we could figure out if there's a way to implement undo in a store-agnostic way, and then we could check for a null txn.
Comment 9 neil@parkwaycc.co.uk 2012-04-01 16:04:25 PDT
Comment on attachment 608924 [details] [diff] [review]
ugh, you're right.

Sorry, I didn't understand your previous answer, so I'll ask this instead:
>+  nsCOMPtr<nsITransaction> undoTxn;
>+  rv = msgStore->CopyMessages(isMove, messages, this, listener,
>+                              getter_AddRefs(undoTxn), &storeDidCopy);
>   if (storeDidCopy)
Any reason not to use if (undoTxn) here?
Comment 10 David :Bienvenu 2012-04-01 17:09:53 PDT
(In reply to neil@parkwaycc.co.uk from comment #9)
> Comment on attachment 608924 [details] [diff] [review]
> ugh, you're right.
> 
> Sorry, I didn't understand your previous answer, so I'll ask this instead:
> >+  nsCOMPtr<nsITransaction> undoTxn;
> >+  rv = msgStore->CopyMessages(isMove, messages, this, listener,
> >+                              getter_AddRefs(undoTxn), &storeDidCopy);
> >   if (storeDidCopy)
> Any reason not to use if (undoTxn) here?

No, you're right, we can do that - that was meant to be implicit in my suggestion of adding an assertion, since assertions are supposed to imply the handling of the case the assertion was warning about (though, sadly, that's not always the case). I'll attach a version that does that.
Comment 11 David :Bienvenu 2012-04-01 17:17:51 PDT
Created attachment 611328 [details] [diff] [review]
fix with assertion for null undo txn
Comment 12 neil@parkwaycc.co.uk 2012-04-02 03:04:44 PDT
Comment on attachment 611328 [details] [diff] [review]
fix with assertion for null undo txn

>+  nsCOMPtr<nsITransaction> undoTxn;
>+  rv = msgStore->CopyMessages(isMove, messages, this, listener,
>+                              getter_AddRefs(undoTxn), &storeDidCopy);
>   if (storeDidCopy)
>+  {
>+    NS_ASSERTION(undoTxn, "if store does copy, it needs to add undo action");
>+    if (msgWindow && undoTxn)
Fair enough; as you think it would be confusing to assume the store did the copy based on the existence of an undo action then don't bother.
Comment 13 David :Bienvenu 2012-04-02 08:02:29 PDT
standard8, sr ping - I'd like to move maildir support forward, and this bug definitely blocks dogfooding of maildir.
Comment 14 David :Bienvenu 2012-04-04 13:37:57 PDT
Created attachment 612319 [details] [diff] [review]
fix for sr
Comment 15 tom 2012-04-18 01:27:29 PDT
the dogs are really hungry! :-)
Comment 16 David :Bienvenu 2012-04-19 07:40:23 PDT
fixed on trunk - http://hg.mozilla.org/comm-central/rev/26c2c8008156

the xpcshell tests revealed these issues, when I ran them with maildir turned on, so I'm going to mark this in testsuite+

Note You need to log in before you can comment on or make changes to this bug.