<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Comment 2

•

18 days ago

•

Edited

As suggested by Hartmut here https://phabricator.services.mozilla.com/D267518#9249473
reversing the calls (and just doing UpdateFolder() w/o the listener) seems to work for me. However, if I add a simple printf after the calls, it fails again, usually. If I then again remove the printf it seems OK for the messages I've tried to filter after sending.

Comment 3

•

18 days ago

Attached file WIP: Bug 2008314 - Fix timing in 'after send' filters. (obsolete) — Details

Comment 4

•

18 days ago

This works for me. I left the reviewer open, there are no changes planned.

Comment 5

•

17 days ago

Yury, for me your comment 3 patch hangs in the new while loop. FYI, my filter is set like this:

Only "After Sending" checked
Match subject contains "moveit"
Actions:
Tag msg "Personal"
Move msg to "test" on Local Folders

Sent message is saved to Sent folder and appears tagged as Personal, but never gets moved to LF/test folder.
When sending, "save complete" popup remains present until I cancel it.
I added a printf inside the while loop and it prints until I shutdown tb.

Comment 6

•

17 days ago

•

Edited

That is rather strange. I retested it and it works fine here. You noticed that I updated the patch:

+                nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
+                while (NS_ProcessNextEvent(thread)) {
+                  // Do nothing, just process all the pending events.
+                }

The loop should run while there are pending events. One could add a break that somehow gets triggered when rule execution is finished from here:
https://searchfox.org/comm-central/rev/c6d3230444583664b4ef87a8466a99634f5a6284/mailnews/compose/src/nsMsgCopy.cpp#93
That's what triggers the "after send" rule execution here:
https://searchfox.org/comm-central/rev/c6d3230444583664b4ef87a8466a99634f5a6284/mailnews/compose/src/MessageSend.sys.mjs#541

Update: this hacky approach also works:

                (void)OnCopyCompleted(m_copyState->m_srcSupport, aExitCode);
                // Clear event queue, that is, execute "after send" filters,
                // before calling `UpdateFolderWithListener()`.
                nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
                PRTime startTime = PR_Now();
                while (NS_ProcessNextEvent(thread) && PR_Now() - startTime < 1000000) {
                  // Do nothing, just process all the pending events.
                }
                UpdateFolderWithListener(msgWindow, m_urlListener);

I'll see whether I can implement something that waits for the rule execution to finish before calling UpdateFolderWithListener().

Update 2: In Phab use of NS_ProcessPendingEvents(thread); was suggested. I'll try this later, thanks Hartmut!

Assignee

Comment 7

•

17 days ago

