Closed Bug 370872 Opened 14 years ago Closed 13 years ago

29826174px wide page with colspan and marquee

Categories

(Core :: Layout: Tables, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 363858

People

(Reporter: jruderman, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached file 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.
Works in 2006-12-07, fails in 2006-12-08, so probably a regression from bug 300030.
Flags: blocking1.9?
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.
Attached file 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.
Flags: blocking1.9? → blocking1.9+
Depends on: 363722
OS: Mac OS X → All
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.
Depends on: 367673
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
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.)
Blocks: 393656
Just checked in the fix for bug 367673, which fixes first testcase, but not the second and third.
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 363858
Flags: blocking1.9+
Flags: blocking1.9+
You need to log in before you can comment on or make changes to this bug.