Closed Bug 1470049 Opened 6 years ago Closed 6 years ago

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

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set
normal

Tracking

(thunderbird_esr6064+ fixed, thunderbird64 fixed, thunderbird65 fixed)

RESOLVED FIXED
Thunderbird 65.0
Tracking Status
thunderbird_esr60 64+ fixed
thunderbird64 --- fixed
thunderbird65 --- fixed

People

(Reporter: markh, Assigned: jorgk-bmo)

References

Details

(Keywords: regression, Whiteboard: [regression:TB58])

Attachments

(3 files)

Attached image inbox-sort-order.png (deleted) —
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.
I have exactly this same problem, in Thunderbird 60.2.1 (64-bit) for Linux (Ubuntu Xenial 16.04.5).
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)
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.
Perhaps the earliest report of a potential version 60 regression, where https://mzl.la/2POjAlv may contain other reports
Version: unspecified → 60
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.
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.
(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.
Hi Alice, can you reproduce the STR? If so, can you find a regression? Quite laborious really. Thanks in advance!!
Flags: needinfo?(alice0775)
What is "Unified Inbox"?
Flags: needinfo?(alice0775)
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.
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.
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.
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.
(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”?
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?
Flags: needinfo?(alta88)
Just to note: Problem remains with the 60.3.1 (32 bit) update from Nov. 15th. (Windows 7 Pro)
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.
Flags: needinfo?(alta88)
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?
Summary: Unified inbox sort order sometimes incorrect when new messages arrive → Unified inbox sort order incorrect when new messages arrive in threaded view
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.
Blocks: 1385573
Whiteboard: [regression:TB58]
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?
Flags: needinfo?(alice0775)
(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.
> ... 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.
> 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".
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.
(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))
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.
(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.
Flags: needinfo?(alice0775)
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.
(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);
Attached patch 1470049-partial-backout.patch (deleted) — Splinter Review
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!!!
Assignee: nobody → jorgk
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.
Flags: needinfo?(ynovetsky)
Flags: needinfo?(markh)
Flags: needinfo?(frank.schaper)
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(alice0775)
Pushed by mozilla@jorgk.com: 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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Attachment #9028046 - Flags: approval-comm-esr60+
Attachment #9028046 - Flags: approval-comm-beta+
Just installed the 60.3.2 (32-bit) release (Windows 7) and the bug is indeed fixed! Thanks, guys, that was awesome!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: