New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 808189 link

Starred by 9 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

Implement new logic for maximum number of fragments

Project Member Reported by chrishtr@chromium.org, Feb 1 2018

Issue description

Currently, it's possible to have a huge number of fragments, in cases
like this:

<div style="columns: 2; height: 10px; width: 10px">
  <div style="width: 10px; height: 10000000px"></div>
</div>

due to the very large height of the inner div. Currently,
we limit the maximum number of fragments to 500 (see kMaxNumFragments
in PaintPropertyTreeBuilder.cpp).

Proposal:

1. Limit to max(10, 100 * (per-column height) / (height of content under fragmentation container))

For cases like the above example, this would limit to just 10 columns But for
cases where there really need to be a a large number of columns, each column must
consume at least a certain percentage of the total height.

2. For cases where content exceeds the limits from #1, overflow into the last column.
 
Components: -Blink>Layout Blink>Layout>MultiCol
Cc: e...@chromium.org
The proposed formula [1] simply means that the number of columns will be the percentage of the total content that can be fit into one column. I.e. if a column can take 5% of the total content, you get to use 5 columns in total. If a column can take 20%, you get to use 20 columns. If a column can take 50%, you get to use 50 columns. But if one column can take 50% of the content, you'll typically only need 2 columns. :) So this is a bit backwards. The more content you have, the fewer columns you're allowed to create.

[1] 100 * (per-column height) / (height of content under fragmentation container)

Maybe just k * "per-column height"? This should work, because LayoutUnit is limited to 2^25 (approx 33 million pixels). With k=1.0, you'll never be able to get more than 5793 columns (2^12.5). With lower k values than 1.0, the maximum column count will be lower.
I agree, my formula didn't make much sense.

Your proposal is sort of like enforcing an aspect ratio, which sounds nice.
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/469db7dcdae3cea3a26ca5349cb89995bc96fc7c

commit 469db7dcdae3cea3a26ca5349cb89995bc96fc7c
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Fri Feb 02 21:32:30 2018

New logic for maximum number of fragments.

Instead of unconditionally clamping at 500 columns, try to identify legitimate
use cases for many columns, and allow those. At the same time, limit (what we
consider to be) illegitimate reasons even more brutally.

