Fix RenderSVGResourceContainer task re-ordering bug

Fix RenderSVGResourceContainer task re-ordering bug

The bug was affecting
LayoutTests/css3/filters/effect-reference-rename.html

Normally document.getElementById("NotMyFilter").id = "MyFilter"; executes
before RenderSVGResourceContainer::registerResource adds MyFilter.

With the scheduler these can happen in the opposite order leading to the
assert because a non SVG element (the Filter) gets added to the cache of
pending clients and the second call to
RenderSVGResourceContainer::registerResource chokes because it calls
SVGResourcesCache::clientStyleChanged on that element.


BUG=432129

[alex clarke (OOO till 29th), 2014-12-08 10:40:02]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[alex clarke (OOO till 29th), 2014-12-08 10:40:03]

This patch fixes the assert and image diff for
css3/filters/effect-reference-rename.html with the scheduler.

Unfortunately there are no unit tests for any of these SVG classes :(

[kouhei (in TOK), 2014-12-08 11:11:16]

kouhei@chromium.org changed reviewers:
+ fmalita@chromium.org, kouhei@chromium.org

[kouhei (in TOK), 2014-12-08 11:11:17]

+Florin

https://codereview.chromium.org/787563003/diff/20001/LayoutTests/css3/filters...
File LayoutTests/css3/filters/effect-reference-rename.html (right):

https://codereview.chromium.org/787563003/diff/20001/LayoutTests/css3/filters...
LayoutTests/css3/filters/effect-reference-rename.html:16:
document.getElementById("NotMyFilter").id = "MyFilter";
Would you add a comment here why notifyDone is needed?

https://codereview.chromium.org/787563003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceContainer.cpp (right):

https://codereview.chromium.org/787563003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceContainer.cpp:228: if
(renderer->node() && renderer->node()->isSVGElement()) {
Please add a comment above this line that SVGPendingElements may include
arbitrary element that uses CSS filter referencing SVG filter.

Nit: unneeded {}

[Sami, 2014-12-08 11:27:34]

Thanks for the fix Alex.

https://codereview.chromium.org/787563003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceContainer.cpp (right):

https://codereview.chromium.org/787563003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceContainer.cpp:226: StyleDifference
diff;
Looks like these two lines could go inside the if statement too.

[fs, 2014-12-08 12:22:00]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2014-12-08 12:22:01]

Based on the description it sounds like could've previously (scheduler-less)
been triggered by just delaying the "rename" (or insertion) of the filter
appropriately?

> a non SVG element (the Filter)

This makes no sense to me - the Filter most certainly is an SVG element - did
you mean "the image"?

> The LayoutTest also needs to be made asynchronous since it captures the image
too soon if the tasks are re-ordered.

This sounds a bit scary... Would moving the script-block into the body/html have
the same effect? (I'm a bit keen on knowing when the test runner thinks it
should sample the output...)

[f(malita), 2014-12-08 14:37:38]

fmalita@chromium.org changed reviewers:
+ senorblanco@chromium.org

[f(malita), 2014-12-08 14:48:11]

+senorblanco

On 2014/12/08 12:22:01, fs wrote:
> > a non SVG element (the Filter)
> 
> This makes no sense to me - the Filter most certainly is an SVG element - did
> you mean "the image"?

I don't recall all the details, but we most certainly allow non-SVG filter
clients also.

Which makes me wonder - is the clientStyleChanged() assert no longer accurate?
Or is the CSS filter path completely different in this regard and it's OK to not
support non-SVG clients here?

[alex clarke (OOO till 29th), 2014-12-08 14:56:10]

On 2014/12/08 14:48:11, Florin Malita wrote:
> +senorblanco
> 
> On 2014/12/08 12:22:01, fs wrote:
> > > a non SVG element (the Filter)
> > 
> > This makes no sense to me - the Filter most certainly is an SVG element -
did
> > you mean "the image"?
> 
> I don't recall all the details, but we most certainly allow non-SVG filter
> clients also.

Right, we where getting an HTML node passed in.

> Which makes me wonder - is the clientStyleChanged() assert no longer accurate?
I actually tried commenting out the assert and the test seemed to work.  I don't
know if the assert is safe to remove, although that would be simpler if true.

> Or is the CSS filter path completely different in this regard and it's OK to
not
> support non-SVG clients here?

[fs, 2014-12-08 15:13:03]

On 2014/12/08 14:56:10, alexclarke1 wrote:
> On 2014/12/08 14:48:11, Florin Malita wrote:
> > +senorblanco
> > 
> > On 2014/12/08 12:22:01, fs wrote:
> > > > a non SVG element (the Filter)
> > > 
> > > This makes no sense to me - the Filter most certainly is an SVG element -
> did
> > > you mean "the image"?
> > 
> > I don't recall all the details, but we most certainly allow non-SVG filter
> > clients also.
> 
> Right, we where getting an HTML node passed in.
> 
> > Which makes me wonder - is the clientStyleChanged() assert no longer
accurate?
> I actually tried commenting out the assert and the test seemed to work.  I
don't
> know if the assert is safe to remove, although that would be simpler if true.

clientStyleChanged has the rendererCanHaveResources predicate which pretty much
contradicts the asserts.

[Stephen White, 2014-12-08 15:15:52]

On 2014/12/08 14:48:11, Florin Malita wrote:
> +senorblanco
> 
> On 2014/12/08 12:22:01, fs wrote:
> > > a non SVG element (the Filter)
> > 
> > This makes no sense to me - the Filter most certainly is an SVG element -
did
> > you mean "the image"?
> 
> I don't recall all the details, but we most certainly allow non-SVG filter
> clients also.
> 
> Which makes me wonder - is the clientStyleChanged() assert no longer accurate?
> Or is the CSS filter path completely different in this regard and it's OK to
not
> support non-SVG clients here?

The CSS filter path is different, in that it maintains its own list of HTML
nodes 
with render layers via RenderSVGResourceContainer::addClientRenderLayer(), which

it invalidates via Node::filterNeedsPaintInvalidation(). So it's probably ok to 
skip the style changes here for now, since they'll be picked up by that
mechanism
instead (I think?).

(Tangentially, filterNeedsPaintInvalidation() is a pretty big hammer, and also
violates
the no-style-recalc-during-layout rule. It was the only way I could figure to
turn
an attribute change in SVG into a style update and repaint in HTML, but if
someone 
more savvy in the repaint code could come up with a better way, that would be 
much appreciated.)

[dsinclair, 2014-12-08 15:22:27]

On 2014/12/08 15:15:52, Stephen White wrote:
> (Tangentially, filterNeedsPaintInvalidation() is a pretty big hammer, and also
> violates
> the no-style-recalc-during-layout rule. It was the only way I could figure to
> turn
> an attribute change in SVG into a style update and repaint in HTML, but if
> someone 
> more savvy in the repaint code could come up with a better way, that would be 
> much appreciated.)


Is there a bug filed for this? If not, could you create one?

[Stephen White, 2014-12-08 15:26:21]

On 2014/12/08 15:22:27, dsinclair wrote:
> On 2014/12/08 15:15:52, Stephen White wrote:
> > (Tangentially, filterNeedsPaintInvalidation() is a pretty big hammer, and
also
> > violates
> > the no-style-recalc-during-layout rule. It was the only way I could figure
to
> > turn
> > an attribute change in SVG into a style update and repaint in HTML, but if
> > someone 
> > more savvy in the repaint code could come up with a better way, that would
be 
> > much appreciated.)
> 
> 
> Is there a bug filed for this? If not, could you create one?

Done. http://crbug.com/439970.

[alex clarke (OOO till 29th), 2014-12-08 16:02:46]

On 2014/12/08 12:22:01, fs wrote:
> Based on the description it sounds like could've previously (scheduler-less)
> been triggered by just delaying the "rename" (or insertion) of the filter
> appropriately?
> 
> > a non SVG element (the Filter)
> 
> This makes no sense to me - the Filter most certainly is an SVG element - did
> you mean "the image"?
> 
> > The LayoutTest also needs to be made asynchronous since it captures the
image
> too soon if the tasks are re-ordered.
> 
> This sounds a bit scary... Would moving the script-block into the body/html
have
> the same effect? (I'm a bit keen on knowing when the test runner thinks it
> should sample the output...)

Sadly no that didn't help. For synchronous tests there seems to be a race
between side effects of
HTMLDocumentParser::didReceiveParsedChunkFromBackgroundParser and the test
exiting.  I think it should be possible to make the test harness smarter but I'm
not sure how exactly.

[fs, 2014-12-08 16:15:28]

On 2014/12/08 16:02:46, alexclarke1 wrote:
> On 2014/12/08 12:22:01, fs wrote:
> > Based on the description it sounds like could've previously (scheduler-less)
> > been triggered by just delaying the "rename" (or insertion) of the filter
> > appropriately?
> > 
> > > a non SVG element (the Filter)
> > 
> > This makes no sense to me - the Filter most certainly is an SVG element -
did
> > you mean "the image"?
> > 
> > > The LayoutTest also needs to be made asynchronous since it captures the
> image
> > too soon if the tasks are re-ordered.
> > 
> > This sounds a bit scary... Would moving the script-block into the body/html
> have
> > the same effect? (I'm a bit keen on knowing when the test runner thinks it
> > should sample the output...)
> 
> Sadly no that didn't help.
TBH I didn't expect it to =) (they should be equivalent, since the <script> will
be reparented to <body> - or something like that, my HTML-parser nomenclature
may be faulty...)

