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.
// 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.
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?
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.
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.
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.
Marking P1 for now, so this doesn't get lost in the shuffle. Let's talk about this on IRC if you disagree, Eric.
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.
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.
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.
Element::shouldCreateRendererWithParent is not needed, given Node::rendererIsNeeded, as Rob discovered.
Created attachment 13344 [details] Improved patch + testcase After pointers from Maciej, this should be much better. Cheers, Rob.
Comment on attachment 13344 [details] Improved patch + testcase r=me
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.
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.
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>?
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.
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
<rdar://problem/5028151>
This was landed.