Bug 62147 – Use styling test from ietestcenter fails

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

See the url.

[Rob Buis]

Created attachment 96251 [details]
Patch

[WebKit Review Bot]

Comment on attachment 96251 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 96251 [details]
Patch

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

[Nikolas Zimmermann]

Comment on attachment 96251 [details]
Patch

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

r- because of some style and build issues and some remaining questions:

> Source/WebCore/css/CSSStyleSelector.cpp:1295
> +        // TODO: shadow root?

What does that mean?
How does Dimitris solution handle this?

> Source/WebCore/css/CSSStyleSelector.cpp:6874
> +#if ENABLE(SVG)
> +void CSSStyleSelector::setUseElementContext(SVGUseElement *useElement)
> +{
> +    m_useElementContext = useElement;
> +}
> +#endif

Just inline it in the header.

> Source/WebCore/css/CSSStyleSelector.h:154
> +        void setUseElementContext(SVGUseElement *);

Extra whitespace.

> Source/WebCore/rendering/svg/RenderSVGShadowTreeRootContainer.cpp:47
> +        document()->styleSelector()->setUseElementContext(useElement);

How about introducing a SVGUseElementContextGuard, similar to your SVGDisplayPropertyGuard in css/ that you landed a while ago, seems safer?

[Rob Buis]

Hi Niko,

