Open Bug 770137 Opened 12 years ago Updated 11 months ago

"Filter after Junk Classification" causes it to affect all directories for imap CallFilterPlugins() is invoked for all new mail (for new UID/no \Seen in any imap folder, then "Filter After" is executed in all imap folder)

Categories

(MailNews Core :: Filters, defect)

defect
Not set
major

Tracking

(Not tracked)

People

(Reporter: mozilla, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: reproducible)

User Story

This bug is problem since initial of "Filter after Junk Classification", or regression between Tb 3.0("Filter after" was introduced) and Tb 13(this bug was reported).
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120614114901

Steps to reproduce:

I created two filters that run before and after the junk filter.

Filter 1:
Run on: Fetching messages or manual
Condition: 'From' ends with 'local.host'
Action: Move to folder '1'

Filter 2:
Run on: Fetching messages or manual (after Junk Filter)
Condition: 'From' ends with 'remote.host'
Action: Move to folder '2'



Actual results:

Filter 1:
Moved all eMails from Inbox to folder 1 that matched condition.
Filter 2:
Moved all eMails from Inbox, Drafts and Archive to folder 2 that matched condition.


Expected results:

Filter 2 should not affect Drafts and Archive at any time.
Component: General → Filters
Product: Thunderbird → MailNews Core
QA Contact: general → filters
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.
Phenomenon of bug 1215933 #47 can easily be observed on Thunderbird 45, because Bug 1320676(Bug 232569 came back) startred to occur from Thunderbird 37, in addition to old this bug.

Thid bug is 100% reproducible.
Confirming.

Kent James, can you analyze this issue?
Blocks: 1215933
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(rkent)
OS: Windows 7 → All
Hardware: x86_64 → All
FYI.
Loop like bug 1215933 is perhaps reproducible with this bug only by following.
  imap account#1 : Getting New Mail: Filter after Junk Classification
    FilterRule#1 : if subject contains ����, copy to accout#2/FFFD_mail
  imap account#2 : Getting New Mail: Filter after Junk Classification
    FilterRule#1 : if subject contains ����, copy to accout#1/FFFD_mail
  This is funny definition, but is never abnormal definition.
    If such junk mail newly arrived, notify to another imap accout by copying the mail.
"Copy to folder of different imap account" is "select FolderX + append", and in order to know UID of the copied mail, "uid fetch HighestUID:* Flags" is needed. This is same as new mail check, folder open.
It may be different if UIDPLUS(APPENDUID,COPYUID) is supported, because UID of copied mail is known by response to append/copy.
Following is an explanation on message filter/junk filter by perhaps Kent James.
> http://mesquilla.com/2009/08/28/managing-spam-with-after-classification-filters/
> The order of message processing in Mozilla mailnews is:
>  1. Run normal filters (on each message as it is received)
>  2. Check whitelisting (on a message batch, this and subsequent steps)
>  3. Run bayes classifier on non-whitelisted messages, and mark messages as junk or good.
>  4. Apply “after classification” filters.
>  5. Apply junk message moves using default junk processing.

I didn't know that Junk move is last action.
And, I now understand "Junk Classification" != "Junk move"

(A) Following is clear.
 Order : Filter before Classification -> Junk Classification -> Filter after Classification -> Junk move
 Used rule : Filter before = rules defined as "before"
             Junk filter   = Bayesian Filter(training.da)
             Filter after  = rules defined as "after"
             Junk move     = Jank score, and Junk folder setting
 Target mail : Filter before = New mails detected at Inbox by new mail check or via. IDLE,
                 and if mail.server.server#.applyIncomingFilters==true,
                     + New mails in "folder included in new mail check"(new mail check/IDLE)
