Open Bug 474725 Opened 12 years ago Updated 8 years ago

messagereader: message header / body can get out of sync, don't match

Categories

(Thunderbird :: Message Reader UI, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: dmose, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [no l10n impact])

Attachments

(4 files, 1 obsolete file)

Which is to say that the header from one message often remains displayed with the body of another message.  This makes for some (temporary, but very unsettling) moments of person X said _that_?

At the time we display the header of the new message, if we don't yet have the body available, we should also clear the body of the old message out.
Flags: blocking-thunderbird3?
I do agree it's an annoying bug.  I'm poking at some related code, i may get to it.
Assignee: nobody → david.ascher
Component: Mail Window Front End → Message Reader UI
QA Contact: front-end → message-reader
Flags: blocking-thunderbird3? → blocking-thunderbird3+
If you could set the target milestone to when you think you might be able to get to this, that would be super helpful; thanks!
I think David wanted me to look at this - which might make this a dup.
Assignee: david.ascher → bienvenu
Target Milestone: --- → Thunderbird 3.0b3
here's the general plan of attack:

When the selection changes such that we will try to load a new message, set a short timer, and clear the message pane when the timer fires. If we get a notification that we've actually got data for the new message, clear the timer before we've cleared the message pane.

Furthermore, we should make clearing the message pane faster. What I see now when we clear the message pane is that we hide the header area, display the message contents, then show the header area, which causes us to redisplay the message contents. We should rearrange some code so that we show the message pane before displaying message contents, and instead of hiding the header area, we should just clear it out.

