Closed Bug 1600500 Opened 7 months ago Closed 7 months ago

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

Categories

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

defect

Tracking

(thunderbird_esr68 fixed, thunderbird72 fixed, thunderbird73 fixed)

RESOLVED FIXED
Thunderbird 73.0
Tracking Status
thunderbird_esr68 --- fixed
thunderbird72 --- fixed
thunderbird73 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached image 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.

Flags: needinfo?(vseerror)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
Attached image 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.

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.

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

Flags: needinfo?(vseerror)
Attached patch 1600500-fix-history-menu.patch (obsolete) — Splinter Review

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.

Assignee: nobody → jorgk-bmo
Attached patch 1600500-fix-history-menu.patch (obsolete) — Splinter Review

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

Attachment #9112766 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
Attachment #9112767 - Flags: review?(acelists)

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

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

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 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?

This does look like bug 533504.

Duplicate of this bug: 533504

(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 :-(

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.

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 :-(

Attachment #9112767 - Attachment is obsolete: true
Attachment #9112767 - Flags: review?(acelists)
Attachment #9113134 - Flags: review?(acelists)
Attachment #9113134 - Attachment is patch: true
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.

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.

Attachment #9113134 - Attachment is obsolete: true
Attachment #9113134 - Flags: review?(acelists)
Attachment #9113906 - Flags: review?(acelists)
Attachment #9113906 - Attachment is patch: true
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.
Attachment #9113906 - Flags: review?(acelists) → review+
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.
Attachment #9113906 - Flags: approval-comm-esr68+
Attachment #9113906 - Flags: approval-comm-beta+

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

Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 73.0
Target Milestone: Thunderbird 72.0 → Thunderbird 73.0
You need to log in before you can comment on or make changes to this bug.