Don't use a SubtreeLayoutScope object for SVG resource layout

Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout
when a child finishes. The assert is invalid, because the object is
about to be laid out so will not miss layout (we layout out of tree
order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence
avoiding the assert.

R=fs@opera.com
BUG=589389
Committed: https://crrev.com/c80d2b59c2357c6cae1dee9baef6fbaecb3c8136
Cr-Commit-Position: refs/heads/master@{#378858}

[Stephen Chennney, 2016-03-02 17:17:15]

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

[commit-bot: I haz the power, 2016-03-02 17:17:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1749423005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1749423005/20001

[Stephen Chennney, 2016-03-02 17:21:07]

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

[Stephen Chennney, 2016-03-02 17:21:08]

Fixing an issue with a pattern inside a pattern that is almost but not quite a
cycle. Re-ordering layout works to fix this and doesn't introduce other problems
according to tests.

In reality I'm thinking about refactoring this in a big way by adding a
post-style pre-layout pass that handles resource association and dirtying, then
a distinct layout pass for resources that we do before we lay out anything else.
We have so many problems right now that a big cleanup seems in order.

[fs, 2016-03-02 17:52:17]

On 2016/03/02 at 17:21:08, schenney wrote:
> Fixing an issue with a pattern inside a pattern that is almost but not quite a
cycle. Re-ordering layout works to fix this and doesn't introduce other problems
according to tests.
> 
> In reality I'm thinking about refactoring this in a big way by adding a
post-style pre-layout pass that handles resource association and dirtying, then
a distinct layout pass for resources that we do before we lay out anything else.
We have so many problems right now that a big cleanup seems in order.

Yes, I think it's clear _something_ needs to be done... We sort of have a
"post-style pre-layout pass" already, but I guess the interesting bit is how we
should manage that "set" of elements (the resource elements). (I've been having
ideas that we could register "active"/"referenced" resources during
style-recalc, and then we could look at that set at the right time/phase and run
updates.) It's possible we have a bug for this, but maybe it's old and dusty -
anyway - we should have one =).

As for this change, it LGTM with some suggestions. Hopefully two childlist-walks
is not _too_ bad (it's double the virtual calls.)

[f(malita), 2016-03-02 18:24:50]

On 2016/03/02 17:21:08, Stephen Chenney wrote:
> Fixing an issue with a pattern inside a pattern that is almost but not quite a
> cycle. Re-ordering layout works to fix this and doesn't introduce other
problems
> according to tests.

I'm kinda wary of giving up transitive resource layout - that property is what's
supposed to give us a topological order for the resource DAG.

Are you sure we can rely on the new rule (which is tied to document order) to
provide the same guarantees?  Can elements reference resources defined later,
but not visible within its parent container?

E.g.

<g>
  <rect fill="url(#p1)"/>
</g>

<g>
  <pattern id="p1">
  </pattern>
</g>

IIUC this CL, the resource is not a direct descendant of the common ancestor =>
it doesn't get laid out before the rect.

 
> In reality I'm thinking about refactoring this in a big way by adding a
> post-style pre-layout pass that handles resource association and dirtying,
then
> a distinct layout pass for resources that we do before we lay out anything
else.
> We have so many problems right now that a big cleanup seems in order.

Yeah, I think we all agree on this :)

I would also keep http://crbug.com/294900 in mind: if my theory is correct and
resource instancing is unavoidable, than that particular solution (instancing)
would prolly solve (some of) the layout order issues.

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/custom/patterns-nested-no-geometry.svg
(right):

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/patterns-nested-no-geometry.svg:4:
<pattern id="p1">
One way to look at solving the problem within the current framework is that p2
should not attempt to layout p1 - it should only layout structural elements not
definition elements.  Definition elements get laid out on reference, so it
should be safe to skip at this point, no?

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right):

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:262: void
layoutChild(LayoutObject* child, bool forceLayout, bool layoutSizeChanged, bool
transformChanged, bool isResourceContainer)
static or anonymous namespace

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:310: for
(LayoutObject* child = start->slowFirstChild(); child; child =
child->nextSibling()) {
Cache the start child and reuse below (to avoid two slowFirstChild calls)?

[commit-bot: I haz the power, 2016-03-02 18:34:45]

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

[commit-bot: I haz the power, 2016-03-02 18:34:46]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2016-03-02 19:08:37]

Thanks for the comments. I had the same thoughts Florin did about this not
fixing all the issues, and we're right. One of the clusterfuzz test cases still
crashes.

I'll hold off on updating and landing this until I figure out what's going wrong
in the other case.

[fs, 2016-03-02 19:13:24]

Meh, publish fail.

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/custom/patterns-nested-no-geometry.svg
(right):

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/patterns-nested-no-geometry.svg:4:
<pattern id="p1">
On 2016/03/02 at 18:24:50, f(malita) wrote:
> One way to look at solving the problem within the current framework is that p2
should not attempt to layout p1 - it should only layout structural elements not
definition elements.  Definition elements get laid out on reference, so it
should be safe to skip at this point, no?

It can't be skipped always though I think (i.e when there are no references to
#p1), or we might end up with a dirty tree in the end?

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp (right):

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:262: void
layoutChild(LayoutObject* child, bool forceLayout, bool layoutSizeChanged, bool
transformChanged, bool isResourceContainer)
Nit: Make this local to the compilation unit.

https://codereview.chromium.org/1749423005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp:312:
layoutChild(child, selfNeedsLayout, layoutSizeChanged, transformChanged, true);
I'm not sure we need to have this call layoutChild (it can rather be confusing),
since it will in practice do very little - child->isSVGText() and
child->isSVGShape() will be false and forceLayout itself will be cancelled out
by isResourceContainer==true. So we're left with:

    SubtreeLayoutScope layoutScope(*child);
    SVGLayoutSupport::layoutResourcesIfNeeded(child);
    child->layoutIfNeeded();

for the resource-container case (and the scope is only for the assert in the
constructor I think.)

So maybe it would make sense to recognize that the two cases are different
enough that using the same function might be more confusing than not? So keep
the old loop unchanged (adding the !isResourceContainer branch) and add a new
loop that just does the simple obvious stuff? If not, then  at least add a
comment explaining stuff.

[fs, 2016-03-02 19:23:15]

On 2016/03/02 at 18:24:50, fmalita wrote:
> On 2016/03/02 17:21:08, Stephen Chenney wrote:
> > Fixing an issue with a pattern inside a pattern that is almost but not quite
a
> > cycle. Re-ordering layout works to fix this and doesn't introduce other
problems
> > according to tests.
> 
> I'm kinda wary of giving up transitive resource layout - that property is
what's supposed to give us a topological order for the resource DAG.

I guess it's still reasonably transitive? pre-order-reference discovery (before)
vs. ... uhm, yeah, hard to call the new ordering something...

> Are you sure we can rely on the new rule (which is tied to document order) to
provide the same guarantees?  Can elements reference resources defined later,
but not visible within its parent container?
> 
> E.g.
> 
> <g>
>   <rect fill="url(#p1)"/>
> </g>
> 
> <g>
>   <pattern id="p1">
>   </pattern>
> </g>
> 
> IIUC this CL, the resource is not a direct descendant of the common ancestor
=> it doesn't get laid out before the rect.

I don't think anything changes in this example w/ this CL? rect -> pattern via
layoutResourcesIfNeeded.

> I would also keep http://crbug.com/294900 in mind: if my theory is correct and
resource instancing is unavoidable, than that particular solution (instancing)
would prolly solve (some of) the layout order issues.

[f(malita), 2016-03-02 19:49:38]

On 2016/03/02 19:23:15, fs wrote:
> On 2016/03/02 at 18:24:50, fmalita wrote:
> > On 2016/03/02 17:21:08, Stephen Chenney wrote:
> > > Fixing an issue with a pattern inside a pattern that is almost but not
quite
> a
> > > cycle. Re-ordering layout works to fix this and doesn't introduce other
> problems
> > > according to tests.
> > 
> > I'm kinda wary of giving up transitive resource layout - that property is
> what's supposed to give us a topological order for the resource DAG.
> 
> I guess it's still reasonably transitive? pre-order-reference discovery
(before)
> vs. ... uhm, yeah, hard to call the new ordering something...
> 
> > Are you sure we can rely on the new rule (which is tied to document order)
to
> provide the same guarantees?  Can elements reference resources defined later,
> but not visible within its parent container?
> > 
> > E.g.
> > 
> > <g>
> >   <rect fill="url(#p1)"/>
> > </g>
> > 
> > <g>
> >   <pattern id="p1">
> >   </pattern>
> > </g>
> > 
> > IIUC this CL, the resource is not a direct descendant of the common ancestor
> => it doesn't get laid out before the rect.
> 
> I don't think anything changes in this example w/ this CL? rect -> pattern via
> layoutResourcesIfNeeded.

Ah, good point - for some reason I assumed we no longer do that - I feel much
better about it then :)