> For synchronous tests there seems to be a race
> between side effects of
> HTMLDocumentParser::didReceiveParsedChunkFromBackgroundParser and the test
> exiting.  I think it should be possible to make the test harness smarter but
I'm
> not sure how exactly.

I would expect the test runner to sample the output after 'load', and include
any changes produced by the corresponding style-update/layout/etc. IIUC here, at
least the latter is not guaranteed? (That would be the "scary" bit, because it
seems to imply that a "too large" test could randomly fail.)

[alex clarke (OOO till 29th), 2014-12-08 18:21:29]

On 2014/12/08 16:15:28, fs wrote:
> On 2014/12/08 16:02:46, alexclarke1 wrote:
> > On 2014/12/08 12:22:01, fs wrote:
> > > Based on the description it sounds like could've previously
(scheduler-less)
> > > been triggered by just delaying the "rename" (or insertion) of the filter
> > > appropriately?
> > > 
> > > > a non SVG element (the Filter)
> > > 
> > > This makes no sense to me - the Filter most certainly is an SVG element -
> did
> > > you mean "the image"?
> > > 
> > > > The LayoutTest also needs to be made asynchronous since it captures the
> > image
> > > too soon if the tasks are re-ordered.
> > > 
> > > This sounds a bit scary... Would moving the script-block into the
body/html
> > have
> > > the same effect? (I'm a bit keen on knowing when the test runner thinks it
> > > should sample the output...)
> > 
> > Sadly no that didn't help.
> TBH I didn't expect it to =) (they should be equivalent, since the <script>
will
> be reparented to <body> - or something like that, my HTML-parser nomenclature
> may be faulty...)
> 
> > For synchronous tests there seems to be a race
> > between side effects of
> > HTMLDocumentParser::didReceiveParsedChunkFromBackgroundParser and the test
> > exiting.  I think it should be possible to make the test harness smarter but
> I'm
> > not sure how exactly.
> 
> I would expect the test runner to sample the output after 'load', and include
> any changes produced by the corresponding style-update/layout/etc. IIUC here,
at
> least the latter is not guaranteed? (That would be the "scary" bit, because it
> seems to imply that a "too large" test could randomly fail.)

