Rework the checks for too-few values in feColorMatrix filter.

Rework the checks for too-few values in feColorMatrix filter.

Revert the prevous fix because it was flat-out wrong for some
filters. Here we add checks at all points the values are used,
because it is impossible to enforce an always-valid m_values vector
inside the FEColorMatrix object.

For example, we need to support any ordering of setAttribute('type', ...)
and setAttribute('values', ...), but the valid number of values depends
on the type. We couldn't set the type from "hueRotate" to "matrix", for
example, without first adding more values than are necessary for the
"hueRotate". That's a bad user experience.

R=fs@opera.com,ed@opera.com,pdr@chromium.org
BUG=468519
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193911

[Stephen Chennney, 2015-04-15 17:37:41]

This is not ready to land because the test does not crash in run-webkit-tests.
But feedback would be appreciated.

[Stephen Chennney, 2015-04-15 17:38:23]

Oops, not the right patch.

[Stephen Chennney, 2015-04-15 17:42:26]

Patchset #1 (id:1) has been deleted

[pdr., 2015-04-15 18:34:22]

https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters...
File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right):

https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters...
LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:16:
setTimeout(changeValues, 100);
What's this timer waiting on?

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
File Source/platform/graphics/filters/FEColorMatrix.cpp (right):

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:128: if (values.size() < 20)
{
Named constant please :)

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:129: for (size_t i = 0; i <
20; i++)
One of these for loops uses a size_t with i++, the other an int with ++i.

[fs, 2015-04-15 22:07:27]

On 2015/04/15 17:37:41, Stephen Chenney wrote:
> This is not ready to land because the test does not crash in run-webkit-tests.

Possibly because the filter primitive starts out "invalid", so a FilterEffect is
never created, and hence the update path can't do anything to update the
FilterEffect.

https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters...
File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right):

https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters...
LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:16:
setTimeout(changeValues, 100);
On 2015/04/15 18:34:22, pdr wrote:
> What's this timer waiting on?

For SVGFEColorMatrixElement::build to be called I suspect? Waiting for a paint
ought to suffice I think.

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
File Source/platform/graphics/filters/FEColorMatrix.cpp (right):

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:122: // Use defaults if
values contains too few values. See SVGFEColorMatrixElement::build
Presumably said method would be made much simpler with this in place - else
there'd be one case where things would work (valid -> invalid) and one where it
wouldn't (invalid -> invalid) - for non.obvious reasons.

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:128: if (values.size() < 20)
{
On 2015/04/15 18:34:22, pdr wrote:
> Named constant please :)

arraysize(matrix)? =)

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:142: saturateMatrix(1,
matrix);
Since all these "error" cases end up generating an identity matrix, it'd also be
possible to do that up front and then just break on the conditions in each case.
It's a matter of how uch you want to avoid double writes I guess =).

[Stephen Chennney, 2015-04-16 18:12:19]

New patch imminent.

https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters...
File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right):

https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters...
LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:16:
setTimeout(changeValues, 100);
On 2015/04/15 22:07:27, fs wrote:
> On 2015/04/15 18:34:22, pdr wrote:
> > What's this timer waiting on?
> 
> For SVGFEColorMatrixElement::build to be called I suspect? Waiting for a paint
> ought to suffice I think.

Yes. But I found the way to make it happen.

https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters...
LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:16:
setTimeout(changeValues, 100);
On 2015/04/15 22:07:27, fs wrote:
> On 2015/04/15 18:34:22, pdr wrote:
> > What's this timer waiting on?
> 
> For SVGFEColorMatrixElement::build to be called I suspect? Waiting for a paint
> ought to suffice I think.

Yes, a paint is needed. In fact, 2 paints are needed. I've used the appropriate
method to make that happen.

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
File Source/platform/graphics/filters/FEColorMatrix.cpp (right):

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:128: if (values.size() < 20)
{
On 2015/04/15 22:07:27, fs wrote:
> On 2015/04/15 18:34:22, pdr wrote:
> > Named constant please :)
> 
> arraysize(matrix)? =)

The named constant lets us use it everywhere, including the declarations.

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:129: for (size_t i = 0; i <
20; i++)
On 2015/04/15 18:34:22, pdr wrote:
> One of these for loops uses a size_t with i++, the other an int with ++i.

Good point. That's what I get for copy-paste.

