1836511 - folder.getNumNewMessages(false) returns the status before a filter move for new messages in POP accounts and thus does not display the "new message received" popup notification (also breaks a WebExtension event, for the same reason)

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[Mihai]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/113.0

Steps to reproduce:

Registered a listener with messages.onNewMailReceived, and I have filters in place to move the incoming emails into a custom folder.

Reproduced in a daily build of "115.0a1 (2023-05-23) (64-bit)" as well.

Actual results:

The listener was never called.
It does get called for the messages that are not filtered.

Expected results:

The listener should have been called.

Comment 1

[Mihai]

I have added a console.debug line in ext-mail.js, and it seems to bail out here:

      if (
        !(flags & Ci.nsMsgFolderFlags.Inbox) &&
        flags & (Ci.nsMsgFolderFlags.SpecialUse | Ci.nsMsgFolderFlags.Virtual)
      ) {
        // Do not notify if the folder is not Inbox but one of
        // Drafts|Trash|SentMail|Templates|Junk|Archive|Queue or Virtual.
        console.debug("Won't notify about new messages");
        continue;
      }

Comment 2

[Mihai]

And that seems to be due to the Ci.nsMsgFolderFlags.SpecialUse flag:

  /// Special-use folders
  const nsMsgFolderFlagType SpecialUse = Inbox|Drafts|Trash|SentMail|
                                         Templates|Junk|Archive|Queue;

Once it's SentMail, and then Trash.

Maybe the user folder is not even monitored for new emails?

Comment 3

[Mihai]

John, can you please have a look? I see that you have worked on this before. TIA.

Comment 4

[John Bieling]

I will have a look later this week.

Comment 5

[Mihai]

Thank you!

Comment 6

[Mihai]

John, please let me know if I can assist in any way. Thanks.

Comment 7

[John Bieling]

Looking at it now.

Comment 8

[John Bieling]

The code quoted in Comment #1 excludes the following special folders from being monitored for new mail notifications:

• Drafts
• Trash
• SentMail
• Templates
• Junk
• Archive
• Queue
• Virtual

It is the same restriction which is used by Thunderbirds own notification:
https://searchfox.org/comm-central/rev/b3e8077882ac90abee6e045d7a2f99c707be6659/mailnews/base/src/MailNotificationManager.jsm#241-261

What exact type of folder are you experiencing this with, and do you get a new mail notification from Thunderbird itself?

Comment 9

[Mihai]

What exact type of folder are you experiencing this with,

I have manually created a regular folder, called "Inbox2", and I have a message filter to move what arrives in Inbox to it. Can you please try to reproduce?

and do you get a new mail notification from Thunderbird itself?

Yes, I do.

Comment 10

[John Bieling]

Attachment: filter.JPG

Did some more tests. My test filter is pictured in the attached image.

1. Daily, IMAP account: Fully working. I get the TB notification and also the add-on notification.
   This is the data received by the onNewMailReceived event:

Object { folder: {…}, messages: {…} }
​ folder: Object { accountId: "account17", name: "Test ", path: "/Test " }
​ messages: Object { id: null, messages: (1) […] }
​​  id: null
​​  messages: Array [ {…} ]
​​​   0: Object { id: 3, author: "John Bieling <[e-mail]>", subject: "d2", … }

2. Daily, POP: Completely broken, no TB notification and also no add-on notification.
   It returns 0 here for folder.getNumNewMessages(false) and exits early (same in TBs own code).

3. Beta, IMAP: Fully working, same as Daily

4. Beta, POP: Completely broken, same as Daily.

---

So I did find something broken, which I have to understand and address (but it is not strictly extension related, there is a general bug in TB for pop accounts).

You however stated something different. Can you provide as much details as possible?

• What kind of account?
• What kind of folder? How many levels nested? Directly below Inbox?
• Can you state the flags value of the folder in question?
• Can you create a new folder in that account and change your filter to use that, any change in observed behaviour?
• If so, what is the flags value of the working folder?

Comment 11

[Mihai]

