Closed Bug 827048 Opened 11 years ago Closed 10 years ago

Filtered messages lost (POP) going to imap or maildir folder (with mail.serverDefaultStoreContractID = @mozilla.org/msgstore/maildirstore;1)

Categories

(Thunderbird :: Filters, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: sboehringer, Assigned: sshagarwal)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss)

Attachments

(1 file, 11 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 2012111600

Steps to reproduce:

For a POP account I have several filters installed. I use the maildir store backend.
mail.serverDefaultStoreContractID: @mozilla.org/msgstore/maildirstore;1



Actual results:

Messages being filtered are lost and do not appear in the destination folder. Unfiltered messages are correctly retained in the inbox.



Expected results:

Filtered messages should appear as unread emails in the destination folder of a move action.
Severity: normal → blocker
Severity: blocker → critical
Component: Untriaged → Filters
Keywords: dataloss
This happens both when filtering to a maildir folder or an IMAP destination. Manual movements work and the filters allow the destination folders to be selected.
Filtering works when the destination is a local directory otherwise the messages is lost.
Summary: Filtered messages lost (POP) → Filtered messages lost (POP) going to imap or maildir folder
I can confirm same behavior on Windows 7. Message does not appear in destination folder (from user view) but you can find it on filesystem in correct place.
No longer blocks: 856058
(In reply to sboehringer from comment #0)
> I use the maildir store backend.
> mail.serverDefaultStoreContractID: @mozilla.org/msgstore/maildirstore;1

It's perhaps default of newly created account, so existent account's setting is not affected. 
What is actual msgstore setting of accounts relevant to your problem?
> mail.server.serverN.storeContractID=@mozilla.org/msgstore/??????store;1

Filter move from what kind of mbox to what kind of mbox?
With which combination does your problem occur? 
To which combination is following in your comment #1 applicable?
(i)   Manual movements work
(ii)  Filtering works when the destination is a local directory
(iii) otherwise the messages is lost.

  Mbox type, copy/move type.
  (a-1) msgstore/maildirstore;1
  (a-2) msgstore/berkeleystore;1
  (b-1) POP3(or Local Folders)
  (b-2) IMAP, Offline-Use=On  (Folder Properties/Synchronization)
  (b-3) IMAP, Offline-Use=Off (Folder Properties/Synchronization)
  (c-1) Filter move within same account
  (c-2) Filter move to mbox of different account

What kind of filter rule?
- "Body" is used in condition or not
- action of "Delete" or "Stop Execution" is used or not
- action of "Copy to folder" is used too, or not
- "Delete from POP3 server" is used or not
etc.

If POP3, is Quarantine option used?
  mailnews.downloadToTempFile = true/false
  (Tools/Option/Security/Anti-Virus)

When does your problem of "Not moved by filter as you expect" occur?
- only on Checking Mail
- only on Manually Run
- both when new mail arrival and manual filter run
I don't think message filter Copy/Move works well despite that even simple mail copy doesn't work well yet. See dependency tree for meta bug 859011 which is set in Blocks: field of this bug.

Message filter;
  - Has action=Copy, Move, which is almost similar to mail Copy/Move
  - Writes data to message folder without explicit open of MsgDatabase
    as seen in bug 261419 and bug 495760.
  - Doesn't check return code at various steps, and if error occurs,
    silently stops to execute requested action, as seen in bug 854172.
How can it wort as expected even though mail data is not correctly written to file by Copy/Move and MsgDatabase is not correctly updated by Copy/Move? :-)

I think reporting bug for "Message filter with MaildirStore" is too early, except for crash problem due to not-correctly-updated mail file and MsgDatabase produced by various bugs. Even if broken message folder, Tb shouldn't crash.
Summary: Filtered messages lost (POP) going to imap or maildir folder → Filtered messages lost (POP) going to imap or maildir folder (with mail.serverDefaultStoreContractID = @mozilla.org/msgstore/maildirstore;1)
(In reply to WADA from comment #4)
> I think reporting bug for "Message filter with MaildirStore" is too early,

This is why Thunderbird is ****: Bugs aren't fixed and reports are flagged invalid without even checking the code.
> Message filter;
> I think reporting bug for "Message filter with MaildirStore" is too early,

Perhaps now (5/2014) is not too early?

I have been trying  'maildir-lite' TBird 24.5 on Linux, as a possble app to move over to from KMail.

On the whole, quite promising, but I hit the above problem when filtering email from Inbox into other folders.

Although TBird does update the display lists for Inbox & Sent when email is put into them, that does not happen when filtering files into other folders, when the only way they can then be seen (or even know they have arrived!) is to do a Repair on the folder. thus rendering 'maildir-lite' TBird unusable.

I understand further work on maildir-lite is imminent. If so, where would the best place be to report this problem (if not here)?
Attached patch b827048.patch (obsolete) — Splinter Review
So, this makes the pop3 filter moves work fine with maildir.
Test (test_pop3MoveFilter.js) also passes successfully.

ToDo: to convert test_pop3MoveFilter.js to use Promises
Also, I have been trying to run both, the mbox and maildir
instances in the same test. But things aren't working, possibly
due to gPOP3Pump's already created server. Resetting it is failing.

The following patch was an attempt for that.
In the meanwhile, please verify that this patch is fine and successfully
does what it intends to.

Thanks.
Attachment #8428220 - Flags: feedback?(kent)
Blocks: 1011399
OS: Linux → All
Hardware: x86_64 → All
Version: 17 → unspecified
Status: UNCONFIRMED → NEW
Ever confirmed: true
This patch runs two iterations of the test, but
unfortunately both of mbox.
Things like this should be abstracted in a way irrespective of the storage system. It's bad software design.
(In reply to Bachsau from comment #9)
> Things like this should be abstracted in a way irrespective of the storage
> system. It's bad software design.
Isn't this what you mean by abstraction?
http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgPluggableStore.idl
So, this patch modifies test_pop3MoveFilter.js, test_pop3MoveFilter2.js, POP3pump.js and localAccountUtils.js and now, tests are run over both
mbox and maildir and are successful (with the base patch applied).

ToDo: Use promises.

To be applied over the base patch.

Thanks.
Attachment #8428222 - Attachment is obsolete: true
Attachment #8428559 - Flags: feedback?(kent)
I forgot to mention in my last comment, these both patches fix both
tests viz. test_pop3MoveFilter.js and test_pop3MoveFilter2.js.
(In reply to Suyash Agarwal (:sshagarwal) from comment #10)
> Isn't this what you mean by abstraction?
> http://mxr.mozilla.org/comm-central/source/mailnews/base/public/
> nsIMsgPluggableStore.idl

If it was properly abstracted, you wouldn't need different code for filters. It would work regardless of the storage method. Even with a completely new one or third party.
(In reply to Suyash Agarwal (:sshagarwal) from comment #11)
> Created attachment 8428559 [details] [diff] [review]
> Patch to run tests over both mbox and maildir
> 
> So, this patch modifies test_pop3MoveFilter.js, test_pop3MoveFilter2.js,
> POP3pump.js and localAccountUtils.js and now, tests are run over both
> mbox and maildir and are successful (with the base patch applied).

 Sounds good! 
$64 question: When might this fix be available in a T'bird update?
(In reply to Maurice from comment #14)
>  Sounds good! 
> $64 question: When might this fix be available in a T'bird update?

Assuming yours is not a rhetorical question ... in a nightly build when he finished the patch.  Longer for next public release version which is 38.
(In reply to Wayne Mery (:wsmwk) from comment #15)
> (In reply to Maurice from comment #14)

> Assuming yours is not a rhetorical question ... in a nightly build when he
> finished the patch.  Longer for next public release version which is 38.

OK - so will await Release 38 (as do not know how to pick up 'nightly build').
  Many thanks!
Comment on attachment 8428559 [details] [diff] [review]
Patch to run tests over both mbox and maildir

Review of attachment 8428559 [details] [diff] [review]:
-----------------------------------------------------------------

I've added a few comments, but the basic problem is that you do not yet understand how to convert a test to be promise based. Maybe you should start with a simpler test first, and just do the conversion. One simple test you could try would be test_imapID.js which would use the existing promise url listener.

I really think that you should first try to do the conversion to promise, then try to add the maildir test.

::: mailnews/local/test/unit/test_pop3MoveFilter.js
@@ +9,5 @@
>  load("../../../resources/POP3pump.js");
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource:///modules/mailServices.js");
>  Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm");

You added this but are not actually using it. I'll suggest below what you need to do to use it.

@@ +50,3 @@
>    },
>    // just get a message into the local folder
>    function getLocalMessages1() {

As we modernize this test, gTestArray should be replaced by a series of calls to add_task (or add_test) which is the modern testing framework way of doing this, rather than rolling our own.

Because we want to run the same series of tests twice, perhaps the correct thing to do is to keep these tests in an array, but then loop through the array twice (adding calls to add_task for each item in the array), with a call in the middle to reset the POP3 pump.

@@ +56,5 @@
>      gPOP3Pump.run();
>    },
> +  function waitForCopyToFinish() {
> +   do_timeout(1000, function() {++gCurTestNum; doTest();});
> +  },

This is an act of desperation, and typically leads either to slow tests or random oranges. You really need to figure out the correct terminal notification and use it.

POP3Pump is designed with a callback function onDone which is used to notify when the operation is finished. You need to use this as an async callback, which calls somehow deferred.resolve() to complete the promise.

@@ +139,1 @@
>    MailServices.mailSession.AddFolderListener(FolderListener,

This listener is how you define the terminal event for the async operation. What you need to do is to define a generic class for a PromiseFolderListener, add it to PromiseTestUtils, and then use it here as the method of managing async operations.

@@ +152,5 @@
> +// tests iteration for different pluggable stores.
> +let iteration = 0;
> +function run_test()
> +{
> +  run_all_tests(iteration++);

As I implied earlier, rather than have an "iteration" which is a form of spaghetti code, I would rather have steps added sequentially to add_task in a clearer manner.

@@ +211,5 @@
> +  }
> +
> +  // Start the other iteration of tests (for maildir).
> +  gCurTestNum = 1; // Re-initialize to 1 for doTest() to run all functions.
> +  run_all_tests(iteration++);

This series of code will end up being a function that you will use add_task on to put it in at the appropriate place in the sequence of operations.

::: mailnews/test/resources/POP3pump.js
@@ +45,5 @@
>    this._firstFile = true;
>    this._tests = [];
>    this._finalCleanup = false;
>    this._expectedResult = 0;
> +  this._mailbox = Services.prefs.getCharPref("mail.serverDefaultStoreContractID");

It would never occur to me that "mailbox" meant "contract ID of local store". Please choose a more appropriate name.

@@ +67,3 @@
>  
>      // Let OnStopRunningUrl return cleanly before doing anything else.
> +    do_timeout(0, this._checkBusy.bind(this));

It would be really good to convert POP3pump itself to use promises, which means that the OnStopRunningUrl would call deferred.resolve() rather than this call. But it might be better to wait until you have a better understanding of promises.

@@ +101,5 @@
> +{
> +  if (aMailbox == this._mailbox)
> +    return this;
> +
> +  Services.prefs.setCharPref("mail.serverDefaultStoreContractID", aMailbox);

It is a trap to set a global default value like this in the middle of POP3 pump. Can;t you store this local store type, and only use it for the server created by POP3 pump?

@@ +113,5 @@
> +  }
> +  this.fakeServer = null;
> +  localAccountUtils.clearAll();
> +
> +  this._incomingServer = this._createPop3ServerAndLocalFolders();

When you create the server and folders, you will need to probably pass in the pluggable store type.
Attachment #8428559 - Flags: feedback?(kent) → feedback-
Comment on attachment 8428220 [details] [diff] [review]
b827048.patch

Review of attachment 8428220 [details] [diff] [review]:
-----------------------------------------------------------------

I looked quite a bit at this, and as far as I can tell it does the correct thing. Yet there are many different functions that have to work, and their application is spread seemingly haphazardly over different methods. It is really hard to tell if things will work correctly in all cases when, for example, critical notifications are sometimes done in the parser, and sometimes in the imap move coalescer. Also, I don't believe that the underlying functions will work correctly in cases where there are mixed servers, some with mbox and some with maildir (which is an important use case at least for the conversion scenario).

But that seems out of scope of the current bug. AFAICT the suggested changes help immensely the filtering problem, so f+=me
Attachment #8428220 - Flags: feedback?(kent) → feedback+
Attached patch Patch v2.6 (obsolete) — Splinter Review
So, I have tried to use promises.
Though, I am not sure if this is the correct way.
Once confirmed that modification in the folderlistener
is correct, I'll move it to PromiseTestUtils.

(In reply to Kent James (:rkent) from comment #17)
> ::: mailnews/local/test/unit/test_pop3MoveFilter.js
> ::: mailnews/test/resources/POP3pump.js
> @@ +101,5 @@
> > +{
> > +  Services.prefs.setCharPref("mail.serverDefaultStoreContractID", aMailbox);
> 
> It is a trap to set a global default value like this in the middle of POP3
> pump. Can;t you store this local store type, and only use it for the server
> created by POP3 pump?
Though I couldn't find the method, but still, if done that way, wouldn't it require
passing store to nsMailServer and localAccountUtils and for fakeserver too?
Whereas, this way we just need to do it once and all those helper methods do it
based on the pref.
And, init() is just almost re-initializing all the servers and folders so, should
we still consider it as the middle of POP3 pump?
> @@ +113,5 @@
> > +  this._incomingServer = this._createPop3ServerAndLocalFolders();
> 
> When you create the server and folders, you will need to probably pass in
> the pluggable store type.
Since I am setting the global pref before it, it works for me as is.

Please let me know the changes I am supposed to make.

Thanks.
Attachment #8428220 - Attachment is obsolete: true
Attachment #8428559 - Attachment is obsolete: true
Attachment #8429888 - Flags: feedback?(kent)
Attached patch Patch v2.8 (obsolete) — Splinter Review
Patch re-based on the patch from bug 1017879.
Attachment #8429888 - Attachment is obsolete: true
Attachment #8429888 - Flags: feedback?(kent)
Attachment #8431313 - Flags: feedback?(kent)
Comment on attachment 8429888 [details] [diff] [review]
Patch v2.6

Review of attachment 8429888 [details] [diff] [review]:
-----------------------------------------------------------------

Irving requested, and I agree, that the conversion to Promises be in a separate patch to make it easier to review. So do not mix issues here (like adding maildir, and converting to Promises). So I am only going to comment here on issues associated with Promises.

I'm posting this feedback on an obsolete patch because I had already made many comments on it. Please incorporate these comments in the current patch.

::: mailnews/local/test/unit/test_pop3MoveFilter.js
@@ +32,3 @@
>  const gTestArray =
>  [
> +  function* createFilters() {

This is not a generator, so no function*

@@ +47,3 @@
>    },
>    // just get a message into the local folder
> +  function* getLocalMessages1() {

no function*

@@ +61,5 @@
>      localAccountUtils.inboxFolder.msgDatabase = null;
>      localAccountUtils.inboxFolder.ForceDBClosed();
>      try {
> +      localAccountUtils.inboxFolder
> +                       .getDatabaseWithReparse(ParseListener, null);

The original test does not appear to me to be properly coded as an async test. Let's instead try to do it correctly.

ParseListener can just be a generic PromiseTestUtils.PromiseUrlListener().

For the added tests, realize that Promise.then returns a promise, so you can add those as extra steps like this:

yield promiseUrlListener.promise.then((aResult) => {
    do_check_true(localAccountUtils.inboxFolder.msgDatabase.summaryValid));
    do_check_eq(folderCount(localAccountUtils.inboxFolder), 0));
  });

@@ +67,1 @@
>        do_check_true(ex.result == Cr.NS_ERROR_NOT_INITIALIZED);

you can add this test in the "reject" part of the promise return.

@@ +84,2 @@
>    },
> +  function *endTest() {

again, not a generator, so no *

@@ +90,3 @@
>  ];
>  
>  var ParseListener =

You will probably completely remove ParseListener since it will be replaced with a generic PromiseUrlListener()

@@ +113,5 @@
>    }
>    return count;
>  }
>  
> +function* run_all_tests(store)

again, not a generator so no *

@@ +155,5 @@
>  
>  // nsIFolderListener implementation
>  var FolderListener = {
>    OnItemAdded: function OnItemAdded(aParentItem, aItem) {
> +    Promise.resolve(this._showEvent(aParentItem, "OnItemAdded"));

There is no need to convert this listener to a Promise. This is not actually any async function, it is simply a listener used in the test. So leave this listener alone.

@@ +170,5 @@
>           "\n");
>    }
>  };
>  
> +function *exitTest()

Not a generator, no *
Attachment #8429888 - Flags: feedback-
I can't really make any progress with this patch until Bug 1018300 gets finished, and the changes that will end up in that big are removed from this bug.
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #8431313 - Attachment is obsolete: true
Attachment #8431313 - Flags: feedback?(kent)
Attachment #8438332 - Flags: feedback?(kent)
Depends on: 1018300
Comment on attachment 8438332 [details] [diff] [review]
Patch v3

Review of attachment 8438332 [details] [diff] [review]:
-----------------------------------------------------------------

There is a lot of subtle issues associated with the message move, and I am not sure that I have caught them all yet. But I'll finish my feedback for now as I have plenty of notes already.

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +775,5 @@
>      rv = toPath->Create(nsIFile::DIRECTORY_TYPE, 0755);
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
>    rv = fromPath->MoveToNative(toPath, fileName);

You are not checking the return value of this move.

This is equivalent to the nsParseMailbox.cpp error:

2513     if (NS_FAILED(rv))
2514       destIFolder->ThrowAlertMsg("filterFolderWriteFailed", msgWindow);

@@ +782,5 @@
> +  NS_WARN_IF_FALSE(destMailDB && NS_SUCCEEDED(rv),
> +                   "failed to open mail db parsing folder");
> +  nsCOMPtr<nsIMsgDBHdr> newHdr;
> +  if (destMailDB)
> +    destMailDB->CopyHdrFromExistingHdr(nsMsgKey_None, aHdr, true,

In ParseMailbox, this is "rv = destMailDB ..."  That is, the rv from GetMsgDatabase is warn only, and the destMailDB is used even if the rv reports an error, as long as the db was returned. This is by design, so please match these details here.

Also, these 6 lines dealing with destMailDB should all be executed before the fromPath->MoveToNative, or so it seems to me. That is, do not move the file until you have successfully done the database work.

@@ +789,5 @@
> +    rv = NS_ERROR_UNEXPECTED;
> +  if (NS_FAILED(rv))
> +    aDestFolder->ThrowAlertMsg("filterFolderHdrAddFailed", nullptr);
> +
> +  nsCOMPtr<nsIMsgFolderNotificationService> notifier(

In nsParseMailbox, before the folder notification there is a section of code that deals with new status. You need to include that code.

::: mailnews/local/test/unit/test_pop3MoveFilter.js
@@ +101,5 @@
>    }
>    return count;
>  }
>  
> +function *run_all_tests(storeID)

Without the promise and yield, this will not be a generator.

Also, this does not "run_all_tests" so why do you call it that? it is more "setupLocalAccount".

@@ +108,5 @@
> +  let promise = new Promise(function (resolve, reject) {
> +    resolve(gPOP3Pump.init(storeID));
> +  });
> +
> +  gPOP3Pump = yield promise;

I don't understand why you setup a promise for this, then resolve the gPOP3Pump. Can't you just call gPOP3Pump.init(storeID) directly? There are no async operations there that I see.

@@ +112,5 @@
> +  gPOP3Pump = yield promise;
> +
> +  // Set the default mailbox store.
> +  Services.prefs.setCharPref("mail.serverDefaultStoreContractID",
> +                             storeID);

This is being done in the init/resetStoreContractID method so does not need to be repeated here.

::: mailnews/test/resources/POP3pump.js
@@ +96,5 @@
>  
>    return this.fakeServer;
>  };
>  
> +POP3Pump.prototype.init = function(aMailbox)

This is intended as an externally used method, so you need to add documentation for it at the top of the file along with other methods that are there, such as gPOP3Pump.run().

I also don't like the name "init". That sounds like something that you have to run, but in fact this is optional. This method is specifically about changing the pluggable store, so let's call it "resetPluggableStore".

I also don't know what a "aMailbox" is, let's call that "aStoreContractID"

@@ +99,5 @@
>  
> +POP3Pump.prototype.init = function(aMailbox)
> +{
> +  if (aMailbox == this._mailboxStoreContractID)
> +    return this;

There is no reason for this function to return anything that I see. The caller already has a reference to gPOP3Pump
Attachment #8438332 - Flags: feedback?(kent) → feedback+
Attached patch 827048_v4.patch (obsolete) — Splinter Review
(This patch was emailed to me due to network problems)
Attachment #8438332 - Attachment is obsolete: true
Attachment #8443165 - Flags: feedback?(kent)
Comments submitted with the patch:

I have tried to incorporate the changes you suggested in bug 827048.
Removing * from run_all_tests(storeID); makes the test to fail with the
reason "couldn't identify the server type". So, I suspect that's because
gPOP3Pump.init() isn't finished and hence the function value is returned
by add_task() and using run_all_tests() as generator function, its value is
returned and hence the method waits before running to another task.
*And*, the test fails on Windows.
Please let me know if my suspicion makes any sense.
Thanks.


Sincere Regards
Suyash Agarwal
Comment on attachment 8443165 [details] [diff] [review]
827048_v4.patch

Review of attachment 8443165 [details] [diff] [review]:
-----------------------------------------------------------------

I want to get my comments posted about the *run_all_tests issue. I still want to do some more testing with the actual app before I want to approve this approach for this bug. But please try my approach to the run_all_tests

::: mailnews/local/test/unit/test_pop3MoveFilter.js
@@ +101,5 @@
>    }
>    return count;
>  }
>  
> +function *run_all_tests(storeID)

Get rid of the * since this is not a generator.

In the next line, wrap everything like this:
  return function _run_all_tests() {

then it will return a function.

@@ +119,5 @@
>  
> +function run_test()
> +{
> +  for (let store of gPluggableStores) {
> +    add_task(run_all_tests(store));

What you have done is that, instead of adding a function that can be run, you are adding the result of calling run_all_tests(store) (which is not a function in the original patch). Telling run_all_stores that it is a generator changes that, but that is a confusing way to return a function. With the changes I suggest to run_all_tests it will indeed return an function, so you can leave this as is.

::: mailnews/test/resources/POP3pump.js
@@ +9,5 @@
>   *  gPOP3Pump.files:  (in) an array of message files to load
>   *  gPOP3Pump.onDone: function to execute after completion
>                        (optional and deprecated)
>   *  gPOP3Pump.fakeServer:  (out) the POP3 incoming server
> + *  gPOP3Pump.resetPluggableStore(): function to reset the pluggable store.

"reset" is an ambiguous term, you should really try to better describe what it is going to do here.
Attachment #8443165 - Flags: feedback?(kent) → feedback+
Attached patch Patch v4.2 (obsolete) — Splinter Review
It works now!
Thanks.
Attachment #8443165 - Attachment is obsolete: true
Attachment #8443430 - Flags: review?(kent)
With my tests so far, the latest approach works for pop3 -> pop3 or local, but I am having issues with imap -> local.

What I see is this stack:

 	xul.dll!nsMsgMaildirStore::FinishNewMessage(nsIOutputStream * aOutputStream, nsIMsgDBHdr * aNewHdr)  Line 718 + 0x1c bytes	C++
>	xul.dll!nsMsgLocalMailFolder::EndCopy(bool aCopySucceeded)  Line 2286	C++
 	xul.dll!nsCopyMessageStreamListener::EndCopy(nsISupports * url, tag_nsresult aStatus)  Line 114 + 0x21 bytes	C++
 	xul.dll!nsCopyMessageStreamListener::OnStopRequest(nsIRequest * request, nsISupports * ctxt, tag_nsresult aStatus)  Line 144	C++

and the key issue is that, in FinishNewMessage, instead of being in "tmp" the message is already in "cur".

I'm tempted to add a kludge here, checking if it is already in cur, and file a followup bug to try to find the root cause of this. I think there are just poor specifications of what state things are supposed to actually be in during these moves.
One additional issue is that, on moving with a filter a POP3 message from a POP3 account to a local account, the "New" decoration remains on the POP3 root folder instead of switching to the Local Accounts root folder, though this same thing happens with mbox so I think it is an unrelated issue.
I filed Bug 1028372 anticipating that I will suggest a kludge in this bug.
Comment on attachment 8443430 [details] [diff] [review]
Patch v4.2

Review of attachment 8443430 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good (though with one trailing space), though it needs the fix/kludge that I suggested.

What I will do is upload a patch with that fix, and ask for your feedback.
Attachment #8443430 - Flags: review?(kent) → review+
Attached patch 827048_v5.patch (obsolete) — Splinter Review
Attachment #8443718 - Flags: feedback?(syshagarwal)
Comment on attachment 8443718 [details] [diff] [review]
827048_v5.patch

Review of attachment 8443718 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +737,2 @@
>      NS_ERROR("FinishNewMessage - oops! file does not exist!");
>      return NS_ERROR_FAILURE;

Ya, this seems to be a nice workaround for this issue.
Just one thing though, should we return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST[1]
here instead of the generic NS_ERROR_FAILURE ?

Thanks.

[1] http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/ErrorList.h#431
Attachment #8443718 - Flags: feedback?(syshagarwal) → feedback+
So, can we go for check-in (with the trailing space removed)?
Comment on attachment 8443718 [details] [diff] [review]
827048_v5.patch

Review of attachment 8443718 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry to do this to you, but I discovered some new issues when I did another look over all of this. So please fix these new issues.

One lesson learned here is that we should have separated the issue of fixing this particular issue with the issue of making tests run with both maildir and mbox. By combining them, we are delaying landing things that you need for your other tests.

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +797,5 @@
>  
> +  nsCOMPtr<nsIMsgDatabase> destMailDB;
> +  rv = aDestFolder->GetMsgDatabase(getter_AddRefs(destMailDB));
> +  NS_WARN_IF_FALSE(destMailDB && NS_SUCCEEDED(rv),
> +                   "failed to open mail db parsing folder");

This message "failed to open mail db parsing folder" was copied from the parsing folder object, but we are not there. So please change to "failed to open mail db moving message"

@@ +807,5 @@
> +  if (NS_SUCCEEDED(rv) && !newHdr)
> +    rv = NS_ERROR_UNEXPECTED;
> +
> +  if (NS_FAILED(rv))
> +    aDestFolder->ThrowAlertMsg("filterFolderHdrAddFailed", nullptr);

You are passing a null msgWindow to this method. Looking at the method, it does nothing if a null window is passed. So these alerts will not show.

I think that the best way to fix this will be to add a lookup using MailServices.mailSession.topmostMsgWindow in the ThrowAlertMsg method if the msgWindow is null since there is no msgWindow available here.

@@ +817,5 @@
> +  if (NS_FAILED(rv)) {
> +    if (destMailDB)
> +      destMailDB->Close(true);
> +
> +    *aResult = NS_SUCCEEDED(NS_MSG_ERROR_WRITING_MAIL_FOLDER);

This is really odd, and caused me to investigate further.

In general (though frequently violated in mailnews) when a failure rv is returned, we should make no assumptions about the validity of the other returned values, in this case aResult. In the users of this method, that is violated, checking only the value of aResult to decide what to do. This is not valid behavior. So I think we should fix this.

So in the callers to MoveNewlyDownloadedMessage, check for valid return, and take appropriate action if this is an error. Then you do not need to set the aResult here if you are going to be returning an error. (If we do not do this, then errors returned earlier will result in strange behavior).

@@ +833,5 @@
> +  if (!(newFlags & nsMsgMessageFlags::Read))
> +  {
> +    nsCString junkScoreStr;
> +    (void) newHdr->GetStringProperty("junkscore", getter_Copies(junkScoreStr));
> +    if (atoi(junkScoreStr.get()) == nsIJunkMailPlugin::IS_HAM_SCORE)

I realized you copied this, but this is not correct. Rather than checking that junkscore equals IS_HAM_SCORE, we should be checking that junkscore is not IS_SPAM_SCORE (empty junkscore should result in setting the new flag)
Attachment #8443718 - Flags: review-
Attached patch Patch v5.2 (obsolete) — Splinter Review
Made the suggested changes.
Thanks.
Attachment #8443430 - Attachment is obsolete: true
Attachment #8443718 - Attachment is obsolete: true
Attachment #8444968 - Flags: review?(kent)
Comment on attachment 8444968 [details] [diff] [review]
Patch v5.2

Review of attachment 8444968 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/public/nsIMsgPluggableStore.idl
@@ +168,4 @@
>     * @exception If the moved failed. values TBD
>     */
> +  void moveNewlyDownloadedMessage(in nsIMsgDBHdr aNewHdr,
> +                                  in nsIMsgFolder aDestFolder);

This is not actually what I intended, that is I did not expect you to change this.

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +212,5 @@
>    nsresult rv = GetStringFromBundle(aMsgName, getter_Copies(alertString));
> +  if (!aMsgWindow) {
> +    nsCOMPtr<nsIMsgMailSession> mailSession ( do_GetService(NS_MSGMAILSESSION_CONTRACTID, &rv));
> +    if (NS_SUCCEEDED(rv))
> +      rv = mailSession->GetTopmostMsgWindow(&aMsgWindow);

This will leak, as there is no mechanism to free the XPCOM ptr that is returned from GetTopmostMsgWindow. You need to define a local nsCOMPtr, and either assign it to aMsgWindow or get it from GetTopmostMsgWindow

::: mailnews/local/src/nsMsgBrkMBoxStore.cpp
@@ +725,4 @@
>  {
>    NS_ENSURE_ARG_POINTER(aNewHdr);
>    NS_ENSURE_ARG_POINTER(aDestFolder);
> +  return NS_ERROR_NOT_IMPLEMENTED;

Please leave this method as is.

I don't really like this design, where this method behaves completely different depending on whether it is maildir or mbox, but that is what we have to work with. I would rather though keep the original design, where it returns "OK" but then states that it did not move the message.

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +742,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsMsgMaildirStore::MoveNewlyDownloadedMessage(nsIMsgDBHdr *aHdr,
> +                                              nsIMsgFolder *aDestFolder)

As I said, leave the original calling sequence.

::: mailnews/local/src/nsParseMailbox.cpp
@@ +1839,5 @@
>                  nsCOMPtr<nsIMsgPluggableStore> msgStore;
>                  rv = m_downloadFolder->GetMsgStore(getter_AddRefs(msgStore));
>                  if (NS_SUCCEEDED(rv))
> +                  rv = msgStore->MoveNewlyDownloadedMessage(m_newMsgHdr, trash);
> +                if (!NS_SUCCEEDED(rv))

So here is the issue. What do we do on failure? MoveIncorporatedMessage is not intended as an error recovery routine, it is an alternate way of dealing with the move which is designed for mbox. So we only do that if MoveNewlyDownloadedMessage succeeds, but does not move the message (which is another way of saying it is mbox).

If MoveNewlyDownloadedMessage fails for maildir, then we are in a pathological situation, and trying MoveIncorporatedMessage is not the correct thing to do. Given the existing code logic, all we can really do at this point is set m_msgMovedByFilter to false, and continue with the parse.

So what I wanted you to do was simply to change this line from if (!msgMoved) to if (NS_SUCCEEDED(rv) && !msgMoved) plus make sure that m_msgMovedByFilter is false in the case of error.

@@ +2055,5 @@
>              nsCOMPtr<nsIMsgPluggableStore> msgStore;
>              err = m_downloadFolder->GetMsgStore(getter_AddRefs(msgStore));
>              if (NS_SUCCEEDED(err))
> +              err = msgStore->MoveNewlyDownloadedMessage(msgHdr, destIFolder);
> +            if (!NS_SUCCEEDED(err))

See comments above, same issue here.
Attachment #8444968 - Flags: review?(kent) → review-
Attached patch Patch v5.4 (obsolete) — Splinter Review
Oh okay, made the suggested changes.

Thanks.
Attachment #8444968 - Attachment is obsolete: true
Attachment #8445278 - Flags: review?(kent)
Blocks: 1030116
Comment on attachment 8445278 [details] [diff] [review]
Patch v5.4

Review of attachment 8445278 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.
Attachment #8445278 - Flags: review?(kent) → review+
Thanks.
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Keywords: checkin-needed
Renamed "run_all_tests" to "setup_store".
Attachment #8445278 - Attachment is obsolete: true
Attachment #8446060 - Flags: review+
Blocks: 1030291
Checked in https://hg.mozilla.org/comm-central/rev/4dc251e08562
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.