398215 - Hang in nsStackLayout::Layout with display: -moz-stack, direction: rtl and display: list-item;

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which hangs current trunk build on load.
The testcase consists of this:
<hx style="display: -moz-stack;direction: rtl;">
<b style="display: list-item;">
<span>
<param>t

This regressed between 2006-01-19 and 2006-01-21:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-01-19+04&maxdate=2006-01-21+09&cvsroot=%2Fcvsroot
No idea what caused it, maybe bug 310638?

Comment 1

[Daniel Holbert [:dholbert]]

The hang seems triggers tons and tons of copies of this error output:

###!!! ASSERTION: bad width: 'Not Reached', file /scratch/work/builds/trunk.07-10-17.13-07/mozilla/layout/generic/nsLineLayout.cpp, line 176
Block(b)(1)@0x893f53c: Init: bad caller: width WAS 500736(0x7a400)
###!!! ASSERTION: bad width: 'Not Reached', file /scratch/work/builds/trunk.07-10-17.13-07/mozilla/layout/generic/nsLineLayout.cpp, line 176
Block(b)(3)@0x893f9f0: Init: bad caller: width WAS 500736(0x7a400)
###!!! ASSERTION: bad width: 'Not Reached', file /scratch/work/builds/trunk.07-10-17.13-07/mozilla/layout/generic/nsLineLayout.cpp, line 176
Block(b)(3)@0x893f9f0: Init: bad caller: width WAS 501600(0x7a760)
###!!! ASSERTION: bad width: 'Not Reached', file /scratch/work/builds/trunk.07-10-17.13-07/mozilla/layout/generic/nsLineLayout.cpp, line 176
Block(b)(1)@0x893f53c: Init: bad caller: width WAS 501600(0x7a760)
###!!! ASSERTION: bad width: 'Not Reached', file /scratch/work/builds/trunk.07-10-17.13-07/mozilla/layout/generic/nsLineLayout.cpp, line 176
....

Two interesting things to notice:
 - Each consecutive pair of "Block(b)" messages have the same size
 - The difference in size from one "Block(b)" message-pair to the next is 864, which is 32 * 27.

Comment 2

[Daniel Holbert [:dholbert]]

We're looping forever in nsStackLayout::Layout.

Link to source: http://mxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsStackLayout.cpp#249

This function basically has a 
   do { ... child->Layout(); ... } while (grow);
loop, and the "grow" variable is turned on every time because the child grows in size by 864 each time we call Layout (and perform the reflows that that triggers).

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (hangs branch as well)

This testcase hangs branch *and* trunk.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch v1 (first pass...)

(This patch fixes the hang, although it's not yet finalized.)

Comparing the RTL case with the LTR case, here's what I've discovered.   At the point in the code affected with the patch, we originally did this:
  1. We set aDesiredSize.width to "XMost()" of the overflow area.
  2. If this new aDesiredSize.width is GREATER than the original allowable width of the block, we do an additional reflow (and other tasks) with an increased desired size to address any discrepancy.

However, the use of "XMost()" here isn't really fair (or correct?) when bullets are concerned.  

Bullets are offset a bit from the block, which means the following:
 - In the LTR case, the overflow "x" value is *Negative* and so XMost() actually returns a negative value, outside the block on the *left* side. This negative value will clearly be less than the block's width, so we don't do the additional reflow.
 - In the RTL case, the overflow "x" value is at the right edge of the block, and XMost() will put us outside of the block on the *right* side, at a positive value GREATER than the block's width.  So we *do* do the additional reflow in this case. (with an increased desired size)

This discrepancy is (part of) what's causing the infinite loop in the RTL case.

I'm not yet sure what we really want to do at this point in the code.  But it definitely seems like it's wrong to have a whole bunch of extra reflow code conditioned on whether the overflow area's XMost() is less than a width value, because that does extra work for right-side overflow while ignoring left-side overflow.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: patch v2

I guess the "unfairness" I described is partly due to what this comment here describes:

6248 // This kinda sucks. We should be able to handle the case
6249 // where there's overflow above or to the left of the
6250 // origin. But for now just chop that stuff off.

So, we were behaving differently for RLT bullets because (as this comment says) we always chop off overflow to the left, but we do some work to try to absorb overflow to the right.

This new patch "equalizes" that by chopping off left-overflow for LTR regions, and chopping off right-overflow for RTL regions, thereby making us behave more symmetrically.


This patch also removes these unnecessary bits of code:

 -- "if (aDesiredSize.width > aWidth)", which must always be satisfied because of the "else" clause that it's the first line of.

 -- the "changedSize" variable, which is set but never used

Comment 7

[Daniel Holbert [:dholbert]]

(patch v2 passes reftests, too)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch v2 (ignore whitespace version)

Comment 9

[Daniel Holbert [:dholbert]]

patch v2 checked in:

/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.764; previous revision: 3.763

Comment 10

[Martijn Wargers (dead)]

Thanks for fixing this, Daniel!

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110705 Minefield/3.0a9pre

Comment 11

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).