Bug 15443 – SVGImage does not support sub-resource loading

• Component: SVG
• Status: RESOLVED INVALID

[Eric Seidel (no email)]

SVGImage does not support sub-resource loading

This is kinda a good thing, because it prevents any sort of sub-resource loading loops, but it does lead to incorrect behavior.  I expect most authors would expect an SVG referenced by <img> to support at least one level of subresources.

I've attached a zip file containing a DRT-ready test case.

[Eric Seidel (no email)]

Created attachment 16606 [details]
test case

[Morley Abbott]

Created attachment 59152 [details]
svg image that should cause an error due to circular reference, but does not.

It seems that WebKit doesn't handle <image xlink:href> to svg's correctly. Circular references aren't causing an error, but instead recurse on themselves one time. 

According to the W3 SVG spec, 5.3:
"URI references that directly or indirectly reference themselves are treated as invalid circular references."
"An invalid circular URI reference represents an error"

Attached is an svg which has a red rect background, an X drawn with two lines in black, and an image in the top left quarter, with itself as the xlink:href. This should result in an error, do to the circular reference. Instead, we see that it has loaded itself in the top left corner, but only 1 level of recursion deep. 

This leads me to suspect that WebKit loads image resources, but specifically only to one level of recursive depth. Perhaps this was designed this way, as to avoid infinite recursion. As a simple solution, it could be possible to specify a larger depth, but it would still mean that it doesn't follow W3 spec for circular references. 

I'm still examining more test cases, and playing with gdb to better understand the issue.

[Morley Abbott]

Created attachment 59154 [details]
Circular Reference - should cause error, does not

Including svg inside archive, to maintain correct filename. Apologies for previous incorrect upload.

[Morley Abbott]

Created attachment 59398 [details]
sub-resource of non-svg file

Attached test case: img-recursion.svg has image element loading img-recursion2.svg. img-recursion2.svg has image element img-recursion.png. The last image is intentionally a png instead of an svg. This was to test whether the issue was specifically with loading SVG images as sub-resources, or if other image types have the same problem. 

Result: all images loaded within a sub-resource svg are ignored. 

On the one hand, this is good, as it means that the problem is probably in what happens during the loading of items inside the sub-resourced svg file. On the other, it leads me to suspect there is no code to handle infinite loops, and we're only avoiding the problem now because of this bug. 

Will continue to examine issue.

[Morley Abbott]

Created attachment 59767 [details]
HTML file including SVG image with sub-resources

Early on, I had made the assumption that WebKit would render an SVG file on its own the same way it would render it if it were included in an html file. This was incorrect. 

The attached test case contains:
img-recursion2.svg: a 50x100 rectangle, entirely green
img-recursion2.png: a png, identical to img-recursion2.svg
img-recursion.svg:  an svg image containing, a red rect 100x100, and side by side, the two other images over top of the red rect.
test.html: html tag, body tag, img tag with src=img-recursion.svg. 

When img-recursion.svg is loaded by webkit, the final render is of an entire green 100x100 rectangle (but is really two sub-resourced images, both 50x100 green rectangles, img-recursion2.svg and img-recursion2.png, overtop of a red rect). 

When we load the html file, we should expect to see an identical output. We do not- instead, we see the red rect object only. This means that svg image elements don't load any sub-resources at all, unless they are loaded on their own. Render dumps of both cases provide some interesting information. 

