message move/copying issues with maildir

RESOLVED FIXED in Thunderbird 14.0

Status

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

(Blocks 2 bugs)

Trunk
Thunderbird 14.0
x86_64
Windows 7
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

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

Updated

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

Comment 2

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

Comment 4

7 years ago
Posted 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?
Assignee

Comment 6

7 years ago
(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!"?
Assignee

Comment 8

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

Comment 10

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

Comment 11

7 years ago
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+
Assignee

Comment 13

7 years ago
standard8, sr ping - I'd like to move maildir support forward, and this bug definitely blocks dogfooding of maildir.
Assignee

Comment 14

7 years ago
Posted patch fix for srSplinter Review
Attachment #611328 - Attachment is obsolete: true
Attachment #611328 - Flags: superreview?(mbanner)
Attachment #612319 - Flags: superreview?(mbanner)

Comment 15

7 years ago
the dogs are really hungry! :-)
Attachment #612319 - Flags: superreview?(mbanner) → superreview+
Assignee

Comment 16

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