Thanks for reproducing!

What kind of account?

POP3.

What kind of folder? How many levels nested? Directly below Inbox?

Regular folder on the same level as "Inbox", manually created by using right-click \ "New Folder...".

Can you state the flags value of the folder in question?

I have already added extra log lines in findNewMessages, but I don't think it gets called for the user folder. I wrote in comment #2 that for whatever folders (two) it gets called when the email comes in in the scenario described, the flags are: "Once it's SentMail, and then Trash."

Is there any other way to check the flags for any given folder? I see that the WebExtension MailFolder only exposes a "type" property, not the actual "flags". And the "type" for the user folder seems to be "undefined".

Can you create a new folder in that account and change your filter to use that, any change in observed behaviour?

I have already created that folder specifically to reproduce in 115 (daily local build) what I got in 102 (release channel).

If so, what is the flags value of the working folder?

Please see the question above.

Comment 12

[Mihai]

and do you get a new mail notification from Thunderbird itself?

To clarify on this, I do get the sound beep and the system tray icon for "1 unread message", and then also the folder itself gets blue & bold, with an indication of the newly unread email.

Comment 13

[John Bieling]

Attachment: popup.JPG

You are on Windows, right?

To clarify on this, I do get the sound beep and the system tray icon for "1 unread message"

Do you get the popup notification for new messages in a POP account which get moved by filters on arrival? If you do not get the popup notification, then you do see the same issue as I described in Comment 10 (no popup for new messages moved by filters in POP accounts)

I wrote in comment #2 that for whatever folders (two) it gets called when the email comes in in the scenario described, the flags are: "Once it's SentMail, and then Trash."

I do not understand that sentence. As described in Comment 10, the integer flags value of the folder in question would be helpful. Set a breakpoint here:

https://searchfox.org/comm-central/rev/b3e8077882ac90abee6e045d7a2f99c707be6659/mail/components/extensions/parent/ext-mail.js#2513

and watch folder.flags and folder.prettyName (enter these two in the "Watch Expression" area of the debugger). This should tell you which folder is currently being processed and what its flags value is. Single stepping through the code (F10) shows you what the function is doing for your folder.

Comment 14

[Mihai]

You are on Windows, right?

Right.

Do you get the popup notification for new messages in a POP account which get moved by filters on arrival?

No.

If you do not get the popup notification, then you do see the same issue as I described in Comment 10 (no popup for new messages moved by filters in POP accounts)

Exactly.

I do not understand that sentence.

What I meant was that I added lines like this in ext-mail.js, for each possible flag:

if (flags & Ci.nsMsgFolderFlags.Queue)
            console.debug("Is Queue");

and when a new email was received and filtered into the custom folder, the findNewMessages was called twice. For the 1st call, the log showed that the "SentMail" flag was set, and then the "Trash" flag for the 2nd call.

As described in Comment 10, the integer flags value of the folder in question would be helpful. Set a breakpoint here:

Again, by the two calls and the flags detected, I don't think that's about the target folder. Is a move actually performed through a copy and a delete?

But in any case, how do I set a breakpoint? I come from the Visual Studio world. :)

and watch folder.flags and folder.prettyName (enter these two in the "Watch Expression" area of the debugger). This should tell you which folder is currently being processed and what its flags value is. Single stepping through the code (F10) shows you what the function is doing for your folder.

Ah, let me log the "folder.prettyName" and see.

Comment 15

[Mihai]

I've added the following line in the folders loop in findNewMessages:

console.debug(`folder: '${folder.prettyName}', flags: ${flags}`);

and got this upon receiving a new email that got filtered into the user "Inbox2" folder:

WebExtensions: folder: 'Sent', flags: 516 ext-mail.js:2449
WebExtensions: folder: 'Trash', flags: 260 ext-mail.js:2449

516 is for SpecialUse (SentMail), and 260 is for SpecialUse (Trash).

Comment 16

[Mihai]

Ah, wait. I see three more log lines before that, for three other folders:

