Bug 12609

Summary:

Any SVG element will create renderers even when children of HTML elements

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

Keywords:

InRadar

Priority:

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
First attempt	mjs: review-
Improved patch + testcase	mjs: review-
Improved patch	mjs: review+

Eric Seidel (no email)

Reported 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.

Attachments
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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 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.

Maciej Stachowiak

Comment 2 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?

Eric Seidel (no email)

Comment 3 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.

Eric Seidel (no email)

Comment 4 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.

Geoffrey Garen

Comment 5 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.

Geoffrey Garen

Comment 6 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.

Eric Seidel (no email)

Comment 7 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'> <body> <div> <circle xmlns='http://www.w3.org/2000/svg' /> </div> </body> </html> 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.

Rob Buis

Comment 8 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. Cheers, Rob.

Maciej Stachowiak

Comment 9 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.

Maciej Stachowiak

Comment 10 2007-02-23 00:57:33 PST

Element::shouldCreateRendererWithParent is not needed, given Node::rendererIsNeeded, as Rob discovered.

Rob Buis

Comment 11 2007-02-23 01:30:23 PST

Created attachment 13344 [details] Improved patch + testcase After pointers from Maciej, this should be much better. Cheers, Rob.

Darin Adler

Comment 12 2007-02-23 08:38:20 PST

Comment on attachment 13344 [details] Improved patch + testcase r=me

Maciej Stachowiak

Comment 13 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.

Rob Buis

Comment 14 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. Cheers, Rob.

Eric Seidel (no email)

Comment 15 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>?

Rob Buis

Comment 16 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. Cheers, Rob.

Maciej Stachowiak

Comment 17 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. r=me

Maciej Stachowiak

Comment 18 2007-02-27 19:33:24 PST

<rdar://problem/5028151>

Maciej Stachowiak

Comment 19 2007-02-28 00:57:10 PST

This was landed.

Note You need to log in before you can comment on or make changes to this bug.