128809 - Mail layout goes into infinite loop when "view headers all" is activated

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[carljohan]

I was viewing my mail with "View Headers All" activated. I encountered
some email from a mailinglist with a huge amount of headers. When Mozilla
tried to display the mail the layout of the mail entered an infinite loop
and started flashing at the bottom ov the mail.

I can repeat this with several mails in my inbox, all with huge amounts
of headers. The headers are so big that mozilla can't display the mailbox,
the headers and the mail body at the same time without squeezing the
mailbox down to just two entries.

Steps to reproduce:
View the mail I'm going to attach with "View Headers All" activated.

Expected Result:
Mozilla should display the mail.

Actual Result:
Mozilla enters an infinite layout loop.

Mozilla is still responsive but takes a lot of CPU. Viewing another
mail/mailbox makes it alright again.

Comment 1

[carljohan]

Attachment: Mail that causes mozilla to go into infinite layout loop

This mail is one of several mail with large amounts of headers that
forces mozilla into an infinite layout loop.

Comment 2

[carljohan]

Attachment: A screenshot of Mozilla flashing

Look at the lower part of the mail. The layout is a bit broken but in
a tenth of a second it changes its' layout to the corect one that is
shown in the second attachment.

Comment 3

[carljohan]

Attachment: Another screenshot of mozilla flashing

This is the correct layout which is displayed for some tenths of
a second and then the cycle repeats.

Comment 4

[carljohan]

The build I'm using is

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.8) Gecko/20020212

And I can reproduce it every time.

Comment 5

[Andrew Schultz]

please change mime type of image attachments to image/png

Comment 6

[carljohan]

Attachment: Picture one again.

Comment 7

[carljohan]

Attachment: Picture 2 gain

Comment 8

[carljohan]

Sorry. I must have been sleeping when I uploaded the first pictures
as text/plain. Someone who is authorized please obsolete the first
two pictures.

Comment 9

[Koike Kazuhiko]

Reproduced with 2002040211/Linux.

Another mail file that causes an infinite loop:
http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=648

Comment 10

[carljohan]

The Japaneese mail sends my Mozilla into an infinite loop as well, but only if I
make the mailwindow smaller first. The problem occurs when there is so many
headers so that the listing of mails, the headers and the mail itself can fit
in one window.

Comment 11

[(not reading, please use [e-mail] instead)]

accepting.

yulian just found this on win2k, and laurel can see it on win98, so marking all.

so trick for reproducing it:

1) open a folder with 4 messages
2) I found it easier to reproduce with a message with no body.
3) view headers all
4) play with the size of the whole window, get small enough, and you'll probably
hit the freeze

note, yulian tried in build that contained that "no height set" fix that mscott,
hyatt and kin came up with.

note, view all headers is off by default

I'll try to get some stack traces.

Comment 12

[(not reading, please use [e-mail] instead)]

see also bug http://bugzilla.mozilla.org/show_bug.cgi?id=121583

"work around the infinite reflow loop in various windows that use tree widgets by
forcing a min height and min width on the the tree. This fixes the famous 100 %
CPU hang"

that fix has only landed on the 1.0 branch and the trunk, not the mach v beta
branch.

Comment 13

[(not reading, please use [e-mail] instead)]

unless of course it landed before we branched for mach v beta.  I'll check...

Comment 14

[(not reading, please use [e-mail] instead)]

yes, it must have landed before the mach v branch branched from the 1.0 branch.

Maybe a similar workaround can fix this problem.

Comment 15

[(not reading, please use [e-mail] instead)]

met with kin today, he's going to do some investigation that will hopefully
yield to a "real" fix in layout.

on my end, I'm going to try to come up with something less risky, but more of a
hack to our xul (or .css or .xml), like they did for #121583

Comment 16

[kinmoz]

It looks like this bug is a variation on bug #121583, except it's not the
scrollbar in the tree that's changing the size of the tree body frame. It looks
like in this particular case, the box layout code is doing a multi-pass-layout
at several points in the box tree. One of the first passes makes the size of the
tree big enough that it thinks it doesn't need a scrollbar, so it posts an
underflow DOMEvent, and then during a subsequent pass, it resizes the tree so
that it does need a scrollbar, so it posts an overflow DOMEvent.