test.html:
layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x600
  RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x584
      RenderImage {IMG} at (0,0) size 100x100
      RenderText {#text} at (0,0) size 0x0

img-recursion.svg:
layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 100x100
  RenderSVGRoot {svg} at (0,0) size 100x100
    RenderPath {rect} at (0,0) size 100x100 [fill={[type=SOLID] [color=#FF0000]}] [data="M0.00,0.00 L100.00,0.00 L100.00,100.00 L0.00,100.00 Z"]
    RenderSVGImage {image} at (0,0) size 50x100
    RenderSVGImage {image} at (50,0) size 50x100

This leads me to suspect that the SubResource loading classes are not handling svg images correctly, while MainResource loading classes do. I will continue to dig deeper into these classes, and how they handle each case.

[Eric Seidel (no email)]

SVGImage is a gigantic hack:
http://trac.webkit.org/browser/trunk/WebCore/svg/graphics/SVGImage.cpp

It has to drive the load itself because the loader doesn't understand the idea of cached "Documents".

[Morley Abbott]

After speaking with various people within WebKit SVG, it seems the scope of this problem is quite large. 

Like Eric said, "SVGImage is a giant hack". In short, WebKit is cheating in a few ways when it comes to loading SVG images. For the most part, this does get the majority of images to render mostly correctly. Sub-resource loading, well, there are still issues. 

The potential fix is to re-architecture SVGImage in a manner than makes it work in a less hack-ish way. Discussions about this came down to an idea of a CachedDocument class to handle documents like SVG files. 

At present, I'm still very new to WebKit and SVG, so I'm following their recommendations to start with some smaller bugs, rather than make my first one a big architecture fix, as this would likely get very complex. If someone with more experience in the area would like to take a shot at it, feel free. 

The only other option is to attempt an even bigger hack to make this particular case work. But layering hacks on top of hacks is, in general, not a good idea. This problem needs a real solution.

[Cosmin Truta]

On the platforms affected by the bug 33971 (such as Chromium), there is an additional issue.
There are no visible effects, but when debugging, I noticed the XML processor loads the same file twice, because of a bug SVGImageLoader::sourceURI.
Adding bug 33971 to the list of blockers.

On a slightly related note, I'm not sure I understand why is bug 52593 said to block this bug.

[Cosmin Truta]

There are two problems to solve here:
(1) Load all subresources at all levels. (Currently, only the first level is loaded.)
(2) Do not render images that reference themselves cyclically, either directly or indirectly. (Currently, cyclic references are rendered.)

To solve (1), I must de-hack SVGImage::dataChanged, although I still have to understand how exactly this hack works in the first place.

To solve (2), Dirk suggested me to use Niko's SVGResourcesCache and SVGResourcesCycleSolver. As far as I understood from examining these classes and the surrounding code, the infrastructure already exists:
- RenderSVGModelObject::updateFromElement calls the cache / cycle solver, by calling SVGResourcesCache::clientUpdatedFromElement.
- RenderSVGImage is a subclass of RenderSVGModelObject, therefore the cycle solver should automagically work for SVG image elements.

However, the even the simplest case of self-recursion, involving one <image> referencing itself, fails to go through this path. See the test case below: WebKit shows two circles instead of one. (Morley Abbot also posted a similar test case in attachment 59154 [details]; see his comment #2 above.)

<!-- self-cycle.svg -->
<svg width="300px" height="100px" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <circle cx="50" cy="50" r="50" fill="blue"/>
  <image x="100" y="0" width="200" height="100" xlink:href="self-cycle.svg"/>
</svg>

I suppose this happens because the two instances of "self-cycle.svg" are assigned different render objects, and yet they should be the same object?

[Cosmin Truta]

(In reply to comment #7)
> The potential fix is to re-architecture SVGImage in a manner than makes it work in a less hack-ish way. Discussions about this came down to an idea of a CachedDocument class to handle documents like SVG files. 

After a brief discussion with Dirk on #ksvg, I understood that the caching mechanism that exists in rendering/svg/ should do this trick. Assuming that my understanding is correct, I wonder: how can SVGImage and SVGResourcesCache communicate (if they can communicate at all)?

SVGImage loads subresources through the appropriately-named "giant hack", which is SVGImage::dataChanged. SVGResourcesCache and SVGResourcesCycleSolver kick in later, after everything is loaded. I am having difficulty seeing how could I use at load-time the cache-handling code from the renderer, and I would greatly appreciate if someone could enlighten me.

[Dirk Schulze]

(In reply to comment #9)
> There are two problems to solve here:
> (1) Load all subresources at all levels. (Currently, only the first level is loaded.)
> (2) Do not render images that reference themselves cyclically, either directly or indirectly. (Currently, cyclic references are rendered.)
IIRC ie9 is rendering cycles up to 6 to 7 times. The point is, that we have to detect the cycle on a certain point and end the rendering at a certain point in time.


> To solve (2), Dirk suggested me to use Niko's SVGResourcesCache and SVGResourcesCycleSolver. As far as I understood from examining these classes and the surrounding code, the infrastructure already exists:
> - RenderSVGModelObject::updateFromElement calls the cache / cycle solver, by calling SVGResourcesCache::clientUpdatedFromElement.
> - RenderSVGImage is a subclass of RenderSVGModelObject, therefore the cycle solver should automagically work for SVG image elements.
> 
> However, the even the simplest case of self-recursion, involving one <image> referencing itself, fails to go through this path. See the test case below: WebKit shows two circles instead of one. (Morley Abbot also posted a similar test case in attachment 59154 [details]; see his comment #2 above.)
> 
I did not take a look to the code of RenderSVGModelObject. But, if it checks the subresources, we should see two circles, since self-cycle.svg is the source, the first <image> loads the first subresource and should be drawn. IMHO it's important that we don't draw the circle a third time. Can you check this please?

[Nikolas Zimmermann]

(In reply to comment #10)
> (In reply to comment #7)
> > The potential fix is to re-architecture SVGImage in a manner than makes it work in a less hack-ish way. Discussions about this came down to an idea of a CachedDocument class to handle documents like SVG files. 
> 
> After a brief discussion with Dirk on #ksvg, I understood that the caching mechanism that exists in rendering/svg/ should do this trick. Assuming that my understanding is correct, I wonder: how can SVGImage and SVGResourcesCache communicate (if they can communicate at all)?

grml, I don't have lot of time today unfortunately. Just wanted to say that SVGResourcesCache and co have nothing to do with solving your problem.

SVGResources stores a set of resource pointers used by a certian render object.
eg. RenderSVGPath using a fill gradient and a stroke pattern.

SVGResourcesCache just stores pointers to these RenderSVGResource* objects, which are in use by a certain RenderObject.

No way SVGImage could be integrated with that. Note SVGImage != RenderSVGImage. RenderSVGImage _uses_ resources itself, when eg. an <image> is clipped or filtered, but it's not a resource itself.

That's the same reason, why <use> doesn't participate in the SVGResourcesCycleSolver.

[Cosmin Truta]

(In reply to comment #11)
> I did not take a look to the code of RenderSVGModelObject. But, if it checks the subresources, we should see two circles, since self-cycle.svg is the source, the first <image> loads the first subresource and should be drawn. IMHO it's important that we don't draw the circle a third time. Can you check this please?

Maybe I'm reading the spec the wrong way, but in Section 17.1.4 "Processing of IRI references", there's the following statement:
"IRI references that directly or indirectly reference themselves are treated as invalid circular references."

So we should only see the first circle. Everything that follows is derived from an invalid circular reference and should not be rendered.

There is no circle drawn a 3rd time, but that's because no sub-resource (be it circularly-referenced or not) is loaded after the first level. That's what I meant in point (1) in my comment #9. For example:
  main.svg contains <image xlink:href="subres1.svg"/>
  subres1.svg contains <image xlink:href="subres2.svg"/>
  subres2.svg contains <image xlink:href="subres3.svg"/> etc.

Only the elements in main.svg and subres1.svg are rendered, regardless of cycles, and that's because of the way in which subresources are loaded through SVGImage::dataChanged.
See Morley's comment #5 and his attachment 59767 [details].

[Cosmin Truta]

Here is the summary of a discussion that I had with Eric Seidel:

SVGImage is a subclass of Image and is owned by CachedImage. It lives in MemoryCache and is shared by possibly many pages. This MemoryCache only knows how to hold subresources. It does not hold whole documents, and all the complexity that a whole document implies.

The proposed CachedDocument class would handle entire documents, unlike the current cached objects that aren't designed to hold onto other cached objects.

The current design (a bit old, but still mostly true):
http://webkit.org/blog/1188/how-webkit-loads-a-web-page/

The proposed design (how the loader *should* work):
https://docs.google.com/drawings/edit?id=1ko0LFteYpoXdmfYO1rYme6t-QXLPQdI1Z_ysejpOVYk&hl=en

[Cosmin Truta]

It appears there is ongoing work on caching main resources, in bug 49246.

[Cosmin Truta]

Adam Barth explained to me some a list of issues (which he believes to be SVG design errors) that affect the SVG <image> tag.

The <image> tag is good to be used with raster graphics, but not so much with SVG graphics, because it loses both semantics and performance during rasterization. The <use> tag serves the same purpose, while providing a cleaner design and a faster performance.

Let us make comparison with <iframe> in HTML. Everything that is contained by the iframe becomes the part of the render tree. For example, if a specific region within the contained iframe changes its appearance, the render tree "knows" to re-render that particular region.

In contrast, <image> in SVG is an abstraction that needs to be rendered, and not much else is known besides the fact that it's a bunch of pixels. The image referred to in <image> is a leaf in the render tree, regardless whether it is a JPEG, and animated GIF, or an SVG (be it a static or an animated image). Of course, the SVG image itself needs to be rendered, but the SVG render tree exists in isolation, it is not an organic part of the container's render tree, nor does it communicate with it. (As a side consequence, animations are not rendered.)

This situation can be resolved by augmenting the logic of the container that embeds the top-level SVG. If the container "knows" that the SVG <image> tag happens to point to another SVG image (not just a generic kind of image), then it should be possible to work around this design limitation. This means every kind of container (the broser window, the HTML container, the CSS container, etc.) must be modified accordingly. (For example, the CSS attribute "background-image" must know about it, too.) This is doable, but requires a huge amount of work.

In short, to make a comparison with HTML, external images (non-HTML) are embedded via the <img> tag, while external HTML files are embedded via the <iframe> tag, and the separate logic applies to each of these two separate cases by design. On the other hand, in SVG, external SVG images are embedded in the same manner as non-SVG images via the <image> tag, and this can only work if the SVG's container has specific logic to handle the two cases separately. Since this logic does not currently exist in WebKit, it needs to be implemented, separately, in each of the SVG's container classes.

[Cosmin Truta]

It is, in fact, much easier to use the <use> element instead of <image>, but with a caveat: the SVG specification does not allow <use> to reference an entire file. (This is unfortunate, the limitation is un-natural, and yet it would be "the right thing" to use when referencing external SVG files, just like <iframe> would do in HTML.)

This limitation of the <use> element can be worked around by adding an id at the top-level <svg> tag, as follows:

** file main.svg
<svg ...>
...
<use x="0" y="0" xlink:href="embedded.svg#main">
...
</svg>

** file embedded.svg
<svg id="main" ...>
...
</svg>

Unfortunately, external references in <use> do not work in WebKit at this moment. Mozilla and Opera handles them well, though.

[Cosmin Truta]

I am posting the previous example again, because I have just realized that I used "main" in two places, confusingly. Here is the non-confusing version:

** file "main.svg"
<svg ...>
...
<use x="0" y="0" xlink:href="embedded.svg#TOP">
...
</svg>

** file "embedded.svg"
<svg id="TOP" ...>
...
</svg>

[Nikolas Zimmermann]

(In reply to comment #16)
> Adam Barth explained to me some a list of issues (which he believes to be SVG design errors) that affect the SVG <image> tag.
> 
> The <image> tag is good to be used with raster graphics, but not so much with SVG graphics, because it loses both semantics and performance during rasterization. The <use> tag serves the same purpose, while providing a cleaner design and a faster performance.
> 
> Let us make comparison with <iframe> in HTML. Everything that is contained by the iframe becomes the part of the render tree. For example, if a specific region within the contained iframe changes its appearance, the render tree "knows" to re-render that particular region.

Agreed, that's the key difference.
 
> In short, to make a comparison with HTML, external images (non-HTML) are embedded via the <img> tag, while external HTML files are embedded via the <iframe> tag, and the separate logic applies to each of these two separate cases by design. On the other hand, in SVG, external SVG images are embedded in the same manner as non-SVG images via the <image> tag, and this can only work if the SVG's container has specific logic to handle the two cases separately. Since this logic does not currently exist in WebKit, it needs to be implemented, separately, in each of the SVG's container classes.

I'm not sure I follow here.
The 'SVGImage' class, is used to render an external foo.svg images, when using <img src="foo.svg"..> from a HTML or SVG document (HTMLImageElement / SVGImageElement). When rendering pure raster images aka. <img src="test.jpg"> the class is not used.
SVGImage doesn't integrate the 'target' documents rendering/DOM tree and the host document in any way.

I'm having trouble with:
" On the other hand, in SVG, external SVG images are embedded in the same manner as non-SVG images via the <image> tag, and this can only work if the SVG's container has specific logic to handle the two cases separately."

This is exactly what SVGImage is used for, any-non .svg image source is handled through WebCore::Image, and external .svg images are handled through WebCore::SVGImage.

To summarize, I don't see the problem "Since this logic does not currently exist in WebKit" as this kind of distinction exists and is implemented in trunk.

Can you elaborate what I'm misunderstanding or you? :-)

[Cosmin Truta]

(In reply to comment #19)
> > In short, to make a comparison with HTML, external images (non-HTML) are embedded via the <img> tag, while external HTML files are embedded via the <iframe> tag, and the separate logic applies to each of these two separate cases by design. On the other hand, in SVG, external SVG images are embedded in the same manner as non-SVG images via the <image> tag, and this can only work if the SVG's container has specific logic to handle the two cases separately. Since this logic does not currently exist in WebKit, it needs to be implemented, separately, in each of the SVG's container classes.
> 
> I'm not sure I follow here.
> The 'SVGImage' class, is used to render an external foo.svg images, when using <img src="foo.svg"..> from a HTML or SVG document (HTMLImageElement / SVGImageElement). When rendering pure raster images aka. <img src="test.jpg"> the class is not used.
> SVGImage doesn't integrate the 'target' documents rendering/DOM tree and the host document in any way.

I meant to say (if my understanding is correct) that, when rendering an SVG image, be it from an <img> tag in HTML, or from an <image> tag in SVG, the rendering below that SVGImageElement is disconnected from the rendering of the enclosing render tree above. Everything below that SVG image leaves in an isolated world, without communicating with the outside world, and that's because the concept of image (be it bitmap or vector) does not provide means for such communication.

Example:
When loading an SVG with subresources at the top level in the browser, or when embedding it in an HTML5 document, all the SVG elements, incl. subresources at the 1st level, show up in DumpRT. On the other hand, when loading the same SVG with subresources via the <img> tag in a HTML container, or via the <image> tag in an SVG container, or via the background-image CSS attribute, the subresources are not loaded, and the top-level SVG image is only a leaf (with nothing underneath) in DumpRT. Rendering of that top-level SVG image is done in a separate render tree, and the main render tree only sees the final array of pixels.

> This is exactly what SVGImage is used for, any-non .svg image source is handled through WebCore::Image, and external .svg images are handled through WebCore::SVGImage.

Yes, but Image and SVGImage do the same task: deliver pixels. RenderImage and RenderSVGImage sit in leaves.

I wanted to make a contrast between SVGImageElement and SVGUseElement. The spec does not allow <use> to refer to a whole external image file but let's ignore that for the moment. Compare:

<image href="foo.svg">
vs.
<use href="foo.svg"> (or <use href="foo.svg#TOP">)

In <use> case, the contents of "foo.svg" is part of the main render tree, just like the contents <iframe> is part of HTML's render tree, and everything that's below (animations, interactions, subresources, dynamic updates, etc.) is propagated all the way to the top.

In <image> case, the container sees only the pixels delivered either by BitmapImage or SVGImage, depending on whether href="foo.png" or href="foo.svg".

I will ask Adam to clarify if what I'm saying is unclear and/not entirely the way he meant it.
But from what I understood so far, I agree with him that it would have been better if <image> were restricted to foreign (i.e. non-SVG) images, while using <use> for handling external SVG content.

[Cosmin Truta]

(In reply to comment #19)
> To summarize, I don't see the problem "Since this logic does not currently exist in WebKit" as this kind of distinction exists and is implemented in trunk.

I was referring to the behavior of the SVG <image> tag. A separation in logic, between handling images in native SVG format vs. foreign non-SVG formats (e.g. by making SVGImageElement act more like a SVGUseElement if <image href=...> points to an SVG resource), would allow SVG subresources to behave exactly like their main SVG containers.

This kind of distinction would be one possible solution to the problem of loading and interacting with SVG subresources, but it does not currently exist.

[Tim Horton]

*** Bug 63670 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 97223 has been marked as a duplicate of this bug. ***

[Dirk Schulze]

SVGImages should never load sub resources. This is part of the security strategy.

[Martijn]

(In reply to comment #24)
> SVGImages should never load sub resources. This is part of the security strategy.

This is bollocks.

An HTML page can load css, and so should an SVG. There is no more security concern on loading css from a, SVG than there is from loading one from HTML.

On top of that, it breaks (or not-fully-implements) the standard.

On top of that, IE11 applies external stylesheets just fine. Do you want Microsoft to be better?

On top of that, this makes having multiple SVG assets with a single stylesheet impossible. We'll have to write a Grunt script of sorts to embed a stylesheet generated from SASS into the corresponding SVG files. This is an absolutely horrible workaround that increases bandwidth usage and takes up valuable time.

On top of that yet again, if there is some sort of security concern. Fix that. Don't remove a feature because one tinywiny part of it might possible have a chance of doing something naughty.

[Philip Rogers]

(In reply to comment #25)
> (In reply to comment #24)
> > SVGImages should never load sub resources. This is part of the security strategy.
> 
> This is bollocks.
> 
> An HTML page can load css, and so should an SVG. There is no more security concern on loading css from a, SVG than there is from loading one from HTML.
> 
> On top of that, it breaks (or not-fully-implements) the standard.
> 
> On top of that, IE11 applies external stylesheets just fine. Do you want Microsoft to be better?
> 
> On top of that, this makes having multiple SVG assets with a single stylesheet impossible. We'll have to write a Grunt script of sorts to embed a stylesheet generated from SASS into the corresponding SVG files. This is an absolutely horrible workaround that increases bandwidth usage and takes up valuable time.
> 
> On top of that yet again, if there is some sort of security concern. Fix that. Don't remove a feature because one tinywiny part of it might possible have a chance of doing something naughty.

Unfortunately the security vulnerabilities are serious and have been exploited in the wild. The web just wasn't designed to have images make subresource requests :/

This is being canonicalized in a spec, see: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26114 (and related bugs). If you would like to discuss the merits of this, please follow up with Anne. Microsoft has shown interest in this, and all other browsers block external resources. I think it is unlikely external requests will be made from SVG images in IE12.

Baking your assets into one file can improve the user experience in some cases since the number of round trips is reduced. Cacheability can be reduced though.

[Martijn]

> Unfortunately the security vulnerabilities are serious and have been exploited in the wild. The web just wasn't designed to have images make subresource requests :/

Neh, I'm not convinced that simply disabling said feature is the solution. It's a horrible workaround, and it's doing the SVG standard and Chromium's image not much good.

If there's a security concern, it should be fixed, not disabled. If regular CSS poses a security concern, we're not disabling it either.

> This is being canonicalized in a spec, see: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26114 (and related bugs). If you would like to discuss the merits of this, please follow up with Anne. Microsoft has shown interest in this, and all other browsers block external resources. I think it is unlikely external requests will be made from SVG images in IE12.

That's highly unlikely. Microsoft tends to bring out security updates to their browser fairly timely, so if it's still in IE11 (which it is), I suspect it will be in IE12 as well. And for good reason - it's a great feature.
 
> Baking your assets into one file can improve the user experience in some cases since the number of round trips is reduced. Cacheability can be reduced though.

I'm aware of that, and making my choices as appropriate.

[Philip Rogers]

(In reply to comment #27)

Martijn,

To summarize this thread, sites expect images to be self-contained and iframe/object/embed to allow subresources. The change to disallow subresources in svg images is just enforcing the expectations authors have around images. The security concern here is, in part, due to how websites expect images to work. You may respond that it's the web authors who are wrong, but we should take that discussion to a vendor-neutral spec process.

You clearly care about this issue and I agree with you that there's more we can do. I suspect there is a path forward for allowing sites to opt-in to svg image subresources, for example. I encourage you to pursue this through the spec process via the w3 bug I linked to earlier. If you have ideas on how to safely allow subresources to be loaded from images, please share that with the spec that's being written right now.

I'm not sure what your specific usecase looks like, but it may be possible for you to include your svg content via <object> or <embed> instead of <img> so that subresources are allowed.