SummaryFrameManager logic is wrong

RESOLVED FIXED in Thunderbird 13.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: protz, Assigned: protz)

Tracking

Trunk
Thunderbird 13.0
x86_64
Linux

Thunderbird Tracking Flags

(thunderbird11 fixed, thunderbird12 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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 :-/.
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → jonathan.protzenko
Status: NEW → ASSIGNED
Attachment #596327 - Flags: review?(squibblyflabbetydoo)
(Assignee)

Comment 2

5 years ago
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

5 years ago
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.)
(Assignee)

Comment 4

5 years ago
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

5 years ago
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+
(Assignee)

Comment 6

5 years ago
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 :)
(Assignee)

Comment 7

5 years ago
http://hg.mozilla.org/comm-central/rev/e4fa0ccbfb03
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
(Assignee)

Updated

5 years ago
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+
Checked into branches:

http://hg.mozilla.org/releases/comm-aurora/rev/27c9708bfc66
http://hg.mozilla.org/releases/comm-beta/rev/f1f57e95314c
status-thunderbird11: --- → fixed
status-thunderbird12: --- → fixed

Comment 9

5 years ago
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...
(Assignee)

Comment 10

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

Comment 11

5 years ago
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

4 years ago
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.