Prevent LayoutTest setting -1 page scale factor.

Prevent LayoutTest setting -1 page scale factor.

Blink did not clamp page scale factor when it is -1 probably because it
is the initial value. This can result in a clusterfuzz crash as cc tries
to clamp the value it got from blink, and later found that the value (1)
does not match the one (-1) it already stores, cc then tries to update the
value and only to find that the layer tree has not be synced.

This CL prevents clusterfuzz from setting a -1 page scale factor.

BUG=640500
Committed: https://crrev.com/2b5d011b487c215564e66393bd7dd1e71021ea95
Cr-Commit-Position: refs/heads/master@{#419839}

[sunxd, 2016-09-19 15:51:58]

Description was changed from

==========
Clamp page scale factor even if it is -1.

Blink did not clamp page scale factor when it is -1 probably because it
is the initial value. This can result in a clusterfuzz crash as cc Trieste
to clamp the value it got from blink, and later found that the value (1)
does not match the one (-1) it already stores, cc then tries to update the
value and only to find that the layer tree has not be synced.

Since it does not make sense to ignore the -1 case in blink: if min and
max values are set, the page scale factor should be in that range, if not,
the page scale factor will remain the same after clamping.

This CL removes the logic where -1 page scale factor is ignored.

BUG=640500
==========

to

==========
Clamp page scale factor even if it is -1.

Blink did not clamp page scale factor when it is -1 probably because it
is the initial value. This can result in a clusterfuzz crash as cc Trieste
to clamp the value it got from blink, and later found that the value (1)
does not match the one (-1) it already stores, cc then tries to update the
value and only to find that the layer tree has not be synced.

Since it does not make sense to ignore the -1 case in blink: if min and
max values are set, the page scale factor should be in that range, if not,
the page scale factor will remain the same after clamping.

This CL removes the logic where -1 page scale factor is ignored.

BUG=640500
==========

[sunxd, 2016-09-19 15:51:58]

sunxd@chromium.org changed reviewers:
+ ajuma@chromium.org

[sunxd, 2016-09-19 15:53:58]

Hi Ali, I'm not sure if I put the test in the right directory, and if
"capturePixelsAsyncThen" is the correct function to call. I tried
layoutAndPaintAysncThen but it didn't trigger the bug.

[ajuma, 2016-09-19 17:37:40]

https://codereview.chromium.org/2350163002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-invalid-page-scale-factor.html
(right):

https://codereview.chromium.org/2350163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-invalid-page-scale-factor.html:8:
::-webkit-scrollbar {
Is this part needed?

https://codereview.chromium.org/2350163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-invalid-page-scale-factor.html:16:
return window.visualViewport;
This seems unused.

https://codereview.chromium.org/2350163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-invalid-page-scale-factor.html:23:
window.testRunner.capturePixelsAsyncThen(function() {});
Would it work to set this up in the waitUntilDone/notifyDone style that many
layout tests use? e.g. set the page scale factor, then register a
requestAnimationFrame callback, then inside that callback call notifyDone?

Also, I'd suggest adding some text to this page along the lines of "This test
passes if it doesn't crash.". And then the expect result would have just that
text and nothing else.

[sunxd, 2016-09-19 18:54:22]

sunxd@chromium.org changed reviewers:
+ bokan@chromium.org

[sunxd, 2016-09-19 18:54:33]

PTAL

https://codereview.chromium.org/2350163002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-invalid-page-scale-factor.html
(right):

https://codereview.chromium.org/2350163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-invalid-page-scale-factor.html:8:
::-webkit-scrollbar {
On 2016/09/19 17:37:39, ajuma wrote:
> Is this part needed?

Done.

https://codereview.chromium.org/2350163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-invalid-page-scale-factor.html:16:
return window.visualViewport;
On 2016/09/19 17:37:39, ajuma wrote:
> This seems unused.

Done.

[ajuma, 2016-09-19 18:58:58]

https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:131: crbug.com/640500
fast/dom/viewport/viewport-invalid-page-scale-factor.html [ NeedsRebaseline ]
Would it be possible to make this a ref test (with an -expected.html file) so no
baseline is needed?

https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-invalid-page-scale-factor.html
(right):

https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-invalid-page-scale-factor.html:17:
window.setTimeout(notifyDone, 1200);
Using setTimeout can make tests flaky, so using requestAnimationFrame is
preferred.

[bokan, 2016-09-19 19:01:21]

https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/PageScaleConstraints.cpp (left):

https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PageScaleConstraints.cpp:61: if
(pageScaleFactor == -1)
I don't think this is the right way to fix this. -1 is used as an "unset" value
here. In the case of initial scale, we need to differentiate between a set and
unset initial scale.

The actual page scale factor should never be -1 so we shouldn't ever call into
this method with the real page scale factor at -1. The bug is somewhere higher
up in the call stack.

[sunxd, 2016-09-19 20:02:33]

On 2016/09/19 19:01:21, bokan wrote:
>
https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/PageScaleConstraints.cpp (left):
> 
>
https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/PageScaleConstraints.cpp:61: if
> (pageScaleFactor == -1)
> I don't think this is the right way to fix this. -1 is used as an "unset"
value
> here. In the case of initial scale, we need to differentiate between a set and
> unset initial scale.
> 
> The actual page scale factor should never be -1 so we shouldn't ever call into
> this method with the real page scale factor at -1. The bug is somewhere higher
> up in the call stack.

Yes I agree that the factor should never be -1. But the clusterfuzz bot directly
sets it to be -1.

In the bug:

<script>
  window.internals.setPageScaleFactor(-1);
  window.onload = function() {
  if (!window.testRunner)
    return;
  window.testRunner.capturePixelsAsyncThen(function() {
  });
};
</script>

Do we want to clamp it or prevent this call somehow? (by ignoring a -1 value?)

[bokan, 2016-09-19 20:31:26]

On 2016/09/19 20:02:33, sunxd wrote:
> On 2016/09/19 19:01:21, bokan wrote:
> >
>
https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/frame/PageScaleConstraints.cpp (left):
> > 
> >
>
https://codereview.chromium.org/2350163002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/PageScaleConstraints.cpp:61: if
> > (pageScaleFactor == -1)
> > I don't think this is the right way to fix this. -1 is used as an "unset"
> value
> > here. In the case of initial scale, we need to differentiate between a set
and
> > unset initial scale.
> > 
> > The actual page scale factor should never be -1 so we shouldn't ever call
into
> > this method with the real page scale factor at -1. The bug is somewhere
higher
> > up in the call stack.
> 
> Yes I agree that the factor should never be -1. But the clusterfuzz bot
directly
> sets it to be -1.
> 
> In the bug:
> 
> <script>
>   window.internals.setPageScaleFactor(-1);
>   window.onload = function() {
>   if (!window.testRunner)
>     return;
>   window.testRunner.capturePixelsAsyncThen(function() {
>   });
> };
> </script>
> 
> Do we want to clamp it or prevent this call somehow? (by ignoring a -1 value?)

Oh, it's via event sender. In that case, feel free to just clamp in
EventSender::setPageScaleFactor. This isn't a real bug.

[sunxd, 2016-09-20 17:18:51]

Description was changed from

==========
Clamp page scale factor even if it is -1.

Blink did not clamp page scale factor when it is -1 probably because it
is the initial value. This can result in a clusterfuzz crash as cc Trieste
to clamp the value it got from blink, and later found that the value (1)
does not match the one (-1) it already stores, cc then tries to update the
value and only to find that the layer tree has not be synced.

Since it does not make sense to ignore the -1 case in blink: if min and
max values are set, the page scale factor should be in that range, if not,
the page scale factor will remain the same after clamping.

This CL removes the logic where -1 page scale factor is ignored.

BUG=640500
==========

to

==========
Prevent LayoutTest setting -1 page scale factor.

Blink did not clamp page scale factor when it is -1 probably because it
is the initial value. This can result in a clusterfuzz crash as cc tries
to clamp the value it got from blink, and later found that the value (1)
does not match the one (-1) it already stores, cc then tries to update the
value and only to find that the layer tree has not be synced.

This CL prevents clusterfuzz from setting a -1 page scale factor.

BUG=640500
==========

[sunxd, 2016-09-20 17:19:08]

PTAL

[bokan, 2016-09-20 17:32:09]

No need to test Internals methods, just remove the test.

https://codereview.chromium.org/2350163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2350163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.cpp:1883: if (scaleFactor ==
-1)
it's not the initial value. It's a value used by the various limits (minimum,
maximum, initial-scale) but never page scale factor. Just make this `if
(scaleFactor <= 0) return;`, no need for a comment.

[sunxd, 2016-09-20 17:46:30]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 17:47:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunxd, 2016-09-20 17:47:10]

PTAL

https://codereview.chromium.org/2350163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2350163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.cpp:1883: if (scaleFactor ==
-1)
On 2016/09/20 17:32:09, bokan wrote:
> it's not the initial value. It's a value used by the various limits (minimum,
> maximum, initial-scale) but never page scale factor. Just make this `if
> (scaleFactor <= 0) return;`, no need for a comment.

