Bug 78902 – Integrate SVGUseElement within the new shadow root concept

• Status: NEW
• Component: SVG

[Nikolas Zimmermann]

Integrate SVGUseElement within the new shadow root concept.

[Nikolas Zimmermann]

Created attachment 127590 [details]
<use> without cloning prototype

Historical patch, some months ago: My attempt to rewrite <use> completly w/o any shadow trees.
The approach failed, as <text> selection etc. is not possible with that approach, as it renders <use> targets into ImageBuffers.
Maybe we can opt-it in for simple use-on-shape, situations, etc. as it's incredibe much faster than cloning.

NOTE: This is just for the record, so schenney can see it - it contains debug code, etc.

[Nikolas Zimmermann]

Created attachment 127591 [details]
Current prototype patch

And here's my new attempt: switches SVGUseElement to use ShadowRoot.
Die RenderSVGShadowTreeRootContainer & SVGShadowTreeElements!

[Nikolas Zimmermann]

Created attachment 128185 [details]
Patch v1

[Nikolas Zimmermann]

The code changes are actually below 50k, the rest is build system changes, file removal, and expectations updates - lots of them, due the changed shadow tree handling.

The patch should eliminate the root of all our past security bugs.

[Nikolas Zimmermann]

CC'ing more shadow dom folks. A pity, the bots don't like the patch (never seen that kind of crash)

[Early Warning System Bot]

Comment on attachment 128185 [details]
Patch v1

Attachment 128185 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11562467

[WebKit Review Bot]

Comment on attachment 128185 [details]
Patch v1

Attachment 128185 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11557492

[Philippe Normand]

Comment on attachment 128185 [details]
Patch v1

Attachment 128185 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11558488

[Nikolas Zimmermann]

Created attachment 128203 [details]
Patch v2

[Dimitri Glazkov (Google)]

Comment on attachment 128203 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=128203&action=review

Whoa, awesome patch (both in intent and in size :P)

The shadow DOM stuff looks good. Haven't looked at the SVG stuff closely, other than shadow tree management.

> Source/WebCore/css/SelectorChecker.cpp:-61
> -#if ENABLE(SVG)

(here and elsewhere) Why don't we need this flagged anymore?

> Source/WebCore/dom/Node.cpp:1523
> +bool Node::isInShadowTree() const

This could've probably be done as a separate patch.

> LayoutTests/platform/chromium/test_expectations.txt:-951
> -BUGCR23463 LEOPARD : svg/W3C-SVG-1.1/struct-symbol-01-b.svg = IMAGE
> -BUGCR23463 LINUX WIN : svg/W3C-SVG-1.1/struct-symbol-01-b.svg = IMAGE+TEXT
> -BUGCR23463 LINUX WIN : svg/W3C-SVG-1.1/struct-use-01-t.svg = PASS IMAGE+TEXT IMAGE

In these cases, we usually just comment them out, so that the previous bug info is preserved. Unless of course you fixed these!! :)

[Nikolas Zimmermann]

(In reply to comment #10)
> Whoa, awesome patch (both in intent and in size :P)
Thanks :-)

> The shadow DOM stuff looks good. Haven't looked at the SVG stuff closely, other than shadow tree management.
Excellent, I was looking for that kind of feedback. The SVG part can be reviewed by someone else, I think.
 
> > Source/WebCore/css/SelectorChecker.cpp:-61
> > -#if ENABLE(SVG)
> 
> (here and elsewhere) Why don't we need this flagged anymore?
isSVGElement() is always available now, and XLinkNames isn't surrounded by ENABLE(SVG) guards, so we can safely remove this.

> 
> > Source/WebCore/dom/Node.cpp:1523
> > +bool Node::isInShadowTree() const
> 
> This could've probably be done as a separate patch.
Sure, but it's just this single line?