(In reply to gene smith from comment #2)

As suggested by Hartmut here https://phabricator.services.mozilla.com/D267518#9249473
reversing the calls (and just doing UpdateFolder() w/o the listener) seems to work for me.

I'd not recommend doing that anymore. In my case, the issue from bug 1990129 only got worse: After dragging an .eml file onto an IMAP folder, the message appeared twice after a while (one with a fake key, one with an ordinary key). Switching the folder fixed it.

However, if I add a simple printf after the calls, it fails again, usually. If I then again remove the printf it seems OK for the messages I've tried to filter after sending.

That I find really curious!

Comment 8

•

17 days ago

Ok, I'm now using the latest patch at Yury's comment 3 which doesn't use the loop. Messages now get moved to LF/test as they should and there is no hang. However, the first few times I ran with it, the messages transferred to LF/test were not tagged. I still had the printf after the calls. I removed the printf and re-built and filtered messages were then tagged in LF/test. Put the printf back and re-built and filtered messages continued to be tagged in LF/test. So the printf, present or not present, is probably not relevant. There must be something else random that causes it to tag or not tag.

Just to make sure I am using the right code, here's the place where the main patch occurs:

                (void)OnCopyCompleted(m_copyState->m_srcSupport, aExitCode);
                // Clear event queue, that is, execute "after send" filters,
                // before calling `UpdateFolderWithListener()`.
                nsCOMPtr<nsIThread> thread(do_GetCurrentThread());
                NS_ProcessPendingEvents(thread);
                UpdateFolderWithListener(msgWindow, m_urlListener);
                printf("gds: imapAction= 0x%x\n", imapAction);

Comment 9

•

17 days ago

Attached patch 2008314-suppress-offline.patch (obsolete) — Details — Splinter Review

I noticed that this code is causing the "damage":
https://searchfox.org/comm-central/rev/60a66232310979b430b99cd8044ca61dd0b284a7/mailnews/imap/src/nsImapMailFolder.cpp#685-699

Clearing the flag avoids the problem. Is this a valid approach?

Attachment #9535996 - Flags: feedback?(gds)

Comment 10

•

17 days ago

•

Edited

Maybe obvious, but looking at a IMAP:4,Filters:5 log, for a good filter action I see

Imap append to Sent folder (not due to filter)
imap "UID STORE" of flag "label3" in Sent folder
imap FETCH from Sent folder of the message (has label3 flag)
copy of fetched message to LF (not explicitly shown in Filter:5 log)

For bad filter action (no tag in LF) I see this:

Imap append to Sent folder (not due to filter)
imap FETCH from Sent folder of the message (label3 not yet applied)
imap "UID STORE" of flag "label3" in Sent folder.
copy of fetched message to LF (not explicitly shown in Filter:5 log)

I don't use offline store so that might be why I get random results even with Yury's latest comment 3 patch.
Edit: Didn't see comment 9.. Will now.

Comment 11

•

17 days ago

So fetching from sent folder for copying and applying the tag/label happen in random order? Do you see any issues with the "Re: " prefix as well?

The puzzling thing is that the additional call to UpdateFolderWithListener() introduced in bug 1990129 is causing all the damage, so how does that relate to the course of the events?

Comment 12

•

17 days ago

•

Edited

So fetching from sent folder for copying and applying the tag/label happen in random order? Do you see any issues with the "Re: " prefix as well?

~~"Re:" is the same issue since missing Re would be caused by imap FETCH of message triggered by the filter occurring before the \Answered flag is set on the new message in Sent folder.~~
Edit: This is wrong. The Re: on the subject is in the file put into Sent folder by imap append. Why it is cleared in the move destination folder (apparently by the update of appended Sent folder that I added post 140) is TBD. See comment 21.

The puzzling thing is that the additional call to UpdateFolderWithListener() introduced in bug 1990129 is causing all the damage, so how does that relate to the course of the events?

It causes an "update" on the Sent folder resulting in an imap SELECT of Sent folder and then fetch of all its imap FLAGS. This probably varies the timing or order of when the UID STORE and message FETCH occur.

I noticed that this code is causing the "damage":
https://searchfox.org/comm-central/rev/60a66232310979b430b99cd8044ca61dd0b284a7/mailnews/imap/src/nsImapMailFolder.cpp#685-699

Clearing the flag avoids the problem. Is this a valid approach?

Not sure what this does but, by itself, it doesn't fix the problem when I tried it. My first filter run failed to transfer the tag to LFs.

Comment 13

•

17 days ago

With the Sent folder not synchronised for offline use (offline store), https://phabricator.services.mozilla.com/D277943 doesn't fix anything, I don't even get a random good result. Attachment 9535996 [details] [diff] however also works in this case.

Comment 14

•

17 days ago

(In reply to gene smith from comment #12)

Not sure what this does but, by itself, it doesn't fix the problem when I tried it. My first filter run failed to transfer the tag to LFs.

It suppressed this code:
https://searchfox.org/comm-central/rev/60a66232310979b430b99cd8044ca61dd0b284a7/mailnews/imap/src/nsImapMailFolder.cpp#685-699

Works for me with and without "offline store" (synched Sent folder).

Phabricator Automation

Updated

•

17 days ago

Attachment #9535804 - Attachment is obsolete: true

Comment 15

•

16 days ago

From comment 12, I wrote:

My first filter run failed to transfer the tag to LFs.

This is with this code:

                (void)OnCopyCompleted(m_copyState->m_srcSupport, aExitCode);
                ClearFlag(nsMsgFolderFlags::OfflineEvents);
                UpdateFolderWithListener(msgWindow, m_urlListener);

and left tb running. Then came back in a few hours and tried again and it worked ok.
Then looked again to make sure the code was per your comment 9 patch, re-built and it failed again. Tried again and it worked then failed again.
So appears I'm seeing the same random results with the above code in place. (Sent folder is still not using offline store.)

Comment 16

•

16 days ago

I tried a couple more things last night.

As Hartmut pointed out (comment 7), reversing the OnCopyCompleted and UpdateFolderWithListener calls does fix the subject bug. But it brings back (regresses) the original bug that the unconditional call to UpdateFoldersWithListerner fixed. I thought maybe doing this might fix it:

UpdateFolderWithListener(msgWindow, m_urlListener);
(void)OnCopyCompleted(m_copyState->m_srcSupport, aExitCode);
UpdateFolderWithListener(msgWindow, m_urlListener);

but it didn't help (and it does double folder updates, of course).
(FYI, I only see the regression reversing the calls when online store is used for the updated folder.)

Tried to determine if a filter on Sent folder is happening after OnCopyCompleted() is called and skip the UpdateFolderWithListener call if it is. But what I tried didn't successfully detect that a filter is pending. I'll look at this again.

Comment 17

•

16 days ago

Looking some more I'm pretty sure that the UpdateFolderWithListener call as it currently is in daily isn't actually causing the problem. I tried delaying the call for 1 second and it worked. I then tried delaying the call for 10s and it failed. Looking at the log, I see the UID STORE occurs for label3 (on its own thread) but before it finishes, the fetch of the full message occurs (on another thread) and finishes and then the UID STORE responds after the fetch finishes. I then just completely removed the call to UpdateFolderWithListener and see the same thing.
So it looks to me like this could fail even with the current 140esr code which only calls UFWL() for appendtodraft URL. It's just a matter of timing (a race condition).
What appears to fix it is to just set the number of cached imap connection down to 1. This ensures that the UID STORE finishes before the message FETCH so that the tag (or \Answered flag) is set before the message is copied to Local Folders. Of course, setting cached conns to 1 is just a work-around and I'll try to find a better solution.

Francesco

Reporter

Comment 18

•

12 days ago

(In reply to gene smith from comment #17)

What appears to fix it is to just set the number of cached imap connection down to 1.

I set "Maximum number of server connection to cache" to a value of 1, and the problem persists. Tested in 147.0b4. The Sent folder is synchronised for offline use.

Comment 19

•

11 days ago

(In reply to Francesco from comment #18)

(In reply to gene smith from comment #17)

What appears to fix it is to just set the number of cached imap connection down to 1.

I set "Maximum number of server connection to cache" to a value of 1, and the problem persists. Tested in 147.0b4. The Sent folder is synchronised for offline use.

I should have updated my comment on this sooner since I quickly found that cached conns at 1 (serializing imap) only "fixed it" for me with no offline store. But now I'm not even sure that that always fixes it with no offline store and definitely does not if Sent folder has offline store.

The most 99% reliable workaround I've found (with and without offline store) is to add at least one additional copy to trash in the action list:

Tag msg "Personal"
Copy msg to "Trash" on <imap account running filter for>
Move msg to "test" on Local Folders

The problem is caused because the filtering code does not guarantee the order of most action results. It is possible for the actual imap tagging for Tag action to occur after the Move action. This is because an "OnStop" callback only occurs for Copy or Move action. So once a copy/move action starts, another filter action won't be started until the "OnStopCopy" occurs (see https://searchfox.org/comm-central/rev/65180139fb02d6512e95d12ff0175f15b845af95/mailnews/search/src/nsMsgFilterService.cpp#1290-1297). The "AddCustomTag" url does not have a "OnStop..." associated with it. In the above list it will be kicked off first but will appear instantly finished to the filter code. Then the Copy starts and will prevent the Move until the copy finishes. In the mean time, Tag is queued in to imap and will probably occur before the Copy finishes and the Move starts. But the imap tagging (UID STORE) can get delayed until after the Move causing the bug.

The solution to this is to treat Tag as asynchronous like Copy and Move. If there were an "OnStopTagging" callback in in nsMsgFilterService.cpp like there is for "OnStopCopy" this could guarantee the order and simple action list

Tag msg "Personal"
Move msg to "test" on Local Folders

would always work.

Francesco

Reporter

Comment 20

•

11 days ago

Even without the tagging action, filter execution doesn't work correctly, since the moved reply is missing its "Re: " subject prefix and it's unread. In TB 140, the "Re: " prefix is present and the message is read.

Comment 21

•

11 days ago

(In reply to Francesco from comment #20)

Even without the tagging action, filter execution doesn't work correctly, since the moved reply is missing its "Re: " subject prefix and it's unread. In TB 140, the "Re: " prefix is present and the message is read.

Yes, I see that too. Never tried just the move by itself. See it with and without offline store for Sent. Removing the update I added (post 140) seems to fix it. I'll look closer at this.

FWIW, my reply about \Answered flag in comment 12 is wrong.

Comment 22

•

9 days ago

Attached patch filter-after-sending-fix-v0.diff (obsolete) — Details — Splinter Review

This fixes the reported problems based on my preliminary testing. When the imap append for the "Sent" folder completes, this now first "updates" the Sent folder (imap SELECTs it, fetches flags, headers and body if needed). But now the call to OnCopyComplete (to copy the appended file to offline store and notify copy complete) is deferred until the update URL (nsImapSelectFolder) is completely finished.

So now with "After Sending" filtering enabled with actions, e.g., Tag message Important, move (or copy) to Local Folders/test, the sent message appears correctly in Local Folders/test with the correct tag and with status "read" (not bold).
Also, with "After Sending" action, e.g. ,just "move (or copy) to Local Folders/test, a replied-to message appears correctly in Local Folders/test with the subject starting with "Re: " and with status "read" (not bold).

The new m_doingAppend keeps other places in code that do folder updates from causing OnCopyComplete from being called in OnStopRunningUrl when it shouldn't.

I've only tested this so far with Sent folder having offline store.
Haven't checked if this affect any tests and made a "try" build.

Assignee: nobody → gds

Attachment #9535996 - Attachment is obsolete: true

Attachment #9535996 - Flags: feedback?(gds)

Comment 23

•

9 days ago

•

Edited

Hartmut actually suggested a fix like comment 22 about a week ago since just reversing the order of OnCopyComplete and UpdateFolderWithListener fixed one problem but caused another. See comment 7.

From email he sent me 1/5/26:

The filter is triggered by the OnCopyCompleted call. The UpdateFolderWithListener directly afterwards somehow interferes with that. I also tried delaying the UpdateFolder call by dispatching to the main thread, but that didn't help reliably either. The cleanest solution would probably be to call UpdateFolder first, then wait for its completion in OnStopRunningUrl, and then call OnCopyCompleted, but since UpdateFolder calls also itself, that's quite the mess I think ...

I'm not seeing where UpdateFolder (or UpdateFolderWithListener) is calling itself.

Comment 24

•

9 days ago

Comment on attachment 9537385 [details] [diff] [review]
filter-after-sending-fix-v0.diff

I tested this and it works with and without offline storage for the Sent folder. Also, bug 1990129 isn't regressed. Hopefully the change doesn't cause any side-effects leading to test failures.

Attachment #9537385 - Flags: feedback+

Assignee

Comment 25

•

9 days ago

(In reply to gene smith from comment #23)

From email he sent me 1/5/26:

[…], but since UpdateFolder calls also itself, that's quite the mess I think ...

I'm not seeing where UpdateFolder (or UpdateFolderWithListener) is calling itself.

I didn't word that precisely. This was about https://searchfox.org/comm-central/rev/11dc3ee04aa83b93d75f24df8e4a95a07ac35f52/mailnews/imap/src/nsImapMailFolder.cpp#692-697, where another call of UpdateFolderWithListener is indirectly triggered, I think.

Assignee

Updated

•

9 days ago

Attachment #9537385 - Flags: feedback+

Assignee

Comment 26

•

7 days ago

Unfortunately, a Try run with the patch shows several issues…

Comment 27

•

6 days ago

Yeah, I know. Ran the try a few days ago https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=21b3eeb56a8516fe06eba40ddd53b0871fe149de&selectedTaskRun=VOVSClmWRV-eZZe3fiEdug.0
Been tweaking the patch to try to fix it with no success. Took a break today.

Comment 28

•

4 days ago

Still no success. Pretty much out of ideas at this time.

Comment 29

•

2 days ago

Attached patch partial-fix.diff (obsolete) — Details — Splinter Review

Looks like the only way to fix the tests is to remove the "UpdateFolderWithListener" I added at bug 1990129. I've tried putting it in different places with no success and tests still fail. Of course, with it gone bug 1990129 fails again.
To get bug 1990129 to pass without the UpdateFolderWithListener I found two ways.

Before running the addon to modify the subject, select another folder and return to the original folder and run the addon via right-click. This is just a work-around with no code changes.
In about3Pane.js, call UpdateFolder before right-clicking and invoking the message context menu (see attached patch).. Unless there is something new in the folder the update just does an imap NOOP and checks messages flags for new messages. If there are new messages (ie., the addon was previously run) the update fetches the new headers which fixes the problem. However, you have to right-click twice to invoke the addon since the pop-up context menu goes away when you invoke the addon the first time. I think it needs to use a listener with UpdateFolderWithListener to allow the update to finish before the context menu pops up, but not sure how to do this. Also, not sure if this would cause other test problems too.

Francesco

Reporter

Comment 30

•

2 days ago

Please note that per bug 1990129 comment 0 the issue could be reproduced without the use of an add-on, just by dragging two .eml files onto an IMAP folder. The second one with a differing subject isn't displayed correctly, you need to repair the folder to see the correct subject. So in this case, there isn't a context menu involved.

Assignee

Updated

•

2 days ago

Duplicate of this bug: 2011560

Assignee

Comment 32

•

1 day ago

Attached file Bug 2008314 - Fix copying file messages into IMAP folders in the front end. r=#thunderbird-reviewers — Details

This backs out the revisions from Bug 1990129 and tries to address the issue
from that bug by awaiting the completion of CopyFileMessages and
UpdateFolder in the front end, utilizing the same mechanism as introduced by
Bug 1788159 for attachment methods.

With this patch, there is still a problem dropping more than one attachment at
a time, which should be solvable though, maybe by extending the proposed
CopyFileMessagesAsync method to be able to handle multiple files at once.

Assignee

Comment 33

•

1 day ago

Apparently, Gene and I came to the same conclusion, see bug 1990129 comment 36. Successful Linux Try run of this WIP that still needs some polishing if it turns out to work in general.

Updated

•

17 hours ago

Attachment #9538979 - Attachment is obsolete: true

Comment 34

•

17 hours ago

Attached patch fix-1-22-26.diff — Details — Splinter Review

Hartmut, thanks for working on this!
I ran your patch and it does allow multiple drops of .eml files into folder and thread panes. I assume it's not intended to fix the folder update after modifying the message subject with the addon and that you agree it is probably best to just put the update directly in the addon (assuming addon developer Michel agrees to do that).

Here's my attempt to allow update after each (single file) drop. It also seems to work in that I can drop a file, modify the file subject and drop it again and I have two messages present differing only in the subject.

My change doesn't try to make the copies async like yours does but it still seems to work even when dropping a large file (e.g., 15Mbytes). However, it also has the problem that if you try to drop two or more files at the same time, only the first one automatically shows up. You have to select another folder and come back to see the other one. Clicking back on the original folder triggers another folder update I presume.

My patch also still does a folder update when a message is right clicked which is not needed if the update is put into the addon.

Attachment #9537385 - Attachment is obsolete: true

Phabricator Automation

Updated

•

9 hours ago

Attachment #9539562 - Attachment description: WIP: Bug 2008314 - Fix copying file messages into IMAP folders in the front end. → Bug 2008314 - Fix copying file messages into IMAP folders in the front end. r=#thunderbird-reviewers