726301 - SummaryFrameManager logic is wrong

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Protzenko [:protz]]

So I was hunting down the following bug. Happens only on Thunderbird 11 and later, since the SummaryFrameManager landed in Thunderbird 11.

- Install Conversations (http://jonathan.xulforum.org/files/gcv-nightlies/, 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

[Jonathan Protzenko [:protz]]

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

[Jonathan Protzenko [:protz]]

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

[Jim Porter (:squib)]

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

[Jonathan Protzenko [:protz]]

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

[Jim Porter (:squib)]

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

[Jonathan Protzenko [:protz]]

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

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/comm-central/rev/e4fa0ccbfb03

Comment 8

[Mark Banner (:standard8)]

Checked into branches:

http://hg.mozilla.org/releases/comm-aurora/rev/27c9708bfc66
http://hg.mozilla.org/releases/comm-beta/rev/f1f57e95314c

Comment 9

[Jim Porter (:squib)]

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

[Jonathan Protzenko [:protz]]

Yes, I had the error too... I'm having trouble reproducing it as well.

Comment 11

[Jim Porter (:squib)]

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

[Luke Usherwood]

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.