Closed Bug 730559 Opened 8 years ago Closed 7 years ago

Another -moz-column hang in nsColumnSetFrame::Reflow

Categories

(Core :: Layout, defect, P2, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jruderman, Assigned: jwir3)

References

(Blocks 1 open bug)

Details

(Keywords: hang, testcase)

Attachments

(2 files)

Seems different from bug 458659.
Severity: normal → critical
Assignee: nobody → sjohnson
Priority: -- → P2
This hang still occurs.  It appears we create a huge number of columns.

(gdb) fr
#7  0x00007ffff1ba26df in nsColumnSetFrame::ReflowChildren 
(gdb) p columnCount
$1 = 15663

In the style system, we limit column count to 1000:
http://hg.mozilla.org/mozilla-central/annotate/3a5929ebc886/layout/style/nsRuleNode.cpp#l7106

I think we need to implement that limit for the auto case too,
in nsColumnSetFrame layout.
OS: Mac OS X → All
Hardware: x86 → All
Attached patch b730559Splinter Review
This patch fixes the issue by limiting the number of columns to 1000 in ChooseColumnStrategy, but I did it in a way that utilizes a variable in nsLayoutUtils (I originally put it in nsColumnSetFrame, but I didn't want to include nsColumnSetFrame.h in nsRuleNode.cpp) for consistency between these two locations. 

If you think it should go somewhere else, just let me know & I can change it. I also toyed with the idea of just making it a #define.
Attachment #753714 - Flags: review?(matspal)
Comment on attachment 753714 [details] [diff] [review]
b730559

>layout/base/nsLayoutUtils.h
>+  static uint32_t sMaxNumColumns;

I'd prefer this as a const, with a name that associates it with
'mColumnCount', for example:
  static const uint32_t kMaxColumnCount = 1000;

It seems logical to put it in 'struct nsStyleColumn' (nsStyleStruct.h)
and you can just initialize it directly there.

>layout/generic/nsColumnSetFrame.cpp
>+        // The number of columns should never exceed 1000.

s/1000/kMaxColumnCount/

>layout/style/nsRuleNode.cpp
>     // Max 1000 columns - wallpaper for bug 345583.

s/1000/kMaxColumnCount/
Attachment #753714 - Flags: review?(matspal) → review+
Please also check the other column layout hang bugs we have open.
I expect most of them will be fixed by this patch.
(In reply to Mats Palmgren [:mats] from comment #4)
> Please also check the other column layout hang bugs we have open.
> I expect most of them will be fixed by this patch.

Will do. Thanks.
I think there's a set of cases where it's reasonable to use a very large number of columns:  if columns are essentially being used as overflow within a fixed-height container.  This doesn't break that, right?  (And we don't do column balancing in cases like that, right?)
Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34af515fb6e6

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #6)
> I think there's a set of cases where it's reasonable to use a very large
> number of columns:  if columns are essentially being used as overflow within
> a fixed-height container.  This doesn't break that, right?  
Let me double-check, but I don't think this should affect that.

> (And we don't do column balancing in cases like that, right?)
We don't do column balancing in overflow columns, so this statement is correct.
(In reply to Scott Johnson (:jwir3) from comment #7)
> Inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/34af515fb6e6
> 
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #6)
> > I think there's a set of cases where it's reasonable to use a very large
> > number of columns:  if columns are essentially being used as overflow within
> > a fixed-height container.  This doesn't break that, right?  
> Let me double-check, but I don't think this should affect that.

Yeah, so as I suspected, when the column-fill property is set to 'auto', or we exceed the computed height of our column set frame when balancing, we set the column count to INT32_MAX. (Anytime ChooseColumnStrategy() is called, but isBalancing is false) Basically, this ensures that we're not going to limit the column count if we have overflow columns.
(In reply to Scott Johnson (:jwir3) from comment #8)
> (In reply to Scott Johnson (:jwir3) from comment #7)
> > Inbound:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/34af515fb6e6
> > 
> > (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> > comment #6)
> > > I think there's a set of cases where it's reasonable to use a very large
> > > number of columns:  if columns are essentially being used as overflow within
> > > a fixed-height container.  This doesn't break that, right?  
> > Let me double-check, but I don't think this should affect that.
> 
> Yeah, so as I suspected, when the column-fill property is set to 'auto', or
> we exceed the computed height of our column set frame when balancing, we set
> the column count to INT32_MAX. (Anytime ChooseColumnStrategy() is called,
> but isBalancing is false) Basically, this ensures that we're not going to
> limit the column count if we have overflow columns.

BTW - I verified this both by looking at the code, and changing the value of kMaxColumnCount to '2' and ensuring that it actually runs as expected in the case you described.
https://hg.mozilla.org/mozilla-central/rev/34af515fb6e6
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.