1320676 - "move to folder" does not stop subsequent rules if the mail is already in target folder, so it is moved to other folder by susequent filter rule (Run filters on move target folder of filter rule, Bug 232569 came back)

Status: NEW

(MailNews Core :: Filters, defect)

Description

[WADA:World Anti-bad-Duping Agency]

+++ This bug was initially created as a clone of Bug #232569 +++

Problem of bug 232569 came back in Thunderbird 37, and still remain in Thunderbird 45.

[STR]
(1) Create filters of following rules.
 MailX : Subject=xxxxxxxx
 FilterRule#1 : If subject begins with xxxx, move to Folder#1
 FilterRule#2 : If subject begins with xxxx, move to Folder#2
(2) When MailX is held in Folder#1, Run Filters on Folder#1
    => MailX is moved to Folder#2 (doesn't stop at FilterRule#1)
(3) When MailX is held in Folder#2, Run Filters on Folder#2
    => MailX is moved to Folder#1, according to filter definition.

[Regression wimdow]
> Problem doesn't occur:
> https://archive.mozilla.org/pub/thunderbird/nightly/2014/12/2014-12-20-03-02-03-comm-central/
> Problem occurs:
> https://archive.mozilla.org/pub/thunderbird/nightly/2014/12/2014-12-21-03-02-07-comm-central/

