Keep effectiveZoom finite

Keep effectiveZoom finite

Through repeated application, it's quite possible to get the effective
zoom to overflow, resulting in Infinity in ComputedStyle.
This is a very uncommon case in reality (outside fuzzers), but make
sure to clamp the effective zoom value to a finite range to avoid the
simplest cases of Infinity havoc-wrecking.

BUG=490757, 502997
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197734

[fs, 2015-06-23 21:11:56]

fs@opera.com changed reviewers:
+ pdr@chromium.org, timloh@chromium.org

[fs, 2015-06-23 21:11:57]

https://codereview.chromium.org/1193203003/diff/20001/Source/core/style/Compu...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1193203003/diff/20001/Source/core/style/Compu...
Source/core/style/ComputedStyle.h:1890: float clampedEffectiveZoom =
clampTo<float>(f);
In practice this is just a "gratuitous max value" - a fairly large one at that.
Could (should?) quite possibly be a smaller value. With this value, it still
fairly trivial to overflow to Infinity (multiplication with anything > 1 will do
it for instance). It works better than the actual Infinity though, because a
quite common operation/case is "0 * Infinity => NaN" (value == 0, zoom ==
Infinity), which now becomes "0 * max() => 0". Will change to smaller value for
no fee at all.

[pdr., 2015-06-23 22:33:34]

On 2015/06/23 at 21:11:57, fs wrote:
>
https://codereview.chromium.org/1193203003/diff/20001/Source/core/style/Compu...
> File Source/core/style/ComputedStyle.h (right):
> 
>
https://codereview.chromium.org/1193203003/diff/20001/Source/core/style/Compu...
> Source/core/style/ComputedStyle.h:1890: float clampedEffectiveZoom =
clampTo<float>(f);
> In practice this is just a "gratuitous max value" - a fairly large one at
that. Could (should?) quite possibly be a smaller value. With this value, it
still fairly trivial to overflow to Infinity (multiplication with anything > 1
will do it for instance). It works better than the actual Infinity though,
because a quite common operation/case is "0 * Infinity => NaN" (value == 0, zoom
== Infinity), which now becomes "0 * max() => 0". Will change to smaller value
for no fee at all.

I think this is reasonable. LGTM

[Timothy Loh, 2015-06-24 04:34:04]

CSSToLengthConversionData has some code (ctor and setZoom) that clamps to avoid
zero, maybe we should remove that if we're clamping in setEffectiveZoom. I don't
think we should allow such a large range though, perhaps something smaller like
[1e-6, 1e6] might do a better job at avoiding overflow/underflow elsewhere?

[fs, 2015-06-24 10:09:15]

On 2015/06/24 04:34:04, Timothy Loh wrote:
> CSSToLengthConversionData has some code (ctor and setZoom) that clamps to
avoid
> zero, maybe we should remove that if we're clamping in setEffectiveZoom.

Yeah, twice seems overkill - dropped.

> I don't
> think we should allow such a large range though, perhaps something smaller
like
> [1e-6, 1e6] might do a better job at avoiding overflow/underflow elsewhere?

Changed range to the above.

[Timothy Loh, 2015-06-24 10:27:48]

On 2015/06/24 10:09:15, fs wrote:
> On 2015/06/24 04:34:04, Timothy Loh wrote:
> > CSSToLengthConversionData has some code (ctor and setZoom) that clamps to
> avoid
> > zero, maybe we should remove that if we're clamping in setEffectiveZoom.
> 
> Yeah, twice seems overkill - dropped.
> 
> > I don't
> > think we should allow such a large range though, perhaps something smaller
> like
> > [1e-6, 1e6] might do a better job at avoiding overflow/underflow elsewhere?
> 
> Changed range to the above.

lgtm

[fs, 2015-06-24 11:36:15]

The CQ bit was checked by fs@opera.com

[fs, 2015-06-24 11:36:15]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1193203003/#ps40001
(title: "Reduced clamp range; Remove clamp in CSSToLength...")

[commit-bot: I haz the power, 2015-06-24 11:36:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1193203003/40001

[fs, 2015-06-24 11:36:22]

The CQ bit was unchecked by fs@opera.com

[fs, 2015-06-24 11:36:43]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-06-24 11:36:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1193203003/40001

[commit-bot: I haz the power, 2015-06-24 11:40:32]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197734