Last Comment Bug 726301 - SummaryFrameManager logic is wrong
: SummaryFrameManager logic is wrong
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: x86_64 Linux
-- normal (vote)
: Thunderbird 13.0
Assigned To: Jonathan Protzenko [:protz]
Depends on:
  Show dependency treegraph
Reported: 2012-02-11 07:20 PST by Jonathan Protzenko [:protz]
Modified: 2013-01-30 13:32 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch for the issue (1.60 KB, patch)
2012-02-11 07:44 PST, Jonathan Protzenko [:protz]
squibblyflabbetydoo: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description User image Jonathan Protzenko [:protz] 2012-02-11 07:20:33 PST
So I was hunting down the following bug. Happens only on Thunderbird 11 and later, since the SummaryFrameManager landed in Thunderbird 11.

- Install Conversations (, just grab the latest file) and Mail Summaries (latest version on AMO).
- View a message in the conversation view.
- Click the "Delete" button in the conversation toolbar (rightmost button).
- The conversation view is in a bad state (unable to recover unless you switch folders or select multiple messages) and the folder summary is in a bad state too.

I hunted it down to the following line: if I add a dump statement like so in summaryFrameManager.js:87

      this.iframe.contentDocument.location.href = aUrl;
      dump("--- -> now is "+this.iframe.contentDocument.location.href+" "+aUrl+"\n");

Then the code dumps:

--- -> now is chrome://conversations/content/stub.xhtml chrome://mailsummaries/content/folderSummary.xhtml

That is to say, right after we set the .href to "folderSummary.xhtml", its value still is "stub.xhtml". What happens next is the conversation view kicks back in to grab the multimessage. Because the old "stub.xhtml" value is still held in ".href", the FrameManager thinks everything is fine, and instructs the conversation view to proceed (because the document currently loaded is the right one). Next thing we know, loading folderSummary.xhtml *actually* happens, but:
- the folderSummary callback was cancelled by the conversation view's
- the conversation view's callback fails miserably because, of course, the loaded document is not the right one.

I'm really puzzled, and I'm unsure what we should do at that stage :-/.
Comment 1 User image Jonathan Protzenko [:protz] 2012-02-11 07:44:15 PST
Created attachment 596327 [details] [diff] [review]
Patch for the issue

So, after chatting a little while about this in #developers, it turns out this is somehow « normal »; otherwise, there would be weird states where the document's href property does not contain the *actual* document's location...

The patch above fixes this, and displays a nice error message if we figure out someone messed with us.
Comment 2 User image Jonathan Protzenko [:protz] 2012-02-11 07:45:00 PST
Also, if the solution is satisfying enough, I think we should backport this all the way to beta. I can foresee the complaints coming from users who use both addons.
Comment 3 User image Jim Porter (:squib) 2012-02-11 14:50:18 PST
Before I review this, what's your opinion on using setAttribute("src", ...) to set the URL? Would that fix the race condition we're seeing? (I think this is actually how it worked originally, but then I switched to the location.href method since that's usually the preferred way to do this.)
Comment 4 User image Jonathan Protzenko [:protz] 2012-02-11 14:57:09 PST
The src attribute would indeed do pretty much what we want, but then you'd be in severe conflict if someone else plays with the multimessage using location.href. At least the location.href method allow us to deal with "outside changes" somehow (though not perfectly, granted).
Comment 5 User image Jim Porter (:squib) 2012-02-11 22:28:58 PST
Comment on attachment 596327 [details] [diff] [review]
Patch for the issue

Review of attachment 596327 [details] [diff] [review]:

This works for me*, and the rationale for handling it this way seems solid. r=me with my comment below addressed.

* It doesn't work when Mail Summaries, Conversations, and Deselect on Delete are all installed, but I'm not sure that's an issue with the patch; if I have to guess, I'd say it might be an issue with how Conversations overrides the selection code. In any case, there are under 600 users for Deselect on Delete, and I'm not too inclined to debug the issue unless someone complains. :)

::: mail/base/modules/summaryFrameManager.js
@@ +53,4 @@
>    this.iframe.addEventListener("DOMContentLoaded", this._onLoad.bind(this),
>                                 true);
>    this.pendingCallback = null;
> +  this.pendingOrLoadedUrl = "about:blank";

Is this the right thing to put here? I think that aFrame.contentDocument.location.href would be better.
Comment 6 User image Jonathan Protzenko [:protz] 2012-02-12 05:05:21 PST
Thanks for the fast review. Your comment is right, but we need to be more careful. I went for:

  this.pendingOrLoadedUrl = this.iframe.docShell
    ? this.iframe.contentDocument.location.href
    : "about:blank";

(because usually, at load-time, the XBL binding's get_contentDocument method will throw because there's no docShell yet).

I'll commit this as soon as the tree reopens :)
Comment 7 User image Jonathan Protzenko [:protz] 2012-02-14 00:05:18 PST
Comment 9 User image Jim Porter (:squib) 2012-02-20 13:29:06 PST
Just a note: somehow, I managed to break this on nightly with Mail Summaries. The folder summary refused to load, and I got the "Please do not load stuff in the multimessage browser directly" error in the error console. I can't reproduce this though, so be on the lookout for it, I guess...
Comment 10 User image Jonathan Protzenko [:protz] 2012-02-20 13:31:55 PST
Yes, I had the error too... I'm having trouble reproducing it as well.
Comment 11 User image Jim Porter (:squib) 2012-02-20 15:06:11 PST
We could add some more diagnostic output in the case of that error. That might help us narrow things down. We could also try to recover from the error and just show the message as a helpful tip.
Comment 12 User image Luke Usherwood 2013-01-30 13:32:42 PST
My error console shows the "Please do not load stuff..." Error message every time I start Thunderbird 17.0.2.  Just thought you might like to know.

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