marking of message in virtual folder as read when viewed marks incorrect message as read

RESOLVED FIXED in Thunderbird 57.0

Status

Thunderbird
Folder and Message Lists
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: bieber.dresden, Assigned: Jorg K (GMT+2) (bustage-fix and urgent reviews only))

Tracking

(Blocks: 1 bug)

52 Branch
Thunderbird 57.0

Thunderbird Tracking Flags

(thunderbird_esr5257+ fixed, thunderbird56 fixed, thunderbird57 fixed)

Details

(Whiteboard: TB 56 beta 4 => TB 52.5 ESR)

Attachments

(2 attachments)

(Reporter)

Description

10 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/603.3.8 (KHTML, like Gecko) Version/10.1.2 Safari/603.3.8

Steps to reproduce:

Create a new account for rss feeds.
Add at least two feeds (http://pastutopia.com/?feed=rss2 and https://tapas.io/rss/series/32980 seem to be reproducible).
Set the setting "automatically mark mark messages as read" to immediately.
Mark all their items as unread.
Create a new virtual folder which searches those feeds for unread items.
Select the lowermost message in the virtual folder


Actual results:

A (seemingly) random message from the second feed was marked as read.


Expected results:

The message that was selected should be marked as read
Virtual folders in TB have many bugs. The first thing to try is to remove "smart mailboxes" from your profile. You get to your profile via Help > Troubleshooting Information.

Alta88: Nothing special here about feed messages/folders, right?
(Reporter)

Comment 2

10 months ago
I have not activated "smart mailboxes".
My troubleshooting information pane also does not mention "smart mailboxes" anywhere.

The Problem may not be related to rss feeds (though i've only seen it happen there), but it IS related to virtual folders, since it only happens inside them.
Also the messages that get marked incorrectly ALWAYS are from the feed that WASN'T selected.
The problem is not related to any Add-Ons, since it also occurs in safe-mode.
You must visit your profile folder with the link on the trouble shooting page. You'll find it in folder "Mail".
(Reporter)

Comment 4

10 months ago
I have removed the "smart mailboxes" folder inside the "Mail" folder and restarted Thunderbird.
After following the reproduction steps again I was able to reproduce the bug.
Thunderbird automatically re-created the smart mailboxes folder after startup.
Then we should look into it, sadly, as I said, virtual folders are bug-ridden.

Comment 6

10 months ago
(In reply to bieber.dresden from comment #2)
> I have not activated "smart mailboxes".
> My troubleshooting information pane also does not mention "smart mailboxes"
> anywhere.
> 
> The Problem may not be related to rss feeds (though i've only seen it happen
> there), but it IS related to virtual folders, since it only happens inside
> them.
> Also the messages that get marked incorrectly ALWAYS are from the feed that
> WASN'T selected.
> The problem is not related to any Add-Ons, since it also occurs in safe-mode.

It can happen anywhere, both local folders and imap too I believe.  If you display the Order Received column (a total misnomer) you will notice identical index numbers for 2 messages (given 2 folders). The number used to be the byte offset of the message in the file, which would have greatly reduced the chance of this problem occurring for local files.  But it was changed to be an increment by 1 index (which it always was for imap).

So if you mark as read a message with 0, it can instead mark the other message with 0; this should be easily proven. It's pretty incredible that multifolder virtual view was implemented without strictly considering all places where both folder and index are needed for uniqueness. It's probably not a difficult fix.
(Reporter)

Comment 7

10 months ago
(In reply to alta88 from comment #6)
> (In reply to bieber.dresden from comment #2)
> > I have not activated "smart mailboxes".
> > My troubleshooting information pane also does not mention "smart mailboxes"
> > anywhere.
> > 
> > The Problem may not be related to rss feeds (though i've only seen it happen
> > there), but it IS related to virtual folders, since it only happens inside
> > them.
> > Also the messages that get marked incorrectly ALWAYS are from the feed that
> > WASN'T selected.
> > The problem is not related to any Add-Ons, since it also occurs in safe-mode.
> 
> It can happen anywhere, both local folders and imap too I believe.  If you
> display the Order Received column (a total misnomer) you will notice
> identical index numbers for 2 messages (given 2 folders). The number used to
> be the byte offset of the message in the file, which would have greatly
> reduced the chance of this problem occurring for local files.  But it was
> changed to be an increment by 1 index (which it always was for imap).
> 
> So if you mark as read a message with 0, it can instead mark the other
> message with 0; this should be easily proven. It's pretty incredible that
> multifolder virtual view was implemented without strictly considering all
> places where both folder and index are needed for uniqueness. It's probably
> not a difficult fix.

This is EXACTLY the behaviour that occurs!
If there are multiple Feeds in one (virtual) folder and I read a message with Order Recieved of (e.g.) 1 it will always mark another message with Order Recieved of 1.
If there is no other Feed with that Order Recieved Number, it will mark the correct message as read.

Another thing I notice, when I sort by the Order Recieved column (descending):
It will ALWAYS mark the TOPMOST message with that Order Recieved Number as read!
Alta88, thanks for the comment, care to send a patch? ;-)
I've been triaging some new bugs and came across bug 1398526 where someone has a problem with marking messages as read in a unified view. That reminded me of this bug.

I've briefly looked at where marking a message as read happens and got down to mailCommands.js::MarkSelectedMessagesRead() which calls gDBView.doCommand(nsMsgViewCommandType.markMessagesRead):
https://dxr.mozilla.org/comm-central/rev/b0d8d878f62741503ed15a597029bf37a617de4b/mail/base/content/mailCommands.js#479

Further in the debugger I see this:
xul.dll!nsMsgHdr::MarkRead(bool bRead) Line 219	C++
xul.dll!nsMsgDBFolder::MarkMessagesRead(nsIArray * messages, bool markRead) Line 4746	C++
xul.dll!nsMsgLocalMailFolder::MarkMessagesRead(nsIArray * aMessages, bool aMarkRead) Line 1288	C++
xul.dll!nsMsgDBView::ApplyCommandToIndices(int command, unsigned int * indices, int numIndices) Line 3045	C++
xul.dll!nsMsgDBView::DoCommand(int command) Line 2612	C++

To turn indices into message headers we come into nsMsgDBView::GetMsgHdrForViewIndex() called by nsMsgDBView::GetHeadersFromSelection() which is called by nsMsgDBView::ApplyCommandToIndices() which is in the stack above.

nsMsgDBView::GetMsgHdrForViewIndex() has this funny caching scheme which works on keys only and not folders, so that will fail in a unified view where messages can come from many folders. I think adding a folder to the caching mechanism will fix the problem and perhaps also bug 1398526.
Or maybe that I wrote there is incorrect, since there is also nsMsgSearchDBView::GetMsgHdrForViewIndex() which overrides the other one for search views. Looks like I need to set up the test case first.
OK, I've set up a search folder including messages from two real folders, one message each. Both show "Order Received" of 1, and when I display one of them, the other one gets marked as read.

I've debugged this a bit:
xul.dll!nsMsgHdr::MarkRead(bool bRead) Line 219	C++
xul.dll!nsMsgDBFolder::MarkMessagesRead(nsIArray * messages, bool markRead) Line 4746	C++
xul.dll!nsMsgLocalMailFolder::MarkMessagesRead(nsIArray * aMessages, bool aMarkRead) Line 1288	C++
xul.dll!XPTC__InvokebyIndex() Line 99	Unknown

and the JS call comes from mailWindowOverlay.js:3491 (OnMsgLoaded()) via mailWindowOverlay.js:3369 (MarkMessageAsRead()). Those function pass a message header around which comes from gMessageDisplay.displayedMessage, so the header should have the correct folder. Hmm.

That pretty much reproduces comment #0.

<off topic>
Marking a message as read manually, bug 1398526, however works, and proves that my comment #9 was wrong. Here I see:
xul.dll!nsMsgSearchDBView::GetMsgHdrForViewIndex(unsigned int index, nsIMsgDBHdr * * msgHdr) Line 345	C++
xul.dll!nsMsgDBView::GetHeadersFromSelection(unsigned int * indices, unsigned int numIndices, nsIMutableArray * messageArray) Line 2855	C++
xul.dll!nsMsgDBView::ApplyCommandToIndices(int command, unsigned int * indices, int numIndices) Line 2988	C++
xul.dll!nsMsgSearchDBView::DoCommand(int command) Line 787	C++
xul.dll!XPTC__InvokebyIndex() Line 99	Unknown
so we end up in nsMsgSearchDBView::GetMsgHdrForViewIndex() which does the right thing.
</off topic>

So to address this bug here, we need to see why gMessageDisplay.displayedMessage is wrong and points to the wrong message.
Whichever of the two message I click, msgHdr.folder.URI, always shows the same folder, never the other:
function OnMsgLoaded(aUrl)
{
...
  var msgHdr = gMessageDisplay.displayedMessage;
  dump(msgHdr.folder.URI+"=====================\n");

So the next step if to check, where gMessageDisplay.displayedMessage is set to the wrong header.
Duplicate of this bug: 1398526
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 14

9 months ago
The key here is to:
1) Ensure that the search view's GetMsgHdrForViewIndex() is run, and not the regular view's function, which must be overridden.
2) Ensure the integrity of the m_keys[index] and m_folders[index] arrays in search view's instance; ie that the message matches the folder, at a given index.
Created attachment 8906392 [details] [diff] [review]
1387581-debug.patch

Here's my debug patch. I have two folders, ZZUni1 and ZZUni2, each with one message, and a search folder that shows those two messages.

When hovering the mouse over the messages in the search folder I get:
=== nsMsgSearchDBView::GetMsgHdrForViewIndex 0
=== nsMsgSearchDBView::GetMsgHdrForViewIndex ZZUni1 1
=== nsMsgSearchDBView::GetMsgHdrForViewIndex 1
=== nsMsgSearchDBView::GetMsgHdrForViewIndex ZZUni2 1
There are no calls of nsMsgSearchDBView::GetMsgHdrForViewIndex. So to me that looks like the view logic is working as designed.

However, when finally clicking onto the message from Uni2, I get:
=== displayMessageChanged 0 mailbox://nobody@Local%20Folders/ZZUni1
=== onDisplayingMessage mailbox://nobody@Local%20Folders/ZZUni1
=== OnMsgLoaded mailbox://nobody@Local%20Folders/ZZUni1
OnMsgLoaded() subsequently calls MarkMessageAsRead(msgHdr) on the wrong header. So everything Alta88 suggested to check in comment #14 seems OK, however, gMessageDisplay.displayedMessage has the wrong message/header.

And that comes from displayMessageChanged() retrieving the wrong index from
  let viewIndex = this.view.dbView.currentlyDisplayedMessage;

I'm open to further inspiration since I spent 1 1/4 hours on this and now need to move on to a hotter burning fire.
That would cause a lot of problems in unified views since gMessageDisplay.displayedMessage is used for may things. And if that holds the wrong message, there will be a lot of malfunction.

Comment 17

9 months ago
The root cause is GetCurrentlyDisplayedMessage() gets the view index based on messagekey, via FindViewIndex() and FindKey(), which makes no allowance at all for identical keys and different folders.  It's also not clear why displayMessageChanged() first gets a view index then gets a key then gets an msgHdr, when it should just get the msgHdr directly.  It may be as simple as just getting the msgHdr by
1) gDBView.getSelectedMsgHdrs()[0]
2) gDBView.getMsgHdrAt(gDBView.selection.currentIndex)

or redoing FindViewByIndex entirely. Nothing more to add here.
Created attachment 8906409 [details] [diff] [review]
1387581-fix-selection.patch (v1)

Works :-)