When reflow is done, processing the 1st dom event, collapses the scrollbar and
posts a reflow event. The 2nd DOMEvent simply un-collapses the scrollbar so that
when we process the posted reflow event, we are back to square one and the cycle
continues.

I'm thinking we need to break this cycle by either:

1. Removing the DOMEvent posting mechanism to hide/show the scrollbars, and
   do it some other way.

or:

2. Add some mechanism that allows us to track and revoke previously posted
   events when we're about to post one that supercedes it.

I'm leaning towards trying to find a way to do #1.

Comment 17

[kinmoz]

kmcclusk says [e-mail] is handling box bugs while hyatt is away.

hewitt, can you take a look at this one?

Comment 18

[Segura M.]

*** Bug 145252 has been marked as a duplicate of this bug. ***

Comment 19

[(not reading, please use [e-mail] instead)]

neil's patch (http://bugzilla.mozilla.org/attachment.cgi?id=84232&action=view) 
from http://bugzilla.mozilla.org/show_bug.cgi?id=121583#c136 seems to fix this.

I'll confirm.

Comment 20

[(not reading, please use [e-mail] instead)]

>neil's patch (http://bugzilla.mozilla.org/attachment.cgi?id=84232&action=view) 
>from http://bugzilla.mozilla.org/show_bug.cgi?id=121583#c136 seems to fix this.
>I'll confirm.

false alarm.  it doesn't fix it.

Comment 21

[Bartosz Wucke]

May I suggest a fix for this that would solve this and many more problems?
Making the "headers" a separate resizable frame with a scrollbar, like the other
frames in the window, just read-only: With most bigger headers the current
solution leaves very little room for the other frames - folder list, folders and
mail contents, swallowing most of the available area. It would be very neat if
the headers were selectable (it's no fun retyping "references" or such...) and
one could decide how much screen real estate they want to spend on the headers.

Comment 22

[[e-mail]]

Attachment: Proposed Patch

This patch fixes some issues I was having displaying attachment 72383 [details] in the
standalone message window. Note: I also used my attachment to bug 121583.

Comment 23

[laurel]

*** Bug 147534 has been marked as a duplicate of this bug. ***

Comment 24

[(not reading, please use [e-mail] instead)]

note:  I've tried out all neils patches together, and I'm still getting the hang.

Comment 25

[KOYAMA Hiroshi]

*** Bug 147767 has been marked as a duplicate of this bug. ***

Comment 26

[[e-mail]]

Oops, patch only fixes the message window, not the 3panes :-(
But just to make sure that I'm on the right lines, what happens if you:
1. Collapse the message pane
2. Select the dodgy message
3. Uncollapse the message pane
I don't see any problems in this case.

Comment 27

[[e-mail]]

Attachment: 3 pane patch

HideAccountCentral() removes these collapsed attributes.

Comment 28

[R.K.Aa.]

*** Bug 148392 has been marked as a duplicate of this bug. ***

Comment 29

[laurel]

*** Bug 140969 has been marked as a duplicate of this bug. ***

Comment 30

[(not reading, please use [e-mail] instead)]

I tried out all neil's patches, including the new one to messagepane.xul, no 
luck.

> Oops, patch only fixes the message window, not the 3panes :-(
> But just to make sure that I'm on the right lines, what happens if you:
> 1. Collapse the message pane
> 2. Select the dodgy message
> 3. Uncollapse the message pane

with the "evil" message, and with the right 3 pane window size, that can cause 
the infinite loop that kin describes in 
http://bugzilla.mozilla.org/show_bug.cgi?id=128809#c16

(Note, the "evil" message I'm using is 
http://bugzilla.mozilla.org/attachment.cgi?id=72383&action=view, but I've seen 
it with a variety of other message, when in view all headers mode.)

to reproduce this problem:

1) put the evil message in a folder with no other messages.  
2) go into view all header mode
3) grab the bottom of the 3 pane window and move upwards.