WebExtensions: folder: '<sanitized-email-account>', flags: 28 ext-mail.js:2449
WebExtensions: folder: 'Inbox', flags: 4100 ext-mail.js:2449
WebExtensions: folder: 'Inbox2', flags: 4 ext-mail.js:2449

It's the "4" that you were after, right?

Comment 17

[Mihai]

And that would be this, according to "comm\mailnews\base\public\nsMsgFolderFlags.idl":

  /// This folder is a mail folder.
  const nsMsgFolderFlagType Mail            = 0x00000004;

Comment 18

[John Bieling]

You can open the debugger via CTRL+SHIFT+I (capital i) with focus in the main window.

In the opened developer tools window, go to the debugger tab and then press CTRL+P and enter "ext-mails.js". Breakpoints are set by clicking on a line number.

Comment 19

[John Bieling]

WebExtensions: folder: 'Inbox2', flags: 4 ext-mail.js:2449

That is what I see as well.

So the folder type is not the reason for the event not being fired, it is the return value of folder.getNumNewMessages(false) being 0 even though the folder does have new messages. Correct?

Can you confirm that this is not a regression in Beta, but also does not work in Thunderbird 102? I just tested it, but would like to have confirmation.

Comment 20

[Mihai]

So the actual flags that make up the values above are:

• folder: '<sanitized-email-account>', flags: 28 = 0x1C => Elided | Directory | Mail
• folder: 'Inbox', flags: 4100 = 0x1004 => Inbox | Mail
• folder: 'Inbox2', flags: 4 => Mail
• folder: 'Sent', flags: 516 = 0x204 = SentMail | Mail
• folder: 'Trash', flags: 260 = 0x104 = Trash | Mail
  You would then need to allow the plain "Mail" through, maybe?

Comment 21

[John Bieling]

If you step through the code, you should see that the folder type is irrelevant for this issue. For your inbox2 folder, the function is moving past that if statement, and only the check for folder.getNumNewMessages(false) is causing the function to exit early.

Comment 22

[Mihai]

and then press CTRL+P and enter "ext-mails.js"

That's gold, thanks!

So the folder type is not the reason for the event not being fired

You're right, it won't qualify as the special case to skip the folder.

it is the return value of folder.getNumNewMessages(false) being 0 even though the folder does have new messages. Correct?

Yes, and the value is actually "1" for "Inbox", that no longer has the message.

Can you confirm that this is not a regression in Beta, but also does not work in Thunderbird 102? I just tested it, but would like to have confirmation.

Conformed: the issue is the same in both 102.11.2 (64-bit) (release channel) & "115.0a1 (2023-05-23) (64-bit)" (daily local build).
Even though it's clear now that the flags are not the issue, the user folder in 102 has the flags = 20 = 0x14 = Elided | Mail.

Comment 23

[John Bieling]

So we do see the same thing. Great!

The wrong return value of folder.getNumNewMessages(false) for POP accounts is what is causing this. You added important info, that it simply does not get updated by the filter move and returns 0 for the folder which actually has the new message after the filter move and 1 for the inbox folder, which no longer has the message after the filter move.

This is a core issue, as Thunderbird uses the same code for its popup notification.
https://searchfox.org/comm-central/rev/b3e8077882ac90abee6e045d7a2f99c707be6659/mailnews/base/src/MailNotificationManager.jsm#241-261

There is however other code, which correctly triggers the changes in the folder tree and the system tray / task bar. I now have to check for how this can be fixed.

@mkmelin: Do you see an obvious reason for why the filter move is not reflected in the return value of folder.getNumNewMessages(false) here:
https://searchfox.org/comm-central/rev/b3e8077882ac90abee6e045d7a2f99c707be6659/mailnews/base/src/MailNotificationManager.jsm#256-258

Can you point me to code which updates the UI in the folder tree, or the tray or task bar icon? Because that is correctly indicating where the new message is at, after the filter move.

This is also broken in 102, not a Supernova regression.

Comment 24

[Magnus Melin [:mkmelin]]

