Bug 12609 - Any SVG element will create renderers even when children of HTML elements
Summary: Any SVG element will create renderers even when children of HTML elements
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2007-02-05 05:47 PST by Eric Seidel (no email)
Modified: 2007-02-28 00:57 PST (History)
0 users

See Also:

First attempt (21.33 KB, patch)
2007-02-22 23:56 PST, Rob Buis
mjs: review-
Details | Formatted Diff | Diff
Improved patch + testcase (27.62 KB, patch)
2007-02-23 01:30 PST, Rob Buis
mjs: review-
Details | Formatted Diff | Diff
Improved patch (27.55 KB, patch)
2007-02-26 02:24 PST, Rob Buis
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-02-05 05:47:50 PST
Any SVG element will create renderers even when children of HTML elements

This likely will result in all sorts of bad behavior and crashes.  Either every createRenderer() needs a check to makes sure it's parent is an SVG element, or HTMLElement::childShouldCreateRenderer() should return false for SVG elements when the document is not an xhtml document and the element is not an <svg> element.
Comment 1 Eric Seidel (no email) 2007-02-05 06:42:38 PST
    // These two methods are mutually exclusive.  The former is used to do strict error-checking
    // when adding children via the public DOM API (e.g., appendChild()).  The latter is called only when parsing, 
    // to sanity-check against the DTD for error recovery.
    void checkAddChild(Node* newChild, ExceptionCode&); // Error-checking when adding via the DOM API
    virtual bool childAllowed(Node* newChild);          // Error-checking during parsing that checks the DTD

We need something like "childAllowed" which is called during appendChild, etc.  Something that can check the node name, etc.
Comment 2 Maciej Stachowiak 2007-02-05 20:01:15 PST
We should block creating the renderer, not adding the child. I don't think xhtml and html documents should behave any differently for this. Ideally it would be SVG elements that implement the rule, they can check their parent is an svg element at createRenderer() time, and the <svg> element can override that and not check. Should this really be a P1 without a known symptom?
Comment 3 Eric Seidel (no email) 2007-02-06 05:49:00 PST
I'd like to fix this by adding a

bool HTMLElement::childShouldCreateRenderer(Node* child)
    if (!child->isSVGElement() || child->hasTagName(SVGNames::svgTag))
        return true;

The problem with checking in createRenderer() is that then *every* SVG createRenderer would need to check, since each of them is unique.
Comment 4 Eric Seidel (no email) 2007-02-06 05:49:53 PST
Also, although I'm confident we could find at least one crash here, I'll downgrade this to a p2 until a crash is demonstrated.
Comment 5 Geoffrey Garen 2007-02-21 20:57:36 PST
If SVG elements put the render tree in an invalid state by design, I think this should be a P1. If nothing else, this is a serious security concern.
Comment 6 Geoffrey Garen 2007-02-21 21:00:42 PST
Marking P1 for now, so this doesn't get lost in the shuffle. Let's talk about this on IRC if you disagree, Eric.
Comment 7 Eric Seidel (no email) 2007-02-22 13:34:00 PST
I'm not sure what you mean "by design". :)  But yes, SVG renderers (and elements) generally assume that their parents are also SVG renderers (or elements).  This is part of the SVG spec, as SVG renderers require a "viewport" in order to measure distances from, etc.  (The concept of "containing block" in CSS is similar but not identical.)

WebKit currently would probably try to render something like this:

<html xmlns='http://www.w3.org/1999/xhtml'>
         <circle xmlns='http://www.w3.org/2000/svg' />

Even though it shouldn't, and it's likely that certain combinations of javascript calls (or other things which cause the renderer to reach for its containing viewport) could crash.