(B) But following is ambiguous for me.
 Target mail in Junk filter :
   "filter moved mail by before" is included in "mail to which Junk filter is applied".
   How UID of moved mail is obtained when "move from imap to different imap" && APPEND UID is not supported?
 Action of "Filter before" :
   Action is also done "on each message as it is received" basis?
   When is "Move to folder, so do nothing any more on the mail" and "Stop Execution" activated?
   Following is design?
     All filter rules are applied and actions are listed up
     -> first Move is executed
     -> when other rule tries to do Move, mail doesn't exist, then fails
     -> nothing is done by non-first rule.
   It is not a kind of batch on "newly arrived mails" at least for Move?
   How UID of moved mail in move target imap folder is passed to Junk filter?
 Target mail of "Filter after" :
   What is design?
 Move mail(s) to folder/Junk by Message filter and Junk filter :
   When/How "Move mail" is executed by (i) "Filter before" and (ii) "Filter after" and (iii) "Junk move"?
   Is (i) / (ii) / (iii) correctly serialized?
   If yes, why "some error looks to have occurred due to contention of multiple moves on a mail"
   are reported for long time?
As I wrote in bug 1215933 comment #52, from perspective of execution of "FilterAfterTheFact", it was always batch type "ApplyFiltersToMessages", although main object is nsMsgFilterAfterTheFact and nsMsgApplyFiltersToMessages is override on nsMsgFilterAfterTheFact.
> Run filters on folder     : selected messages == all messages in requested folder, selected filters = Manually Run
> Run filters on messages   : selected messages == selected messages of a folder at gDBView, selected filters = Manually Run
> Run Now of Message Filter : selected messages == all messages in requested folder, selected filters = Selected filters here
> Filter "after"            : selected messages == selected messages by someone, selected filters = Getting New Mail:Filter after

I believe that problem is not in FilterAfterTheFact side.
Problem is in who invoked message filter execution of "Getting New Mail:Filter after Junk Classification" on "newly detected Unread mails" in a mail folder where message filter shouldn't be invoked by *DESIGN* of Message Filter.

Fault in imap code instead of message filter code? Or fault of that message filter accepts/executes wrong request from imap?
(Off-Topic)
I think main/base object should be ApplySelectedFiltersToSelectedMessages.
Others can be override of it.
> ApplyManuallyRunFiltersToRequestedFolder,
> ApplyManuallyRunFiltersToSelectedFolder*S*AtFolderPane,
> ApplyFilterAfterToSelectedMessages,
> ApplyFilterAfterToSelectedMessagesRegardlessOfMessageFilterDesign,
> ApplyFilterAfterToSelectedMessagesBasedOnMessageFilterDesign
User Story: (updated)
Oh, Ludo did following on 2012-07-04.
 Component: General → Filters
 Product: Thunderbird → MailNews Core
Why no one could follow this bug? If someone did duplication test, because this bug is 100% reproducible by pretty 
simple/easy test, this bug was pretty quickly confirmed. And, if this bug was resolved after release of "Filter after Junk Classification", no one was suffered from pretty annoying problem like bug 1215933.
"Kent James, can you analyze this issue?"

