1600500 - Back widget "Go back one message" can easily go broken

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect, P2)

Description

[Jorg K (CEST = GMT+2)]

Attachment: bad go back list.png

I use the "Back widget" which you can place onto the toolbar via customisation a lot. Sadly it's terribly buggy.

I frequently notice that the message I'm looking for does not show up in the drop-down. Often the widget is enabled, but clicking the arrow to summon the history of visited messages gives an empty list. I'll attach a screenshot when it happens next.

I was looking for some STR, and found this:
Show a few messages in the preview and check that they show up in the list. Then delete/move one of those. That results in an empty entry in the list.

I'm sure there other malfunctions, I'll report more STR later.

Given that this is such an obvious bug, I wonder whether it has been reported before. I can't remember having seen it. Magnus, Aceman, Wayne, does this ring a bell?

BTW, I tried TB 52 and 60 and the mess-up after message move is there as well.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: empty list.png

OK, here's the empty list which also should the white dot as do all empty pulldowns which should have been disabled. How did I reproduce it? Hmm, I visited some messages in one folder, delete one of them, then visited a message in another folder, and then checked.

The feature is really pretty useless since it's as buggy as hell. Whether you'll find a message you looked at previously in day-to-day operation is pure luck, I'd say mostly you won't find it.

Comment 2

[Jorg K (CEST = GMT+2)]

I've done a bit of digging, this is connected to cmd_goBack and button_goBack from here:

https://searchfox.org/comm-central/rev/4476e6b96f49e626f32923258250f60484851851/mail/base/content/mainMailToolbox.inc.xul#103

Looking at cmd_goBack first, the relevant code could be in these places:
https://searchfox.org/comm-central/rev/4476e6b96f49e626f32923258250f60484851851/mail/base/content/mail3PaneWindowCommands.js#502
calling gDBView.navigateStatus()
https://searchfox.org/comm-central/rev/4476e6b96f49e626f32923258250f60484851851/mail/base/content/mail3PaneWindowCommands.js#844
calling GoNextMessage()
And for stand-alone windows we have:
https://searchfox.org/comm-central/source/mail/base/content/messageWindow.js#1178
calling gDBView.navigateStatus()
and
https://searchfox.org/comm-central/rev/4476e6b96f49e626f32923258250f60484851851/mail/base/content/messageWindow.js#1453
calling performNavigation() here:
https://searchfox.org/comm-central/rev/4476e6b96f49e626f32923258250f60484851851/mail/base/content/messageWindow.js#1496
calling CrossFolderNavigation(type).

And the menu is build in backToolbarMenu_init():
https://searchfox.org/comm-central/rev/4476e6b96f49e626f32923258250f60484851851/mail/base/content/mailWindowOverlay.js#1445
calling populateHistoryMenu() here:
https://searchfox.org/comm-central/rev/4476e6b96f49e626f32923258250f60484851851/mail/base/content/mailWindowOverlay.js#1460

That calls messenger.getNavigateHistory() which is here:
https://searchfox.org/comm-central/rev/4476e6b96f49e626f32923258250f60484851851/mailnews/base/src/nsMessenger.cpp#1937
and simply returns the content of mLoadedMsgHistory.

On a cursory look I can't see any code deleting entries for deleted messages. That would explain the blank entries, but not the empty but non-disabled dropdown.

Comment 3

[Wayne Mery (:wsmwk)]

Doesn't sound familiar, but I don't use navigation nor pay much attention to such bugs

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1600500-fix-history-menu.patch

Here's a first attempt. While testing it, I ran into the empty list issue (attachment 9112740 [details]) and saw this in the console:
JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 1496: TypeError: folder is null

Interesting.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1600500-fix-history-menu.patch

This does the trick for me. I'm sure the reviewer will tell me whether I'm glossing over more severe issues.

Comment 6

[Jorg K (CEST = GMT+2)]

I've patched my omni.ja file and this is working fine on TB 68 for the last 24 hours in production.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9112767 [details] [diff] [review]
1600500-fix-history-menu.patch