There are several ways to fix this:
1.  Make it disallowed for svg elements to be inserted under a parent other than an SVG element (mjs previously expressed over IRC that this would be "bad" since we currently only throw exceptions based on trying to insert bad node types, not bad node names).
2.  Change Element:: childShouldCreateRenderer() to disallow creation of SVG element renderers unless it's an SVGSVGElement, this would be overridden by SVGElement:: childShouldCreateRenderer() of course.  This is the easiest fix, but is perhaps a little hackish.
3.  Add some sort of "Element::shouldCreateRendererWithParent(Element*)" call and override it for SVGElement to produce this behavior.  (also clean, possibly hot code we're talking about here though.)
4.  Change every since SVGElement subclass createRenderer() to not create a renderer if the parent is not an SVGElement subclass.  (This is decent wrt OO design, but sucks in terms of the amount of duplicate code, and is prone to error.)

I recommend either 2. or 3.
Comment 8 Rob Buis 2007-02-22 23:56:15 PST
Created attachment 13343 [details]
First  attempt

This is another possible approach, maybe we need to call it 5? :) It makes svg renderer building slightly slower since more checks are needed, but doesnt change Node.h. I am not sure whether we should check that parent can be xhtml element or just parent can't be a html element. I am waiting with the testcase until I am sure I have the right approach.

Comment 9 Maciej Stachowiak 2007-02-23 00:56:11 PST
Comment on attachment 13343 [details]
First  attempt

+    if (parentNode() && (parentNode()->isSVGElement() ||
+        parentNode()->isDocumentNode() ||
+        parentNode()->namespaceURI() != HTMLNames::xhtmlNamespaceURI))
+        return StyledElement::rendererIsNeeded(style);
+    return false;

1) I don't think you should check for the HTML namespace, instead, you should check that the parent is in the SVG namespace (or an SVG element or whatever).

2) I don't think the document node check is right - <circle> as a root of a document should not render.

3) This should specifically allow the <svg:svg> element to render inside a parent of any namespace.
Comment 10 Maciej Stachowiak 2007-02-23 00:57:33 PST
Element::shouldCreateRendererWithParent is not needed, given Node::rendererIsNeeded, as Rob discovered.
Comment 11 Rob Buis 2007-02-23 01:30:23 PST
Created attachment 13344 [details]
Improved patch + testcase

After pointers from Maciej, this should be much better.

Comment 12 Darin Adler 2007-02-23 08:38:20 PST
Comment on attachment 13344 [details]
Improved patch + testcase

Comment 13 Maciej Stachowiak 2007-02-23 09:00:28 PST
Comment on attachment 13344 [details]
Improved patch + testcase

Actually, I think the latest patch is still wrong (discussing with Rob).

It will allow all SVG elements to render as children of non-elements (which I think should not be allowed) and will allow the to render with no parent (not sure why this is needed or how it can even come up).

r- for now, waiting on revised version.
Comment 14 Rob Buis 2007-02-26 02:24:20 PST
Created attachment 13375 [details]
Improved patch

The new check is much better. Unfortunately we have to keep checking for parentNode() because of the code in SVGUseElement I think.

Comment 15 Eric Seidel (no email) 2007-02-26 03:38:56 PST
Comment on attachment 13375 [details]
Improved patch

This changes behavior.

At least for things which already had overridden rendererIsNeeded.

Also, what about <svg:switch>?   That also overrides rendererIsNeeded().  What if you place one of those inside a <div>?
Comment 16 Rob Buis 2007-02-26 08:23:04 PST
Hi Eric,

(In reply to comment #15)
> (From update of attachment 13375 [details] [edit])
> This changes behavior.
> At least for things which already had overridden rendererIsNeeded.

I am not sure what you mean? You mean compared to the previous patch? I am trying to implement it according to comment #2, which seems to make sense to me.

> Also, what about <svg:switch>?   That also overrides rendererIsNeeded().  What
> if you place one of those inside a <div>?

AFAIK this is not allowed as direct child? AFAIK the only way for compound docs by inclusion is by using the document element (<svg>) as only possible child? The compound docs by inclusion specs doesnt make clear to me what is allowed here though.

Comment 17 Maciej Stachowiak 2007-02-27 17:30:40 PST
Comment on attachment 13375 [details]
Improved patch

I think this patch is fine, if it passes the layout tests. If Eric has concerns about specific cases, we can make new tests for those.

Comment 18 Maciej Stachowiak 2007-02-27 19:33:24 PST
Comment 19 Maciej Stachowiak 2007-02-28 00:57:10 PST
This was landed.