370872 - 29826174px wide page with colspan and marquee

Status: RESOLVED DUPLICATE of bug 363858

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Steps to reproduce:
1. Load the testcase.

Result: The page's width is 29826174px (about 2^32/144) and a bunch of width-related assertion failures occur.

Expected: There should be no horizontal scrollbar, and no assertion failures.

Comment 1

[Martijn Wargers (dead)]

Works in 2006-12-07, fails in 2006-12-08, so probably a regression from bug 300030.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think the basic problem is that there are three fundamental intrinsic widths rather than two.  There are really two variants of pref width, one (the wider one) that is allowed to become infinite, and the other (the narrower one) that does not include expansion based on reverse computation of percentages.  Shrink-wrapping uses the wider one, but table cell containment and XUL pref width use the narrower one.  That said, there are advantages to using the wider one whenever possible since it means that things with percentage widths will fit.

Right now the only place where we distinguish between the two is BasicTableLayoutStrategy, so that we can use the wider one for shrink-wrapping of tables.  But we might need to do the same thing for other places where we do percent-expansion.

If we need to do that, I think I want to fix what I sort of consider a design mistake with the reflow branch, and combine GetMinWidth and GetPrefWidth into a single GetIntrinsicWidth function that takes a parameter for which intrinsic width to get.  It shouldn't be too hard to combine the existing implementations -- most places that override one override both.  It would also make switching into and out of nsLayoutUtils::IntrinsicForContainer simpler.  Then we can add a third enum value and make the necessary distinctions based on that enum value.  I suppose this means we should start caching all three values in nsBlockFrame as well, though.


That said, this analysis of the bug might be entirely wrong.  I'm not sure why a colspan is necessary.  I'd have expected marquees (with their 100% margins internally) to expand tables all the time if the do so at all.  So I'm clearly missing something.

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase2

This is also giving a very wide page. This also regressed during the reflow branch landing. I minimised it from a nested marquee testcase.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Related to bug 370866?

Comment 5

[Daniel Holbert [:dholbert]]

