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

VERIFIED FIXED in mozilla1.1alpha

Status

SeaMonkey
MailNews: Message Display
--
critical
VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: carljohan, Assigned: janv)

Tracking

({hang, polish})

Trunk
mozilla1.1alpha
x86
All
hang, polish
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 RTM],custrtm- [Needs Reviews] [ETA Needed])

Attachments

(7 attachments, 6 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 72383 [details]
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.
(Reporter)

Comment 2

17 years ago
Created attachment 72384 [details]
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.
(Reporter)

Comment 3

17 years ago
Created attachment 72385 [details]
Another screenshot of mozilla flashing

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

Comment 4

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

Updated

17 years ago
QA Contact: esther → laurel

Comment 5

17 years ago
please change mime type of image attachments to image/png
(Reporter)

Comment 6

17 years ago
Created attachment 72571 [details]
Picture one again.
(Reporter)

Comment 7

17 years ago
Created attachment 72572 [details]
Picture 2 gain
(Reporter)

Comment 8

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

17 years ago
Reproduced with 2002040211/Linux.

Another mail file that causes an infinite loop:
http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=648
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

17 years ago
Attachment #72384 - Attachment is obsolete: true

Updated

17 years ago
Attachment #72385 - Attachment is obsolete: true
(Reporter)

Comment 10

17 years ago
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.
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.
Severity: major → critical
Status: NEW → ASSIGNED
Keywords: hang
OS: Linux → All
Whiteboard: nsbeta1
Target Milestone: --- → mozilla1.1alpha

Updated

16 years ago
Keywords: nsbeta1

Updated

16 years ago
Whiteboard: nsbeta1

Updated

16 years ago
Blocks: 143047
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt1 RTM]
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.
unless of course it landed before we branched for mach v beta.  I'll check...
yes, it must have landed before the mach v branch branched from the 1.0 branch.

Maybe a similar workaround can fix this problem.
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

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

16 years ago
kmcclusk says hewitt@netscape.com is handling box bugs while hyatt is away.

hewitt, can you take a look at this one?

Comment 18

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

Comment 21

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

Updated

16 years ago
Whiteboard: [adt1 RTM] → [adt1 RTM],custrtm-

Comment 22

16 years ago
Created attachment 85063 [details] [diff] [review]
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.

Updated

16 years ago
Keywords: patch, polish, review, ui

Comment 23

16 years ago
*** Bug 147534 has been marked as a duplicate of this bug. ***
note:  I've tried out all neils patches together, and I'm still getting the hang.

Comment 25

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

Comment 26

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

16 years ago
Created attachment 85775 [details] [diff] [review]
3 pane patch

HideAccountCentral() removes these collapsed attributes.

Comment 28

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

Comment 29

16 years ago
*** Bug 140969 has been marked as a duplicate of this bug. ***
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.
Created attachment 86879 [details] [diff] [review]
hack, but might inspire a better hack
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."

Created attachment 86892 [details]
screen shot of the side effect of this change:  scroll bars shot be collapsed but they arent
re-assign to shuehan.

shuehan see http://bugzilla.mozilla.org/show_bug.cgi?id=121583 to see how a 
similar bug was fixed.  
Assignee: sspitzer → shliang
Status: ASSIGNED → NEW

Comment 35

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

Comment 36

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

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

Comment 38

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

Comment 39

16 years ago
Shuehan, should this be reassigned to Joe?

Comment 40

16 years ago
reassigning to joe since he has said that he wanted to work on it.
Assignee: shliang → hewitt

Comment 41

16 years ago
Created attachment 87778 [details] [diff] [review]
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.

Updated

16 years ago
Attachment #85063 - Attachment is obsolete: true
Attachment #85775 - Attachment is obsolete: true
Attachment #86879 - Attachment is obsolete: true

Updated

16 years ago
Whiteboard: [adt1 RTM],custrtm- → [adt1 RTM],custrtm- [Needs Reviews] [ETA Needed]
(Assignee)

Comment 42

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

Comment 43

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

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

Comment 45

16 years ago
lowering impact to adt2 rtm.
Whiteboard: [adt1 RTM],custrtm- [Needs Reviews] [ETA Needed] → [adt2 RTM],custrtm- [Needs Reviews] [ETA Needed]

Comment 46

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

Updated

16 years ago
Blocks: 153207

Comment 47

16 years ago
*** Bug 153626 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 48

16 years ago
Created attachment 89249 [details] [diff] [review]
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.
(Assignee)

Comment 49

16 years ago
Comment on attachment 89249 [details] [diff] [review]
other fix

sorry, I just ran into infinite loop again with this patch
Attachment #89249 - Flags: needs-work+

Comment 50

16 years ago
Created attachment 89278 [details] [diff] [review]
Alternative approach

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

Comment 51

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

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

Comment 53

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

Comment 54

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

Comment 55

16 years ago
Comment on attachment 87778 [details] [diff] [review]
fix

r=varga
Attachment #87778 - Flags: review+

Comment 56

16 years ago
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) {
(Assignee)

Comment 57

16 years ago
taking
Assignee: hewitt → varga

Comment 58

16 years ago
*** Bug 157599 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 59

16 years ago
Created attachment 91545 [details] [diff] [review]
new fix

This fix uses the nsIReflowCallback mechanism to check vertical overflow only
when reflow finished.
Attachment #89249 - Attachment is obsolete: true

Comment 60

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

Comment 61

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

Comment 62

16 years ago
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 on attachment 91545 [details] [diff] [review]
new fix

sr=bryner
Attachment #91545 - Flags: superreview+

Comment 64

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

Comment 65

16 years ago
Comment on attachment 91545 [details] [diff] [review]
new fix

r=kin@netscape.com
Attachment #91545 - Flags: review+

Comment 66

16 years ago
> sometimes the thread pane is not visible

That's bug 106679.

Comment 67

16 years ago
Comment on attachment 91545 [details] [diff] [review]
new fix

a=scc for checkin to the mozilla trunk
Attachment #91545 - Flags: approval+
(Assignee)

Comment 68

16 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 69

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

Comment 70

16 years ago
OK using sep16 commercial trunk build: win98, mac OS 10.1, linux rh6.2
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.