Done.

[bokan, 2016-09-20 17:47:43]

lgtm

[bokan, 2016-09-20 17:50:28]

FYI for the future, if there's a clusterfuzz crash that's only exploitable via
unexposed internal functions, that's not really a problem so there's no need to
waste time on it. WontFix or a quick fix, in cases where it's trivial as here,
is enough.

Of course, if it's using an internal method but is otherwise exposing real issue
in our code, that should be fixed.

[commit-bot: I haz the power, 2016-09-20 19:15:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 19:15:53]

Dry run: This issue passed the CQ dry run.

[sunxd, 2016-09-20 19:18:15]

On 2016/09/20 17:50:28, bokan wrote:
> FYI for the future, if there's a clusterfuzz crash that's only exploitable via
> unexposed internal functions, that's not really a problem so there's no need
to
> waste time on it. WontFix or a quick fix, in cases where it's trivial as here,
> is enough.
> 
> Of course, if it's using an internal method but is otherwise exposing real
issue
> in our code, that should be fixed.

Thanks for the tips!

[sunxd, 2016-09-20 19:18:17]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2016-09-20 19:18:22]

The CQ bit was checked by sunxd@chromium.org

[commit-bot: I haz the power, 2016-09-20 19:18:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 19:25:02]

Description was changed from