I've been digging into this and I think it's an accident that making the test
async fixes the image diff.  For some reason FEColorMatrix::createImageFilter
doesn't always get invoked.

I'll try and find out why, unfortunately it looks like there's some kind of
timing bug involved so it make take a while to track down.

[alex clarke (OOO till 29th), 2014-12-09 16:00:26]

On 2014/12/08 18:21:29, alexclarke1 wrote:
> On 2014/12/08 16:15:28, fs wrote:
> > On 2014/12/08 16:02:46, alexclarke1 wrote:
> > > On 2014/12/08 12:22:01, fs wrote:
> > > > Based on the description it sounds like could've previously
> (scheduler-less)
> > > > been triggered by just delaying the "rename" (or insertion) of the
filter
> > > > appropriately?
> > > > 
> > > > > a non SVG element (the Filter)
> > > > 
> > > > This makes no sense to me - the Filter most certainly is an SVG element
-
> > did
> > > > you mean "the image"?
> > > > 
> > > > > The LayoutTest also needs to be made asynchronous since it captures
the
> > > image
> > > > too soon if the tasks are re-ordered.
> > > > 
> > > > This sounds a bit scary... Would moving the script-block into the
> body/html
> > > have
> > > > the same effect? (I'm a bit keen on knowing when the test runner thinks
it
> > > > should sample the output...)
> > > 
> > > Sadly no that didn't help.
> > TBH I didn't expect it to =) (they should be equivalent, since the <script>
> will
> > be reparented to <body> - or something like that, my HTML-parser
nomenclature
> > may be faulty...)
> > 
> > > For synchronous tests there seems to be a race
> > > between side effects of
> > > HTMLDocumentParser::didReceiveParsedChunkFromBackgroundParser and the test
> > > exiting.  I think it should be possible to make the test harness smarter
but
> > I'm
> > > not sure how exactly.
> > 
> > I would expect the test runner to sample the output after 'load', and
include
> > any changes produced by the corresponding style-update/layout/etc. IIUC
here,
> at
> > least the latter is not guaranteed? (That would be the "scary" bit, because
it
> > seems to imply that a "too large" test could randomly fail.)
> 
> I've been digging into this and I think it's an accident that making the test
> async fixes the image diff.  For some reason FEColorMatrix::createImageFilter
> doesn't always get invoked.
> 
> I'll try and find out why, unfortunately it looks like there's some kind of
> timing bug involved so it make take a while to track down.