(In reply to comment #4)
> (From update of attachment 96251 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96251&action=review
> 
> r- because of some style and build issues and some remaining questions:
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:1295
> > +        // TODO: shadow root?
> 
> What does that mean?

I am sorry, I have been thinking ahead of time for bug 54465. It would be great if we could remove the isSVGShadowRoot check in checkSelector and take the selector fast path, but I have no "corresponding element" to go to for the SVG shadow root.

> How does Dimitris solution handle this?

I have to revisit that one, not sure if that is covered since this may be SVG specific.

> > Source/WebCore/css/CSSStyleSelector.cpp:6874
> > +#if ENABLE(SVG)
> > +void CSSStyleSelector::setUseElementContext(SVGUseElement *useElement)
> > +{
> > +    m_useElementContext = useElement;
> > +}
> > +#endif
> 
> Just inline it in the header.

I wanted to avoid including SVG Headers in CSSStyleSelector.h

> > Source/WebCore/css/CSSStyleSelector.h:154
> > +        void setUseElementContext(SVGUseElement *);
> 
> Extra whitespace.

Ok.

> > Source/WebCore/rendering/svg/RenderSVGShadowTreeRootContainer.cpp:47
> > +        document()->styleSelector()->setUseElementContext(useElement);
> 
> How about introducing a SVGUseElementContextGuard, similar to your SVGDisplayPropertyGuard in css/ that you landed a while ago, seems safer?

Good idea!
So I wanted basically to get discussion going. I don't really like that it is intrusive in css/ and I have to check whether it fits in with <use>/shadow dom redesign plans. OTOH having a fix in, even before redesign, may be good :)
Cheers,

Rob.

[Rob Buis]

It probably is a good idea to look at bug 62333 first. Fixing that should fix this problem as well, so it is good to keep this patch around, but hopefully we'll not need it since it adds even more SVG specific stuff to css/.
Cheers,

Rob.

[Rob Buis]

Created attachment 97595 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97595 [details]
Patch

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

[Nikolas Zimmermann]

Comment on attachment 97595 [details]
Patch

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

> Source/WebCore/svg/SVGUseElement.cpp:1078
> +    element->setCorrespondingElement(originalElement);

As discussed on IRC, this is very dangerous. No one clears the corresponding element, this is likely a source of dangling pointer crashes, if 'originalElement' dies and someone calls element->styleForRenderer() on the 'element' (which is the shadow tree element).

[Rob Buis]

(In reply to comment #9)
> (From update of attachment 97595 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97595&action=review
> 
> > Source/WebCore/svg/SVGUseElement.cpp:1078
> > +    element->setCorrespondingElement(originalElement);
> 
> As discussed on IRC, this is very dangerous. No one clears the corresponding element, this is likely a source of dangling pointer crashes, if 'originalElement' dies and someone calls element->styleForRenderer() on the 'element' (which is the shadow tree element).

Actually I am not sure whether that is a problem at present ; what I understand is that SVGStyledElement::childrenChanged causes recreation of the shadow tree, so the shadow element having this correspondingElement pointer will be deleted anyway.
Cheers,

Rob.

[Nikolas Zimmermann]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 97595 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=97595&action=review
> > 
> > > Source/WebCore/svg/SVGUseElement.cpp:1078
> > > +    element->setCorrespondingElement(originalElement);
> > 
> > As discussed on IRC, this is very dangerous. No one clears the corresponding element, this is likely a source of dangling pointer crashes, if 'originalElement' dies and someone calls element->styleForRenderer() on the 'element' (which is the shadow tree element).
> 
> Actually I am not sure whether that is a problem at present ; what I understand is that SVGStyledElement::childrenChanged causes recreation of the shadow tree, so the shadow element having this correspondingElement pointer will be deleted anyway.

You have to keep in mind that childrenChanged does not cause any recreation, it marks the shadow tree for recreation... That means the 'element' will have a dangling pointer, until it gets freed. But who gurantees that styleForRenderer() is _not called_ in between, from the time the shadow tree is marked to recreation to its recreation. This is a potential source of problems.

[Rob Buis]

Created attachment 101917 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 101917 [details]
Patch

Attachment 101917 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9244174

[Rob Buis]

Created attachment 101923 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101923 [details]
Patch

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

New failing tests:
svg/W3C-SVG-1.1-SE/struct-use-11-f.svg
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg

[Rob Buis]

Created attachment 101955 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 101955 [details]
Patch

Attachment 101955 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9251130

[Rob Buis]

Created attachment 101959 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 101959 [details]
Patch

Attachment 101959 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9236002

[Rob Buis]

Created attachment 101961 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101961 [details]
Patch

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

New failing tests:
svg/W3C-SVG-1.1-SE/struct-use-11-f.svg

[Nikolas Zimmermann]

Comment on attachment 101961 [details]
Patch

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

Looks great, almost r+. I'd like you to resolve some questions before I set r+:

> Source/WebCore/rendering/svg/SVGShadowTreeElements.cpp:103
> +    return document()->styleSelector()->styleForElement(this, 0, true/*allowSharing*/);

Can this ever become null? document() or styleSelector()?

> Source/WebCore/svg/SVGElement.cpp:264
> +    return hasRareSVGData() ? rareSVGData()->correspondingElement() : 0;

These methods are only ever used for shadow tree elements right? You should assure that the SVGElement lives in a shadow tree here.

> Source/WebCore/svg/SVGElementInstance.cpp:-112
> -        ASSERT((*it)->correspondingElement() == element);

Why did you remove this assert? It should still hold true, no?

> Source/WebCore/svg/SVGElementRareData.h:72
> +    SVGElement* correspondingElement() { return m_correspondingElement; }
> +    void setCorrespondingElement(SVGElement* correspondingElement) { m_correspondingElement = correspondingElement; }

I'm slightly worried about the point when this is cleared up.
Is it guaranteed that if the corresponding element dies, that the shadow tree element is _immediately_ rebuilt? No chance, that we'll store dangling pointers?

A possibility would be to lookup all instances of an element, in the SVGElement dtor, then walk all associated shadow tree elements, and clear the corresponding element...
Lemme know what you think, am I wrong? Is this not needed?

[Rob Buis]

Hi Niko,

(In reply to comment #22)
> (From update of attachment 101961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101961&action=review
> 
> Looks great, almost r+. I'd like you to resolve some questions before I set r+:
> 
> > Source/WebCore/rendering/svg/SVGShadowTreeElements.cpp:103
> > +    return document()->styleSelector()->styleForElement(this, 0, true/*allowSharing*/);
> 
> Can this ever become null? document() or styleSelector()?

No, also see Node::styleForRenderer.

> > Source/WebCore/svg/SVGElement.cpp:264
> > +    return hasRareSVGData() ? rareSVGData()->correspondingElement() : 0;
> 
> These methods are only ever used for shadow tree elements right? You should assure that the SVGElement lives in a shadow tree here.

Fixed locally.

> > Source/WebCore/svg/SVGElementInstance.cpp:-112
> > -        ASSERT((*it)->correspondingElement() == element);
> 
> Why did you remove this assert? It should still hold true, no?

True, fixed locally.

> > Source/WebCore/svg/SVGElementRareData.h:72
> > +    SVGElement* correspondingElement() { return m_correspondingElement; }
> > +    void setCorrespondingElement(SVGElement* correspondingElement) { m_correspondingElement = correspondingElement; }
> 
> I'm slightly worried about the point when this is cleared up.
> Is it guaranteed that if the corresponding element dies, that the shadow tree element is _immediately_ rebuilt? No chance, that we'll store dangling pointers?
> 
> A possibility would be to lookup all instances of an element, in the SVGElement dtor, then walk all associated shadow tree elements, and clear the corresponding element...
> Lemme know what you think, am I wrong? Is this not needed?

I thought when removing the element it would just call invalidateAllInstancesOfElement so it would be cleared, but it doesnt happen atm. It does get called for childrenChanged, but that is not enough. Doing what you said in dtor should work, but maybe better call invalidateAllInstancesOfElement in removedFromDocument?
Cheers,

Rob.

[Nikolas Zimmermann]

Comment on attachment 101961 [details]
Patch

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

Ok, can you upload a new patch?

> Source/WebCore/svg/SVGElement.cpp:431
> +        return document()->styleSelector()->styleForElement(correspondingElement(), parentNode() ? parentNode()->renderer()->style() : 0, false/*allowSharing*/);

Why is allowSharing false here and true in the other case?

>>> Source/WebCore/svg/SVGElementRareData.h:72
>>> +    void setCorrespondingElement(SVGElement* correspondingElement) { m_correspondingElement = correspondingElement; }
>> 
>> I'm slightly worried about the point when this is cleared up.
>> Is it guaranteed that if the corresponding element dies, that the shadow tree element is _immediately_ rebuilt? No chance, that we'll store dangling pointers?
>> 
>> A possibility would be to lookup all instances of an element, in the SVGElement dtor, then walk all associated shadow tree elements, and clear the corresponding element...
>> Lemme know what you think, am I wrong? Is this not needed?
> 
> I thought when removing the element it would just call invalidateAllInstancesOfElement so it would be cleared, but it doesnt happen atm. It does get called for childrenChanged, but that is not enough. Doing what you said in dtor should work, but maybe better call invalidateAllInstancesOfElement in removedFromDocument?
> Cheers,
> 
> Rob.

Sounds good as well.

[Rob Buis]

Created attachment 102250 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 102250 [details]
Patch

Looks great, r=me.

[Rob Buis]

Hi Niko,

(In reply to comment #24)
> (From update of attachment 101961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101961&action=review
> 
> Ok, can you upload a new patch?

I had it ready and thought I uploaded it yesterday afternoon :(

> > Source/WebCore/svg/SVGElement.cpp:431
> > +        return document()->styleSelector()->styleForElement(correspondingElement(), parentNode() ? parentNode()->renderer()->style() : 0, false/*allowSharing*/);
> 
> Why is allowSharing false here and true in the other case?

Unfortunately the styles in the shadow elements can't be shared due to different inheriting from the parent properties.
Cheers,

Rob.

[WebKit Review Bot]

Comment on attachment 102250 [details]
Patch

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

New failing tests:
svg/W3C-SVG-1.1-SE/struct-use-11-f.svg

[Rob Buis]

Landed in r91915.