The use of .descendands in _getFirstRealFolderWithNewMail reminded me of something I ran into at some point where that doesn't fully work, while .subFolders worked. If you have an easy way to reproduce, try changing that and see if it makes a difference.

Other than that, I don't have any good clues on what might be up.

The folder will be updated in the UI when it gets notified of a change - https://searchfox.org/comm-central/rev/164fbd6fe027a2804e97bef6bdec6e065a0e6955/mail/base/content/about3Pane.js#1549

Comment 25

[John Bieling]

I tried replacing .descendants by .subFolders (and recursively walked through them), but that did not change the situation: for filter-moved-on-incomming events, folder.getNumNewMessages(false) returns 0 for the destination folder and 1 for the original folder.

• For POP accounts the value of folder.getNumNewMessages(false) is not updated in the source and destination folder, if a message is filter-moved-on-incomming.

The observer pointed to by Magnus does not seem to be called for the update event on new messages, but the folder listeners for onFolderIntPropertyChanged with a "BiffState" or "NewMailReceived" topic, and onFolderBoolPropertyChanged with a "NewMessages" topic. The last one seems to be responsible for the UI update and is marking the destination folder as "has new mail" in a filter-moved-on-incomming event.

• Even for POP accounts, the folder.hasNewMessages flag is correctly set on the source and destination folders.

States of a POP folder after filter-move-on-incomming event (moved to test):

name: "[e-mail]", hasNewMessages: false, numNewMessagesThis: 0, numNewMessagesDeep: 1
name: "Inbox", hasNewMessages: false, numNewMessagesThis: 1, numNewMessagesDeep: 1
name: "test", hasNewMessages: true, numNewMessagesThis: 0, numNewMessagesDeep: 0
name: "test2", hasNewMessages: false, numNewMessagesThis: 0, numNewMessagesDeep: 0

Besides folder.getNumNewMessages(false) not working correctly, I believe there is a general logic flaw in the alert-trigger-code.

Edit: The observations and conclusions mentioned here were wrong and removed.

Comment 26

[Mihai]

Thanks John for the thorough analysis! What's would the next step be? Wait for Magnus to confirm?

My 2c on your #1 above: checking hasNewMessages instead of "(folder.getNumNewMessages(false) > 0)" makes sense, unless hasNewMessages is "deep".

Comment 27

[John Bieling]

I was working on related code and learned that getNumNewMessages() only returns the new messages since last biff [1], not the number of currently new messages in that folder (that is folder.msgDatabase.getNewList().length). So my statement in Comment 25 seems wrong. Will investigate more.

[1] : https://searchfox.org/comm-central/rev/451150f82556f927edc20a47d2b5f6fb396aa341/mailnews/base/public/nsIMsgFolder.idl#522-528

Comment 28

[Mihai]

Any progress on this one? I'm interested in using the currently broken messages.onNewMailReceived.

Comment 29

[crystalidea]

Any progress, please? It's still broken on 128.0.1esr (64-bit)

Comment 30

[crystalidea]

Same for 129.0b6 (64-bit), no popup notification neither onNewMailReceived is triggered
POP, Windows, Message is moved by a custom filter in a sub-folder of Inbox

Comment 32

[crystalidea]

John, it seems that you fixed this by modifying the onNewMailReceived API and adding 'monitorAllFolders' flag
https://phabricator.services.mozilla.com/D192024
https://hg.mozilla.org/comm-central/rev/d9d52f582bf5

Seems that your fix has never been accepted, right?

Thank you for looking into this, I personally consider this issue a major one because onNewMailReceived is not working for no folders except Inbox for POP3 accounts (however the docs never mention that). Neither popup notification appear for those folders.

Comment 33

[crystalidea]

My bad, that was another issue https://bugzilla.mozilla.org/show_bug.cgi?id=1848787.
Any hope for this one?

Comment 34

[Sarim Khan]

Attachment: Bug 1836511 - Track filter-moved messages for POP3 accounts to properly update new message counts and notify. r=BenC