at a certain point, that seems to make the thread pane just small enough where 
we go into the infinite loop.

hewitt and I are talking over aim about some things we can do to the underflow 
event handler in tree.xml

hewitt's idea:

you're going to allow us to see the overflow state (nsTreeBoxFrame's 
mVerticalOverlow) as a property on nsITreeBoxObject, and then when handling 
underflow, we would check that, and not collapse the toolbar in that scenario.

Comment 31

[(not reading, please use [e-mail] instead)]

Attachment: hack, but might inspire a better hack

Comment 32

[(not reading, please use [e-mail] instead)]

that last hack depends on shaver's fix for 
http://bugzilla.mozilla.org/show_bug.cgi?id=146210, as it uses Date.now()

basically, I'm cheating and deciding that if we get an underflow event followed 
by an overflow event, followed by an underflow event within 1 second, to ignore 
the second underflow event, and not collapse the scrollbars.

this prevents the infinite loop, but as you can guess, if the loop is on a slow 
machine, 1 second might not be enough.

as you can also guess, we are dropping underflow events on the floor, which can 
lead to scenarios where we show the scroll bar, when we shouldn't.
see the upcoming screen shot.

hewitt's idea sounds much better, but this is just an alternative patch, in the 
spirit of kin's suggestion from http://bugzilla.mozilla.org/show_bug.cgi?
id=128809#c16

"2. Add some mechanism that allows us to track and revoke previously posted
    events when we're about to post one that supercedes it."

Comment 33

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of the side effect of this change: scroll bars shot be collapsed but they arent

Comment 34

[(not reading, please use [e-mail] instead)]

re-assign to shuehan.

shuehan see http://bugzilla.mozilla.org/show_bug.cgi?id=121583 to see how a 
similar bug was fixed.  

Comment 35

[R.K.Aa.]

*** Bug 150573 has been marked as a duplicate of this bug. ***

Comment 36

[[e-mail]]

> at a certain point, that seems to make the thread pane just small enough where
> we go into the infinite loop.

D'oh! Perhaps this doesn't apply to the alternate 3-pane layout? There's a limit
to how small I can make the thread pane in that layout...

Comment 37

[Pádraig Brady]

*** Bug 123882 has been marked as a duplicate of this bug. ***

Comment 38

[scottputterman]

Shuehan, what's the eta on the fix for this?

Comment 39

[scottputterman]

Shuehan, should this be reassigned to Joe?

Comment 40

[shliang]

reassigning to joe since he has said that he wanted to work on it.

Comment 41

[Joe Hewitt (gone)]

Attachment: fix

This fix plugs the infinite loop by checking the overflow state of the tree
before collapsing/hiding the scrollbar, and only doing so if it has changed.

Comment 42

[Jan Varga [:janv]]

kin, your analysis gave me an idea
I think once I saw, box code calling SetBounds() 2 times in some layout
function. We call CheckVerticalOverflow() in nsTreeBodyFrame::SetBounds()
and CheckVerticalOverflow() posts underflow or overflow events when needed.
I think it would be better to call CheckVerticalScrollbar() after reflow
completed its work - maybe in DidReflow() or there is a reflow callback
mechanism which
hyatt added a long time ago to fix problems with scrollbar in the old tree
implementation.

If we don't figure out a real fix, we will have to stick with the hack.

Comment 43

[Jan Varga [:janv]]

ok, I added DidReflow() to nsTreeBodyFrame, but it is never called.
I checked nsLeafBoxFrame - the same (nsTreeBodyFrame inherits from nsLeafBoxFrame)
It seems that this stopped working recently, since I'm sure that it worked some
time ago.

Comment 44

[laurel]

*** Bug 152302 has been marked as a duplicate of this bug. ***

Comment 45

[Jaime Rodriguez, Jr.]

lowering impact to adt2 rtm.

Comment 46

[[e-mail]]

With a standard Mozilla build, loading the message causes it to take over all
available space in the window. With all my 30+ patches that I have written and
installed on my PC for my day-to-day use the message loads in the space that the
splitter has allocated to it thus completely avoiding this problem. As you have
noticed I have not yet tracked down the patch that actually resolves the problem.