Please review and thanks for the support.
Attachment #8906409 - Flags: review?(alta88)

Comment 19

9 months ago
Comment on attachment 8906409 [details] [diff] [review]
1387581-fix-selection.patch (v1)

The view code is fragile, this should have a try run despite its seeming obviousness.  It should also have a test, checking mark read works in a multifolder search (as opposed to a single folder search, which is an entirely different view class); there are existing xfvf tests so it shouldn't be onerous.
Attachment #8906409 - Flags: review?(alta88) → review+
Thanks, I was planning a try run, so here it is:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=de14c1d484411b96eb4ea59079fe61c60601b906

Yes, I agree, there should be a test. Could you point me to one that could be copied/enhanced? What is "xfvf"?

Comment 21

9 months ago
It's the name of the class contractid for the search view, cross folder virtual folder.  You can search for xfvf for the tests.

Also, a good followup would be to evict GetCurrentlyDisplayedMessage() and the very few .currentlyDisplayedMessage references as it's no good for multifolder.  In cases where the view index is wanted, gDBView.selection.currentIndex should be used.
I filed the bug 1398843 as you suggested.

Looking for tests with "xfvf" I could only find test_nsMsgDBView.js and that looks by no means "non-onerous" looking at the high-calibre contributors there (including yourself):
https://hg.mozilla.org/comm-central/log/tip/mailnews/base/test/unit/test_nsMsgDBView.js

