RESOLVED FIXED 62147
Use styling test from ietestcenter fails
https://bugs.webkit.org/show_bug.cgi?id=62147
Summary Use styling test from ietestcenter fails
Rob Buis
Reported 2011-06-06 14:06:39 PDT
See the url.
Attachments
Patch (43.86 KB, patch)
2011-06-07 09:44 PDT, Rob Buis
no flags
Patch (45.26 KB, patch)
2011-06-17 07:12 PDT, Rob Buis
no flags
Patch (43.98 KB, patch)
2011-07-25 14:57 PDT, Rob Buis
no flags
Patch (84.86 KB, patch)
2011-07-25 15:23 PDT, Rob Buis
no flags
Patch (43.98 KB, patch)
2011-07-25 17:56 PDT, Rob Buis
no flags
Patch (43.98 KB, patch)
2011-07-25 18:44 PDT, Rob Buis
no flags
Patch (44.15 KB, patch)
2011-07-25 19:16 PDT, Rob Buis
no flags
Patch (44.81 KB, patch)
2011-07-28 06:03 PDT, Rob Buis
zimmermann: review+
webkit.review.bot: commit-queue-
Rob Buis
Comment 1 2011-06-07 09:44:19 PDT
WebKit Review Bot
Comment 2 2011-06-07 09:50:17 PDT
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 3 2011-06-07 10:04:17 PDT
Nikolas Zimmermann
Comment 4 2011-06-07 10:14:41 PDT
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
Comment 5 2011-06-07 13:36:31 PDT
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
Comment 6 2011-06-13 08:29:23 PDT
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
Comment 7 2011-06-17 07:12:58 PDT
WebKit Review Bot
Comment 8 2011-06-17 07:20:49 PDT
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 9 2011-06-17 07:44:12 PDT
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
Comment 10 2011-06-17 09:18:35 PDT
(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
Comment 11 2011-06-17 23:29:38 PDT
(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
Comment 12 2011-07-25 14:57:51 PDT
Gyuyoung Kim
Comment 13 2011-07-25 15:05:30 PDT
Rob Buis
Comment 14 2011-07-25 15:23:45 PDT
WebKit Review Bot
Comment 15 2011-07-25 17:21:09 PDT
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
Comment 16 2011-07-25 17:56:19 PDT
Gyuyoung Kim
Comment 17 2011-07-25 18:03:30 PDT
Rob Buis
Comment 18 2011-07-25 18:44:23 PDT
Gyuyoung Kim
Comment 19 2011-07-25 19:00:34 PDT
Rob Buis
Comment 20 2011-07-25 19:16:00 PDT
WebKit Review Bot
Comment 21 2011-07-25 20:08:21 PDT
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 22 2011-07-27 00:49:07 PDT
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
Comment 23 2011-07-27 11:36:02 PDT
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 24 2011-07-28 01:01:42 PDT
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
Comment 25 2011-07-28 06:03:00 PDT
Nikolas Zimmermann
Comment 26 2011-07-28 06:32:51 PDT
Comment on attachment 102250 [details] Patch Looks great, r=me.
Rob Buis
Comment 27 2011-07-28 07:03:54 PDT
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 28 2011-07-28 07:30:29 PDT
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
Comment 29 2011-07-28 08:36:19 PDT
Landed in r91915.
Note You need to log in before you can comment on or make changes to this bug.