Bug 81515 – SVG Resources layout needs refactoring

• Status: NEW
• Component: SVG

[Stephen Chenney]

SVG Resources are currently found in the same render tree as geometry elements, and use all the same layout infrastructure and are laid out in whatever order they happen to appear. This has been the source of at least one security problem (bug 81006) and possibly more. It may also lead to poor results, as things that depend on the resources may not have layout done when the resource changes.

Resources should be laid out before any geometry that uses them, and in a dependency ordering that lays out resources needed by other resources before their users. This will also aid in catching circular references in resources and other resource problems.

If it turns out that the resources and the geometry that uses them cannot be separated, then the entire SVG render tree should have layout done in dependency order. That is, rendered geometry that is also a resource must be part of the dependency ordering.

[Nikolas Zimmermann]

I think we only rely that layout for the resource is done, in order to access the "resourceBoundingBox" from SVGRenderSupport, when it figures out the repaint rect of an element, which has clipper/filter/maskers applied.

FloatRect RenderSVGResourceClipper::resourceBoundingBox(RenderObject* object)
{
    // Resource was not layouted yet. Give back the boundingBox of the object.
    if (selfNeedsLayout())
        return object->objectBoundingBox();
...

This could potentially lead to problems, agreed, but only regarding the repaint rect of an element, which may be too small.

For all other cases we don't need to access the resource renderers during layout, so its really independent. For rendering RenderSVGResourceContainers don't matter, they don't participate in that, as they are never directly painted.

All we do on layout() of a resource is:
void RenderSVGResourceContainer::layout()
{
    // Invalidate all resources if our layout changed.
    if (everHadLayout() && selfNeedsLayout())
        removeAllClientsFromCache();

    RenderSVGHiddenContainer::layout();
}

Note: we moved resources to create renderers explicitly some years ago, there were good reasons to do that. (I can elaborate on that if you want: manual style resolution, CSS style changes, etc.)

[Nikolas Zimmermann]

I can't access bug 81006 btw.

[Dirk Schulze]

(In reply to comment #2)
> I can't access bug 81006 btw.

me either.

[Stephen Chenney]

removeAllClientsFromCache calls setNeedsLayout, during layout, which is generally bad form.

I know that SVGRenderSupport, during layout, calls setNeedsLayout on children, but that is OK because they are children and the layout flags are explicitly not allowed to escape the child's subtree. However, code inside removeAllClientsFromCache calls setNeedsLayout all over the tree, including on things already laid out in the current layout pass.

The comments in #1 may also indicate why we are getting the bounding box for filter elements wrong. See bug 77660.

The fact that removeAllClientsFromCache is needed at all implies that other objects depend on things that occur during resource layout. Otherwise, why would we need to invalidate the thing using the resource?