[Pushlog, 2014-12-20 00:00 to 2014-12-21 23:59
> https://hg.mozilla.org/comm-central/pushloghtml?startdate=2014-12-20+00%3A00&enddate=2014-12-21+23%3A59

Perhaps affected by change of Bug 695671.
> Bug 695671 - Filtering stops -- Only 1 filter executes for "Run Filters on Folder"
> (after execution of action="Delete", further/different actions on other mails is not executed and filter log is not written to filterlog.html).

Comment 1

[WADA:World Anti-bad-Duping Agency]

Can add code like following in filter?
  if( action==move or copy ) {
    if(CopyMoveSourceFolder(i.e. CurrentFolder)==CopyMoveTargetFolder)
      { do nothing, and if move, do "Stop execution" ; return ; }
  }

Comment 2

[WADA:World Anti-bad-Duping Agency]

Phenomenon like following?
 1. NN-th msg, messageKey=A, in msgDBHdrList is processed. action = Move to Folder#1 by Rule#1
 2. Do move to, but because mail is already held in Folder#1, messageKey=A is not removed from Folder#1
 3. "Move to already held folder" returns error state, because "Move operation" was not normally executed.
 4. Before Bug 695671 : (a) filter on NN-th msg is terminated because of "Move to" action,
                        and (b) advance to next (NN+1)-th message in msgDBHdrList.
    After  Bug 695671 : (c) filter on NN-th msg is not terminated because of error, even though it's "Move to" action,
                        and (d) filter continues on current NN-th msg,
                        and (e) advance to next rule on same NN-th msg.

IIUC, change was made to resolve problem in action=Delete(and maybe in Stop Execution).
Even if code like (c)/(d)/(e) is needed, it shouldn't be applied on at least action=Move.

When bug 232569 disappered in the past, bug 393668 was observed. I don't think problem of bug 393668 still occurs in current Thunderbird.
Bug 232569, bug 393668, bug 695671, this bug, bug 770137 etc. is an indicator of that behavior of "message filter code" is frequently inconsistent.
Behavior of message filter can be altered by any change in filter relevant code, regardless of design of "Message Filter".

Comment 3

[WADA:World Anti-bad-Duping Agency]

I lookerd patch of Bug 695671 in order to know "Why this bug didn't occur before the change but this bug occurs after the change".

Patch of Bug 695671 replaced control of "don't go to further step" from ApplyMore to m_stopFiltering.
>        case nsMsgFilterAction::StopExecution:
> -      {
> -        // don't apply any more filters
> -        *aApplyMore = false;
> -      }
> +        {
> +          // don't apply any more filters
> +          m_stopFiltering.AppendElements(m_searchHits);
> +          m_nextAction = numActions;
> +        }
>        break;

ApplyMore was not seen for nsMsgFilterAction::MoveToFolder case and nsMsgFilterAction::Delete case in old code,
but m_stopFiltering.AppendElements is executed in new code for all of StopExecution, MoveToFolder, Delete cases.

With above change, following change was made by the patch.
m_stopFilteringAppendElements is not seen in this module.

> nsMsgFilterAfterTheFact::ApplyFilter
> 
>      // We start from m_nextAction to allow us to continue applying actions
>      // after the return from an async copy.
> -    for (uint32_t actionIndex = m_nextAction;
> -         actionIndex < numActions && *aApplyMore;
> -         actionIndex++)
> +    while (m_nextAction < numActions)
>      {

Is cause of problem lack of m_stopFiltering.Contains(msgKey) check at this step?
Is code like  "while ( (m_nextAction < numActions) && !(m_stopFiltering.Contains(msgKey)) )" needed here?
Code like this at here prevents consecutive execution of non-StopExecution/non-MoveToFolder/non-Delete actions when these dangerous action or actions(both MoveToFolder and StopExecution can be requested at UI) is contained in required actions for this msgDBHdr/msgKey?
Control is done at other place(s), so there is no need to care about m_stopFiltering.Contains(msgKey) in this module?

Comment 4

[WADA:World Anti-bad-Duping Agency]

Another concern.

Following is a big change by patch in nsMsgApplyFiltersToMessages::RunNextFilter.
>  nsresult nsMsgApplyFiltersToMessages::RunNextFilter()
>  {
> -  while (m_curFilterIndex < m_numFilters)
> +  nsresult rv = NS_OK;
> +  while (true)
>    {
> +    m_curFilter = nullptr; // we are done with the current filter
> +    if (!m_curFolder || // Not an error, we just need to run AdvanceToNextFolder()
> +        m_curFilterIndex >= m_numFilters)
> +      break;
> (snip, because error handling improvements part)
>      for (int32_t i = 0; i < m_msgHdrList.Count(); i++)
>      {
> -      nsIMsgDBHdr* msgHdr = m_msgHdrList[i];
> +      nsCOMPtr<nsIMsgDBHdr> msgHdr = m_msgHdrList[i];
> +      CONTINUE_IF_FALSE(msgHdr, "null msgHdr");
> +
>        bool matched;
> -
>        rv = m_curFilter->MatchHdr(msgHdr, m_curFolder, m_curFolderDB, nullptr, 0, &matched);
> -
>        if (NS_SUCCEEDED(rv) && matched)
>        {
>          // In order to work with nsMsgFilterAfterTheFact::ApplyFilter we initialize
>          // nsMsgFilterAfterTheFact's information with a search hit now for the message
>          // that we're filtering.
>          OnSearchHit(msgHdr, m_curFolder);
>        }
>      }
>      m_curFilter->SetScope(nullptr);
>  
>      if (m_searchHits.Length() > 0)
>      {
> -      bool applyMore = true;
> -
>        m_nextAction = 0;
> -      rv = ApplyFilter(&applyMore);
> -      NS_ENSURE_SUCCESS(rv, rv);
> -      if (applyMore)
> -      {
> -        // If there are more filters to apply, then ApplyFilter() would have
> -        // called RunNextFilter() itself, and so we should exit out afterwards
> -        return NS_OK;
> -      }
> -
> -      // If we get here we're done applying filters for those messages that
> -      // matched, so remove them from the message header list
> -      for (uint32_t msgIndex = 0; msgIndex < m_searchHits.Length(); msgIndex++)
> -      {
> -        nsCOMPtr <nsIMsgDBHdr> msgHdr;
> -        m_searchHitHdrs->QueryElementAt(msgIndex, NS_GET_IID(nsIMsgDBHdr), getter_AddRefs(msgHdr));
> -        if (msgHdr)
> -          m_msgHdrList.RemoveObject(msgHdr);
> -      }
> -
> -      if (!m_msgHdrList.Count())
> -        break;
> +      rv = ApplyFilter();
> +      if (NS_SUCCEEDED(rv))
> +        return NS_OK; // async callback will continue, or we are done.
>      }
>    }

In old code, "advancing to action by next hit rule" is controlled by returning false==applyMore from ApplyFilter.
> -      bool applyMore = true;
>        m_nextAction = 0;
> -      rv = ApplyFilter(&applyMore);
> -      NS_ENSURE_SUCCESS(rv, rv);
> -      if (applyMore)
> -      {
> -        // If there are more filters to apply, then ApplyFilter() would have
> -        // called RunNextFilter() itself, and so we should exit out afterwards
> -        return NS_OK;
> -      }
> -      // If we get here we're done applying filters for those messages that
> -      // matched, so remove them from the message header list

Above code part of RunNextFilter was changed to following by patch.
> +      rv = ApplyFilter();
> +      if (NS_SUCCEEDED(rv))
> +        return NS_OK; // async callback will continue, or we are done.

Following is done in called ApplyFilter() after patch for action=StopExecution/MoveToFolder/Delete at this step.
> +        // don't allow any more filters on this message
> +        m_stopFiltering.AppendElements(m_searchHits);

When above "+ rv = ApplyFilter(); if (NS_SUCCEEDED(rv)) return NS_OK;" after patch is executed by RunNextFilter after patch,,
following was done before patch/is done after patch, by ApplyFilter.
> -  if (*aApplyMore)
> -    rv = RunNextFilter();
> -
> -  return rv;
> +  } while (false); // end error management block
> +  return RunNextFilter();

i.e.
"m_stopFiltering.AppendElements(m_searchHits);" done on action=StopExecution/MoveToFolder/Delete by ApplyFilter() after patch is not processed upon next new code in RunNextFilter after patch. 
> +      rv = ApplyFilter();
> +      if (NS_SUCCEEDED(rv))
> +        return NS_OK; // async callback will continue, or we are done.

Is this a cause of problem of that "action=Move to Folder#2 by Rule#2 which is defined after Rule#1" is executed
when "Move to Folder#1 by Rule#1 which is defined before Rule#2 can not be normally executed because here is Folder#1" occurred?  

It seems that all hit conditions/actions in filter definition are associated to a msgHdr/msgKey,
instead of that condition/action check stops when action=StopExecution/MoveToFolder/Delete is requested by a filter rule.
It looks for me that all of "Stop further action when StopExecution/MoveToFolder/Delete" is done in "action execution phase".
If "Stop further action when StopExecution/MoveToFolder/Delete" were processed during filtering analysis phase, I think this kind of problem can't occur.

Comment 5

[WADA:World Anti-bad-Duping Agency]

aceman and Joprg K, can you help me?
I don't know and can't understand filter design/code and content of filter related objects or data blocks or variables...

Comment 6

[Jorg K (CEST = GMT+2)]

Interesting edge case. Do you have "Stop Filter Execution" at the end of the conditions? Otherwise I'd expect to happen what is happening.

Sadly I don't have time to look into it since there are other fires burning much hotter.

Comment 7

[WADA:World Anti-bad-Duping Agency]

Jorg K, thanks for comment. I'm relieved to know that I'm not "Bug Alone", Kevin of Home Alone :-)

I couldn't understand reason why problem occurs when move to folder failed because the folder is where I'm held in, so I tried to find root of "When move error occurred".
But it was perhaps wrong. It's not "Move failure".
It's perhaps that msgDBHdr/msgKey is not removed from msgDBHdr/msgKey list because msgDBHdr is not removed from move source folder when "Move to Self" case. Phenomenon may be following.
 If "Move to Other", msgKey in msgKeyList is removed because the msgKey is removed from Move Source==processing folder.
   After "Move to Other" by Rule#1, msgKey is not held any more, then go to next mail
 If "Move to Self",  msgKey in msgKeyList is not removed because the msgKey is not removed from Move Source(==Move Target).
   After "Move to Self" by Rule#1, msgKey is still held, then go to next filter rule.
   In old code, "go to next filter rule" is skipped by "returning ApplyMore=false" and "ApplyMore=false check by caller".
   In new code, "go to next filter rule" is not skipped in this special "Move to Self" case.

If simple for(nn=0;nn<something.length;nn++) loop, this kind of issue is easily bypassed merely by "nn++;continue;" at somewhere in loop.
But in filter code, complex recursive call is used.
  RunNextFilter calls ApplyFilter -> ApplyFilter calls RunNextFilter -> RunNextFilter calls ApplyFilter -> ...
And, CallBack is used during this complex recursive call because asynchronous execution is required.
I gave up to discover cause and to find possible code change by myself.
I wait for Kent James will modify his code in patch of bug 695671 or someone will find problem of new code in this special "Move to Self" case.

By the way, this "Move to Self" is not so edge case, if "Filter after Junk Classification" is used on imap folder, as easily known by bug 1215933.

(a) Following is not so rare in filter definition.
    Rule #1=Move to Folder#1, Rule#2=Move to Folder#2 && a mail can match with both rules
Mail of "From=aaa/Subject=bbb" can match with both of "if from is aaa" and "if subject=bbb", and rules are never abnormal.
Filter rules is "If Cond#1, move to Folder#1, Else If Cond#2, move to Folder#2, Else If ...", and filter rules are defined by users based on that "if action=move by rule#1 is executed, further rules/actions won't be applied/executed on this mail".
(b) As known by bug 1215933 and bug 770137, "Filter after Junk Classification" is applied on any new mail in any imap folder, as Junk Filter is applied on any new mail in any imap folder(except Trash/Junk).

And, tag team of "(a) + (b) + this bug" can pretty easily produce "infinite loop" phenomenon in Thunderbird.
Even if each is pretty small issue and edge case or negligible case for developers, tag team of them is horribly powerful :-)