I think I can see what's going on now, although I'm not sure what the best fix
is.

In a successful run the order of operations is:

1

[alex clarke (OOO till 29th), 2014-12-09 16:02:58]

Ooops I hit send too soon, anyway, I think I can see what's going on now,
although I'm not sure what the best fix
is.

In a successful run the order of operations is:
1. WebTestProxyBase::AnimateNow
2. Run script from body
3. Run the other script and set the filter
4. WebTestProxyBase::AnimateNow - layout
5. WebKitTestRunner::TestFinished

In a failed run the order of operations is:
1. WebTestProxyBase::AnimateNow
2. Run script from body
3. WebTestProxyBase::AnimateNow - layout
4. Run the other script  and set the filter
5. WebKitTestRunner::TestFinished

It feels like there's a missing layout, although I'm not sure it's safe to just
add one because of all the tests that rely on layout prints.

[fs, 2014-12-09 17:19:16]

On 2014/12/09 16:02:58, alexclarke1 wrote:
...
> In a failed run the order of operations is:
> 1. WebTestProxyBase::AnimateNow
> 2. Run script from body
> 3. WebTestProxyBase::AnimateNow - layout
> 4. Run the other script  and set the filter
> 5. WebKitTestRunner::TestFinished
> 
> It feels like there's a missing layout, although I'm not sure it's safe to
just
> add one because of all the tests that rely on layout prints.

When TestFinished is called, is there a new frame scheduled then
(WebTestProxyBase::animate_scheduled_), and is TestFinished being called from
TestRunner::WorkQueue::ProcessWorkSoon? If so, then maybe the latter should
always post a task to ensure ordering w.r.t pending work? Theorizing wildly...

[alex clarke (OOO till 29th), 2014-12-10 10:58:07]

After a bit more instrumentation I was able to find out why the order of the
script and the AnimateNow mattered.
In a successful run there where no pending resources and in a failed run there
where pending resources but crucially the enclosing layer's
RenderLayer::updateFilters never got called.  This meant the filter wasn't
applied.

I'm trying a different fix now.  I've removed the assert and added a call to
SVGResourcesCache::clientStyleChanged which calls  styleChanged on the enclosing
later if the renderer has a filter.  This fixed the image diff.

PTAL

[fs, 2014-12-10 12:32:46]

On 2014/12/10 10:58:07, alexclarke1 wrote:
> After a bit more instrumentation I was able to find out why the order of the
> script and the AnimateNow mattered.
> In a successful run there where no pending resources and in a failed run there
> where pending resources but crucially the enclosing layer's
> RenderLayer::updateFilters never got called.  This meant the filter wasn't
> applied.

So, this sounds very related to the "SVG filter layer update hack" mentioned
above... I take it the TC at http://jsfiddle.net/zw6b3vdg/ suffers from the same
problem (even without the scheduler)? Ie. what is missing is the magic
incantation for the hack, so maybe something like:

if (resourceType() == FilterResourceType && renderer->hasLayer())
    toRenderLayerModelObject(renderer)->layer()->filterNeedsPaintInvalidation();

(in registerResource, potentially w/ extra suspenders and asserts removed)

senorblanco?

[alex clarke (OOO till 29th), 2014-12-17 17:16:44]

On 2014/12/10 12:32:46, fs wrote:
> On 2014/12/10 10:58:07, alexclarke1 wrote:
> > After a bit more instrumentation I was able to find out why the order of the
> > script and the AnimateNow mattered.
> > In a successful run there where no pending resources and in a failed run
there
> > where pending resources but crucially the enclosing layer's
> > RenderLayer::updateFilters never got called.  This meant the filter wasn't
> > applied.
> 
> So, this sounds very related to the "SVG filter layer update hack" mentioned
> above... I take it the TC at http://jsfiddle.net/zw6b3vdg/ suffers from the
same
> problem (even without the scheduler)? Ie. what is missing is the magic
> incantation for the hack, so maybe something like:
> 
> if (resourceType() == FilterResourceType && renderer->hasLayer())
>    
toRenderLayerModelObject(renderer)->layer()->filterNeedsPaintInvalidation();
> 
> (in registerResource, potentially w/ extra suspenders and asserts removed)
> 
> senorblanco?

BUMP :) I'm really not that familiar with this part of the code so it would be
great if somebody more knowledgeable could chime in (assuming they're not on
vacation).

[alex clarke (OOO till 29th), 2015-01-28 16:09:58]

BUMP :)

https://codereview.chromium.org/787563003/diff/20001/LayoutTests/css3/filters...
File LayoutTests/css3/filters/effect-reference-rename.html (right):

https://codereview.chromium.org/787563003/diff/20001/LayoutTests/css3/filters...
LayoutTests/css3/filters/effect-reference-rename.html:16:
document.getElementById("NotMyFilter").id = "MyFilter";
On 2014/12/08 11:11:17, kouhei wrote:
> Would you add a comment here why notifyDone is needed?

Acknowledged.

https://codereview.chromium.org/787563003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceContainer.cpp (right):

https://codereview.chromium.org/787563003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceContainer.cpp:226: StyleDifference
diff;
On 2014/12/08 11:27:34, Sami wrote:
> Looks like these two lines could go inside the if statement too.

Acknowledged.

https://codereview.chromium.org/787563003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceContainer.cpp:228: if
(renderer->node() && renderer->node()->isSVGElement()) {
On 2014/12/08 11:11:17, kouhei wrote:
> Please add a comment above this line that SVGPendingElements may include
> arbitrary element that uses CSS filter referencing SVG filter.
> 
> Nit: unneeded {}

Acknowledged.

[kouhei (in TOK), 2015-01-29 06:54:14]

Would you add a pointer to the
LayoutTests/css3/filters/effect-reference-rename.html test in description?

I'm not completely sure if deleting the assert is OK or not. lg tm to my
knowledge, but I'm delegating review to fs@ and fmalita@.

[alex clarke (OOO till 29th), 2015-01-30 13:44:00]

On 2015/01/29 06:54:14, kouhei wrote:
> Would you add a pointer to the
> LayoutTests/css3/filters/effect-reference-rename.html test in description?
> 
> I'm not completely sure if deleting the assert is OK or not. lg tm to my
> knowledge, but I'm delegating review to fs@ and fmalita@.

Done

[fs, 2015-01-30 14:11:27]

On 2015/01/30 13:44:00, alexclarke1 wrote:
> On 2015/01/29 06:54:14, kouhei wrote:
> > Would you add a pointer to the
> > LayoutTests/css3/filters/effect-reference-rename.html test in description?
> > 
> > I'm not completely sure if deleting the assert is OK or not. lg tm to my
> > knowledge, but I'm delegating review to fs@ and fmalita@.
> 
> Done

I'd suggest not including it in the subject (subject and first line of
description now mismatch.)

I'd like to hear senorblanco opinion on
https://codereview.chromium.org/787563003/#msg22 (and this in general.)

[fs, 2015-04-08 16:05:57]

Picked up and continued in https://codereview.chromium.org/1068073005/