Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1405393002:
Crashes when percent calculation of row's height goes very high. (Closed)
DescriptionCrashes when percent calculation of row's height goes very high.
When calculating the percent value of the row becomes negative because
percent calculation goes beyond integer limit So height of the row does
not increase due to negative value considered as 0. In this case extra
rowspanning height remains same and after that no other row present to
consume remaining extra row spanning height. So assertion fails.
Now, While calculating the percent height of the row, percent would be considered
max 100.
R=jchaffraix@chromium.org
BUG=408066
Committed: https://crrev.com/4a095d9e3d1e01175ef2c757d686a350bf3d88fc
Cr-Commit-Position: refs/heads/master@{#370924}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Review comments addressed #
Total comments: 20
Patch Set 3 : Review comments addressed #
Total comments: 6
Patch Set 4 : Rebase expected file #Patch Set 5 : Review comments addressed #
Total comments: 5
Messages
Total messages: 37 (12 generated)
a.suchit@chromium.org changed reviewers: + a.suchit@chromium.org
Layout test remain to add. I will add on Monday.
The CQ bit was checked by a.suchit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405393002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405393002/1
a.suchit@chromium.org changed reviewers: + dsinclair@chromium.org, eseidel@chromium.org, esprehn@chromium.org, pdr@chromium.org
jchaffraix@chromium.org changed reviewers: + mstensho@opera.com - a.suchit@chromium.org, eseidel@chromium.org, esprehn@chromium.org, pdr@chromium.org
Trimming the reviewer list (some are not working on Blink anymore). Please only include only *relevant* reviewers and ideally set your expectations for what you expect from people. FYI a patch without a test seems like it's not finished and thus would be better off being finished before asking for review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you please proofread and improve the description?
The first line ("RowSpan cell's percent height more than 100.") needs an
overhaul to be meaningful.
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:317: // those total
percent is 100. Other percent rows would be uneffected even extra spanning
height is remain.
I have trouble with my overall understanding of this method. This comment seems
wrong. It doesn't stop at 100%
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:323: float
maxPercent = 100.0;
I see no need for |maxPercent|. Just put 100.0 in the expression below.
If I were to introduce a local variable, it'd be for
m_grid[row].logicalHeight.percent(), which is used twice here.
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:325:
std::min(m_grid[row].logicalHeight.percent(), maxPercent) / 100)
Say, shouldn't you really divide by |totalPercent| and not 100 here? Especially
since we distribute the extra height over |totalPercent|%, which may be more
than 100, as far as I can tell.
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:327: // FIXME: Note
that this is wrong if we have a percentage above 100% and may make us grow
I guess this would no longer be the case, and that we'd behave correctly, even
with percent > 100?
a.suchit@chromium.org changed reviewers: + a.suchit@chromium.org
Thanks for the review. Please check my comments. https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:317: // those total percent is 100. Other percent rows would be uneffected even extra spanning height is remain. On 2015/10/16 18:19:13, mstensho wrote: > I have trouble with my overall understanding of this method. This comment seems > wrong. It doesn't stop at 100% line 320, 'percent > 0' have condition for stopping the further row. and percent value is modified in line 333. https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:323: float maxPercent = 100.0; On 2015/10/16 18:19:13, mstensho wrote: > I see no need for |maxPercent|. Just put 100.0 in the expression below. > > If I were to introduce a local variable, it'd be for > m_grid[row].logicalHeight.percent(), which is used twice here. 100.0 can not be used directly inside std::min because it is considered as double and value return by m_grid[row].logicalHeight.percent() is float and std::min gives compilation error. https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:325: std::min(m_grid[row].logicalHeight.percent(), maxPercent) / 100) On 2015/10/16 18:19:13, mstensho wrote: > Say, shouldn't you really divide by |totalPercent| and not 100 here? Especially > since we distribute the extra height over |totalPercent|%, which may be more > than 100, as far as I can tell. If we are using the |totalPercent| in place of 100 then row height is not based on table height percent and we are consuming the whole |extraRowSpanningHeight| height in percent cells. It means that whole |extraRowSpanningHeight| height distributed in percent rows based on their ratio. https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:327: // FIXME: Note that this is wrong if we have a percentage above 100% and may make us grow On 2015/10/16 18:19:13, mstensho wrote: > I guess this would no longer be the case, and that we'd behave correctly, even > with percent > 100? Yes, we can remove it.
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:317: // those total percent is 100. Other percent rows would be uneffected even extra spanning height is remain. On 2015/10/19 10:01:04, a.suchit2 wrote: > On 2015/10/16 18:19:13, mstensho wrote: > > I have trouble with my overall understanding of this method. This comment > seems > > wrong. It doesn't stop at 100% > > line 320, 'percent > 0' have condition for stopping the further row. > and percent value is modified in line 333. Yeah, but what if the initial value of |percent| is 5000, for instance? https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:323: float maxPercent = 100.0; On 2015/10/19 10:01:04, a.suchit2 wrote: > On 2015/10/16 18:19:13, mstensho wrote: > > I see no need for |maxPercent|. Just put 100.0 in the expression below. > > > > If I were to introduce a local variable, it'd be for > > m_grid[row].logicalHeight.percent(), which is used twice here. > > 100.0 can not be used directly inside std::min because it is considered as > double and value return by m_grid[row].logicalHeight.percent() is float and > std::min gives compilation error. So float(100.0) should work, then.
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:317: // those total percent is 100. Other percent rows would be uneffected even extra spanning height is remain. On 2015/10/19 18:17:47, mstensho wrote: > On 2015/10/19 10:01:04, a.suchit2 wrote: > > On 2015/10/16 18:19:13, mstensho wrote: > > > I have trouble with my overall understanding of this method. This comment > > seems > > > wrong. It doesn't stop at 100% > > > > line 320, 'percent > 0' have condition for stopping the further row. > > and percent value is modified in line 333. > > Yeah, but what if the initial value of |percent| is 5000, for instance? yes, I am fixing same scenario here. But I should use remaining percent in place of 100 in line 323. https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:323: float maxPercent = 100.0; On 2015/10/19 18:17:47, mstensho wrote: > On 2015/10/19 10:01:04, a.suchit2 wrote: > > On 2015/10/16 18:19:13, mstensho wrote: > > > I see no need for |maxPercent|. Just put 100.0 in the expression below. > > > > > > If I were to introduce a local variable, it'd be for > > > m_grid[row].logicalHeight.percent(), which is used twice here. > > > > 100.0 can not be used directly inside std::min because it is considered as > > double and value return by m_grid[row].logicalHeight.percent() is float and > > std::min gives compilation error. > > So float(100.0) should work, then. ok I will do it but I am not sure that it is acceptable or not.
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:317: // those total percent is 100. Other percent rows would be uneffected even extra spanning height is remain. On 2015/10/20 05:15:07, a.suchit2 wrote: > On 2015/10/19 18:17:47, mstensho wrote: > > On 2015/10/19 10:01:04, a.suchit2 wrote: > > > On 2015/10/16 18:19:13, mstensho wrote: > > > > I have trouble with my overall understanding of this method. This comment > > > seems > > > > wrong. It doesn't stop at 100% > > > > > > line 320, 'percent > 0' have condition for stopping the further row. > > > and percent value is modified in line 333. > > > > Yeah, but what if the initial value of |percent| is 5000, for instance? > > yes, I am fixing same scenario here. But I should use remaining percent in place > of 100 in line 323. I think you should divide by |totalPercent|, but I think it's time for you to upload another patch, and we'll take it from there.
Description was changed from ========== RowSpan cell's percent height more than 100. When calculating the percent value of the row and percent value is too high than value becomes negative. So height of the row does not increase because calculated value is negative and it is considered 0. so extra rowspanning height remains same and after that no other row present to consume remainning extra row spanning height. In this case assertion fails. Now, While calculating the percent height of the row, percent would be considered max 100. R=jchaffraix@chromium.org BUG=408066 ========== to ========== Crashes when RowSpan cell's percent height is too high. When calculating the percent value of the row and percent value is too high than value becomes negative. So height of the row does not increase because calculated value is negative and it is considered 0. so extra rowspanning height remains same and after that no other row present to consume remainning extra row spanning height. In this case assertion fails. Now, While calculating the percent height of the row, percent would be considered max 100. R=jchaffraix@chromium.org BUG=408066 ==========
Description was changed from ========== Crashes when RowSpan cell's percent height is too high. When calculating the percent value of the row and percent value is too high than value becomes negative. So height of the row does not increase because calculated value is negative and it is considered 0. so extra rowspanning height remains same and after that no other row present to consume remainning extra row spanning height. In this case assertion fails. Now, While calculating the percent height of the row, percent would be considered max 100. R=jchaffraix@chromium.org BUG=408066 ========== to ========== Crashes when percent calculation of row's height goes very high. When calculating the percent value of the row becomes negative because percent calculation goes beyond integer limit So height of the row does not increase due to negative value considered as 0. In this case extra rowspanning height remains same and after that no other row present to consume remaining extra row spanning height. So assertion fails. Now, While calculating the percent height of the row, percent would be considered max 100. R=jchaffraix@chromium.org BUG=408066 ==========
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:660: if (totalPercent < 100 && !totalAutoRowsHeight && !totalRemainingRowsHeight) { > I think you should divide by |totalPercent|, but I think it's time for you to > upload another patch, and we'll take it from there. Yes, when auto height and remaining height are 0 then divide by |totalPercent| should be used. So changes are required here. Condition should be : if (totalPercent && !totalAutoRowsHeight && !totalRemainingRowsHeight) {
On 2015/10/30 01:43:50, a.suchit2 wrote: > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): > > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:660: if > (totalPercent < 100 && !totalAutoRowsHeight && !totalRemainingRowsHeight) { > > I think you should divide by |totalPercent|, but I think it's time for you to > > upload another patch, and we'll take it from there. > > Yes, when auto height and remaining height are 0 then divide by |totalPercent| > should be used. > So changes are required here. Condition should be : > if (totalPercent && !totalAutoRowsHeight && !totalRemainingRowsHeight) { @mstensho, Please check my comment and review it. Thanks
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:1: <!DOCTYPE html> Expectation file missing. Right now this looks like a crash test, so you should add <script> if (window.testRunner) testRunner.dumpAsText(); </script> ... and store and upload the -expected.txt file. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:7: <h3>Test for chromium bug : <a href="https://code.google.com/p/chromium/issues/detail?id=408066">408066</a>. Page is crashing on assression when table row span cell's percent height is too height.</h3> Please wrap this line and proof-read the text. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:11: <td height="876543210" rowspan="2">This row span cell height is too height.</td> "too large" https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:13: <tr id="row-height"> Two elements with the same ID. Maybe use .class instead of #id? https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:307: LayoutTableCell* cell, float totalPercent, int& extraRowSpanningHeight, Please put this back on one line. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:326: std::min(m_grid[row].logicalHeight.percent(), percent) / 100) OK, so what you want to avoid here is integer overflow. How about using an intermediary LayoutUnit here, which does saturated arithmetic? We certainly don't want toAdd to be 0, which is the result of your changes. I'm talking about changing toAdd from int to LayoutUnit. Alternatively, change it from int to float. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:660: if (totalPercent < 100 && !totalAutoRowsHeight && !totalRemainingRowsHeight) { On 2015/10/30 01:43:50, a.suchit2 wrote: > > I think you should divide by |totalPercent|, but I think it's time for you to > > upload another patch, and we'll take it from there. > > Yes, when auto height and remaining height are 0 then divide by |totalPercent| > should be used. > So changes are required here. Condition should be : > if (totalPercent && !totalAutoRowsHeight && !totalRemainingRowsHeight) { I guess that's for a separate CL to sort out, right? https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.h:315: int&, Vector<int>&); Please put it back on one line.
ping
Sorry for too much delay. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:307: LayoutTableCell* cell, float totalPercent, int& extraRowSpanningHeight, On 2015/11/02 10:46:42, mstensho wrote: > Please put this back on one line. Done. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:326: std::min(m_grid[row].logicalHeight.percent(), percent) / 100) On 2015/11/02 10:46:42, mstensho wrote: > OK, so what you want to avoid here is integer overflow. How about using an > intermediary LayoutUnit here, which does saturated arithmetic? We certainly > don't want toAdd to be 0, which is the result of your changes. > > I'm talking about changing toAdd from int to LayoutUnit. > > Alternatively, change it from int to float. I am avoiding high value of percent which is not acceptable and trying to take min. value which can be adjusted here. In terms of the overflow, it may happen for int/float/double for all. I did not get here that, how will LayoutUnit/float be helpful for changing toAdd ? https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.h:315: int&, Vector<int>&); On 2015/11/02 10:46:42, mstensho wrote: > Please put it back on one line. Done.
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:1: <!DOCTYPE html> On 2015/11/02 10:46:42, mstensho wrote: > Expectation file missing. Right now this looks like a crash test, so you should > add > > <script> > if (window.testRunner) > testRunner.dumpAsText(); > </script> > > ... and store and upload the -expected.txt file. Done. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:7: <h3>Test for chromium bug : <a href="https://code.google.com/p/chromium/issues/detail?id=408066">408066</a>. Page is crashing on assression when table row span cell's percent height is too height.</h3> On 2015/11/02 10:46:42, mstensho wrote: > Please wrap this line and proof-read the text. Done. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:11: <td height="876543210" rowspan="2">This row span cell height is too height.</td> On 2015/11/02 10:46:42, mstensho wrote: > "too large" It was not reproducing without this too large value. Sp I kept it large value. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:13: <tr id="row-height"> On 2015/11/02 10:46:42, mstensho wrote: > Two elements with the same ID. Maybe use .class instead of #id? Done.
On 2016/01/13 13:37:59, a.suchit2 wrote: > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html > (right): > > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:1: > <!DOCTYPE html> > On 2015/11/02 10:46:42, mstensho wrote: > > Expectation file missing. Right now this looks like a crash test, so you > should > > add > > > > <script> > > if (window.testRunner) > > testRunner.dumpAsText(); > > </script> > > > > ... and store and upload the -expected.txt file. > > Done. > > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:7: > <h3>Test for chromium bug : <a > href="https://code.google.com/p/chromium/issues/detail?id=408066">408066</a>. > Page is crashing on assression when table row span cell's percent height is too > height.</h3> > On 2015/11/02 10:46:42, mstensho wrote: > > Please wrap this line and proof-read the text. > > Done. > > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:11: > <td height="876543210" rowspan="2">This row span cell height is too height.</td> > On 2015/11/02 10:46:42, mstensho wrote: > > "too large" > > It was not reproducing without this too large value. Sp I kept it large value. > > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:13: > <tr id="row-height"> > On 2015/11/02 10:46:42, mstensho wrote: > > Two elements with the same ID. Maybe use .class instead of #id? > > Done. Please review.
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:11: <td height="876543210" rowspan="2">This row span cell height is too height.</td> On 2016/01/13 13:37:59, a.suchit2 wrote: > On 2015/11/02 10:46:42, mstensho wrote: > > "too large" > > It was not reproducing without this too large value. Sp I kept it large value. Yes, but I'm talking about the text. It should be "too large", not "too height". https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:326: std::min(m_grid[row].logicalHeight.percent(), percent) / 100) On 2016/01/12 06:46:35, a.suchit2 wrote: > On 2015/11/02 10:46:42, mstensho wrote: > > OK, so what you want to avoid here is integer overflow. How about using an > > intermediary LayoutUnit here, which does saturated arithmetic? We certainly > > don't want toAdd to be 0, which is the result of your changes. > > > > I'm talking about changing toAdd from int to LayoutUnit. > > > > Alternatively, change it from int to float. > > I am avoiding high value of percent which is not acceptable and trying to take > min. value which can be adjusted here. > In terms of the overflow, it may happen for int/float/double for all. > I did not get here that, how will LayoutUnit/float be helpful for changing toAdd > ? Because LayoutUnit cannot overflow, since it uses saturated arithmetic. The original code is: int toAdd = (tableHeight * m_grid[row].logicalHeight.percent() / 100) - rowsHeight[row - rowIndex]; Just wrapping a LayoutUnit around the right-hand side here will make the assertion go away: int toAdd = LayoutUnit((tableHeight * m_grid[row].logicalHeight.percent() / 100) - rowsHeight[row - rowIndex]); https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:12: <br/>Page is crashing on assression when table row span cell's percent height is too height.</h3> Can you indent it a little? Also "too large" instead of "too height". https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:15: <tr class="row-height"> "too large" instead of "too height" https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:350: int toAdd = (tableHeight * Please indent properly or put this back together on one line.
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:11: <td height="876543210" rowspan="2">This row span cell height is too height.</td> On 2016/01/18 14:37:03, mstensho wrote: > On 2016/01/13 13:37:59, a.suchit2 wrote: > > On 2015/11/02 10:46:42, mstensho wrote: > > > "too large" > > > > It was not reproducing without this too large value. Sp I kept it large value. > > Yes, but I'm talking about the text. It should be "too large", not "too height". Done. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:326: std::min(m_grid[row].logicalHeight.percent(), percent) / 100) On 2016/01/18 14:37:03, mstensho wrote: > On 2016/01/12 06:46:35, a.suchit2 wrote: > > On 2015/11/02 10:46:42, mstensho wrote: > > > OK, so what you want to avoid here is integer overflow. How about using an > > > intermediary LayoutUnit here, which does saturated arithmetic? We certainly > > > don't want toAdd to be 0, which is the result of your changes. > > > > > > I'm talking about changing toAdd from int to LayoutUnit. > > > > > > Alternatively, change it from int to float. > > > > I am avoiding high value of percent which is not acceptable and trying to take > > min. value which can be adjusted here. > > In terms of the overflow, it may happen for int/float/double for all. > > I did not get here that, how will LayoutUnit/float be helpful for changing > toAdd > > ? > > Because LayoutUnit cannot overflow, since it uses saturated arithmetic. > > The original code is: > int toAdd = (tableHeight * m_grid[row].logicalHeight.percent() / 100) - > rowsHeight[row - rowIndex]; > > Just wrapping a LayoutUnit around the right-hand side here will make the > assertion go away: > > int toAdd = LayoutUnit((tableHeight * m_grid[row].logicalHeight.percent() / 100) > - rowsHeight[row - rowIndex]); Replied on latest patch set 5. https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:12: <br/>Page is crashing on assression when table row span cell's percent height is too height.</h3> On 2016/01/18 14:37:03, mstensho wrote: > Can you indent it a little? Also "too large" instead of "too height". Done. https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:15: <tr class="row-height"> On 2016/01/18 14:37:03, mstensho wrote: > "too large" instead of "too height" Done. https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:350: int toAdd = (tableHeight * On 2016/01/18 14:37:03, mstensho wrote: > Please indent properly or put this back together on one line. Done. https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:350: int toAdd = (tableHeight * std::min(m_grid[row].logicalHeight.percent(), percent) / 100) - rowsHeight[row - rowIndex]; Do we need to use LayoutUnit here after this change. IMO, remaining percent should be used if current row percent is bigger then remaining percent. If we use LayoutUnit((tableHeight * m_grid[row].logicalHeight.percent() / 100) - rowsHeight[row - rowIndex]) then percent would be calculated based on the current row value and total percent will exceeds the limit 100.
lgtm https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html (right): https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html:12: <br/>Page is crashing on assression when table row span cell's percent height is too large.</h3> *assertion* And I think you can just remove the first line "Test for chromium bug ...". People can use git log if they really want to know. https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:350: int toAdd = (tableHeight * std::min(m_grid[row].logicalHeight.percent(), percent) / 100) - rowsHeight[row - rowIndex]; On 2016/01/19 13:02:16, a.suchit2 wrote: > Do we need to use LayoutUnit here after this change. > IMO, remaining percent should be used if current row percent is bigger then > remaining percent. > > If we use LayoutUnit((tableHeight * m_grid[row].logicalHeight.percent() / 100) - > rowsHeight[row - rowIndex]) then percent would be calculated based on the > current row value and total percent will exceeds the limit 100. OK, I get it now. I had a hard time following the logic of this method, but I suppose we're just trying to copy the behavior of Firefox. Your change makes sense.
dgrogan@chromium.org changed reviewers: + dgrogan@chromium.org
https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html (right): https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html:12: <br/>Page is crashing on assression when table row span cell's percent height is too large.</h3> On 2016/01/20 13:00:59, mstensho wrote: > *assertion* > > And I think you can just remove the first line "Test for chromium bug ...". > People can use git log if they really want to know. I see you're consistent with this preference :) Just FYI, I swear Julien previously had us add these to the top of layout tests (that's why Suchit and I both did it) but I can't find those code reviews at the moment.
https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html (right): https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html:12: <br/>Page is crashing on assression when table row span cell's percent height is too large.</h3> On 2016/01/21 01:38:13, dgrogan wrote: > On 2016/01/20 13:00:59, mstensho wrote: > > *assertion* > > > > And I think you can just remove the first line "Test for chromium bug ...". > > People can use git log if they really want to know. > > I see you're consistent with this preference :) > > Just FYI, I swear Julien previously had us add these to the top of layout tests > (that's why Suchit and I both did it) but I can't find those code reviews at the > moment. So right now, I will leave as it is.
On 2016/01/21 07:08:40, a.suchit2 wrote: > https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html > (right): > > https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html:12: > <br/>Page is crashing on assression when table row span cell's percent height is > too large.</h3> > On 2016/01/21 01:38:13, dgrogan wrote: > > On 2016/01/20 13:00:59, mstensho wrote: > > > *assertion* > > > > > > And I think you can just remove the first line "Test for chromium bug ...". > > > People can use git log if they really want to know. > > > > I see you're consistent with this preference :) > > > > Just FYI, I swear Julien previously had us add these to the top of layout > tests > > (that's why Suchit and I both did it) but I can't find those code reviews at > the > > moment. > > So right now, I will leave as it is. I'd remove it, I plan on skipping them from here on out.
The CQ bit was checked by a.suchit@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405393002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405393002/80001
Message was sent while issue was closed.
Description was changed from
==========
Crashes when percent calculation of row's height goes very high.
When calculating the percent value of the row becomes negative because
percent calculation goes beyond integer limit So height of the row does
not increase due to negative value considered as 0. In this case extra
rowspanning height remains same and after that no other row present to
consume remaining extra row spanning height. So assertion fails.
Now, While calculating the percent height of the row, percent would be
considered
max 100.
R=jchaffraix@chromium.org
BUG=408066
==========
to
==========
Crashes when percent calculation of row's height goes very high.
When calculating the percent value of the row becomes negative because
percent calculation goes beyond integer limit So height of the row does
not increase due to negative value considered as 0. In this case extra
rowspanning height remains same and after that no other row present to
consume remaining extra row spanning height. So assertion fails.
Now, While calculating the percent height of the row, percent would be
considered
max 100.
R=jchaffraix@chromium.org
BUG=408066
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
Crashes when percent calculation of row's height goes very high.
When calculating the percent value of the row becomes negative because
percent calculation goes beyond integer limit So height of the row does
not increase due to negative value considered as 0. In this case extra
rowspanning height remains same and after that no other row present to
consume remaining extra row spanning height. So assertion fails.
Now, While calculating the percent height of the row, percent would be
considered
max 100.
R=jchaffraix@chromium.org
BUG=408066
==========
to
==========
Crashes when percent calculation of row's height goes very high.
When calculating the percent value of the row becomes negative because
percent calculation goes beyond integer limit So height of the row does
not increase due to negative value considered as 0. In this case extra
rowspanning height remains same and after that no other row present to
consume remaining extra row spanning height. So assertion fails.
Now, While calculating the percent height of the row, percent would be
considered
max 100.
R=jchaffraix@chromium.org
BUG=408066
Committed: https://crrev.com/4a095d9e3d1e01175ef2c757d686a350bf3d88fc
Cr-Commit-Position: refs/heads/master@{#370924}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4a095d9e3d1e01175ef2c757d686a350bf3d88fc Cr-Commit-Position: refs/heads/master@{#370924} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
