Closed Bug 726301 Opened 8 years ago Closed 8 years ago

SummaryFrameManager logic is wrong

Categories

(Thunderbird :: Mail Window Front End, defect)

x86_64
Linux
defect
Not set

Tracking

(thunderbird11 fixed, thunderbird12 fixed)

RESOLVED FIXED
Thunderbird 13.0
Tracking Status
thunderbird11 --- fixed
thunderbird12 --- fixed

People

(Reporter: protz, Assigned: protz)

Details

Attachments

(1 file)

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 :-/.
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.
Assignee: nobody → jonathan.protzenko
Status: NEW → ASSIGNED
Attachment #596327 - Flags: review?(squibblyflabbetydoo)
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.
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.)
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 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.
Attachment #596327 - Flags: review?(squibblyflabbetydoo) → review+
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 :)
http://hg.mozilla.org/comm-central/rev/e4fa0ccbfb03
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
Attachment #596327 - Flags: approval-comm-beta?
Attachment #596327 - Flags: approval-comm-aurora?
Attachment #596327 - Flags: approval-comm-beta?
Attachment #596327 - Flags: approval-comm-beta+
Attachment #596327 - Flags: approval-comm-aurora?
Attachment #596327 - Flags: approval-comm-aurora+
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...
Yes, I had the error too... I'm having trouble reproducing it as well.
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.
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.
You need to log in before you can comment on or make changes to this bug.