https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:142: saturateMatrix(1,
matrix);
On 2015/04/15 22:07:27, fs wrote:
> Since all these "error" cases end up generating an identity matrix, it'd also
be
> possible to do that up front and then just break on the conditions in each
case.
> It's a matter of how uch you want to avoid double writes I guess =).

Done.

[Stephen Chennney, 2015-04-16 18:19:22]

Patchset #2 (id:40001) has been deleted

[Stephen Chennney, 2015-04-16 18:19:28]

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

[commit-bot: I haz the power, 2015-04-16 18:20:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087283002/60001

[pdr., 2015-04-16 18:54:54]

https://codereview.chromium.org/1087283002/diff/60001/LayoutTests/svg/filters...
File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right):

https://codereview.chromium.org/1087283002/diff/60001/LayoutTests/svg/filters...
LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:24:
setTimeout(changeValues, 200);
setTimeout 200ms? Can this be requestAnimationFrame(changeValues);?

https://codereview.chromium.org/1087283002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/filters/FEColorMatrix.cpp (right):

https://codereview.chromium.org/1087283002/diff/60001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:161: return m_type ==
FECOLORMATRIX_TYPE_MATRIX && m_values.size() >= kColorMatrixSize && m_values[19]
> 0;
Is 19 (kColorMatrixSize - 1)?

[Stephen Chennney, 2015-04-16 19:14:14]

Test updated to use requestAnimationFrame. It's a little more opaque, I suppose,
but more portable.

https://codereview.chromium.org/1087283002/diff/60001/LayoutTests/svg/filters...
File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right):

https://codereview.chromium.org/1087283002/diff/60001/LayoutTests/svg/filters...
LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:24:
setTimeout(changeValues, 200);
On 2015/04/16 18:54:53, pdr wrote:
> setTimeout 200ms? Can this be requestAnimationFrame(changeValues);?

Done.

https://codereview.chromium.org/1087283002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/filters/FEColorMatrix.cpp (right):

https://codereview.chromium.org/1087283002/diff/60001/Source/platform/graphic...
Source/platform/graphics/filters/FEColorMatrix.cpp:161: return m_type ==
FECOLORMATRIX_TYPE_MATRIX && m_values.size() >= kColorMatrixSize && m_values[19]
> 0;
On 2015/04/16 18:54:54, pdr wrote:
> Is 19 (kColorMatrixSize - 1)?

Yes, but in this context what's important about 19 is the location in the
matrix, not that it happens to be the last entry. So I would rather leave it as
is.

[Stephen Chennney, 2015-04-16 19:14:24]

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

[commit-bot: I haz the power, 2015-04-16 19:14:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087283002/80001

[pdr., 2015-04-16 19:19:17]

LGTM with or without my suggestion

https://codereview.chromium.org/1087283002/diff/80001/LayoutTests/svg/filters...
File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right):

https://codereview.chromium.org/1087283002/diff/80001/LayoutTests/svg/filters...
LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:25:
requestAnimationFrame(forcePaint);
Does this test really need three requestAnimationFrames? I suspect you can
change this one to requestAnimationFrame(changeValues);

[Stephen Chennney, 2015-04-16 19:30:32]

I think I've described the behavior.

https://codereview.chromium.org/1087283002/diff/80001/LayoutTests/svg/filters...
File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right):

https://codereview.chromium.org/1087283002/diff/80001/LayoutTests/svg/filters...
LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:25:
requestAnimationFrame(forcePaint);
On 2015/04/16 19:19:16, pdr wrote:
> Does this test really need three requestAnimationFrames? I suspect you can
> change this one to requestAnimationFrame(changeValues);

It's needed. The first one comes before any frame, so the scheduled animation
function (forcePaint) is called before the first paint. Then we schedule the
next one (changeValue), which happens after the first paint. It sets the values,
then requests another frame which leads to the second paint and the crash. We
have to schedule something then to happen after that frame (testDone) to finish
the test.

[fs, 2015-04-16 21:49:26]

lgtm

Thanks for fixing!

[commit-bot: I haz the power, 2015-04-16 22:32:42]

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

[commit-bot: I haz the power, 2015-04-16 22:32:43]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2015-04-16 23:39:26]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2015-04-16 23:39:50]

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

[commit-bot: I haz the power, 2015-04-16 23:40:46]

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