Comment 47

[Andrew Schultz]

*** Bug 153626 has been marked as a duplicate of this bug. ***

Comment 48

[Jan Varga [:janv]]

Attachment: other fix

This patch makes us posting only one overflow/underflow event.
I didn't test it much. If it fixes the problem for someone else, please let me
know. Thanks.

Comment 49

[Jan Varga [:janv]]

Comment on attachment 89249 [details] [diff] [review]
other fix

sorry, I just ran into infinite loop again with this patch

Comment 50

[[e-mail]]

Attachment: Alternative approach

This attacks the problem by preventing the message from taking up too much
space.

Comment 51

[Joe Hewitt (gone)]

Has anyone noticed that if you use the splitter above the message header to
decrease the height of the message pane, at a certain point (around 200px) the
message pane browser refuses to shrink any further, and will actually overlap
the status bar?

Comment 52

[Sean Su]

*** Bug 154021 has been marked as a duplicate of this bug. ***

Comment 53

[Asa Dotzler [:asa]]

Joe, are you talking about bug 41994 or bug 119629? (are those two the same?). 

Comment 54

[Jan Varga [:janv]]

Joe's patch fixes this bug for me. I haven't found a better solution yet.
So we can fall back with this fix until we find a better one.

Comment 55

[Jan Varga [:janv]]

Comment on attachment 87778 [details] [diff] [review]
fix

r=varga

Comment 56

[[e-mail]]

Comment on attachment 87778 [details] [diff] [review]
fix

>-          event.preventBubble();
Why don't you want the event to bubble all of the time any more?

>+          if (!this.parentNode.treeBoxObject.overflowed && this.parentNode.treeBoxObject.overflowed != this._lastOverflow) {
Why not just:
if (!this.parentNode.treeBoxObject.overflowed && this._lastOverflow) {

>-          event.preventBubble();
Same again.

>+          if (this.parentNode.treeBoxObject.overflowed && this.parentNode.treeBoxObject.overflowed != this._lastOverflow) {
Almost same again:
if (this.parentNode.treeBoxObject.overflowed && !this._lastOverflow) {

Comment 57

[Jan Varga [:janv]]

taking

Comment 58

[laurel]

*** Bug 157599 has been marked as a duplicate of this bug. ***

Comment 59

[Jan Varga [:janv]]

Attachment: new fix

This fix uses the nsIReflowCallback mechanism to check vertical overflow only
when reflow finished.

Comment 60

[Bill Mason]

*** Bug 131145 has been marked as a duplicate of this bug. ***

Comment 61

[Srilatha Moturi]

I tried the last patch "new fix" and it sems to fix the hang.  One thing I
noticed is that based on size of the mail window, sometimes the thread pane is
not visible. If I increase the size of the window I can see it again. 

Comment 62

[Jan Varga [:janv]]

thanks!

> sometimes the thread pane is not visible
I think, this is expected behaviour, thread pane is flexible and headers take so
much space that thread pane have to shrink to minimal height.
I wonder what whould happen after adding min heigth to thread pane.
I do hope not another hang :)

Comment 63

[Brian Ryner (not reading)]

Comment on attachment 91545 [details] [diff] [review]
new fix

sr=bryner

Comment 64

[Asa Dotzler [:asa]]

Jan, who can review this? We're short on time for 1.1b.

Comment 65

[kinmoz]

Comment on attachment 91545 [details] [diff] [review]
new fix

r=[e-mail]

Comment 66

[[e-mail]]

> sometimes the thread pane is not visible

That's bug 106679.

Comment 67

[Scott Collins]

Comment on attachment 91545 [details] [diff] [review]
new fix

a=scc for checkin to the mozilla trunk

Comment 68

[Jan Varga [:janv]]

checked in

Comment 69

[laurel]

*** Bug 159608 has been marked as a duplicate of this bug. ***

Comment 70

[laurel]

OK using sep16 commercial trunk build: win98, mac OS 10.1, linux rh6.2