1470049 - Unified inbox sort order incorrect when new messages arrive in threaded view

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: inbox-sort-order.png

In my unified inbox I've messages sorted ascending by date (ie, so new messages are at the bottom of the list). Often, but not always, a new message arrives which causes the entire thread to be moved to the top of the list (ie, before the earliest message in the inbox).

See attached image: note the sort indicator on the "Date" column. The second message in that list caused the 2 messages to be moved to the top of the list - the first of those 2 messages was correctly at the bottom of the list until the unread message arrived.

Of note:
* Has been happening for a couple of months.
* I nuked all .msf files in the profile and this continues to happen.
* Clicking the date column twice correctly resorts.
* This only happens in the "unified" view - it never happens when an individual inbox is selected.
* It happens regularly, but not for every new message that arrives.
* I think (but I'm not 100% sure - I'll keep an eye out) that this only happens for messages which are in a thread - messages which don't form part of a thread don't seem to cause this behaviour.
* Windows, 60.0b7.

Comment 1

[Carlos Maziero]

I have exactly this same problem, in Thunderbird 60.2.1 (64-bit) for Linux (Ubuntu Xenial 16.04.5).

Comment 2

[frank.schaper]

Similar, but not the same problem here since the update to 60.3: 

I have two accounts joined in a unified INBOX folder. New messages do appear at the bottom of the list. However, new messages which are in a threaded conversation do move the whole thread to the top of the window above the oldest message.

- This does not happen in the individual INBOX folders of the two accounts, only in the unified INBOX.
- This does not happen if view is switched to "unthreaded" .
- The behavior persists even if all add-ons are disabled and after computer restart. 
- Double-click on "Date" to sort manually restores order

Windows 7 Pro / Thunderbird 60.3.0 (32-bit)

Comment 3

[Inkbug]

Attachment: Screenshot showing incorrect sort order

Comment 4

[Inkbug]

I just updated to Thunderbird 60.3.0 32-bit (Windows 10 Home Version 1803).

I also have two accounts displayed in a unified Inbox, sorted by date.

When I switch from the unified layout to an individual account and then back to the unified layout, the order of the messages suddenly gets messed up, with all of the threaded messages being placed together (see screenshot I attached). This also happens when replying to a message.

Comment 5

[Wayne Mery (:wsmwk)]

Perhaps the earliest report of a potential version 60 regression, where https://mzl.la/2POjAlv may contain other reports

Comment 6

[Jorg K (CEST = GMT+2)]

You know that we are very lucky to have Alice White to find regressions. But we need reproducible steps for that. "It happens regularly, but not for every new message that arrives" (comment #0) doesn't help in. Altogether, unified views are a can of worms.

Comment 7

[frank.schaper]

And how far do you want the user to go before he dares to submit a bug report? If it does not happen to every message, is it useless to report the problem before the **user** has figured out to exactly which messages it applies?

In this case, I can confirm that the problem appears **only** for new messages, which a part of a thread (thus consequently only if threaded view is turned on) and **only** in the unified INBOX, not in the respective individual INBOXES.

Comment 8

[Inkbug]

(In reply to Jorg K (GMT+1) from comment #6)
> You know that we are very lucky to have Alice White to find regressions. But
> we need reproducible steps for that. "It happens regularly, but not for
> every new message that arrives" (comment #0) doesn't help in. Altogether,
> unified views are a can of worms.

Here are reproducable steps:
1) Send message to self.
2) Open Unified Inbox, switch to sort-by-date
3) Reply to self-sent message
4) Receive the reply, while still in Unified Inbox view.
5) Upon receipt, the thread will move out of position.

Comment 9

[Jorg K (CEST = GMT+2)]

Hi Alice, can you reproduce the STR? If so, can you find a regression? Quite laborious really. Thanks in advance!!

Comment 10

[Alice0775 White]

What is "Unified Inbox"?

Comment 11

[Jorg K (CEST = GMT+2)]

You need two accounts, then display |View > Folders > Unified|. Then right-click on the unified inbox, parent of all the inboxes in the tree and select the folders that should be included. It's basically a glorified search folder.

Comment 12

[Jorg K (CEST = GMT+2)]

BTW, Alice, if you have a problem with this, delete "smart mailboxes" from your mail storage. It will recreate itself with all inboxes preselected to show their contents in the unified folder.

Comment 13

[Alice0775 White]

I can not reproduce the issue on Thunderbird 60.3.1 (32-bit) en-US BuildID=20181113231517 Windows10 Home 1809 64bit Japanese Edition.
I tried both ascending order and descending order of Date column. However, It works for me. Newly received mails are positioned properly. These message are not mixed.