Comment 8

[WADA:World Anti-bad-Duping Agency]

My guess on cause of problem in "Move to Self" case:
Loss of "following part of old code" in new code,
> -      // If we get here we're done applying filters for those messages that
> -      // matched, so remove them from the message header list
which was cared in old code by utilizing return code of applyMore.
> -      bool applyMore = true;
>        m_nextAction = 0;
> -      rv = ApplyFilter(&applyMore);
> -      NS_ENSURE_SUCCESS(rv, rv);
> -      if (applyMore)
> -      {
> -        // If there are more filters to apply, then ApplyFilter() would have
> -        // called RunNextFilter() itself, and so we should exit out afterwards
> -        return NS_OK;
> -      }
> -      // If we get here we're done applying filters for those messages that
> -      // matched, so remove them from the message header list

Comment 9

[WADA:World Anti-bad-Duping Agency]

If my guess is never absolutely incorrect, I think there are many possible solutions.
A. Don't add any action defined in subsequent rules to msgDBHdr/msgKey list entry for required actions on the message,
   if action=StopExecution or MoveToFolder or Delete is requested by a matched filter rule,
   because current design = Stop here when StopExecution or MoveToFolder or Delete is requested.
   (Execution order looks MoveToFolder->StopExecution if both actions are requested by a filter rule,    )
   (because actions are shown in this order by "See Execution Order"  when both are defined in any order.)
