730559 - Another -moz-column hang in nsColumnSetFrame::Reflow

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (hangs Firefox when loaded)

Seems different from bug 458659.

Comment 1

[Mats Palmgren (inactive)]

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.

Comment 2

[Scott Johnson (:jwir3)]

Attachment: b730559

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.

Comment 3

[Mats Palmgren (inactive)]

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/

Comment 4

[Mats Palmgren (inactive)]

Please also check the other column layout hang bugs we have open.
I expect most of them will be fixed by this patch.

Comment 5

[Scott Johnson (:jwir3)]

(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.

Comment 6

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

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?)

Comment 7

[Scott Johnson (:jwir3)]

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.

Comment 8

[Scott Johnson (:jwir3)]

(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.

Comment 9

[Scott Johnson (:jwir3)]

(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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34af515fb6e6