• Introduce m_filterTargetFoldersMsgMovedCount to track moved messages
• Set msgIsNew to false for filter-moved messages to decrease inbox new message count
• Update filter target folders new message counts and fire notification at the end of message download

Comment 35

[John Bieling]

Thanks Sarim Khan for tackling this!

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e07340388af6
Track filter-moved messages for POP3 accounts to properly update new message counts and notify. r=BenC

Comment 37

[John Bieling]

I can confirm, that this patch fixes the core of the reported issue with the messages.onNewMailReceived event for POP accounts on Daily. Tested with 132.0a1 (2024-09-21) (64-bit).

Thank you so much for your work on this issue, Sarim Khan!

This is my background script:

browser.messages.onNewMailReceived.addListener(newMessage, true);
function newMessage(folder, messages) {
    console.log({folder, messages});
}

I set up a POP account and added a filter which moved all messages on "Getting New Mail" to a folder "/filtered". I tested with "Filter before Junk Classification". I received the expected event:

folder: { id: "account24://filtered", name: "filtered", path: "/filtered", … }
​messages: { 
   id: null,
   messages: [
     { id: 155, author: "John Bieling <[e-mail]>", subject: "test1", … }
   ]
}​​​

There is a small deviation from IMAP accounts. If I send a second message, without opening the filtered folder and looking at the first message, I get the notification for the first message again:

folder: { id: "account24://filtered", name: "filtered", path: "/filtered", … }
​messages: { 
   id: null,
   messages: [
     { id: 155, author: "John Bieling <[e-mail]>", subject: "test1", … }
     { id: 156, author: "John Bieling <[e-mail]>", subject: "test2", … }
   ]
}​​​

Doing the same on IMAP will only report the second message. Unfiltered POP messages, which have been delivered directly to the inbox do not show this behavior.

There is a second deviation: If I use "Filter after Junk Classification", the message no longer has the "new" flag, after it has been moved by the filter, and no event is fired. Repeating the same test with an IMAP account, reports the message.

Sarim Khan, do you have any idea as to why this might happen? I will attach a fully working add-on for you to reproduce the issue.

Comment 38

[John Bieling]

Attachment: onNewMailReceived.zip

Add-on to reproduce behavior of POP messages being moved by a filter.

To install this add-on, first open Thunderbird and then its Add-on Manager ("Add-Ans and Themes" entry in the tools menu). Then drag the link to the add-on into Thunderbird's Add-on Manager tab.

Comment 39

[Sarim Khan]

The pop3 code is very very old and messy, and I'm afraid it has many other bugs as well. And the way its written originally not every bug might be solvable or worth solvable. There are some comments in the phabricator thread as well, where BenC expressed the desire to have filtering code fully uncoupled/rewritten in future.
From my understanding of reading the codebase, pop3 codebase assumes the only "alive" folder is inbox. Its hardcoded. So many behavior/features are hardcoded to inbox.

Now about the issues you mentioned:

1. Filter before Junk Classification and Filter after Junk Classification has separate code path / functions. I didn't touch Filter after Junk Classification code path, it had some strange behavior before my modification, I think you're seeing some of that.

2. Interesting observation. On theory when downloading a set of mail aka doing a new Biffing, at start: states / count of folders should be reset. I'm pretty sure pop3 codebase is only doing this for hardcoded inbox folder, not the other folders, (see my above line about it). pop3 codebase doesn't have reference or do things with other folders than inbox.

Note that these are my observations from my limited understanding, so I might be wrong.

Comment 40

[Kai Engert [:KaiE:]]

What's the level of importance of this regarding potentially backporting it to the stable 128 release ?

Comment 41

[Ben Campbell]

I honestly don't know.
I think the benefit to webextension would probably be the decider here, so I'll ask John to chime in.
(I don't think it touches code that overlaps with any other recent work, so it's probably an uncomplicated uplift)

Comment 42

[John Bieling]

I would like to backport it at some point, but it has no prio.

If the backport makes it more difficult for you, then we do not need to. If not backporting it however makes it more complicated (merge issues), then we can backport it whenever you want.