B. Stop complex recursive call, if possible. For example.
   Return ReturnCode from ApplyFilter() about next required action=RunNextFilter() or something else,
   and never call higher RunNextFilter() from lower or deeper level ApplyFilter(),
   and RunNextFilter(), who is caller of ApplyFilter(), always checks ReturnCode from ApplyFilter(),
   and RunNextFilter() determines required next action based on ReturnCode and other conditions.

I think Message Filtering can be represented by two dimensional matrix, for example.
>   Requested actions in predeined execution order(any rule can request any actions)
>          AddTag#X                      ... CopyToFolder#X  ... CopyToFolder#Z Delete MoveToFolder#X ... MoveToFolder#Z StopEx
> msgKey#1 [array of rules who requested]    [array of rules who requested]            [array of rules who requested]
> msgKey#2
Once table like above is created by "filtering condition/action analysis phase", I think "doing actions on a mail phase" can be pretty simple Do Loop for messages on a required action only with no need of caring about many conditions.
If Delete/MoveToFolder/StopExecution, pretty simple code for "do only first request for a message" is sufficient.
If this kind of Table is created by JavaScript code instead of C++ code, I feel logic can be simpler and easy to understand.
I feel "complex table for simple doing action" is better than "simple array with complex code for doing actions".

Comment 10

[:aceman]

This is rkent's area. Comment 8 may be helpful.

Comment 11

[Torsten Villnow]

I just want to note that I have had this problem for quite some time (and have been wondering about it), and it is still present in TB 102.6.0.