> 
> > LayoutTests/platform/chromium/test_expectations.txt:-951
> > -BUGCR23463 LEOPARD : svg/W3C-SVG-1.1/struct-symbol-01-b.svg = IMAGE
> > -BUGCR23463 LINUX WIN : svg/W3C-SVG-1.1/struct-symbol-01-b.svg = IMAGE+TEXT
> > -BUGCR23463 LINUX WIN : svg/W3C-SVG-1.1/struct-use-01-t.svg = PASS IMAGE+TEXT IMAGE
> 
> In these cases, we usually just comment them out, so that the previous bug info is preserved. Unless of course you fixed these!! :)
I hope its fixed now, but I can of course only comment them, will include these changes in a follow-up patch, if needed.

[Stephen Chenney]

Comment on attachment 128203 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=128203&action=review

I'm not sure this will address all the security issues we've had, but that is no reason to hold back. I will probably apply it locally and see what difference it makes to the pathological cases we have. I may also need to fix https://bugs.webkit.org/show_bug.cgi?id=77764, sooner rather than later.

> Source/WebCore/css/SelectorChecker.cpp:-449
> -    // Spec: CSS2 selectors cannot be applied to the (conceptually) cloned DOM tree

Are you sure that whatever mechanism controls Style and shadow nodes will correctly work for SVG semantics?

> Source/WebCore/dom/EventDispatcher.cpp:70
> +    if (SVGElementInstance* instance = useElement->instanceForShadowTreeElement(referenceNode))

Won't this fail to build with SVG disabled, due to the lack of SVGElementInstance?

>> Source/WebCore/dom/Node.cpp:1523
>> +bool Node::isInShadowTree() const
> 
> This could've probably be done as a separate patch.

There are a couple of other portion of this patch that add const in existing code. I think they all go in one separate patch.

> Source/WebCore/dom/ScriptElement.cpp:202
>      if (!isScriptForEventSupported())

Is there code elsewhere to prevent script execution inside the shadow tree? https://bugs.webkit.org/show_bug.cgi?id=64419

> Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:165
> +    return renderer->isSVGShape() || renderer->isSVGText() || renderer->isSVGImage() || renderer->node()->hasTagName(SVGNames::useTag);

Maybe a method to encapsulate node()->hasTagName(SVGNames::useTag)? It seems to appear a lot.

> LayoutTests/platform/chromium/test_expectations.txt:-945
> -BUGCR8763 MAC : svg/custom/use-on-g-containing-foreignObject-and-image.svg = IMAGE

Reiterating Dimitri's request. Please comment out rather than removing.

Please also create a bug, assigned to me, to update the expectations when this change goes in and the build bots have produced new results. Mark the new bug as depending on this bug.

> LayoutTests/platform/chromium/test_expectations.txt:4374
> +

You can also mark all these as "... = TEXT IMAGE TEXT+IMAGE PASS" if you want to cover all bases. I generally do this to avoid noise for unexpected passing tests.

[Nikolas Zimmermann]