Deleting a message from a saved search (cross folder?) makes us clear the message pane (perhaps we're not setting next message to load in that case?), so the slowness of ClearMessagePane is painful to me.

I've done the work to make ClearMessagePane faster; now I'll go try the timer thing (though I think I'll attach a patch for the first part now).
Status: NEW → ASSIGNED
this makes a huge difference for me when deleting messages from my saved search.
I also need to figure out a way of distinguishing between the cases where we're clearing the message pane prior to loading an other message, and where we really want to clear the whole message pane (e.g., when the user multi-selects messages, until davida does something interesting with multiple selected msgs).
I think in most cases we actually suppress selection changed notifications (e.g., deleting messages from a normal folder) because we know we're going to select an other message. I need to hook into that code for the timer stuff, I think.
the timer idea turns out to be a bad idea, because in a certain percentage of cases, we will interrupt the message load in order to blank out the message pane (if you try to load a url in a docshell, it interrupts the current url). So we either need to load about:blank in the message pane before loading the next message, or we need to figure out an other way of blanking out the message pane and showing it when we get message contents that doesn't involve loading about:blank. Perhaps make the message pane a deck? I'm not sure that just loading about:blank first might not be just as fast...
I'm just saving off this work in case we decide to go to some sort of timer based solution, though we can't be loading about:blank on the timer for reasons I previously described.
Neil, do you have any thoughts on how to clear the message pane in between message loads efficiently and non-intrusively?  See #c8 for some thoughts.

I stepped through what loading about:blank does, and it does do a fair amount of stuff. It's also async, so I'd need to give it a little time to complete before loading the real url, or the second url will stop the about:blank url before it starts.  Perhaps one trip through the event loop would be enough, but it feels hacky.

I looked at nsIDocShell::LoadStream, which does skip some of the stuff LoadURI does, but it's still essentially async.

Adding a deck to the message pane still feels like it might be a bit slower, but I guess in the normal case we wouldn't be switching decks at all (e.g., set a 1 second timer; if we don't see any message data after a second, switch to a blank deck, and switch back once we see message content).
(In reply to comment #0)
> At the time we display the header of the new message, if we don't yet have the
> body available, we should also clear the body of the old message out.
Perhaps we should defer the display of the header of the new message until we have the body available? I'm not sure but maybe a location change notification on the docshell's web progress listener would fire at the right time?
(In reply to comment #11)
> Perhaps we should defer the display of the header of the new message until we
> have the body available? I'm not sure but maybe a location change notification
> on the docshell's web progress listener would fire at the right time?

The message body should fairly closely follow the header - both come from streaming the message through libmime, so if it's really a case of their being a long delay before getting any data from the server, that doesn't help so much. I rather suspect that the web progress listener might fire pretty early in the process, but I could be wrong...

I've written code to clear the header fields if we haven't seen the new headers within a second or so, and that works. But the larger problem is that the message body sticks around, and that's by far the more noticeable indication of which message is displayed.
This makes us clear the message pane before loading a new message. I'll run with it a while and see how it feels. I definitely notice the message pane getting cleared which I think by definition means that's there's some extra painting going on and something akin to flicker.

I can try the deck thing next, if people think it's worth pursuing. One question about that is how long should we wait before blanking out the message pane? I've been arbitrarily saying one second -  the longer we wait the less flicker there is, but the more likely the user is to get confused about what's selected vs. what's displayed.

One thing to note - I did this in nsMsgDBView.cpp because normally the js code doesn't even know we're about to load a new message; it just knows the selection changed, and tells the view, which then figures out if it should load a new message or not.
Summary: messagereader: message header / body can get out of sync → messagereader: message header / body can get out of sync, don't match
using the smart folders folder pane mode, select the Inbox saved search, and delete a message. You'll see us blank out the whole message area, draw the new message w/o a header area, and then display the header and redisplay the message in just message body part of the message pane.

This fixes that by not hiding the message header area, and showing the message header pane sooner after headers are downloaded.

Since we now have smart folders, this fix shouldn't block on deciding how to blank out the message pane...
Attachment #370976 - Attachment is obsolete: true
Attachment #375628 - Flags: review?(bugzilla)
Attachment #375628 - Flags: review?(bugzilla) → review+
Comment on attachment 375628 [details] [diff] [review]
this speeds up delete from saved search by reducing redraws in msg pane - checked in.

This looks fine, I'm a little bit unsure about doing it this way, but now is the time to get this in and get it tested.

r=Standard8
Attachment #375628 - Attachment description: this speeds up delete from saved search by reducing redraws in msg pane → this speeds up delete from saved search by reducing redraws in msg pane - checked in.
Priority: -- → P3
(In reply to comment #15)

> [...] but now is the time to get this in and get it tested.

This might have caused bug 491840.
Depends on: 491840
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0b4
david, did you backout this patch in bug 491840?
I backed out part of this patch - I believe I left in the part that speeds up delete from saved search, but backed out the part that cleared the message pane.
Whiteboard: [no l10n impact]
I'm at a bit of a loss as to how to proceed here. Here's what I believe is happening, when we try to display a message:

1. We start streaming the message to libmime.
2. Libmime sees the message headers in the stream, and populates at the header display area.
3. libmime sees the message body and starts emitting html to gecko.
4. gecko starts laying out and eventually displaying the message.

Sometimes the delay between 2 and 4 is really long. I instrumented the notifications our webprogress listener gets and I don't see any events I can use to tell when the document has started displaying. There is an event when the document has finished loading, but that can be long after the message has started displaying.

It almost feels as if, at least for some documents, layout is postponing the display for a while. In particular, I have an html message with some href http images that does not start displaying until everything has fetch.  Perhaps our event queue is full of other stuff (e.g., js<->c++ proxying) and that sometimes stalls out the paint events.

The only thing I can really think to do is clear out the message pane before displaying the message, even though I think that will slow things down. As far as the deck approach is concerned, I can't think of a way to know when I should show the deck with the message.

I'm curious what kind of messages you see this issue with. Does it seem to happen more with messages that are more complicated to lay out, or does it happen on very simple messages as well? One thought is to clear out the message pane on larger message, given that they're going to take longer to display anyway...

The separate issue of never loading the right message might be easier to fix, since I think I saw a bug that had a reproducible case...
(In reply to comment #19)
> It almost feels as if, at least for some documents, layout is postponing the
> display for a while.
I think this is done deliberately for websites, as you don't want to tear down the old site until you have a fair amount of the new site with which to replace it, or something. However I think we do nowadays have a paint notification that we might be able to use, or roc may have a better idea?
(In reply to comment #19)

Have you tried looking at profiling data or the call graph with VTune or Quantify or some such?

> I'm curious what kind of messages you see this issue with. Does it seem to
> happen more with messages that are more complicated to lay out, or does it
> happen on very simple messages as well? 

I've seen it happen on relatively simple messages, but I haven't been paying a lot of specific attention.  I'll keep an eye out.  Do you not see this bug regularly?

> One thought is to clear out the message
> pane on larger message, given that they're going to take longer to display
> anyway...

Sounds worthwhile to me.

Here's hoping roc has good advice to offer...
I run mostly debug builds, so I expect to see slowdowns from time to time. I'm also running with gloda and win search integration turned on. Kent and Andrew did work to make gloda not peg the cpu. We've done work recently to reduce the amount of js garbage gloda and win search/spotlight create (only the former is checked in, I believe). And gloda tries harder to get out of the way of user actions, by waiting a little bit longer after notifications. All of these may help to some extent, so it would be interesting to see if it happens less to you going forward. It would also be interesting to know if it happens more after doing some operation, e.g., deleting several messages, vs. just clicking on a new message.

Re getting a paint event that Neil mentioned, would I get that if the message body deck wasn't on top? I ask because one idea is not to show the message body pane until layout has some data to paint.
pushing this out to rc1 - performance is a lot better recently so I haven't seen this issue in a while. I would like to try to figure out how to deal with the underlying issue where an interrupted message load can leave us with the message headers displayed but nothing in the body.
Target Milestone: Thunderbird 3.0b4 → Thunderbird 3.0rc1
We believe the problem is less frequent than it used to be.  We don't have obvious good candidate fixes, next step is probably some JS profiling.  If push came to shove, we probably wouldn't block on this if it were the last bug standing, unless we start seeing more reports of this.  Setting to wanted+.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Duplicate of this bug: 482949
I just had an 'extreme' case happen to me, in that the header/body didn't sync up after a short time.  I was able to capture the attached screen and still the header was out of sync (probably 30 seconds elapsed or so).  I finally clicked on another message and then back to this one and it displayed properly in sync.

I find the problem happens for short intervals while doing quick email triage (eg. quickly hitting Delete on several consecutive emails).
If it takes a long time to load the message, the header and the message area will be out of sync. We know this; we just don't have a good way to fix it. Blanking out the message pane between every message load slows us down, and trying to blank it out if the message load takes a long time has the unfortunate side effect of cancelling the message load. And it's hard to tell from layout when it has starting displaying contents in the message pane. We're open to clever suggestions here.
(In reply to David :Bienvenu from comment #27)
> We're open to clever suggestions here.

Could we be tricky and just set the message body to display: none; while it's loading? That wouldn't actually be blanking things out as the content is still there, but it just wouldn't be visible. Then again, if the main performance bottleneck is rendering, as opposed to emitting HTML, this probably won't help.
(In reply to Jim Porter (:squib) from comment #28)

> Could we be tricky and just set the message body to display: none; while
> it's loading? That wouldn't actually be blanking things out as the content
> is still there, but it just wouldn't be visible. Then again, if the main
> performance bottleneck is rendering, as opposed to emitting HTML, this
> probably won't help.

If we could figure out when gecko will start actually displaying the content, then we could do that. I just saw this bug with an itunes e-mail, and I suspect we'd received quite a bit of data before gecko starting displaying anything.
(In reply to David :Bienvenu from comment #29)
> If we could figure out when gecko will start actually displaying the
> content, then we could do that. I just saw this bug with an itunes e-mail,
> and I suspect we'd received quite a bit of data before gecko starting
> displaying anything.

I was thinking we could set the message body of the old message to display: none; when we start loading a new message. Then, until the new body is ready to be rendered, Gecko will hide the (old) message body. Maybe that's not how this works, though (I don't have any emails that exhibit this issue, so I can't test it).
Assignee: dbienvenu → nobody
Blocks: 699681
Status: ASSIGNED → NEW
Priority: P3 → --
Target Milestone: Thunderbird 3.0rc1 → ---
You need to log in before you can comment on or make changes to this bug.