If we were to test directly what this bug is reporting, that the wrong message gets marked as read in a search view, that would lend itself more to a Mozmill test, no? There appear to be a few tests in mail/test/mozmill/folder-display related to virtual folders. So is that what you were thinking of?
You must think my grammar is terrible: I filed bug 1398843 as you suggested. I started with: I filed the bug ... (that you suggested), but decided to include the bug number and forgot to take out the "the". :-(

Comment 24

9 months ago
dxr finds this:
https://dxr.mozilla.org/comm-central/rev/0531d19ae371c148327c379822f2bfffe9fc3256/mail/base/test/unit/test_viewWrapper_virtualFolder.js#371

so find a regular test that tests marking read, and copy it for a virtual multifolder view, in the above file.
Yes, I saw this comment. I'm still not sure what we're trying to do here. Check that mark read works at all in a search folder based on multiple folders as an Xpcshell test. Or testing the code I'm changing in the patch which is in fact identifying the correct message the user clicked on and then marking that read (which already worked, but maybe didn't have test coverage).

Maybe I can persuade you to do the test?
Assignee: nobody → jorgk

Comment 26

9 months ago
the point would be to mozmill test an action on a message, given only its message key, which is non unique.  but if all actions already (now) have either the view index or the msdHdr already (and don't use message key to get those), there's no point testing the action since there are already such tests and multiple folders don't matter.

auditing for msgHdr lookup by message key is out of scope, here.  since very many if not effectively all lookup by message key failures would have been hidden when file offsets were used as the message key (other than the first 0 message), such an audit would be useful, but as another bug.
Hmm, can you please wind down to my humble level of knowledge. If I read the first paragraph correctly, a Mozmill test doesn't make sense since we eliminated the incorrect use of .currentlyDisplayedMessage and the new approach we take here would fail in other tests if incorrect.

So that leaves an Xpcshell test for marking messages as read in search folders based on multiple folders, yes? So we ensure the underlying function of what we exposed here works correctly. Can I win you for that?
Keywords: leave-open

Comment 28

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d4555ce8e26c
use getSelectedMsgHdrs() instead of currentlyDisplayedMessage to select correct message in multifolder search (idea by alta88). r=alta88
Comment on attachment 8906409 [details] [diff] [review]
1387581-fix-selection.patch (v1)

We should uplift this fix to an ancient puzzling bug.
Attachment #8906409 - Flags: approval-comm-esr52?
Attachment #8906409 - Flags: approval-comm-beta+

Comment 30

9 months ago
(In reply to Jorg K (GMT+2) from comment #27)
> Hmm, can you please wind down to my humble level of knowledge. If I read the
> first paragraph correctly, a Mozmill test doesn't make sense since we
> eliminated the incorrect use of .currentlyDisplayedMessage

correct.

> and the new
> approach we take here would fail in other tests if incorrect.
>

i don't believe anything else will be affected, but existing tests should cover it.
 
> So that leaves an Xpcshell test for marking messages as read in search
> folders based on multiple folders, yes? So we ensure the underlying function
> of what we exposed here works correctly. Can I win you for that?

if removing a bad pathway (getting an msgHdr by message key) is done in an audit type bug then the tests that should have existed won't be necessary, as this only negatively affects multiple folders. in any event, i'm not going to spend free time on it.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
Beta (TB 56):
https://hg.mozilla.org/releases/comm-beta/rev/7b6daa02a908409623dccd98f9ca63fc1ed34500
status-thunderbird56: --- → fixed
status-thunderbird57: --- → fixed
status-thunderbird_esr52: --- → affected
Keywords: leave-open
Whiteboard: TB 56 beta 4 => TB 52.4.1 ESR.
Duplicate of this bug: 1407692
Whiteboard: TB 56 beta 4 => TB 52.4.1 ESR. → TB 56 beta 4 => TB 52.5 ESR
Attachment #8906409 - Flags: approval-comm-esr52? → approval-comm-esr52+
TB 52.5 ESR (should be tracking 57+):
https://hg.mozilla.org/releases/comm-esr52/rev/3f5f50d3c66e
status-thunderbird_esr52: affected → fixed
tracking-thunderbird_esr52: --- → 57+
You need to log in before you can comment on or make changes to this bug.