==========
Prevent LayoutTest setting -1 page scale factor.

Blink did not clamp page scale factor when it is -1 probably because it
is the initial value. This can result in a clusterfuzz crash as cc tries
to clamp the value it got from blink, and later found that the value (1)
does not match the one (-1) it already stores, cc then tries to update the
value and only to find that the layer tree has not be synced.

This CL prevents clusterfuzz from setting a -1 page scale factor.

BUG=640500
==========

to

==========
Prevent LayoutTest setting -1 page scale factor.

Blink did not clamp page scale factor when it is -1 probably because it
is the initial value. This can result in a clusterfuzz crash as cc tries
to clamp the value it got from blink, and later found that the value (1)
does not match the one (-1) it already stores, cc then tries to update the
value and only to find that the layer tree has not be synced.

This CL prevents clusterfuzz from setting a -1 page scale factor.

BUG=640500
==========

[commit-bot: I haz the power, 2016-09-20 19:25:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-20 19:27:30]

Description was changed from

==========
Prevent LayoutTest setting -1 page scale factor.

Blink did not clamp page scale factor when it is -1 probably because it
is the initial value. This can result in a clusterfuzz crash as cc tries
to clamp the value it got from blink, and later found that the value (1)
does not match the one (-1) it already stores, cc then tries to update the
value and only to find that the layer tree has not be synced.

This CL prevents clusterfuzz from setting a -1 page scale factor.

BUG=640500
==========

to

==========
Prevent LayoutTest setting -1 page scale factor.

Blink did not clamp page scale factor when it is -1 probably because it
is the initial value. This can result in a clusterfuzz crash as cc tries
to clamp the value it got from blink, and later found that the value (1)
does not match the one (-1) it already stores, cc then tries to update the
value and only to find that the layer tree has not be synced.

This CL prevents clusterfuzz from setting a -1 page scale factor.

BUG=640500

Committed: https://crrev.com/2b5d011b487c215564e66393bd7dd1e71021ea95
Cr-Commit-Position: refs/heads/master@{#419839}
==========

[commit-bot: I haz the power, 2016-09-20 19:27:31]

Patchset 4 (id:??) landed as
https://crrev.com/2b5d011b487c215564e66393bd7dd1e71021ea95
Cr-Commit-Position: refs/heads/master@{#419839}