The initial testcase is fixed (no assertions, no longer really really wide) by my patch for bug 367673 (which isn't yet checked in -- awaiting review).  That patch basically prevents certain nscoord operations from overflowing nscoord_MAX.  

Having said that, the Trunk rendering using that patch is still different from Branch rendering -- patched Trunk makes the table take up the whole width of the window, whereas Branch makes the table very skinny.  So there might still be something else broken; I'm not sure.

Comment 6

[Daniel Holbert [:dholbert]]

I did some analysis on the second testcase, and it looks like we're getting a width of nscoord_MAX via this chain of calls:

#0  AddPercents (aType=nsLayoutUtils::PREF_WIDTH, aCurrent=2820, aPercent=1)
       nsLayoutUtils.cpp:1249
#1  nsLayoutUtils::IntrinsicForContainer (aRenderingContext=0x8c228c0, aFrame=0x8ac4ab0, aType=nsLayoutUtils::PREF_WIDTH)
       nsLayoutUtils.cpp:1548
#2  nsBlockFrame::GetPrefWidth (this=0x8ac4904, aRenderingContext=0x8c228c0)
       nsBlockFrame.cpp:729
#3  nsFrame::RefreshSizeCache (this=0x8ac4904, aState=@0xbfab6d44)
       nsFrame.cpp:5712
#4  nsFrame::GetMinSize (this=0x8ac4904, aState=@0xbfab6d44)
       nsFrame.cpp:5829
#5  nsSprocketLayout::GetMinSize (this=0x84c3510, aBox=0x8ac47bc, aState=@0xbfab6d44, aSize=@0xbfab6d64)
       nsSprocketLayout.cpp:1393
#6  nsBoxFrame::GetMinSize (this=0x8ac47bc, aBoxLayoutState=@0xbfab6d44)
       nsBoxFrame.cpp:862
#7  nsBoxFrame::GetMinWidth (this=0x8ac47bc, aRenderingContext=0x8c228c0)
       nsBoxFrame.cpp:630
...
#20 nsBlockFrame::Reflow (this=0x8ac462c, aPresContext=0x8915480, aMetrics=@0xbfab7de8, aReflowState=@0xbfab7d14, aStatus=@0xbfab7efc)
       nsBlockFrame.cpp:921

Stack level #20 is a reflow of the body block.

The nsBoxFrame "this" at levels #7 and #6 is the outer -moz-box div.  The nsBlockFrame "this" at #4, #3, and #2 is the intermediate div between the two -moz-box divs.  The aFrame at #1 is the inner -moz-box div.


At level #0, nsLayoutUtils.cpp:1249, we hit this clause:
  1248    if (aPercent >= 1.0f)
  1249      result = nscoord_MAX;

This gets returned up through stack level #2, setting the intermediate div's "mPrefWidth" to nscoord_MAX.  

We then pass the nscoord_MAX up to level #3, setting metrics->mBlockPrefSize.width to nscoord_MAX.

Then we call BoxReflow at nsFrame.cpp:5717, passing in nscoord_MAX as "aWidth",  This is incorporated into parentSize.width which is used to initialize parentReflowState at nsFrame.cpp:6032, and becomes "availableWidth" in that reflowState.  

Finally, when parentReflowState calls Init() in its constructor, we detect that availableWidth == NS_UNCONSTRAINEDSIZE, and the first assertion fails:
  ASSERTION: shouldn't use unconstrained widths anymore: 'availableWidth != NS_UNCONSTRAINEDSIZE', file /scratch/work/builds/trunk.07-09-07.12-28/mozilla/layout/generic/nsHTMLReflowState.cpp, line 302

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase3 (using <select>)

This third testcase has the same sorts of assertions as testcase 2 (unconstrained withs in nsHTMLReflowState), and it has a similar root cause:  IntrinsicForContainer calling AddPercents() with aPercent >= 1, and getting nscoord_MAX returned.

Here's the backtrace for where nscoord_MAX width gets introduced on testcase3:

#0  AddPercents (aType=nsLayoutUtils::PREF_WIDTH, aCurrent=480, aPercent=2)
      at nsLayoutUtils.cpp:1249
#1  nsLayoutUtils::IntrinsicForContainer (aRenderingContext=0x8c808b0, aFrame=0x9067f28, aType=nsLayoutUtils::PREF_WIDTH)
      at nsLayoutUtils.cpp:1548
#2  nsBlockFrame::GetPrefWidth (this=0x90658a4, aRenderingContext=0x8c808b0)
      at nsBlockFrame.cpp:729
#3  nsHTMLScrollFrame::GetPrefWidth (this=0x90657e0, aRenderingContext=0x8c808b0)
      at nsGfxScrollFrame.cpp:674
#4  nsComboboxControlFrame::GetPrefWidth (this=0x906562c, aRenderingContext=0x8c808b0)
      at nsComboboxControlFrame.cpp:598
#5  nsComboboxControlFrame::GetMinWidth (this=0x906562c, aRenderingContext=0x8c808b0)
      at nsComboboxControlFrame.cpp:571
#6  nsFrame::ShrinkWidthToFit (this=0x906562c, aRenderingContext=0x8c808b0, aWidthInCB=24180)
      at nsFrame.cpp:3088
#7  nsContainerFrame::ComputeAutoSize (this=0x906562c, aRenderingContext=0x8c808b0, aCBSize=@0xbfc10780, aAvailableWidth=25680, aMargin=@0xbfc10778, aBorder=@0xbfc10770, aPadding=@0xbfc10768, aShrinkWrap=1)
      at nsContainerFrame.cpp:673
#8  nsFrame::ComputeSize (this=0x906562c, aRenderingContext=0x8c808b0, aCBSize=@0xbfc10824, aAvailableWidth=25680, aMargin=@0xbfc1081c, aBorder=@0xbfc10814, aPadding=@0xbfc1080c, aShrinkWrap=1)
      at nsFrame.cpp:2966
#9  nsHTMLReflowState::InitConstraints (this=0xbfc108f8, aPresContext=0x8e386f0, aContainingBlockWidth=25680, aContainingBlockHeight=1073741824, aBorder=0x0, aPadding=0x0)
      at nsHTMLReflowState.cpp:1820
#10 nsHTMLReflowState::Init (this=0xbfc108f8, aPresContext=0x8e386f0, aContainingBlockWidth=-1, aContainingBlockHeight=-1, aBorder=0x0, aPadding=0x0)
      at nsHTMLReflowState.cpp:315
#11 nsHTMLReflowState (this=0xbfc108f8, aPresContext=0x8e386f0, aParentReflowState=@0xbfc11664, aFrame=0x906562c, aAvailableSpace=@0xbfc109e4, aContainingBlockWidth=-1, aContainingBlockHeight=-1, aInit=1)
      at nsHTMLReflowState.cpp:180
#12 nsLineLayout::ReflowFrame (this=0xbfc10bfc, aFrame=0x906562c, aReflowStatus=@0xbfc10af4, aMetrics=0x0, aPushedFrame=@0xbfc10af0)
      at nsLineLayout.cpp:812
...

(Note: This testcase is reduced from bug 393656, but I thought I'd post it here since it's more related to this bug's assertions + issues.)

Comment 8

[Daniel Holbert [:dholbert]]

Just checked in the fix for bug 367673, which fixes first testcase, but not the second and third.

Comment 9

[Daniel Holbert [:dholbert]]

See also bug 363858.

Comment 10

[Daniel Holbert [:dholbert]]

Resolving as dupe of bug 363858, because testcases 2 and 3 are based on that bug's issue (padding causing exponential growth).  They aren't related to this bug's title ("colspan and marquee") or original problem.