But I still suspect it's possible to trigger the CF problem by hiding resources
under a different container.  Let see what Stephen finds.

[fs, 2016-03-02 20:05:36]

On 2016/03/02 at 19:49:38, fmalita wrote:
> On 2016/03/02 19:23:15, fs wrote:
> > On 2016/03/02 at 18:24:50, fmalita wrote:
> > > On 2016/03/02 17:21:08, Stephen Chenney wrote:
> > > > Fixing an issue with a pattern inside a pattern that is almost but not
quite
> > a
> > > > cycle. Re-ordering layout works to fix this and doesn't introduce other
> > problems
> > > > according to tests.
> > > 
> > > I'm kinda wary of giving up transitive resource layout - that property is
> > what's supposed to give us a topological order for the resource DAG.
> > 
> > I guess it's still reasonably transitive? pre-order-reference discovery
(before)
> > vs. ... uhm, yeah, hard to call the new ordering something...
> > 
> > > Are you sure we can rely on the new rule (which is tied to document order)
to
> > provide the same guarantees?  Can elements reference resources defined
later,
> > but not visible within its parent container?
> > > 
> > > E.g.
> > > 
> > > <g>
> > >   <rect fill="url(#p1)"/>
> > > </g>
> > > 
> > > <g>
> > >   <pattern id="p1">
> > >   </pattern>
> > > </g>
> > > 
> > > IIUC this CL, the resource is not a direct descendant of the common
ancestor
> > => it doesn't get laid out before the rect.
> > 
> > I don't think anything changes in this example w/ this CL? rect -> pattern
via
> > layoutResourcesIfNeeded.
> 
> Ah, good point - for some reason I assumed we no longer do that - I feel much
better about it then :)
> 
> But I still suspect it's possible to trigger the CF problem by hiding
resources under a different container.  Let see what Stephen finds.

