Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23351002:
Crash when opening web page http://build.webkit.org/waterfall. (Closed)
|
Created:
7 years, 4 months ago by a.suchit Modified:
7 years, 3 months ago CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionCrash when opening web page http://build.webkit.org/waterfall.
While distribution of spanning cell's extra height, Sorting of the
cell's comparator function is missing condition of arranging cells
based on accending order of row indexes.
R=jchaffraix@chromium.org
BUG=276253
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157283
Patch Set 1 #Patch Set 2 : Added Test case #
Total comments: 4
Patch Set 3 : Review comments addressed #
Total comments: 7
Patch Set 4 : Review comments addressed #
Total comments: 2
Patch Set 5 : Review comments addressed #
Total comments: 7
Patch Set 6 : Review comments addressed #
Messages
Total messages: 18 (0 generated)
This needs a test, also don't abbreviate, result = ...;
Test case added.
Can't this test be dumpAsText() ? I'd rather we didn't add more pixel tests. Also this test is huge, can you write some JS that produces the really big DOM? :)
https://codereview.chromium.org/23351002/diff/11001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/11001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:386: bool ret = (cellRowIndex2 >= cellRowIndex1 && (cellRowIndex2 + cellRowSpan2) <= (cellRowIndex1 + cellRowSpan1)); If ret is true, you will return true (because !ret is false) so why don't you just early return here? Note that I am still concerned that this is just peppering over the issues: as pointed out in a previous review, you are using non-strict inequality, which makes it hard to see whether your function is really a weak ordering. https://codereview.chromium.org/23351002/diff/11001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:391: return ret; |ret| is not a great name for a variable really.
https://codereview.chromium.org/23351002/diff/11001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/11001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:386: bool ret = (cellRowIndex2 >= cellRowIndex1 && (cellRowIndex2 + cellRowSpan2) <= (cellRowIndex1 + cellRowSpan1)); On 2013/08/21 19:27:02, Julien Chaffraix wrote: > If ret is true, you will return true (because !ret is false) so why don't you > just early return here? > > Note that I am still concerned that this is just peppering over the issues: as > pointed out in a previous review, you are using non-strict inequality, which > makes it hard to see whether your function is really a weak ordering. Done. https://codereview.chromium.org/23351002/diff/11001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:391: return ret; On 2013/08/21 19:27:02, Julien Chaffraix wrote: > |ret| is not a great name for a variable really. Done. https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:390: return false; I think now it is strict equality. We are sorting the cells based on their row indexes but if 2 cells are same size then more height cell will come first and if one cell inside another cell then inner cell will come first.
On 2013/08/21 17:31:17, esprehn wrote: > Can't this test be dumpAsText() ? I'd rather we didn't add more pixel tests. > Also this test is huge, can you write some JS that produces the really big DOM? > :) I tried some scenarios of the rowspan cells order which will reproduce this issue but did not get success. So I took the html source of page http://build.webkit.org/waterfall and modified it. I don't have idea about order of the spanning cells which will reproduce it so creating this big DOM by JS is very difficult.
This looks fine to me, I'll defer to Julien for the final review.
The test case should really be improved: you want your test case to be the *minimal* set of tags / CSS rules that triggers the issue which is not the case here. https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:376: static bool compareRowSpanCellsInHeightDistributionOrder(const RenderTableCell* cell2, const RenderTableCell* cell1) I missed that at the previous review. The arguments are named *backwards*. That makes the code nearly-impossible to get right. I couldn't actually review the correctness as it's too crazy for me to parse. https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:385: else if (cellRowIndex2 >= cellRowIndex1 && (cellRowIndex2 + cellRowSpan2) <= (cellRowIndex1 + cellRowSpan1)) The else is not mandated as you return all the time. I would advocate towards commenting this code (with why comments) as it is getting really hairy. https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:390: return false; On 2013/08/22 10:26:17, a.suchit wrote: > I think now it is strict equality. We are sorting the cells based on their row > indexes but if 2 cells are same size then more height cell will come first and > if one cell inside another cell then inner cell will come first. I don't have your confidence and based on the fact that we got it wrong once already, I am betting that there are other bugs lying in the code.
https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:376: static bool compareRowSpanCellsInHeightDistributionOrder(const RenderTableCell* cell2, const RenderTableCell* cell1) On 2013/08/22 21:48:08, Julien Chaffraix wrote: > I missed that at the previous review. The arguments are named *backwards*. That > makes the code nearly-impossible to get right. > > I couldn't actually review the correctness as it's too crazy for me to parse. Done. https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:385: else if (cellRowIndex2 >= cellRowIndex1 && (cellRowIndex2 + cellRowSpan2) <= (cellRowIndex1 + cellRowSpan1)) On 2013/08/22 21:48:08, Julien Chaffraix wrote: > The else is not mandated as you return all the time. > > I would advocate towards commenting this code (with why comments) as it is > getting really hairy. Done. https://codereview.chromium.org/23351002/diff/18001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:390: return false; On 2013/08/22 21:48:08, Julien Chaffraix wrote: > On 2013/08/22 10:26:17, a.suchit wrote: > > I think now it is strict equality. We are sorting the cells based on their row > > indexes but if 2 cells are same size then more height cell will come first and > > if one cell inside another cell then inner cell will come first. > > I don't have your confidence and based on the fact that we got it wrong once > already, I am betting that there are other bugs lying in the code. Sorry but last time, I don't have whole idea about std::sort(), how does it work so I did mistake.
https://codereview.chromium.org/23351002/diff/26001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/26001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:376: static bool compareRowSpanCellsInHeightDistributionOrder(const RenderTableCell* secondCell, const RenderTableCell* firstCell) The arguments are still backwards :-/ I was fine with the old naming, just that the arguments' order is arg2, arg1 which is the opposite of what every programmer would assume.
https://codereview.chromium.org/23351002/diff/26001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/26001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:376: static bool compareRowSpanCellsInHeightDistributionOrder(const RenderTableCell* secondCell, const RenderTableCell* firstCell) On 2013/08/23 22:00:47, Julien Chaffraix wrote: > The arguments are still backwards :-/ > > I was fine with the old naming, just that the arguments' order is arg2, arg1 > which is the opposite of what every programmer would assume. Done.
lgtm with the test case fixed. https://codereview.chromium.org/23351002/diff/43001/LayoutTests/fast/table/ta... File LayoutTests/fast/table/table-rowspan-crash-with-huge-rowspan-cells.html (right): https://codereview.chromium.org/23351002/diff/43001/LayoutTests/fast/table/ta... LayoutTests/fast/table/table-rowspan-crash-with-huge-rowspan-cells.html:1: <!DOCTYPE html> Can you please reduce this file before landing? I really think a minimal test case - that also states what it tests and the condition for passing - would be way more helpful here. https://codereview.chromium.org/23351002/diff/43001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/43001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:390: if (cell1RowIndex >= cell2RowIndex && (cell1RowIndex + cell1RowSpan) <= (cell2RowIndex + cell2RowSpan)) This should be a function as you use it twice: bool cellIsFullyIncludedInOtherCell(const RenderTableCell*, cosnt RenderTableCell*) https://codereview.chromium.org/23351002/diff/43001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:397: return false; I am concerned about always returning false here but I couldn't find a violation of the weak ordering properties.
https://codereview.chromium.org/23351002/diff/43001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/43001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:375: // 2. For cells spanning the same rows, we sort by intrinsic size. As you inlined the description of what you do, the description of how we sort could be removed from this comment.
https://codereview.chromium.org/23351002/diff/43001/LayoutTests/fast/table/ta... File LayoutTests/fast/table/table-rowspan-crash-with-huge-rowspan-cells.html (right): https://codereview.chromium.org/23351002/diff/43001/LayoutTests/fast/table/ta... LayoutTests/fast/table/table-rowspan-crash-with-huge-rowspan-cells.html:1: <!DOCTYPE html> On 2013/09/04 18:35:45, Julien Chaffraix wrote: > Can you please reduce this file before landing? > > I really think a minimal test case - that also states what it tests and the > condition for passing - would be way more helpful here. I tried to reduce this file. But removing a single row in the table does not reproduce this bug. https://codereview.chromium.org/23351002/diff/43001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/23351002/diff/43001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:375: // 2. For cells spanning the same rows, we sort by intrinsic size. On 2013/09/04 20:46:27, Julien Chaffraix wrote: > As you inlined the description of what you do, the description of how we sort > could be removed from this comment. Done. https://codereview.chromium.org/23351002/diff/43001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:390: if (cell1RowIndex >= cell2RowIndex && (cell1RowIndex + cell1RowSpan) <= (cell2RowIndex + cell2RowSpan)) On 2013/09/04 18:35:45, Julien Chaffraix wrote: > This should be a function as you use it twice: > > bool cellIsFullyIncludedInOtherCell(const RenderTableCell*, cosnt > RenderTableCell*) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/23351002/53001
Message was sent while issue was closed.
Change committed as 157283 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
