Last Comment Bug 718306 - Changing layout breaks with Mail Summaries and/or Conversations
: Changing layout breaks with Mail Summaries and/or Conversations
Status: ASSIGNED
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: x86_64 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Jonathan Protzenko [:protz]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-15 09:44 PST by Jonathan Protzenko [:protz]
Modified: 2014-08-28 08:23 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add extra checks (be paranoid) (1.77 KB, patch)
2012-01-15 09:44 PST, Jonathan Protzenko [:protz]
no flags Details | Diff | Splinter Review

Description Jonathan Protzenko [:protz] 2012-01-15 09:44:08 PST
Created attachment 588748 [details] [diff] [review]
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] 2012-01-15 09:45:22 PST
The try / catch can be replaced by if (document.getElementById("messagepane").destroy) if you prefer.
Comment 2 Mark Banner (:standard8) (afk until 26th July) 2012-01-16 02:27:52 PST
Could this fix bug 531397?
Comment 3 Jonathan Protzenko [:protz] 2012-01-16 02:39:27 PST
Huh I'm not sure, the symptoms here are more like "you can change layout once, but not twice".
Comment 4 Jim Porter (:squib) 2012-01-17 00:15:35 PST
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] 2012-02-11 06:09:37 PST
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. (currently busy elsewhere; needinfo?me) 2012-06-23 09:14:44 PDT
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 2012-12-28 08:28:08 PST
Ping?  Jim, or anyone, have you had a chance to look into this?
Comment 8 Anje 2014-08-28 05:41:37 PDT
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] 2014-08-28 08:03:57 PDT
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 2014-08-28 08:23:46 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.