Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1016863002:
Fix integer overflow in ComputedStyle::computedLineHeight (Closed)
|
Created:
5 years, 9 months ago by rwlbuis Modified:
5 years, 8 months ago CC:
blink-reviews, blink-reviews-rendering, blink-reviews-style_chromium.org, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix integer overflow in ComputedStyle::computedLineHeight
When we compute huge values for line-height through
percentage or CSS calc, we'll overflow the integer and later
on ShapeOutsideInfo::computeDeltasForContainingBlockLine
will ASSERT because it expects non-negative line height.
So for the computed line height use an intermediate LayoutUnit
because it internally clamps to intMaxForLayoutUnit and the int
will not overflow. Note that the code path in computedLineHeight
for percentages does the same.
BUG=458461
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193574
Patch Set 1 #Patch Set 2 : Different approach #
Total comments: 1
Patch Set 3 : Retry patch set 1 #Patch Set 4 : Add tests #
Total comments: 1
Patch Set 5 : Use intermediate LayoutUnit #Patch Set 6 : Remove unrelated change #
Total comments: 1
Patch Set 7 : Clamp explicitly #
Messages
Total messages: 29 (6 generated)
rob.buis@samsung.com changed reviewers: + bjonesbe@adobe.com
PTAL
pdr@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... File Source/core/layout/style/LayoutStyle.cpp (right): https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... Source/core/layout/style/LayoutStyle.cpp:1257: return LayoutUnit::clamp(minimumValueForLength(lh, fontSize())); This fix seems too deep if it's only for shapes. Do we only hit this with shape code? I wonder if this should be closer to the "The shape-outside image is too large" warning?
On 2015/03/18 03:49:42, pdr wrote: > https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... > File Source/core/layout/style/LayoutStyle.cpp (right): > > https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... > Source/core/layout/style/LayoutStyle.cpp:1257: return > LayoutUnit::clamp(minimumValueForLength(lh, fontSize())); > This fix seems too deep if it's only for shapes. Do we only hit this with shape > code? I wonder if this should be closer to the "The shape-outside image is too > large" warning? Thanks for looking at this! The tests don't really hit this. The computed line-height follows a different code path and that one does not use integers and therefore seems more accurate. If you look using the inspector you can see my patch does allow for bigger line heights, in my testcase without my patch the height seems clamped to 2 times maximumAllowedFontSize, though I don't see where in the code this happens. The "The shape-outside image is too large" warning just seems to be a natural consequence of the huge line-height. The shapes code reports this if hte shape image size exceeds th platform maxDecodedImageBytes. I am not sure yet how to fix all this. Maybe we need something like maximumAllowedFontSize for line-height?
On 2015/03/18 at 15:03:44, rob.buis wrote: > On 2015/03/18 03:49:42, pdr wrote: > > https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... > > File Source/core/layout/style/LayoutStyle.cpp (right): > > > > https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... > > Source/core/layout/style/LayoutStyle.cpp:1257: return > > LayoutUnit::clamp(minimumValueForLength(lh, fontSize())); > > This fix seems too deep if it's only for shapes. Do we only hit this with shape > > code? I wonder if this should be closer to the "The shape-outside image is too > > large" warning? > > Thanks for looking at this! > The tests don't really hit this. The computed line-height follows a different code path and that one does not use integers and therefore seems more accurate. If you look using the inspector you can see my patch does allow for bigger line heights, in my testcase without my patch the height seems clamped to 2 times maximumAllowedFontSize, though I don't see where in the code this happens. > > The "The shape-outside image is too large" warning just seems to be a natural consequence of the huge line-height. The shapes code reports this if hte shape image size exceeds th platform maxDecodedImageBytes. > > I am not sure yet how to fix all this. Maybe we need something like maximumAllowedFontSize for line-height? I'm sorry, I don't quite follow. Can we hit this assert without shapes?
On 2015/03/18 19:36:10, pdr wrote: > On 2015/03/18 at 15:03:44, rob.buis wrote: > > On 2015/03/18 03:49:42, pdr wrote: > > > > https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... > > > File Source/core/layout/style/LayoutStyle.cpp (right): > > > > > > > https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... > > > Source/core/layout/style/LayoutStyle.cpp:1257: return > > > LayoutUnit::clamp(minimumValueForLength(lh, fontSize())); > > > This fix seems too deep if it's only for shapes. Do we only hit this with > shape > > > code? I wonder if this should be closer to the "The shape-outside image is > too > > > large" warning? > > > > Thanks for looking at this! > > The tests don't really hit this. The computed line-height follows a different > code path and that one does not use integers and therefore seems more accurate. > If you look using the inspector you can see my patch does allow for bigger line > heights, in my testcase without my patch the height seems clamped to 2 times > maximumAllowedFontSize, though I don't see where in the code this happens. > > > > The "The shape-outside image is too large" warning just seems to be a natural > consequence of the huge line-height. The shapes code reports this if hte shape > image size exceeds th platform maxDecodedImageBytes. > > > > I am not sure yet how to fix all this. Maybe we need something like > maximumAllowedFontSize for line-height? > > I'm sorry, I don't quite follow. Can we hit this assert without shapes? No, it can't. ShapeOutsideInfo only comes into play when there are shape-outside+float properties. The more I think about it, there seem only two solutions: 1. clamp to sane(?) values. 2. accept that line-height can overflow in the shape code and in that case give up instead of ASSERT ; the negative value means the requested size is so big that the "The shape-outside image is too large" error would show up anyway. I'd like to hear Bem's thoughts on this too.
On 2015/03/18 20:03:15, rwlbuis (OOO 1 apr) wrote: > On 2015/03/18 19:36:10, pdr wrote: > > On 2015/03/18 at 15:03:44, rob.buis wrote: > > > On 2015/03/18 03:49:42, pdr wrote: > > > > > > > https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... > > > > File Source/core/layout/style/LayoutStyle.cpp (right): > > > > > > > > > > > https://codereview.chromium.org/1016863002/diff/20001/Source/core/layout/styl... > > > > Source/core/layout/style/LayoutStyle.cpp:1257: return > > > > LayoutUnit::clamp(minimumValueForLength(lh, fontSize())); > > > > This fix seems too deep if it's only for shapes. Do we only hit this with > > shape > > > > code? I wonder if this should be closer to the "The shape-outside image is > > too > > > > large" warning? > > > > > > Thanks for looking at this! > > > The tests don't really hit this. The computed line-height follows a > different > > code path and that one does not use integers and therefore seems more > accurate. > > If you look using the inspector you can see my patch does allow for bigger > line > > heights, in my testcase without my patch the height seems clamped to 2 times > > maximumAllowedFontSize, though I don't see where in the code this happens. > > > > > > The "The shape-outside image is too large" warning just seems to be a > natural > > consequence of the huge line-height. The shapes code reports this if hte shape > > image size exceeds th platform maxDecodedImageBytes. > > > > > > I am not sure yet how to fix all this. Maybe we need something like > > maximumAllowedFontSize for line-height? > > > > I'm sorry, I don't quite follow. Can we hit this assert without shapes? > > No, it can't. ShapeOutsideInfo only comes into play when there are > shape-outside+float properties. > > The more I think about it, there seem only two solutions: > 1. clamp to sane(?) values. > 2. accept that line-height can overflow in the shape code and in that case give > up instead of ASSERT ; the negative value means > the requested size is so big that the "The shape-outside image is too large" > error would show up anyway. > > I'd like to hear Bem's thoughts on this too. I took a quick look, and the patch does seem like it's patching the symptom, not the root cause. I assume the integer overflow happens in LayoutStyle::computedLineHeight()? That brings up the following question to me: why does LayoutStyle::computedLineHeight return an int? Every place that uses it expects a LayoutUnit except LayoutBR.cpp:50. What makes this look like even more of an issue is that in the 3 different cases it covers, FontMetrics::lineSpacing() returns an int, but minimumValueForLength() returns a LayoutUnit, and Length::value() returns a float. Is it just me, or does this method seem like it just invites integer overflows? Sure, we can just patch the shapes code, but unless someone explains to me why the code in LayoutStyle::computedLineHeight is correct, patching anywhere after the overflow occurs doesn't seem like the correct fix. - Bem
On 2015/03/27 16:30:17, Bem Jones-Bey (adobe) wrote: > > I'd like to hear Bem's thoughts on this too. > > I took a quick look, and the patch does seem like it's patching the symptom, not > the root cause. I assume the integer overflow happens in > LayoutStyle::computedLineHeight()? That brings up the following question to me: > why does LayoutStyle::computedLineHeight return an int? Every place that uses it > expects a LayoutUnit except LayoutBR.cpp:50. What makes this look like even more > of an issue is that in the 3 different cases it covers, > FontMetrics::lineSpacing() returns an int, but minimumValueForLength() returns a > LayoutUnit, and Length::value() returns a float. Is it just me, or does this > method seem like it just invites integer overflows? > > Sure, we can just patch the shapes code, but unless someone explains to me why > the code in LayoutStyle::computedLineHeight is correct, patching anywhere after > the overflow occurs doesn't seem like the correct fix. > > - Bem Thanks for looking at this CL. I agreed returning the int seemed problematic so patch set 1 addressed that. The layout test regressions need to be looked at but to me it does seem the way to go.
Patchset #4 (id:60001) has been deleted
rob.buis@samsung.com changed reviewers: + leviw@chromium.org
PTAL folks, I think this is the way to go.
https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... File Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... Source/core/style/ComputedStyle.cpp:1259: return truncf(lh.value()); Why do we have to explicitly truncf here? Does the LayoutUnit constructor not do the right thing?
On 2015/04/02 21:53:14, Bem Jones-Bey (adobe) wrote: > https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... > File Source/core/style/ComputedStyle.cpp (right): > > https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... > Source/core/style/ComputedStyle.cpp:1259: return truncf(lh.value()); > Why do we have to explicitly truncf here? Does the LayoutUnit constructor not do > the right thing? There is a constructor for LayoutUnit taking a float, so that one is chosen by the compiler and it won't be truncated.
On 2015/04/02 21:54:58, rwlbuis wrote: > On 2015/04/02 21:53:14, Bem Jones-Bey (adobe) wrote: > > > https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... > > File Source/core/style/ComputedStyle.cpp (right): > > > > > https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... > > Source/core/style/ComputedStyle.cpp:1259: return truncf(lh.value()); > > Why do we have to explicitly truncf here? Does the LayoutUnit constructor not > do > > the right thing? > > There is a constructor for LayoutUnit taking a float, so that one is chosen by > the compiler and it won't be truncated. Maybe I should have just asked: why does it need to be truncated? Is that a fix for all the tests that failed when you originally made this change?
On 2015/04/02 21:56:57, Bem Jones-Bey (adobe) wrote: > On 2015/04/02 21:54:58, rwlbuis wrote: > > On 2015/04/02 21:53:14, Bem Jones-Bey (adobe) wrote: > > > > > > https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... > > > File Source/core/style/ComputedStyle.cpp (right): > > > > > > > > > https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... > > > Source/core/style/ComputedStyle.cpp:1259: return truncf(lh.value()); > > > Why do we have to explicitly truncf here? Does the LayoutUnit constructor > not > > do > > > the right thing? > > > > There is a constructor for LayoutUnit taking a float, so that one is chosen by > > the compiler and it won't be truncated. > > Maybe I should have just asked: why does it need to be truncated? Is that a fix > for all the tests that failed when you originally made this change? Ah, got you. Yes actually the only problem is fast/css/negative-leading.html. It seems to me the calculated value without the truncate ends up too big for that testcase (when compared to FireFox). I am not sure where exactly in the code (RootInlineBox?) the lineHeight for the two lines are actually used to compute the total <div>/LayoutBlock height. If someone can point me to that spot *and* we think it is a good idea to not truncate like before I can look into that.
On 2015/04/02 22:18:36, rwlbuis wrote: > On 2015/04/02 21:56:57, Bem Jones-Bey (adobe) wrote: > > On 2015/04/02 21:54:58, rwlbuis wrote: > > > On 2015/04/02 21:53:14, Bem Jones-Bey (adobe) wrote: > > > > > > > > > > https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... > > > > File Source/core/style/ComputedStyle.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1016863002/diff/80001/Source/core/style/Compu... > > > > Source/core/style/ComputedStyle.cpp:1259: return truncf(lh.value()); > > > > Why do we have to explicitly truncf here? Does the LayoutUnit constructor > > not > > > do > > > > the right thing? > > > > > > There is a constructor for LayoutUnit taking a float, so that one is chosen > by > > > the compiler and it won't be truncated. > > > > Maybe I should have just asked: why does it need to be truncated? Is that a > fix > > for all the tests that failed when you originally made this change? > > Ah, got you. Yes actually the only problem is fast/css/negative-leading.html. It > seems to me the calculated value without the truncate ends up too big for that > testcase (when compared to FireFox). > I am not sure where exactly in the code (RootInlineBox?) the lineHeight for the > two lines are actually used to compute the total <div>/LayoutBlock height. If > someone can point me to that spot *and* we think it is a good idea to not > truncate like before I can look into that. To me, this seems like a prime spot for a FIXME comment. :-)
leviw@chromium.org changed reviewers: + eae@chromium.org
Once upon a time when sub-pixel layout was still cooling we tried to consistently use ints throughout our line-height calculations. This was done to avoid having lines that irregularly accumulated to different pixel values, as this looked bad on a page with long blocks of consistent text. I'm surprised when I look at the callers and see that pretty much all of them immediately assign this number into a LayoutUnit, though I'm too scared to blame the code and see my own name on them ;) Mayhaps eae@ can provide some color to the current thinking. In summary, I'm skeptical that changing to LayoutUnit line heights is what we want, at least when we're accumulating the value and placing RootInlineBoxes.
On 2015/04/03 18:35:00, leviw wrote: > Once upon a time when sub-pixel layout was still cooling we tried to > consistently use ints throughout our line-height calculations. This was done to > avoid having lines that irregularly accumulated to different pixel values, as > this looked bad on a page with long blocks of consistent text. > > I'm surprised when I look at the callers and see that pretty much all of them > immediately assign this number into a LayoutUnit, though I'm too scared to blame > the code and see my own name on them ;) Mayhaps eae@ can provide some color to > the current thinking. > > In summary, I'm skeptical that changing to LayoutUnit line heights is what we > want, at least when we're accumulating the value and placing RootInlineBoxes. ping @eae :)
Like Levi said we need to ensure that the line-height doesn't change from line to line. The way we to that today is by forcing it to an int. Long term we'd like to support subpixel line-height, as some modern fonts have subpixel ascent and decent specified. Doing that without breaking the guarantee that each line is the same hight will be tricky though. As for this change I'd rather not change the signature of computeLineHight as that hides all this.
On 2015/04/09 18:25:43, eae wrote: > Like Levi said we need to ensure that the line-height doesn't change from line > to line. The way we to that today is by forcing it to an int. Long term we'd > like to support subpixel line-height, as some modern fonts have subpixel ascent > and decent specified. Doing that without breaking the guarantee that each line > is the same hight will be tricky though. > > As for this change I'd rather not change the signature of computeLineHight as > that hides all this. Thanks, understood. I found another way, after I investigated why the percentage case (triggered by font-size: 1234567890px / 4000 for example) worked, it was because of LayoutUnit internal clamping. So in my latest patchset I do the same for the still overflowing code path by using an intermediate LayoutUnit and keep the original signature for computedLineHeight. PTAL :)
https://codereview.chromium.org/1016863002/diff/120001/Source/core/style/Comp... File Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1016863002/diff/120001/Source/core/style/Comp... Source/core/style/ComputedStyle.cpp:1259: return LayoutUnit(lh.value()); I'd just use max(lh.value(), LayoutUnit::max()). That's clearer about what you're trying to do I think.
On 2015/04/09 23:29:16, leviw wrote: > https://codereview.chromium.org/1016863002/diff/120001/Source/core/style/Comp... > File Source/core/style/ComputedStyle.cpp (right): > > https://codereview.chromium.org/1016863002/diff/120001/Source/core/style/Comp... > Source/core/style/ComputedStyle.cpp:1259: return LayoutUnit(lh.value()); > I'd just use max(lh.value(), LayoutUnit::max()). That's clearer about what > you're trying to do I think. You mean min(lh.value(), LayoutUnit::max()) I think :) I did not notice LayoutUnit::max() before, will fix.
On 2015/04/09 23:29:16, leviw wrote: > https://codereview.chromium.org/1016863002/diff/120001/Source/core/style/Comp... > File Source/core/style/ComputedStyle.cpp (right): > > https://codereview.chromium.org/1016863002/diff/120001/Source/core/style/Comp... > Source/core/style/ComputedStyle.cpp:1259: return LayoutUnit(lh.value()); > I'd just use max(lh.value(), LayoutUnit::max()). That's clearer about what > you're trying to do I think. Implemented in patch set 6. If you can, feel free to shut down some try runs. In hindsight all that I needed were Debug runs to be sure the problem was fixed...
LGTM
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1016863002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193574 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
