The default bug view has changed. See this FAQ.

message move/copying issues with maildir

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Backend
RESOLVED FIXED
5 years ago
2 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

5 years ago
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.
Attachment #608714 - Flags: superreview?(mbanner)
Attachment #608714 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Blocks: 58308

Comment 1

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

5 years ago
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.
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)

Comment 3

5 years ago
(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

5 years ago
Created attachment 608924 [details] [diff] [review]
ugh, you're right.

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)

Updated

5 years ago
Attachment #608924 - Attachment is patch: true

Comment 5

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

5 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.

Comment 7

5 years ago
(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

5 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 9

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

5 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

5 years ago
Created attachment 611328 [details] [diff] [review]
fix with assertion for null undo txn
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

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

Comment 14

5 years ago
Created attachment 612319 [details] [diff] [review]
fix for sr
Attachment #611328 - Attachment is obsolete: true
Attachment #611328 - Flags: superreview?(mbanner)
Attachment #612319 - Flags: superreview?(mbanner)

Comment 15

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

Comment 16

5 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
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Blocks: 845952
Blocks: 859011
No longer blocks: 859011
Blocks: 1135309
No longer blocks: 1135309
You need to log in before you can comment on or make changes to this bug.