718306 - Changing layout breaks with Mail Summaries and/or Conversations

Status: RESOLVED DUPLICATE of bug 515732

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Protzenko [:protz]]

Attachment: Add extra checks (be paranoid)

The code that's responsible for changing the layout (classic / wide / vertical) is pretty optimistic as to the state we're in when changing the layout. In particular, it assumes that the messagepane has a valid, initialized browser, and that the attachmentList DOM node is also valid and ready to poke.

Both assumptions break when someone's playing with the multimessage pane. For instance, with Mail Summaries, if we change layouts twice in a row, because the multimessage pane remains displayed, both the assumptions above are broken.

The attached patch adds some extra checks in the two places that fail.

Comment 1

[Jonathan Protzenko [:protz]]

The try / catch can be replaced by if (document.getElementById("messagepane").destroy) if you prefer.

Comment 2

[Mark Banner (:standard8)]

Could this fix bug 531397?

Comment 3

[Jonathan Protzenko [:protz]]

Huh I'm not sure, the symptoms here are more like "you can change layout once, but not twice".

Comment 4

[Jim Porter (:squib)]

Comment on attachment 588748 [details] [diff] [review]
Add extra checks (be paranoid)

This doesn't work for me with Mail Summaries; with just a folder selected, it shows an empty thread summary page instead of the folder summary when switching layouts. However, it *does* improve things in that it's possible to change the layout more than once now.

It's entirely possible that this is an issue with the Mail Summaries code and not with Thunderbird, but I'd want to know why this is happening before I give this r+. Unfortunately, I don't have much time to debug this...

Clearing review out for now.

Comment 5

[Jonathan Protzenko [:protz]]

Hey Jim, have you had time to look into this? I'm a little bit puzzled by the fact that document.getElementById("messagepane") seems to be a dummy after you switched layouts once, and I don't really have an idea as to why this is happening...

Comment 6

[Thomas D. (:thomas8)]

Other bugs when changing View > Layout:

Bug 265393 - Changing View Layout does not preserve the mail message encoding
Bug 531397 - switching Layout modes breaks feed content display entirely until restart

Comment 7

[Ben Lerner]

Ping?  Jim, or anyone, have you had a chance to look into this?

Comment 8

[Anje]

Sent email to Jonathan, then found this bug listed.
Thunderbird version 31.0
when I selected the Vertical View and new error occured in previously empty error console.

Error console:
Timestamp: Thu 28/08/14 01:26:03:PM
Error: Please do not load stuff in the multimessage browser directly, use the SummaryFrameManager instead.
Source File: resource://gre/modules/summaryFrameManager.js
Line: 85

Which is in this section of code:
 _onLoad: function(event) {
    try {
      // Make sure we're responding to the summary frame being loaded, and not
      // some subnode.
      if (event.originalTarget != this.iframe.contentDocument)
        return;

      this.callback = this.pendingCallback;
      this.pendingCallback = null;
      if (this.pendingOrLoadedUrl != this.iframe.contentDocument.location.href)
        Components.utils.reportError(
          "Please do not load stuff in the multimessage browser directly, "+
          "use the SummaryFrameManager instead.");
      else if (this.callback)
        this.callback(true);
    }

Also:
Timestamp: Thu 28/08/14 01:26:03:PM
Error: messagepane.contentWindow is undefined
Source File: resource://conversations/modules/monkeypatch.js
Line: 752
which is this line:
 if (messagepane.contentWindow.location.href == "about:blank")

Located in this section:
 let messagepane = window.document.getElementById("messagepane");
    let fightAboutBlank = function () {
      if (messagepane.contentWindow.location.href == "about:blank") {
        Log.debug("Hockey-hack");
        // Workaround the "feature" that disables the context menu when the
        // messagepane points to about:blank
        messagepane.contentWindow.location.href = "about:blank?";
      }

Comment 9

[Jonathan Protzenko [:protz]]

Thanks for the additional information. I guess the current status of this bug is "there's a patch that helps but we don't know the exact cause of the failure for sure". Lacking further investigation, it looks like things are stalled. (As to me, I don't think I changed my layout even once over the past few years, so I can live with having to restart Thunderbird after changing layouts.)

Thanks,

~ jonathan

Comment 10

[Anje]

I agree with you.
I can live with it as well.
As I had some info I just thought it might be useful if and when you need it at a later date.

Comment 11

[userlond]

This bug breaks functionality of https://github.com/thunderbird-conversations/thunderbird-conversations extension and defenetly should be fixed.

Relative issue https://github.com/thunderbird-conversations/thunderbird-conversations/issues/534