Review of attachment 9112767 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mailWindowOverlay.js
@@ +1492,5 @@
>      navDebug("history[" + i + "] = " + historyArray[i] + "\n");
>      navDebug("history[" + i + "] = " + historyArray[i + 1] + "\n");
>      folder = MailUtils.getOrCreateFolder(historyArray[i + 1]);
> +    if (!folder) {
> +      // Hell, what happened?

Lets leave the profanities out of the code shall we?

You didn't browse folders from an add-on did you? https://searchfox.org/comm-central/source/mailnews/base/src/FolderLookupService.jsm#67

Comment 8

[Jorg K (CEST = GMT+2)]

Yes, I can remove the comment or change it. What do you suggest? This is pure TB, and as I said at the end of comment #0, not working in TB 52 or TB 60 when the referenced code didn't exist.

Comment 9

[:aceman]

Comment on attachment 9112767 [details] [diff] [review]
1600500-fix-history-menu.patch

Review of attachment 9112767 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mailWindowOverlay.js
@@ +1487,4 @@
>    for (
>      var i = curPos.value;
>      isBackMenu ? i >= 0 : i < historyArray.length;
>      i += isBackMenu ? -2 : 2

How awful.

@@ +1492,5 @@
>      navDebug("history[" + i + "] = " + historyArray[i] + "\n");
>      navDebug("history[" + i + "] = " + historyArray[i + 1] + "\n");
>      folder = MailUtils.getOrCreateFolder(historyArray[i + 1]);
> +    if (!folder) {
> +      // Hell, what happened?

Maybe the whole folder of the historic messages got removed.
It is also strange this uses getOrCreateFolder() instead of  getFolderForURL(). Why would we want to create the folder if it does not exist? In that case, when is folder null if it always gets created? Did you hit a case when it is null?

@@ +1499,2 @@
>      navDebug(
>        "folder URI = " + folder.URI + "pretty name " + folder.prettyName + "\n"

This message is lacking some structure and definitely a space before "pretty". Not your fault of course :)

@@ +1503,4 @@
>      var menuText = "";
> +    var msgHdr = messenger.msgHdrFromURI(historyArray[i]);
> +    var msgSubject = msgHdr.mime2DecodedSubject;
> +    var msgAuthor = msgHdr.mime2DecodedAuthor;

I could understand the msgAuthor variable (IF that its the right one to check for existence), but what does msgSubject add?

@@ +1505,5 @@
> +    var msgSubject = msgHdr.mime2DecodedSubject;
> +    var msgAuthor = msgHdr.mime2DecodedAuthor;
> +
> +    if (!msgAuthor) {
> +      // If we can't get the author, the message was most likely (re)moved.

Can you check if an existing message can have no author? Like if there is no From: header? In that case, there would need to be a better check for message non-existence.

@@ +1521,5 @@
>      var subject = "";
>      if (msgHdr.flags & Ci.nsMsgMessageFlags.HasRe) {
>        subject = "Re: ";
>      }
>      if (msgHdr.mime2DecodedSubject) {

Could this be msgSubject too?

Comment 10

[:aceman]

This does look like bug 533504.

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #9)

I could understand the msgAuthor variable (IF that its the right one to
check for existence), but what does msgSubject add?

Puts all the assignments into one block instead of spreading them all over the function.

Could this be msgSubject too?

Yes, it should be.

To answer the author question: In general, a message that doesn't have an author will be somewhat corrupt, don't you think? I was going to so a if (!msgSubject || !msgAuthor) since empty subjects don't look good in the menu. I'm open to better ideas, but given that this is just about filling in a menu, it doesn't need to be super precise, IMHO.

That's for the bug reference. As Magnus keeps saying: All the bugs have already been reported :-(

Comment 13

[Jorg K (CEST = GMT+2)]

Did you hit a case when it is null?

See comment #4, but I don't have the exact steps. I only know that it happens all the time in production. With my patched 68.3 I haven't seen it again. One could try to get the exact steps, and then just not access folder if it's null/undefined to see what message that is referring to in the history.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1600500-fix-history-menu.patch (v2)

Fixed the nits. I'm still using msgAuthor for the test. I tried msgHdr.messageKey, but that's not returned null, instead the subject and author come back blank :-(

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9113134 [details] [diff] [review]
1600500-fix-history-menu.patch (v2)

Review of attachment 9113134 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mailWindowOverlay.js
@@ +1505,5 @@
> +    var msgSubject = msgHdr.mime2DecodedSubject;
> +    var msgAuthor = msgHdr.mime2DecodedAuthor;
> +
> +    if (!msgAuthor) {
> +      // If we can't get the author, the message was most likely (re)moved.

I could change that to !msgAuthor && !msgSubject
  // Avoid empty entries in the menu. The message was most likely (re)moved.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1600500-fix-history-menu.patch (v3)

OK, this is my final offer.

I added this debug

     if (!msgAuthor && !msgSubject) {
       // Avoid empty entries in the menu. The message was most likely (re)moved.
+      dump(`=== continuing ${msgHdr.messageKey)\n`);
       continue;
     }

and saw: === continuing 6

I'm not sure what that means, but it's pretty clear that a message that has moved still ha a key.

Comment 17

[:aceman]

Comment on attachment 9113906 [details] [diff] [review]
1600500-fix-history-menu.patch (v3)

Review of attachment 9113906 [details] [diff] [review]:
-----------------------------------------------------------------

I also tested this and the msgHdr.flags is zero, thus we can't even check for Expunged or other 
flags indicating deletion. So it looks like the header is mostly dummy and empty, maybe the key exists because it is available in the original URI.
So in that case, the checks you added seem to be all we can do.
Thanks.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9113906 [details] [diff] [review]
1600500-fix-history-menu.patch (v3)

Ancient and annoying bug we should fix one day. Zero risk since this adds a few null checks.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4b9f7d78c4c9
Don't include (re)moved messages/folders in back/forward history menu. r=aceman DONTBUILD

Comment 20

[Geoff Lankow (:darktrojan)]

72 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/6d36978931f799bf302f9d75949420c271e10614

Comment 21

[Geoff Lankow (:darktrojan)]

68.3.1:
https://hg.mozilla.org/releases/comm-esr68/rev/531d991deaf663bf46d1ce41f086ad38060c97bb