(In reply to comment #12)
> I'm not sure this will address all the security issues we've had, but that is no reason to hold back. I will probably apply it locally and see what difference it makes to the pathological cases we have. I may also need to fix https://bugs.webkit.org/show_bug.cgi?id=77764, sooner rather than later.
It won't address all, but lots of them (instanceUpdates-blocked concept is gone, which was needed as we had races before in that code). Did you have luck with the corner cases?

> > Source/WebCore/css/SelectorChecker.cpp:-449
> > -    // Spec: CSS2 selectors cannot be applied to the (conceptually) cloned DOM tree
> 
> Are you sure that whatever mechanism controls Style and shadow nodes will correctly work for SVG semantics?
Yeah, the SVG shadow tree is now styled correctly, w/o the need for us to early exit in checkSelector, and rely on a custom setStyle call afterwards, that correctly resolves the shadow tree styles with the <use> element as parent. It's all handled transparently by ShadowRoot now.

> > Source/WebCore/dom/EventDispatcher.cpp:70
> > +    if (SVGElementInstance* instance = useElement->instanceForShadowTreeElement(referenceNode))
> 
> Won't this fail to build with SVG disabled, due to the lack of SVGElementInstance?
This is guarded by ENABLE(SVG) already?

> >> Source/WebCore/dom/Node.cpp:1523
> > This could've probably be done as a separate patch.
> 
> There are a couple of other portion of this patch that add const in existing code. I think they all go in one separate patch.
Okay, I'll look for the other cases, and split them off.
 
> > Source/WebCore/dom/ScriptElement.cpp:202
> >      if (!isScriptForEventSupported())
> 
> Is there code elsewhere to prevent script execution inside the shadow tree? https://bugs.webkit.org/show_bug.cgi?id=64419
Thanks for the hint, I found the related bug 64781: that's exactly what I've fixed here. (shadow tree creation time)

> > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:165
> > +    return renderer->isSVGShape() || renderer->isSVGText() || renderer->isSVGImage() || renderer->node()->hasTagName(SVGNames::useTag);
> 
> Maybe a method to encapsulate node()->hasTagName(SVGNames::useTag)? It seems to appear a lot.
I could add a new method, but that needs to be virtual, and on SVGElement - as we certainly don't want to pollute Node.h for this. That would look like static_cast<SVGElement*>(node())->isSVGUseElement() - is it better? Certainly not faster.
 
> > LayoutTests/platform/chromium/test_expectations.txt:-945
> > -BUGCR8763 MAC : svg/custom/use-on-g-containing-foreignObject-and-image.svg = IMAGE
> 
> Reiterating Dimitri's request. Please comment out rather than removing.
Will take care.
 
> Please also create a bug, assigned to me, to update the expectations when this change goes in and the build bots have produced new results. Mark the new bug as depending on this bug.
Ok, will do.

> > LayoutTests/platform/chromium/test_expectations.txt:4374
> You can also mark all these as "... = TEXT IMAGE TEXT+IMAGE PASS" if you want to cover all bases. I generally do this to avoid noise for unexpected passing tests.
Ok.

[Stephen Chenney]

(In reply to comment #13)
> (In reply to comment #12)
> > I'm not sure this will address all the security issues we've had, but that is no reason to hold back. I will probably apply it locally and see what difference it makes to the pathological cases we have. I may also need to fix https://bugs.webkit.org/show_bug.cgi?id=77764, sooner rather than later.
> It won't address all, but lots of them (instanceUpdates-blocked concept is gone, which was needed as we had races before in that code). Did you have luck with the corner cases?

Today I will get to testing all the odd crash cases we have - I hope.

> > > Source/WebCore/dom/EventDispatcher.cpp:70
> > > +    if (SVGElementInstance* instance = useElement->instanceForShadowTreeElement(referenceNode))
> > 
> > Won't this fail to build with SVG disabled, due to the lack of SVGElementInstance?
> This is guarded by ENABLE(SVG) already?

Sorry, needed more context. This is in the #else of a #if ENABLED(SVG) block, so it is not protected. The code is protected at Debug runtime with an ASSERT_NEVER_REACHED, but that line will still try to compile in.

> 
> > > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:165
> > > +    return renderer->isSVGShape() || renderer->isSVGText() || renderer->isSVGImage() || renderer->node()->hasTagName(SVGNames::useTag);
> > 
> > Maybe a method to encapsulate node()->hasTagName(SVGNames::useTag)? It seems to appear a lot.
> I could add a new method, but that needs to be virtual, and on SVGElement - as we certainly don't want to pollute Node.h for this. That would look like static_cast<SVGElement*>(node())->isSVGUseElement() - is it better? Certainly not faster.

Sounds fine. I agree that it should be left as is.

Thanks for the expectations changes.

[Nikolas Zimmermann]

(In reply to comment #14)
> > > Won't this fail to build with SVG disabled, due to the lack of SVGElementInstance?
> > This is guarded by ENABLE(SVG) already?
> 
> Sorry, needed more context. This is in the #else of a #if ENABLED(SVG) block, so it is not protected. The code is protected at Debug runtime with an ASSERT_NEVER_REACHED, but that line will still try to compile in.
Heh, look again, the #else branch is removed as well. This won't affect non-SVG builds, they still work fine.

[Nikolas Zimmermann]

(In reply to comment #12)
> > This could've probably be done as a separate patch.
> 
> There are a couple of other portion of this patch that add const in existing code. I think they all go in one separate patch.
I looked again, the only const change in isInShadowTree - do you think that's worth to split off?? I'd say no, if it's just this single change.

[Stephen Chenney]

(In reply to comment #15)
> (In reply to comment #14)
> > > > Won't this fail to build with SVG disabled, due to the lack of SVGElementInstance?
> > > This is guarded by ENABLE(SVG) already?
> > 
> > Sorry, needed more context. This is in the #else of a #if ENABLED(SVG) block, so it is not protected. The code is protected at Debug runtime with an ASSERT_NEVER_REACHED, but that line will still try to compile in.
> Heh, look again, the #else branch is removed as well. This won't affect non-SVG builds, they still work fine.

O how I wish for a side-by-side review system.

And I too could only find one const. Given you are also changing a comment, and that comment is specific to this change (as is the const-ness?) I would leave it in this patch.

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

Created attachment 128862 [details]
Patch v3

Updated patch against trunk, and fixed chromium expectations.

[Zoltan Herczeg]

Comment on attachment 128862 [details]
Patch v3

Wow, really nice patch. I am always happy if we can simplify the code, and we can remove a lot of lines.

I trust you that the expected updates are correct.

Some thoughts:

View in context: https://bugs.webkit.org/attachment.cgi?id=128862&action=review

> Source/WebCore/ChangeLog:128
> +            <defs><rect id="rect">>/defs> <use xlink:href="#rect"/>. Now from a script we change the rect x/y/width/height attributes:

<defs><rect id="rect">>/defs>
The > should be <

> Source/WebCore/dom/EventDispatcher.cpp:64
> +    ASSERT(referenceNode);
> +    if (!referenceNode->isSVGElement() || !referenceNode->isInShadowTree())
> +        return referenceNode;
> +
>  #if ENABLE(SVG)

I think the lines above should be covered by the #if directive even if it can be compiled without SVG.

> Source/WebCore/rendering/svg/RenderSVGTransformableContainer.cpp:64
> +    FloatSize translation;
> +    if (useElement) {
> +        SVGLengthContext lengthContext(useElement);
> +        translation = FloatSize(useElement->x().value(lengthContext), useElement->y().value(lengthContext));
> +        if (translation != m_lastTranslation)
> +            m_needsTransformUpdate = true;
> +        m_lastTranslation = translation;
> +    }

FloatSize translation probably can be moved inside the if:
FloatSize translation(FloatSize(useElement->x().value(lengthContext), useElement->y().value(lengthContext)));

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:71
> +    SVGElement* correspondingElement = svg->correspondingElement();
> +    if (svg->isInShadowTree() && correspondingElement) {

I think the two conditions around the && could be swapped. Little perf gain.

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:113
> +        SVGLengthContext lengthContext(element);
> +        if (useElement->hasAttribute(SVGNames::widthAttr))
> +            m_viewport.setWidth(useElement->width().value(lengthContext));
> +        else if (isSymbolElement && svg->hasAttribute(SVGNames::widthAttr)) {
> +            SVGLength containerWidth(LengthModeWidth, "100%");
> +            m_viewport.setWidth(containerWidth.value(lengthContext));
> +        }
> +
> +        if (useElement->hasAttribute(SVGNames::heightAttr))
> +            m_viewport.setHeight(useElement->height().value(lengthContext));
> +        else if (isSymbolElement && svg->hasAttribute(SVGNames::heightAttr)) {
> +            SVGLength containerHeight(LengthModeHeight, "100%");
> +            m_viewport.setHeight(containerHeight.value(lengthContext));
> +        }

Just some thinking: setting up lengthContext if both "ifs" are false is unnecessary. Is it cheap enough? If not, perhaps we should avoid that, especially if the "ifs" are likely false.

> Source/WebCore/svg/SVGElement.cpp:484
> +        style = parent->renderer() ? parent->renderer()->style() : 0;

For me personally this looks ugly. Can't we just use a second if? And put a renderer into a local vvariable?

> Source/WebCore/svg/SVGLocatable.cpp:48
> -    for (ContainerNode* n = element->parentNode(); n; n = n->parentNode()) {
> -        if (isViewportElement(n))
> -            return static_cast<SVGElement*>(n);
> +    for (Element* current = element->parentOrHostElement(); current; current = current->parentOrHostElement()) {
> +        if (isViewportElement(current))
> +            return static_cast<SVGElement*>(current);

I know it was not in the old code, but perhaps we could add an ASSERT here. Or is it unnecessary?

> Source/WebCore/svg/SVGLocatable.cpp:60
> -    for (ContainerNode* n = element->parentNode(); n; n = n->parentNode()) {
> -        if (isViewportElement(n))
> -            farthest = static_cast<SVGElement*>(n);
> +    for (Element* current = element->parentOrHostElement(); current; current = current->parentOrHostElement()) {
> +        if (isViewportElement(current))
> +            farthest = static_cast<SVGElement*>(current);

ditto

> Source/WebCore/svg/SVGLocatable.cpp:88
> +    SVGElement* currentElement = element;
> +    while (currentElement) {

can currentElement be NULL? If not, we could make this a do ... while()

> Source/WebCore/svg/SVGUseElement.cpp:99
> +    RefPtr<SVGUseElement> use = adoptRef(new SVGUseElement(tagName, document));
> +    use->ensureShadowRoot();
> +    return use.release();

Can't this ensureShadowRoot() go to the constructor?

[Nikolas Zimmermann]

(In reply to comment #22)
> (From update of attachment 128862 [details])
> Wow, really nice patch. I am always happy if we can simplify the code, and we can remove a lot of lines.
Thanks.

> The > should be <
Fixed.

> I think the lines above should be covered by the #if directive even if it can be compiled without SVG.
Fixed.

> FloatSize translation probably can be moved inside the if:
> FloatSize translation(FloatSize(useElement->x().value(lengthContext), useElement->y().value(lengthContext)));
Fixed.

> I think the two conditions around the && could be swapped. Little perf gain.
Fixed.

> Just some thinking: setting up lengthContext if both "ifs" are false is unnecessary. Is it cheap enough? If not, perhaps we should avoid that, especially if the "ifs" are likely false.
Yes, we could avoid that, but I guess it's not worth it, lengthContext construction is cheap.

> For me personally this looks ugly. Can't we just use a second if? And put a renderer into a local vvariable?
Fixed.
 
> I know it was not in the old code, but perhaps we could add an ASSERT here. Or is it unnecessary?
No, it doesn't need to be a viewport element as parent.


> can currentElement be NULL? If not, we could make this a do ... while()
The whole construct makes no sense, I'll make it use a for-loop instead, operating on Elements, no need to make this SVGElement only, this is ancient legacy code.

> Can't this ensureShadowRoot() go to the constructor?
I followed the existing style in the other shadow tree constructing elements. I guess it can call virtual functions, and thus can't run from the constructor.

[Nikolas Zimmermann]

Created attachment 129223 [details]
Patch v4

[Zoltan Herczeg]

Comment on attachment 129223 [details]
Patch v4

r=me nincs work.

[Nikolas Zimmermann]

Committed r109097: <http://trac.webkit.org/changeset/109097>

[Nikolas Zimmermann]

Ossy rebaselined Qt in r109099. I rebaselined Gtk in r109100.
Win bots are still so broken, that I'm unable to obtain new results for Win, it early exits after 500 failures, before svg/ is reached :(

Still waiting for the SL bots to cycle.

[Nikolas Zimmermann]

Stephen, can you have a look at the chromium expectations, and the other <use> security bugs?

[Stephen Chenney]

(In reply to comment #28)
> Stephen, can you have a look at the chromium expectations, and the other <use> security bugs?

Yep. And I will shortly have up a patch for excluding invalid elements from <use>. That alone fixes the security issues even before your change.

[Nikolas Zimmermann]

I can't access bug 79798. I hope Stephen can take care of 79568, so we can close this bug soon.

[David Kilzer (:ddkilzer)]

<rdar://problem/10980087>

[Stephen Chenney]

You can close this if you would like. https://bugs.webkit.org/show_bug.cgi?id=79568 is tracking the chromium expectations and there is nothing in them that suggest problems with this patch.

[Eric Seidel (no email)]

Comment on attachment 129223 [details]
Patch v4

Cleared Zoltan Herczeg's review+ from obsolete attachment 129223 [details] so that this bug does not appear in http://webkit.org/pending-commit.