Yeah, I suspect there'll still be cases that trigger it...

[Stephen Chennney, 2016-03-02 20:42:28]

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

[commit-bot: I haz the power, 2016-03-02 20:43:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1749423005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1749423005/40001

[Stephen Chennney, 2016-03-02 20:44:45]

This solution seems to fix the assertions simply by avoiding the assert. We do
catch the circular resources layout in the layout code itself, so there's no
real harm in avoiding the assertion here.

I've added another test to cover the case where the client is not in the same
SVG root as the resource.

[fs, 2016-03-02 20:49:40]

https://codereview.chromium.org/1749423005/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/custom/resource-nesting-layout-cycle-2-expected.html
(right):

https://codereview.chromium.org/1749423005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/resource-nesting-layout-cycle-2-expected.html:1:
<!DOCTYPE html>
This could be just the doctype (because it's an "empty" - nothing rendered -
document)

[f(malita), 2016-03-02 20:55:43]

Thanks, I like the latter approach better.  LGTM assuming fs is on board.

[Stephen Chennney, 2016-03-02 20:57:55]

https://codereview.chromium.org/1749423005/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/custom/resource-nesting-layout-cycle-2-expected.html
(right):

https://codereview.chromium.org/1749423005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/resource-nesting-layout-cycle-2-expected.html:1:
<!DOCTYPE html>
On 2016/03/02 20:49:40, fs wrote:
> This could be just the doctype (because it's an "empty" - nothing rendered -
> document)

Done.

[fs, 2016-03-02 20:58:24]

On 2016/03/02 at 20:55:43, fmalita wrote:
> Thanks, I like the latter approach better.  LGTM assuming fs is on board.

I'm on board - set sail for CQ!

[Stephen Chennney, 2016-03-02 21:03:19]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-03-02 21:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com,
fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1749423005/#ps60001
(title: "Simplify the test")

[Stephen Chennney, 2016-03-02 21:03:38]

The CQ bit was unchecked by schenney@chromium.org

[Stephen Chennney, 2016-03-02 21:06:17]

Description was changed from

==========
Lay out SVG resources before laying out geometry

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This was previously enforced within the
layout code for the geometry, but that causes problems when the resource
comes after the geometry in dom order, and the resource is
self-referencing.

This patch forces the resource children to lay out before geometry within
any given layout object, avoiding layout-within-layout problems.

R=fs@opera.com
BUG=589389
==========

to

==========
Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout when a child finishes. The assert
is invalid, because the object is about to be laid out so will not miss layout
(we layout out of tree order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence avoiding
the assert.

R=fs@opera.com
BUG=589389
==========

[Stephen Chennney, 2016-03-02 21:06:51]

Description was changed from

==========
Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout when a child finishes. The assert
is invalid, because the object is about to be laid out so will not miss layout
(we layout out of tree order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence avoiding
the assert.

R=fs@opera.com
BUG=589389
==========

to

==========
Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout
when a child finishes. The assert is invalid, because the object is
about to be laid out so will not miss layout (we layout out of tree
order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence
avoiding the assert.

R=fs@opera.com
BUG=589389
==========

[Stephen Chennney, 2016-03-02 21:07:06]

Description was changed from

==========
Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout
when a child finishes. The assert is invalid, because the object is
about to be laid out so will not miss layout (we layout out of tree
order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence
avoiding the assert.

R=fs@opera.com
BUG=589389
==========

to

==========
Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout
when a child finishes. The assert is invalid, because the object is
about to be laid out so will not miss layout (we layout out of tree
order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence
avoiding the assert.

R=fs@opera.com
BUG=589389
==========

[Stephen Chennney, 2016-03-02 21:07:15]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2016-03-02 21:07:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1749423005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1749423005/60001

[commit-bot: I haz the power, 2016-03-02 22:40:57]

Description was changed from

==========
Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout
when a child finishes. The assert is invalid, because the object is
about to be laid out so will not miss layout (we layout out of tree
order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence
avoiding the assert.

R=fs@opera.com
BUG=589389
==========

to

==========
Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout
when a child finishes. The assert is invalid, because the object is
about to be laid out so will not miss layout (we layout out of tree
order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence
avoiding the assert.

R=fs@opera.com
BUG=589389
==========

[commit-bot: I haz the power, 2016-03-02 22:40:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-02 22:42:11]

Description was changed from

==========
Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout
when a child finishes. The assert is invalid, because the object is
about to be laid out so will not miss layout (we layout out of tree
order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence
avoiding the assert.

R=fs@opera.com
BUG=589389
==========

to

==========
Don't use a SubtreeLayoutScope object for SVG resource layout

SVG geometry depends on having its related resources laid out before
it can be laid out itself. This is enforced within the
layout code for the geometry, but doing so fires an assert in SubtreeLayoutScope
due to the fact that objects still need layout
when a child finishes. The assert is invalid, because the object is
about to be laid out so will not miss layout (we layout out of tree
order for resources).

This patch removes the SubtreeLayoutScope for resource layout, hence
avoiding the assert.

R=fs@opera.com
BUG=589389

Committed: https://crrev.com/c80d2b59c2357c6cae1dee9baef6fbaecb3c8136
Cr-Commit-Position: refs/heads/master@{#378858}
==========

[commit-bot: I haz the power, 2016-03-02 22:42:12]

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