Closed Bug 738651 Opened 8 years ago Closed 8 years ago

message move/copying issues with maildir

Categories

(MailNews Core :: Backend, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 14.0

People

(Reporter: Bienvenu, Assigned: Bienvenu)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch proposed fix (obsolete) — Splinter Review
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.
Attachment #608714 - Flags: superreview?(mbanner)
Attachment #608714 - Flags: review?(neil)
Blocks: 58308
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.
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.
Attachment #608714 - Attachment is obsolete: true
Attachment #608714 - Flags: superreview?(mbanner)
Attachment #608714 - Flags: review?(neil)
Attachment #608901 - Flags: superreview?(mbanner)
Attachment #608901 - Flags: review?(neil)
(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)
Attached patch ugh, you're right. (obsolete) — Splinter Review
I must have done ->forget... or dreamt it all.
Attachment #608901 - Attachment is obsolete: true
Attachment #608901 - Flags: superreview?(mbanner)
Attachment #608901 - Flags: review?(neil)
Attachment #608924 - Flags: superreview?(mbanner)
Attachment #608924 - Flags: review?(neil)
Attachment #608924 - Attachment is patch: true
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?
(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.
(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!"?
(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 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?
(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.
Attachment #608924 - Attachment is obsolete: true
Attachment #608924 - Flags: superreview?(mbanner)
Attachment #608924 - Flags: review?(neil)
Attachment #611328 - Flags: superreview?(mbanner)
Attachment #611328 - Flags: review?(neil)
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.
Attachment #611328 - Flags: review?(neil) → review+
standard8, sr ping - I'd like to move maildir support forward, and this bug definitely blocks dogfooding of maildir.
Attached patch fix for srSplinter Review
Attachment #611328 - Attachment is obsolete: true
Attachment #611328 - Flags: superreview?(mbanner)
Attachment #612319 - Flags: superreview?(mbanner)
the dogs are really hungry! :-)
Attachment #612319 - Flags: superreview?(mbanner) → superreview+
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+
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
No longer blocks: 859011
No longer blocks: 1135309
You need to log in before you can comment on or make changes to this bug.