Bug:  808189 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Icf70d655c2da0b4a1af7e256cf7f3ef8c72f8715
Reviewed-on: https://chromium-review.googlesource.com/898438
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534158}
[add] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/LayoutTests/fast/multicol/column-clamping-expected.html
[add] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/LayoutTests/fast/multicol/column-clamping.html
[modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/LayoutTests/fast/multicol/scrollable-basic.html
[modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/layout/LayoutPagedFlowThread.cpp
[modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp
[modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h
[modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroupTest.cpp
[modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp

Status: Fixed (was: Assigned)

Comment 6 by e...@chromium.org, Apr 30 2018

Cc: sandeepkumars@chromium.org
 Issue 837994  has been merged into this issue.

Comment 7 by g...@kochaniak.com, Apr 30 2018

I do not understand this issue, and why it suddenly proposes to limit columns at some low number??? What are "illegitimate reasons" for such division? My reason is that WebView control is used to implement ebook reader-like interface with pages read by swiping sideways. Why at some page (currently around page 150) suddenly all text must be appended to the last column??? This is insane. There was never such limitation before and all worked beautifully, now you are breaking existing apps with your twisted "illegitimate reason" logic...

All other browsers I tested (Firefox on Android and Firefox and Microsoft Edge on Windows) do not limit the column number, only Chrome is problematic. I would not care if it was only Chrome browser, but you also broke Android WebView control and many apps that use it. Please do whatever you want with Chrome browser, but stop breaking Android WebView control!
Could you please describe an example where more than 500 columns make sense,
to help us understand your use case?

Comment 9 by g...@kochaniak.com, Apr 30 2018

It is not 500, but around 150. Please download my Android app, @Voice Aloud Reader (https://play.google.com/store/apps/details?id=com.hyperionics.avar), open some long text in it (or e.g. some long PDF). Press the T+/- icon on top, MODE tab, turn on "Divide text into pages, scroll horizontally" option. Now you can swipe pages sideways to read. Suddenly at some point (about screen 150, depending on font size) you'll stop seeing any text on pages, because all the remaining text was appended to the last visible column.

The app has been in Google Play for about 4 years, this change broke it. Again, do whatever you want with the browser, but please don't break the WebView control, or make Google provide an alternative stable WebView which is not bound to the wild changes in Chrome...

Comment 10 by g...@kochaniak.com, Apr 30 2018

Forgot to add a hint, if you want to test @Voice: to scroll faster sideways, as regular scrolling animates pages, make a pinching gesture like to zoom-out, then use the horizontal scroll bar that appears at the bottom. Email me directly for a no-ads license if you want easier testing without ads popping up every 30 minutes or banner ad at the bottom.
Is there any hope that you can FIX this fix? You are really breaking existing apps without any possibility to work-around the issue, and no alternatives to Android WebView control in sight. Maybe the fix could be to allow very high or unlimited columns in WebView control only, and limit it in the browser?

The only work-around I can think of would be to split original text into even smaller chunks, but it will not work right for all users. Read-aloud apps like @Voice tend to have a high percentage of visually impaired users or older users, who set very large font sizes for their reading apps, that would multiply the number of pages (columns), easily exceeding the very low limit you permit even for shorter chunks of text.
I think we need to increase the limit drastically, and also make sure that we don't detect "illegitimate reasons" unless they truly are illegitimate. A whole lot of text certainly *isn't* an illegitimate reason. <div style="height:1234567890px;"></div> with nothing inside *is* an illegitimate reason.
Please change the status of this bug. It is not FIXED. The fix causes serious problems in scenarios described above.
Cc: chrishtr@chromium.org
Status: Assigned (was: Fixed)
500 is too low. I think we need to base the column count allowance on the amount of content somehow. Sometimes you may have a legitimate reason to use a few thousand columns, just like there are legitimate reasons for a few thousand lines. The difference between lines and columns, though, is that for lines there's a rather linear correspondence between the amount of content and the number of lines you get (roughly: you'll never get more lines than the number of words that you have). Not so for columns. All it takes is a column height of 1px and an empty DIV child that's 1000000px tall. Bam! One million useless columns! That's the situation we want to avoid.
Cc: mstensho@chromium.org viswa.karala@chromium.org
 Issue 829744  has been merged into this issue.
A recent update changed the behavior of css columns. It went from calculating and applying width for everything (but only painting first 500) to only doing so for the first 500 and then render remaining vertically.
See video for visual example: https://bugs.chromium.org/p/chromium/issues/attachment?aid=334043&signed_aid=oZo51EyyvuLNMcI0htdR2Q==&inline=1

This was a breaking change for us, since we relied on the scroll width to work around pages above 500 not painting. Applying display: none to previous columns content while maintaining the scroll width by setting the container elements absolute left position property to the hidden elements width.

Now, since the scroll width never change when columns count >= 500 and remaining content is rendered vertically, the workaround now have to take scroll height into account.

Anyway, our users was suddenly unable to read past page 500 in our ePub reader due to this update.

Why does Blink have an opinion about column count? Other engines renders large amounts just fine.

500 columns are very easily reached on a smartphone screen rendering a book.

We are now moving away from loading the whole book and instead load one chapter at a time. But there is no guarantee that some book can't contain a chapter taking up more than 500 columns (small screens and users can adjust font size etc.). So please remove this limit.

Comment 17 by g...@kochaniak.com, May 14 2018

Thank you @mik... for your support here, till now it looked like I was the only one affected in the world. Here is a simple test web page to demo the issue

http://hyperionics.com/test/li.html

which I submitted with  Issue 837994 , now merged with this one. Maybe with with more developers and end users complaining, the Chrome gods will finally take us seriously...

Comment 18 by g...@kochaniak.com, May 14 2018

Also worth mentioning, multi-column support in Chrome and Android WebView is a disaster. There is another related issue, long sitting as "assigned" to some minor deity and forgotten, also happens only in Chrome, not other browsers: https://bugs.chromium.org/p/chromium/issues/detail?id=797591


Thank you for your feedback. I'm going to propose a fix for this today.

Just to be clear: This bug is about over-eager clamping of actual column-count, and nothing else.
One more side-effect for this. When rest of content is rendered in last column layer with content can become incredible large.

For example I have book that previously rendered into 700 colums. Now it will be rendered into 500 columns and content of 200 columns will be rendered into last. So layer with text content from (700 * screen_height) become (500 * 200 * screen_height). It's extremely large.


large-layer.png
20.3 KB View Download
One more side-effect for this. When rest of content is rendered in last column layer with content can become incredible large.

For example I have book that previously rendered into 700 colums. Now it will be rendered into 500 columns and content of 200 columns will be rendered into last. So layer with text content from (700 * screen_height) become (500 * 200 * screen_height). It's extremely large.


large-layer.png
20.3 KB View Download

Comment 22 by g...@kochaniak.com, May 15 2018

Indeed, thank you for pointing this out, @yavano! This may be the reason why I see now so many crashes related to "out of memory" condition in my app. I hope that the fix is implemented really soon and makes it rapidly into the full release version of Chrome/WebView on Android at least.
Project Member

Comment 23 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c7424a87fff7f5c4351719597eae8051e96fba3

commit 1c7424a87fff7f5c4351719597eae8051e96fba3
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Fri May 18 23:04:20 2018

Improve used column count allowance calculation.

Better detection of "legitimate" reasons to use many columns. Let the
allowance be based on the amount of "content". The more content (lines
or boxes) inside a multicol container, the more columns will be allowed.
We used to base it on the column height, but it turned out that it
wasn't good enough. Also, there are legitimate reasons to use more than
500 columns in some cases, so increase the limit to 2000. With the
current implementation, this may get very slow, because of quadratic
performance complexity, presumably somewhere in paint code.

One multicol regressed in LayoutNG. Not sure why. It shouldn't have
passed in the first place.

Bug:  808189 
Change-Id: Ia45d52b23f1246331d4b193839d34d7d9527151b
Reviewed-on: https://chromium-review.googlesource.com/1057629
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560088}
[modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/WebKit/LayoutTests/fast/multicol/scrollable-basic.html
[modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.cc
[modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.h
[modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.cc
[modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.h
[modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group_test.cc

Comment 24 Deleted

Please just remove the limit for legitimate columns. If a developer wants X columns shouldn't the browser try to do that, even if the result is taking a long time or even a crash? What does Firefox do?

Note that even a basic test like (attached) is currently broken in Chrome, but doesn't "overflow into vertical" in Firefox, Edge, and IE.

test.html
942 bytes View Download
Status: Fixed (was: Assigned)
Your test should pass with the latest fix, because each line box will effectively increase the column allowance by 1.

We need some limit for now, because time to lay out / invalidate / paint sometimes increases quadratically with the number of columns. That's  bug 797591 .

We can close *this* bug now, since I think all legitimate use cases for many columns will be allowed now. We still set a hard limit of 2000 columns, because 2000 * 2000 is ... a lot. :(
Project Member

Comment 27 by bugdroid1@chromium.org, May 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/215f05199a9205ed75f728ff6343e895c05b4025

commit 215f05199a9205ed75f728ff6343e895c05b4025
Author: calamity <calamity@chromium.org>
Date: Tue May 22 06:11:13 2018

Revert "Improve used column count allowance calculation."

This reverts commit 1c7424a87fff7f5c4351719597eae8051e96fba3.

Reason for revert: Causing flakes on MultiColumnFragmentainerGroupTest.LotsOfContent.
See  https://crbug.com/845155 

Original change's description:
> Improve used column count allowance calculation.
> 
> Better detection of "legitimate" reasons to use many columns. Let the
> allowance be based on the amount of "content". The more content (lines
> or boxes) inside a multicol container, the more columns will be allowed.
> We used to base it on the column height, but it turned out that it
> wasn't good enough. Also, there are legitimate reasons to use more than
> 500 columns in some cases, so increase the limit to 2000. With the
> current implementation, this may get very slow, because of quadratic
> performance complexity, presumably somewhere in paint code.
> 
> One multicol regressed in LayoutNG. Not sure why. It shouldn't have
> passed in the first place.
> 
> Bug:  808189 
> Change-Id: Ia45d52b23f1246331d4b193839d34d7d9527151b
> Reviewed-on: https://chromium-review.googlesource.com/1057629
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Reviewed-by: Emil A Eklund <eae@chromium.org>
> Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#560088}

TBR=chrishtr@chromium.org,eae@chromium.org,mstensho@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  808189 ,  845155 
Change-Id: I28bf3894c4b65a75c7c533a362ca58f3c7e29973
Reviewed-on: https://chromium-review.googlesource.com/1068587
Reviewed-by: calamity <calamity@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560496}
[modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/WebKit/LayoutTests/fast/multicol/scrollable-basic.html
[modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.cc
[modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.h
[modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.cc
[modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.h
[modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group_test.cc

Comment 28 by g...@kochaniak.com, May 22 2018

What does the above post 'Revert "Improve used column count allowance calculation."' mean? Was the fix reverted and we're still stuck with all text appended to certain column after some low limit? In my tests this low limit was not 500, but about 150 screens of text. Please explain the situation, I'm very worried now.
Yes, it was reverted, because the tests included caused timeouts on some test bots. I've uploaded a new patch with less demanding tests, and I hope we'll be able to re-land this today or tomorrow.

You can follow the progress here, if you like: https://chromium-review.googlesource.com/c/chromium/src/+/1068929
Project Member

Comment 30 by bugdroid1@chromium.org, May 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d123c14a79589cb40efc367a178213a0d246f06a

commit d123c14a79589cb40efc367a178213a0d246f06a
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Tue May 22 22:47:14 2018

Reland: Improve used column count allowance calculation.

Better detection of "legitimate" reasons to use many columns. Let the
allowance be based on the amount of "content". The more content (lines
or boxes) inside a multicol container, the more columns will be allowed.
We used to base it on the column height, but it turned out that it
wasn't good enough. Also, there are legitimate reasons to use more than
500 columns in some cases, so increase the limit to 2000. With the
current implementation, this may get very slow, because of quadratic
performance complexity, presumably somewhere in paint code.

One multicol regressed in LayoutNG. Not sure why. It shouldn't have
passed in the first place.

This CL was previously landed as
https://chromium-review.googlesource.com/c/chromium/src/+/1057629 and
then reverted because of test flakiness. This new CL has reduced the
number of columns in those tests, so that the tests don't occasionally
time out on slow bots.

Bug:  808189 , 845155 
Change-Id: I55d560faba73128518a689ae38f628ee89e58568
Reviewed-on: https://chromium-review.googlesource.com/1068929
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560820}
[modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/WebKit/LayoutTests/fast/multicol/scrollable-basic.html
[modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.cc
[modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.h
[modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.cc
[modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.h
[modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group_test.cc

Okay, the fix is in again. Hope it sticks this time.

Comment 32 by g...@kochaniak.com, May 23 2018

Great, thank you! I already see the fix working in my test when I set WebView to Chrome Canary channel!
Project Member

Comment 33 by bugdroid1@chromium.org, Jan 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/64752ee4b1b93f069b2d64a4327af81319f45191

commit 64752ee4b1b93f069b2d64a4327af81319f45191
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Wed Jan 02 16:21:50 2019

Less restrictive internal column count limit.

Just allow up to 10000 columns unconditionally.

We used to have severe performance problems with many columns (O(n^2)),
but that should be long gone, thanks to
https://chromium-review.googlesource.com/c/chromium/src/+/1070562/

This is essentially a revert of
https://chromium-review.googlesource.com/c/chromium/src/+/1068929/
(and earlier clamping attempts).

Bug:  918112 ,  808189 
Change-Id: I3c767fa4f2da5197d1a02c8f4ad89fbc35c5614d
Reviewed-on: https://chromium-review.googlesource.com/c/1392955
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619413}
[modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.cc
[modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.h
[modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.cc
[modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.h
[modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group_test.cc
[modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
[modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/web_tests/TestExpectations
[add] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/web_tests/external/wpt/css/css-multicol/large-actual-column-count.html
[delete] https://crrev.com/702131a0cbed54f73277fe87b544bcc9f74f40b9/third_party/blink/web_tests/fast/multicol/column-clamping-expected.html
[delete] https://crrev.com/702131a0cbed54f73277fe87b544bcc9f74f40b9/third_party/blink/web_tests/fast/multicol/column-clamping.html

Sign in to add a comment