Sorry, I am completely overwhelmed with other issues right now.
(In reply to Kent James (:rkent) from comment #8)

I see. Please read bug 1215933 comment #55 and bug 1215933 comment #52 when you have time.
FYI.
It looks for me that Bug 517849 is a report of same issue as this bug in imap on Junk Cllassification&&Junk Move.
Bug 517849 was opened on 2009-09-21, for "Gecko/20090915 Shredder/3.0b4pre".
> Bug 517849 - Only scan NEW emails for junk
> When thunderbird is setup with an IMAP account,
> it looks to be scanning all unread emails in all folders for junk mail.
It looks for me that Bug 189970 is one of oldest bug reports on same phenomenon in imap && Junk Filter.
Summary: Setting a filter to run after the Junk filter will cause it to affect all directories. → Setting a filter to run after the Junk filter will cause it to affect all directories(When imap, CallFilterPlugins() seems invoked for any new mail(new UID/no \Seen) in any folder, so "Junk Filter"/"Filter After" looks executed on any new mail in imap)
It may be following and by design/implementation.

When pop3, new mail is "mail downloaded by RETR command upon new mail check" only.
When imap, new mail can be detected at any of new mail check, IDLE, folder open.
Because Junk filter should be applied to any new mail, nsMsgDBFolder::CallFilterPlugins() is requested for any new mail.
In CallFilterPlugins(),
  Filter before Junk Classification = elder brother, so he can behave himself,
    then he applies message filter to mails in required folder only based on regulation,
    and passes list of "moved mail to move target folder" to TwinA.
  Junk filter and Filter after = Twins and Active kids, and they play together with.
    TwinA(Junk Classification) always plays with all requested mails including "moved mail to move target folder"
    TwinB(Filter after Junk Classification) always plays with all requested mails("moved mail to move target folder" too?)
  Junk move=tender sister, so she kindly moves mail to Junk folder for TwinA
Flags: needinfo?(rkent)
(In reply to Michael Dirks from comment #0)
> Expected results:
> Filter 2 should not affect Drafts and Archive at any time.

In CallFilterPlugins(), special folder check seems done on Junk, Trash etc. only.
I also think Junk Classification/Filter after Classification is better avoided if Sent,Drafts,Archives,Templates.
If filtering is needed, "Run filters on folder/messages" and "Run Junk Mail Control on folder" are available.
And, when Archives/Sent, Archiving/Sending is already implemented. Automatic filter move at Archives/Sent can produce excess/unwanted problem for user.
Following was target folder selection rule in Junk Classification(filterForJunk).
> https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#2495
>  nsMsgDBFolder::CallFilterPlugins(nsIMsgWindow *aMsgWindow, bool *aFiltersRun)
> https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#2528
> // if this is the junk folder, or the trash folder
> // don't analyze for spam, because we don't care
> //
> // if it's the sent, unsent, templates, or drafts,
> // don't analyze for spam, because the user
> // created that message
> //
> // if it's a public imap folder, or another users
> // imap folder, don't analyze for spam, because
> // it's not ours to analyze

I think Archives is better excluded from target of filterForJunk, because user moved message which is already considered as NonJunk.
I think same rule is better used in "Filter after Junk Classification"(filterPostPlugin), because filterForJunk and  filterPostPlugin are twins.
I believe that one of major purposes of filterPostPlugin is modification of Junk detection by filterForJunk who is pretty hard to control for user.
Although conditions/actions for it is not implemented yet(by JunQilla/FiltaQilla only?), preparation for it is better done now.
If user want filtering of messages in such Special folders, "Run filters on folder/messages" and "Run Junk Control on folder" are already available.

By the way, further, it should be documented for user. I didn't know about above spec until I discovered the source code.
If 0.001% of bug openers at B.M.O would join in documentation project instead of merely adding comments for complaint only, documents of Help->Help Documents would have been far better than current :-)
To Michael Dirks(bug opener): What do you think about current implementation and improvements of it?
Flags: needinfo?(michaeldirks)
Summary: Setting a filter to run after the Junk filter will cause it to affect all directories(When imap, CallFilterPlugins() seems invoked for any new mail(new UID/no \Seen) in any folder, so "Junk Filter"/"Filter After" looks executed on any new mail in imap) → Setting a filter to run after the Junk filter will cause it to affect all directories(When imap, CallFilterPlugins() looks invoked for any new mail(new UID/no \Seen) in any folder, so "Junk Filter"/"Filter After" seems executed on any new mail in imap)
(In reply to WADA from comment #14)
> To Michael Dirks(bug opener): What do you think about current implementation
> and improvements of it?

I do not know what the current implementation is, as I no longer use Thunderbird. After too many complaints, errors and data loss I've switched to a web-mail solution which supports scripting and filtering. Massively lowered data loss and complaints and also is faster to set up.

If the implementation stayed the same, then my initial comment/statement still holds true: "After Junk Filter" currently affects everything, including Archive and Drafts, while the normal one does not. So the ideal fix would be to change the behaviour of either to match the other.

Fix Option 1: Make After Junk Filter behave like the normal one except it doesn't filter Junk mail. This is probably what this filter location was intended to do.

Fix Option 2: Make the normal one behave like After Junk Filter. While I consider this the stupid fix option (since it makes no sense, why would new e-mails be outside of the INBOX or JUNK folder?), it's not up to me to decide which one to implement.
Flags: needinfo?(michaeldirks)
(In reply to Michael Dirks from comment #15)
> I do not know what the current implementation is, as I no longer use
> Thunderbird. After too many complaints, errors and data loss I've switched
> to a web-mail solution which supports scripting and filtering. Massively
> lowered data loss and complaints and also is faster to set up.

You look system management people in corporate environment.
I could understand why there is no complaint only comment in this bug :-)

> Fix Option 1: Make After Junk Filter behave like the normal one except it
> doesn't filter Junk mail. This is probably what this filter location was intended to do.

I initially thought so when I discovered this bug report. I thought difference is "before/after" only. But it was wrong.

Design/concept was:
- Filter before Junk Classification == Message filter on new messages in "Inbox" :
    "Inbox" in this context == mbox named Inbox
           + if(true==mail.server.server#.applyIncomingFilters) (mbox of "include this in new mail check");
- Filter after Junk Classification(filterPostPlugin) ==
    Message filter on new messages to which Junk filter was applied.

Design/concept/implementation/purpose was different. Difference was never "before/after" only.
"Junk Classification" is applied on any new mail in any imap folder, including "mail moved by Filter before".
This is base design.
Because Junk Classification excludes Sent/Drafts etc., filterPostPlugin should have excluded them.
Archives was not excluded in "Junk Classification". It should be excluded in both.

I believe that a fault was "produced misunderstanding by lack of appropriate description about Filtering in Thunderbird".
And, your proposal or request in comment #0 was correct since initial and is still correct.

Thanks for answer, and sorry for too late QA work on this bug in B.M.O.
Brief Summary about from "download/fetch of new message" to "execution of Filter after Junk Classification".

[ 1 ] When any "New Message" is downloaded or fetched, CallFilterPlugins() is called,
      in order to apply Junk Filter.

(1-a) When POP3, someone calls EndMsgDownload() when new mail is downloaded at pop3 account.
> https://dxr.mozilla.org/comm-central/search?q=EndMsgDownload&redirect=false
(POP3-1) EndMsgDownload() is called by someone when new mailis downloaded  at pop3 account.
> https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#2386
>   nsresult nsParseNewMailState::EndMsgDownload()
(POP3-2) EndMsgDownload() calls CallFilterPlugins()
> https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#2409
>   m_filterTargetFolders[index]->CallFilterPlugins(nullptr, &filtersRun);

(1-b) When IMAP, CallFilterPlugins() is called at two places in nsImapMailFolder.cpp.
> https://dxr.mozilla.org/comm-central/search?q=CallFilterPlugins&redirect=false
> #5889 is in nsImapMailFolder::HeaderFetchCompleted()
>   and CallFilterPlugins() is called at here when "if (!m_filterListRequiresBody)".
> #4693 is in nsImapMailFolder::NormalEndMsgWriteStream()
>   and CallFilterPlugins() is called at here when "if (m_filterListRequiresBody)".

(IMAP-a) After initial header fetch of newly detected messages(new UID in any imap folder),
> https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#5889
>   if message filter for "Getting New Mail:" doesn't have Body condition,
>   CallFilterPlugins() is called immediately. 

(IMAP-b) After initial header fetch of newly detected messages(new UID in any imap folder),
> https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#4693
>   if message filter for "Getting New Mail:" contains Body condition,
>   CallFilterPlugins() is postponed, and uid fetch xx:yy body.peek[] is issued,
>   and CallFilterPlugins() is invoked when body[] of each new message is fetched.

[ 2 ] CallFilterPlugins() is controller of Filter.

> https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#2495
> nsMsgDBFolder::CallFilterPlugins(nsIMsgWindow *aMsgWindow, bool *aFiltersRun)
> {
>   Do message filter application("Filter before Junk Classification").
>   Do Junk Classification jobs.
>   When Junk Classification completed,
>   if (filterPostPlugin)
>     OnMessageClassified(nullptr, nsIJunkMailPlugin::UNCLASSIFIED, 0);
> }

[ 3 ] filterPostPlugin is executed

(3-1) OnMessageClassified is called for filterPostPlugin.
> https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#2322
> nsMsgDBFolder::OnMessageClassified(const char *aMsgURI,nsMsgJunkStatus aClassification,uint32_t aJunkPercent)
> {
>   // Call ApplyFilters() for Filter after Junk Classification
>   rv = filterService->ApplyFilters(nsMsgFilterType::PostPlugin,mPostBayesMessagesToFilter,
>   // Notify for final Junk Move process
>   notifier->NotifyMsgsClassified(classifiedMsgHdrs,mBayesJunkClassifying,mBayesTraitClassifying);                                        this, nullptr, nullptr);
> }

(3-2) ApplyFilters invokes filterExecutor named nsMsgApplyFiltersToMessages.
> https://dxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgFilterService.cpp#1097
> NS_IMETHODIMP nsMsgFilterService::ApplyFilters(nsMsgFilterTypeType aFilterType,
>   nsIArray *aMsgHdrList,nsIMsgFolder *aFolder,nsIMsgWindow *aMsgWindow,nsIMsgOperationListener *aCallback)
> {
>   // Create our nsMsgApplyFiltersToMessages object which will be called when ApplyFiltersToHdr
>   // finds one or more filters that hit.
>   RefPtr<nsMsgApplyFiltersToMessages> filterExecutor =
>     new nsMsgApplyFiltersToMessages(aMsgWindow, filterList, folderList,
>                                     aMsgHdrList, aFilterType, aCallback);
>   if (filterExecutor)
>     return filterExecutor->AdvanceToNextFolder();
> }

(3-3) nsMsgApplyFiltersToMessages is executed for "Filter after Junk Classification"

  nsMsgFilterAfterTheFact     : for "Run filters on folder"  of Tools menu
  nsMsgApplyFiltersToMessages : for "Run filters on message" of Tools menu
  nsMsgApplyFiltersToMessages is perhaps also used by "Run selected filters on: / Run Now" of Message Filter.

> https://dxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgFilterService.cpp#276
> // this class holds the list of filters and folders, and applies them in turn, first iterating
> // over all the filters on one folder, and then advancing to the next folder and repeating.
> // For each filter,we take the filter criteria and create a search term list. Then, we execute the search.
> // We are a search listener so that we can build up the list of search hits.
> // Then, when the search is done, we will apply the filter action(s) en-masse, so, for example, if the action is a move,
> // we calls one method to move all the messages to the destination folder. Or, mark all the messages read.
> // In the case of imap operations, or imap/local  moves, the action will be asynchronous, so we'll need to be a url listener
> // as well, and kick off the next filter when the action completes.
> class nsMsgFilterAfterTheFact : public nsIUrlListener, public nsIMsgSearchNotify, public nsIMsgCopyServiceListener
> {
> public:
>   nsMsgFilterAfterTheFact(nsIMsgWindow *aMsgWindow,
>                           nsIMsgFilterList *aFilterList, nsIArray *aFolderList,
>                           nsIMsgOperationListener *aCallback);
> 
> https://dxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgFilterService.cpp#990
> // nsMsgApplyFiltersToMessages overrides nsMsgFilterAfterTheFact in order to
> // apply filters to a list of messages, rather than an entire folder
> class nsMsgApplyFiltersToMessages : public nsMsgFilterAfterTheFact
> {
> public:
>   nsMsgApplyFiltersToMessages(nsIMsgWindow *aMsgWindow,
>                               nsIMsgFilterList *aFilterList,
>                               nsIArray *aFolderList, nsIArray *aMsgHdrList,
>                               nsMsgFilterTypeType aFilterType,
>                               nsIMsgOperationListener *aCallback);
Summary: Setting a filter to run after the Junk filter will cause it to affect all directories(When imap, CallFilterPlugins() looks invoked for any new mail(new UID/no \Seen) in any folder, so "Junk Filter"/"Filter After" seems executed on any new mail in imap) → Setting a filter to run after the Junk filter will cause it to affect all directories(When imap, CallFilterPlugins() is invoked for any new mail(new UID/no \Seen) in any imap folder, then "Filter After" is executed on any new mail in any imap folder)
(simplify summary)
Summary: Setting a filter to run after the Junk filter will cause it to affect all directories(When imap, CallFilterPlugins() is invoked for any new mail(new UID/no \Seen) in any imap folder, then "Filter After" is executed on any new mail in any imap folder) → "Filter after Junk Classification" causes it to affect all directories for imap CallFilterPlugins() is invoked for all new mail (for new UID/no \Seen in any imap folder, then "Filter After" is executed in all imap folder)
See Also: → 1320676
See Also: → 1242257
You need to log in before you can comment on or make changes to this bug.