Comment 14

[frank.schaper]

Alice, you did not mentioned threaded messages. Please note (see comments 7 and 8) that this problems appears **only** if new mails are part of a message thread. If new messages are not part of a thread or if you are in unthreaded view, the bug does not appear.

Easiest reproduction is to turn on threaded view and - in the unified INBOX - forward any message to yourself. If the new incoming message appears in a thread, the latter is moved to the top of the screen, even against the desired sort order. Double-clicking on the the Time heading resorts the thread to the bottom of the list.

Comment 15

[Alice0775 White]

(In reply to frank.schaper from comment #14)
> Alice, you did not mentioned threaded messages. Please note (see comments 7
> and 8) that this problems appears **only** if new mails are part of a
> message thread. If new messages are not part of a thread or if you are in
> unthreaded view, the bug does not appear.
> 
> Easiest reproduction is to turn on threaded view and - in the unified INBOX
> - forward any message to yourself. If the new incoming message appears in a
> thread, the latter is moved to the top of the screen, even against the
> desired sort order. Double-clicking on the the Time heading resorts the
> thread to the bottom of the list.

Umm,
How to make ”threaded view is turned on”?

Comment 16

[Alice0775 White]

Never mind. View > Sort By > Threaded  (I remembered. I have used it once. However, I have not used it even once for over ten years since then :).

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=d7d59e4c13d72e96641c5bcebe5e18bb8d4f46eb&tochange=54b88d31ffabb9bb582646276bc07c742ae3a23c
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=294f332a35538940469b1a2576615ff5ffe1e016&tochange=53bbdaaa2b8c1819061be26101b075c081b23260

Comment 17

[Jorg K (CEST = GMT+2)]

Thanks Alice, and particularly since this wasn't so easy to reproduce. The left-most icon in the header column, Display message threads, turns threading on.

Looks like a regression from bug 1385573. Alta88?

Comment 18

[frank.schaper]

Just to note: Problem remains with the 60.3.1 (32 bit) update from Nov. 15th. (Windows 7 Pro)

Comment 19

[alta88]

You can try to selectively adjust for https://bugzilla.mozilla.org/show_bug.cgi?id=1385573#c27 and https://bugzilla.mozilla.org/show_bug.cgi?id=1385573#c35 until this bug (unified view new messages sort) is fixed. Apparently an out of bounds index (which used to not crash until core tightened things up) was ok and part of the design in this case, and the behavior did change. Brittle?

Since comments in Bug 1385573 and Bug 1509685 indicate there wasn't complete success in solving the grouped view crash, and regressed this bug, you could just back out the hunks in https://bugzilla.mozilla.org/show_bug.cgi?id=1385573#c36.

As for Bug 1509685, maybe testing m_flags and m_keys arrays specifically for oob right before their accesses would help.

Comment 20

[Jorg K (CEST = GMT+2)]

Thanks, but where does this leave me? I have the choice between backing out some potential crash fixes or "adjusting" what we landed taking the quoted comments into consideration :-( - Care to submit a few suggestions we can try?

Comment 21

[alta88]

Yes, those are your choices sheriff. I would suggest trying to replace

+  if (!IsValidIndex(index))
+    return NS_MSG_INVALID_DBVIEW_INDEX;

with

-  if (index == nsMsgViewIndex_None)
-    return NS_ERROR_UNEXPECTED;

in both nsMsgDBView::UpdateDisplayMessage() and nsMsgDBView::LoadMessageByViewIndex(), the STR here seem pretty clear as to determining whether that fixes this regression.

And comment 19 has a further suggestion for the original crashes. I don't have the time to get into these weeds much more than that, sorry.

Comment 22

[Jorg K (CEST = GMT+2)]

Sadly I don't have that set up and I'm also not sure I could spot the problem ... and other fires are burning hotter :-( - Alice, would you be able to check this for us.

Here
https://searchfox.org/comm-central/rev/efacd58fa63199a34709dc95b05bb9ab56d45f13/mailnews/base/src/nsMsgDBView.cpp#1145

and here
https://searchfox.org/comm-central/rev/efacd58fa63199a34709dc95b05bb9ab56d45f13/mailnews/base/src/nsMsgDBView.cpp#1185
change
  if (!IsValidIndex(index))
    return NS_MSG_INVALID_DBVIEW_INDEX;
to
  if (index == nsMsgViewIndex_None)
    return NS_ERROR_UNEXPECTED;


which is undoing:
https://hg.mozilla.org/comm-central/rev/8b61c0301de7#l1.15
https://hg.mozilla.org/comm-central/rev/8b61c0301de7#l1.84

If we revert the hunk in nsMsgDBView::UpdateDisplayMessage(), I'll add the checking for the m_flags and m_keys arrays.

One at a time, so we can see which change was the culprit.

Alta88, I also see similar changes in the second changeset:
https://hg.mozilla.org/comm-central/rev/40f5ba35583e#l1.119
in a function with "thread" in its name. Is that not also a candidate for reversal?

Comment 23

[alta88]

(In reply to Jorg K (GMT+1) from comment #22)
> Sadly I don't have that set up and I'm also not sure I could spot the
> problem ... and other fires are burning hotter :-( - Alice, would you be
> able to check this for us.
> 
> Here
> https://searchfox.org/comm-central/rev/
> efacd58fa63199a34709dc95b05bb9ab56d45f13/mailnews/base/src/nsMsgDBView.
> cpp#1145
> 
> and here
> https://searchfox.org/comm-central/rev/
> efacd58fa63199a34709dc95b05bb9ab56d45f13/mailnews/base/src/nsMsgDBView.
> cpp#1185
> change
>   if (!IsValidIndex(index))
>     return NS_MSG_INVALID_DBVIEW_INDEX;
> to
>   if (index == nsMsgViewIndex_None)
>     return NS_ERROR_UNEXPECTED;

and obviously the |index| value here should be viewPosition and aViewIndex for UpdateDisplayMessage() and LoadMessageByViewIndex() respectively.

> 
> 
> which is undoing:
> https://hg.mozilla.org/comm-central/rev/8b61c0301de7#l1.15
> https://hg.mozilla.org/comm-central/rev/8b61c0301de7#l1.84
> 
> If we revert the hunk in nsMsgDBView::UpdateDisplayMessage(), I'll add the
> checking for the m_flags and m_keys arrays.
> 
> One at a time, so we can see which change was the culprit.
> 
> Alta88, I also see similar changes in the second changeset:
> https://hg.mozilla.org/comm-central/rev/40f5ba35583e#l1.119
> in a function with "thread" in its name. Is that not also a candidate for
> reversal?

good call, the 3 occurrences in this part are probably the best place to start, as the unified view is a search view.

Comment 24

[Jorg K (CEST = GMT+2)]

> ... as the unified view is a search view.
It is indeed. Alice, can you please experiment with undoing some changes in
https://hg.mozilla.org/comm-central/rev/40f5ba35583
You could start with a complete backout and if that fixes the problem, see what hunk does the damage. Sadly right now I don't have time to set this up myself.

Comment 25

[frank.schaper]

> good call, the 3 occurrences in this part are probably the best place to start, as the unified view is a search view.

Checked this just out of curiosity: the same behaviour (incorrect sort order for new messages arriving in threaded view) can indeed be reproduced with any "Saved Search Folder".

Comment 26

[Jorg K (CEST = GMT+2)]

Yes, not a surprise at all. Maybe an easier test case. Now I need someone with some time who can build TB and "play around" with those changesets.

Comment 27

[alta88]

(In reply to Jorg K (GMT+1) from comment #24)
> > ... as the unified view is a search view.
> It is indeed. Alice, can you please experiment with undoing some changes in
> https://hg.mozilla.org/comm-central/rev/40f5ba35583
> You could start with a complete backout and if that fixes the problem, see
> what hunk does the damage. Sadly right now I don't have time to set this up
> myself.

the assertions should not be backed out, as there is potential oob access and there will be a crash if so, and i think there were such signatures.

only the 3 places with this pattern should be reverted:

-  if (newIndex == nsMsgViewIndex_None)
+  if (!IsValidIndex(newIndex))

Comment 28

[Jorg K (CEST = GMT+2)]

Sure, but we can back out 40f5ba35583 to see whether it makes a difference, no? No need to play with the individual bits before we know that we have the right changeset to play with.

Comment 29

[Alice0775 White]

(In reply to Jorg K (GMT+1) from comment #24)
> > ... as the unified view is a search view.
> It is indeed. Alice, can you please experiment with undoing some changes in
> https://hg.mozilla.org/comm-central/rev/40f5ba35583
> You could start with a complete backout and if that fixes the problem, see
> what hunk does the damage. Sadly right now I don't have time to set this up
> myself.

I successfully built of comm-central on Windows 10 Home 64bit.
I confirmed that backing out cset 40f5ba35583 from tip(c-c eb66bb3b1b76  m-c 6c10213a8924) fixed the problem.

Comment 30

[Jorg K (CEST = GMT+2)]

Thanks, could you continue here and see which part of cset 40f5ba35583 causes the problem? There are only four relevant changes:
https://hg.mozilla.org/comm-central/rev/40f5ba35583#l1.12
https://hg.mozilla.org/comm-central/rev/40f5ba35583#l1.31
https://hg.mozilla.org/comm-central/rev/40f5ba35583#l1.53
https://hg.mozilla.org/comm-central/rev/40f5ba35583#l1.31

If you could reverse them individually that would be a great help.

Comment 31

[Alice0775 White]

(In reply to Jorg K (GMT+1) from comment #30)
> Thanks, could you continue here and see which part of cset 40f5ba35583
> causes the problem? There are only four relevant changes:
> https://hg.mozilla.org/comm-central/rev/40f5ba35583#l1.12
> https://hg.mozilla.org/comm-central/rev/40f5ba35583#l1.31
> https://hg.mozilla.org/comm-central/rev/40f5ba35583#l1.53
> https://hg.mozilla.org/comm-central/rev/40f5ba35583#l1.31
> 
> If you could reverse them individually that would be a great help.


2nd and 4th chunk are same. So, I assume that 4th one is the last chunk of patch.

1st: reproduce the issue.
2nd: reproduce the issue.
3rd: reproduce the issue.
4th: works as expected.

diff --git a/mailnews/base/src/nsMsgSearchDBView.cpp b/mailnews/base/src/nsMsgSearchDBView.cpp
--- a/mailnews/base/src/nsMsgSearchDBView.cpp
+++ b/mailnews/base/src/nsMsgSearchDBView.cpp
@@ -634,20 +634,19 @@ void nsMsgSearchDBView::MoveThreadAt(nsM
 
     uint32_t collapseCount;
     CollapseByIndex(threadIndex, &collapseCount);
   }
 
   nsMsgDBView::RemoveByIndex(threadIndex);
   m_folders.RemoveObjectAt(threadIndex);
   nsMsgViewIndex newIndex = GetIndexForThread(threadHdr);
-  NS_ASSERTION(newIndex == m_levels.Length() ||
-               (IsValidIndex(newIndex) && !m_levels[newIndex]),
+  NS_ASSERTION(newIndex == m_levels.Length() || !m_levels[newIndex],
                "inserting into middle of thread");
-  if (!IsValidIndex(newIndex))
+  if (newIndex == nsMsgViewIndex_None)
     newIndex = 0;
 
   nsMsgKey msgKey;
   uint32_t msgFlags;
   threadHdr->GetMessageKey(&msgKey);
   threadHdr->GetFlags(&msgFlags);
   InsertMsgHdrAt(newIndex, threadHdr, msgKey, msgFlags, 0);

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: 1470049-partial-backout.patch

OK, I reverted the line Alice indicated. The NS_ASSERT() doesn't need to be reverted, in an opt build that does nothing. I'll prepare a TB 60 based try run for Windows for the reporters to try.

Thank you so much Alice for your invaluable work!!!

Comment 33

[Jorg K (CEST = GMT+2)]

Can you all please try
https://queue.taskcluster.net/v1/task/BwgX0E9FTBGmjmCE9FljLQ/runs/0/artifacts/public/build/install/sea/target.installer.exe
or
https://queue.taskcluster.net/v1/task/BwgX0E9FTBGmjmCE9FljLQ/runs/0/artifacts/public/build/target.zip

The former is an installer of TB 60.3.2 (preview version) with this fix included. The latter is a ZIP file, unpack to somewhere and execute "thunderbird.exe" from the DOS prompt.

This is safe to run on your production profile.

Comment 34

[Alice0775 White]

(In reply to Jorg K (GMT+1) from comment #33)
> Can you all please try
> https://queue.taskcluster.net/v1/task/BwgX0E9FTBGmjmCE9FljLQ/runs/0/
> artifacts/public/build/install/sea/target.installer.exe
> or
> https://queue.taskcluster.net/v1/task/BwgX0E9FTBGmjmCE9FljLQ/runs/0/
> artifacts/public/build/target.zip

The try build works as expected for me.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9a840a138dbe
Partially revert rev 40f5ba35583 (bug 1385573) to fix insertion into threaded unified/search view. a=backout DONTBUILD

Comment 36

[Jorg K (CEST = GMT+2)]

TB 60.3.2 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/9c373f4cbef707b9cfd7eb01879020f0b58af771

Comment 40

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/f31b3a9ea5a80493a2358ea7708f2f476d289608

Comment 41

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/f31b3a9ea5a80493a2358ea7708f2f476d289608

Comment 42

[frank.schaper]

Just installed the 60.3.2 (32-bit) release (Windows 7) and the bug is indeed fixed! 

Thanks, guys, that was awesome!