547338 - "ASSERTION: No integers in range; 0 is supposed to be in range" with xul listbox

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: No integers in range; 0 is supposed to be in range: 'low <= high', file /Users/jruderman/mozilla-central/layout/generic/nsGfxScrollFrame.cpp, line 1742

This assertion was added in rev 4ccff5df452c.
user:        Robert O'Callahan
date:        Thu Oct 08 16:01:15 2009 +1300
summary:     Bug 526394. Part 31: Move scroll implementation into nsGfxScrollFrame. r=mats

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

The problem is the use of GetOverflowRect() during reflow -
it's not updated yet to take into account the new frame size.
The frame size was changed in stack frame #12, applying minimum height:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2527

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch 1

Take the current frame size into account.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems that this could change the result of GetScrolledRect unnecessarily/unexpectedly.

How about making nsListBoxBodyFrame::VerticalScroll/Layout set up the overflow area correctly before we call ScrollTo?

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch 2

I believe nsListBoxLayout::Layout is the "culprit", so I added it there.
I think this is the only caller of VerticalScroll during reflow so the
overflow rect should be correct in all other cases.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  friend class nsListBoxLayout;  // needs SetOverflowRect

Let's just make SetOverflowRect protected.

+      if (frame->HasOverflowRect()) {
+        nsRect overflowRect = nsRect(nsPoint(0, 0), frame->GetSize());
+        overflowRect.UnionRect(overflowRect, frame->GetOverflowRect());
+        frame->SetOverflowRect(overflowRect);

Using the old, invalid overflow rect seems bad.

Shouldn't this be in a post-reflow callback? The comments in bug 51084 suggest it used to be...

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 428360 [details] [diff] [review]
Patch 2

See comment #5.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch 3

You're comment implies we don't need to vertical scroll before
LayoutInternal?  If so, I think we can just remove this.
Looking at nsListBoxBodyFrame's reflow callback:
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsListBoxBodyFrame.cpp#446
it already does VerticalScroll, and checks for a row height
change afterward and if so schedules another reflow
with a possible scroll request (mAdjustScroll) -- which seems
to be what this block tries to fix, according to the comment.

The patch pass TryServer unit tests, fwiw.  I'm not
sure there are any test that covers the original problem
though (bug 51084).  I did run Firefox and Thunderbird
locally on OSX with the patch without seeing any problems.

Comment 8

[Mats Palmgren (inactive)]

s/You're/Your

Comment 9

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/24b8e061eff0
http://hg.mozilla.org/mozilla-central/rev/01f5e4105dae

Comment 10

[Mats Palmgren (inactive)]

Ouch, I committed the wrong patch for this bug...

Comment 11

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/bc3152ee4dbf
http://hg.mozilla.org/mozilla-central/rev/52efa25f3458