Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
174293005:
Fix crash in which elementData() might be null. (Closed)
|
Created:
6 years, 10 months ago by chrishtr Modified:
6 years, 10 months ago CC:
blink-reviews, sof, eae+blinkwatch, dglazkov+blink, adamk+blink_chromium.org, Inactive, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionFix crash in which elementData() might be null.
The issue is that an SVG animation may animate the class attribute. SVG tracks attributes, including class, internally. But it still needs to track style recalc dependencies on class, so it hooks into the class change machinery in Element. In order to do so, however, elementData() mus be non-null, which requires calling invalidateSVGElement() while mutating class in the SVG code.
BUG=345178
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167817
Patch Set 1 #
Total comments: 1
Patch Set 2 : Addressed comments. #Patch Set 3 : Addressed comments. #
Total comments: 5
Patch Set 4 : Addressed comment. #
Total comments: 2
Patch Set 5 : Fix test expectation. #
Total comments: 6
Patch Set 6 : Fixed. #Patch Set 7 : Fix. #
Messages
Total messages: 39 (0 generated)
This needs a test. Also, I think Adam is a better reviewer for this than me.
https://codereview.chromium.org/174293005/diff/1/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/174293005/diff/1/Source/core/dom/Element.cpp#... Source/core/dom/Element.cpp:1090: if (elementData()) { This is crazy. No element should have classAttributeChanged on it if it doesn't have any attributes (which is happening in this case since <svg> is for some reason I do not understand having classAttributeChanged called on it). Sorry, but I think I'm going to punt again to pdr for SVG expertise, since the SVG stuff just seems wrong. classAttributeChanged should be able to have an ASSERT(elementData()) at the top.
> https://codereview.chromium.org/174293005/diff/1/Source/core/dom/Element.cpp > File Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/174293005/diff/1/Source/core/dom/Element.cpp#... > Source/core/dom/Element.cpp:1090: if (elementData()) { > This is crazy. No element should have classAttributeChanged on it if it doesn't > have any attributes (which is happening in this case since <svg> is for some > reason I do not understand having classAttributeChanged called on it). > > Sorry, but I think I'm going to punt again to pdr for SVG expertise, since the > SVG stuff just seems wrong. classAttributeChanged should be able to have an > ASSERT(elementData()) at the top. This certainly needs a test. You can just copy the CF testcase into a file like so: <!DOCTYPE HTML> <html> <body> Test for crbug.com/345178: This test passes if it does not crash<br/> <svg> <set attributename="class" /> </svg> </body> </html> I agree with Adam that we should add "ASSERT(elementData())" at the top of classAttributeChanged (although I disagree with his assertion about SVG being crazy :) We should be calling classAttributeChanged for SVG elements. SVG special cases the class attribute because it's animatable (the set element is an example of an animation element). See the comments in SVGElement.cpp::parseAttribute and Element.h::classAttributeChanged about how the class attribute is handled. Nothing jumps out to me as regressing here--do you know what changed? Should SVG be calling ensureUniqueElementData before classAttributeChanged?
I changed it to use ensureUniqueElementData(). If you follow all of the paths to set attributes on regular Elements, they are careful to set element data, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... SVGElement is doing a one-off path here for class, so it will need to duplicate that logic (or somehow get rid of the one-off. The comment though indicate that is not easy to do). @pdr, I don't know if there are any other one-offs to be worried about here?
On 2014/02/21 18:19:56, Chris Harrelson wrote: > I changed it to use ensureUniqueElementData(). > > If you follow all of the paths to set attributes on regular Elements, they are > careful to set element data, e.g. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > SVGElement is doing a one-off path here for class, so it will need to duplicate > that logic (or somehow get rid of the one-off. The comment though indicate that > is not easy to do). > @pdr, I don't know if there are any other one-offs to be worried about here? I think this is the right approach. I grep'ed for HTMLNames in the SVG subdirectories and didn't find any other one-offs such as this (nor am I aware of any). LGTM
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishtr@chromium.org/174293005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...
not lgtm, you should not be blindly calling ensureUniqueElementData(), that defeats the ShareableElementData optimization. Why is SVG calling into SVGElement::svgAttributeChanged if there's no ElementData yet? That's a bug.
On 2014/02/24 05:04:59, esprehn wrote: > not lgtm, you should not be blindly calling ensureUniqueElementData(), that > defeats the ShareableElementData optimization. > > Why is SVG calling into SVGElement::svgAttributeChanged if there's no > ElementData yet? That's a bug. Do you have a suggestion for when ElementData should be created in this case? ensureUniqueElementData is called all over the place.
On 2014/02/24 05:14:35, pdr wrote: > On 2014/02/24 05:04:59, esprehn wrote: > > not lgtm, you should not be blindly calling ensureUniqueElementData(), that > > defeats the ShareableElementData optimization. > > > > Why is SVG calling into SVGElement::svgAttributeChanged if there's no > > ElementData yet? That's a bug. > > Do you have a suggestion for when ElementData should be created in this case? > ensureUniqueElementData is called all over the place. It should have been created at the point where we realized you had no elementData and we need to stuff a value into it way up near the top of the callstack. An attributeChanged callback is too far down. The only correct place is probably Element::setAttributeInternal and the things it calls inside Element. In general you only need to create a unique one if you have no elementData at al and you need to put a value into it, or if you have a shared one and you need to put a non-ElementData dependent value into it (ex. adding/removing an attribute). The "non-ElementData dependent value" part is key, for example filling in the idForStyleResolution" for an element doesn't require getting a unique one, since all elements with the same id attribute would share the same idForStyleResolution value so they can continue sharing the same ShareableElementData. At the point where Element::attributeChanged (which is what seems to call into svgAttributeChanged) is called we should have already decided to create an ElementData. How did we manage to get down there without ever allocating an ElementData?
I don't have a full answer to esprehn's question, but I can answer "how" we got here. If you look at the crash stack, you'll see SVGAnimateElement::applyResultsToTarget(). Note that there are multiple elements involved here: there's the instance of some subclass of SVGAnimateElement (in this case, SVGSetElement) and then there's |targetElement|, which in this case is the SVGSVGElement. This is the "crazy" I was hoping pdr could explain, as classAttributeChanged gets called on the target (<svg>), which has no attributes at all.
I changed the code to invalidate SVG attributes when starting SVG animations. invalidateSVGAttributes() will cause unique element data to be ensured, and also sets a dirty bit. Adam, Philip, Elliot, please take another look.
https://codereview.chromium.org/174293005/diff/320001/Source/core/svg/SVGAnim... File Source/core/svg/SVGAnimateElement.cpp (right): https://codereview.chromium.org/174293005/diff/320001/Source/core/svg/SVGAnim... Source/core/svg/SVGAnimateElement.cpp:205: targetElement->invalidateSVGAttributes(); Why did you put this here instead of next to the svgAttributeChanged call? It's not clear to me that all the code paths in this if statement actually touch attribute backing stores. If this is the correct place for it, it probably deserves a comment and some text in your CL description. https://codereview.chromium.org/174293005/diff/320001/Source/core/svg/SVGAnim... Source/core/svg/SVGAnimateElement.cpp:229: targetElement->invalidateSVGAttributes(); Same question as above.
https://codereview.chromium.org/174293005/diff/320001/Source/core/svg/SVGAnim... File Source/core/svg/SVGAnimateElement.cpp (right): https://codereview.chromium.org/174293005/diff/320001/Source/core/svg/SVGAnim... Source/core/svg/SVGAnimateElement.cpp:205: targetElement->invalidateSVGAttributes(); On 2014/02/24 21:33:01, adamk wrote: > Why did you put this here instead of next to the svgAttributeChanged call? It's > not clear to me that all the code paths in this if statement actually touch > attribute backing stores. If this is the correct place for it, it probably > deserves a comment and some text in your CL description. It is indeed the case that not all callers of these animations need invalidation of svg attributes. The alternative is essentially to to go back to the original version of the patch (which is what I think your suggestion amounts to). I also attempted to implement invalidation on update of the animation, in a method that is performing the mutation. Unfortunately that method is editing an SVG-internal class which is storing its own private copy of the attribute, then calling attributeChanged on the name of the attribute. Getting in code there is possible but a lot of work, and still doesn't reuse the Element::setAttribute call path which would solve the issue. The call stack in this example is: WebCore::SVGGraphicsElement::svgAttributeChanged WebCore::SVGSVGElement::svgAttributeChanged WebCore::notifyTargetAboutAnimValChange WebCore::notifyTargetAndInstancesAboutAnimValChange WebCore::SVGAnimateElement::applyResultsToTarget WebCore::SMILTimeContainer::updateAnimations ... Note that svgAttributeChanged is not being called from Element. It's a SVG-specific call path.
Yes, I'm basically asking you to go back toward what the original change did, though not in quite the same place. I think the call to invalidate should be in notifyTargetAboutAnimValChange, right above the call to svgAttributeChanged. esprehn's analysis was based on the assumption that this call stack went through Element::attributeChanged. But what's actually happening here is this: SVGAnimateElement knows that targetElement has some attributes that aren't necessarily reflected in its elementData. Thus the call to invalidateSVGAttributes(), which signals to targetElement that there's some data down in the SVG*Element hierarchy that's yet-to-be-reflected up in WebCore::Element's knowledge about itself. I hope that makes some sense.
Done. https://codereview.chromium.org/174293005/diff/320001/Source/core/svg/SVGAnim... File Source/core/svg/SVGAnimateElement.cpp (right): https://codereview.chromium.org/174293005/diff/320001/Source/core/svg/SVGAnim... Source/core/svg/SVGAnimateElement.cpp:205: targetElement->invalidateSVGAttributes(); On 2014/02/24 21:46:19, Chris Harrelson wrote: > On 2014/02/24 21:33:01, adamk wrote: > > Why did you put this here instead of next to the svgAttributeChanged call? > It's > > not clear to me that all the code paths in this if statement actually touch > > attribute backing stores. If this is the correct place for it, it probably > > deserves a comment and some text in your CL description. > > It is indeed the case that not all callers of these animations need invalidation > of svg attributes. The alternative is essentially to to go back to the original > version of the patch (which is what I think your suggestion amounts to). I also > attempted to implement invalidation on update of the animation, in a method that > is performing the mutation. Unfortunately that method is editing an SVG-internal > class which is storing its own private copy of the attribute, then calling > attributeChanged on the name of the attribute. Getting in code there is possible > but a lot of work, and still doesn't reuse the Element::setAttribute call path > which would solve the issue. > > The call stack in this example is: > WebCore::SVGGraphicsElement::svgAttributeChanged > WebCore::SVGSVGElement::svgAttributeChanged > WebCore::notifyTargetAboutAnimValChange > WebCore::notifyTargetAndInstancesAboutAnimValChange > WebCore::SVGAnimateElement::applyResultsToTarget > WebCore::SMILTimeContainer::updateAnimations > ... > > Note that svgAttributeChanged is not being called from Element. It's a > SVG-specific call path. Done. https://codereview.chromium.org/174293005/diff/320001/Source/core/svg/SVGAnim... Source/core/svg/SVGAnimateElement.cpp:229: targetElement->invalidateSVGAttributes(); On 2014/02/24 21:33:01, adamk wrote: > Same question as above. Done.
lgtm, but please update the CL description before landing
https://codereview.chromium.org/174293005/diff/340001/LayoutTests/svg/dom/set... File LayoutTests/svg/dom/set-class-attribute.html (right): https://codereview.chromium.org/174293005/diff/340001/LayoutTests/svg/dom/set... LayoutTests/svg/dom/set-class-attribute.html:4: Test for crbug.com/345178: This test passes if it does not crash<br/> Actually, you'll also want to add an expected.txt for this test, and set testRunner.dumpAsText() to avoid creating pixel results.
https://codereview.chromium.org/174293005/diff/340001/LayoutTests/svg/dom/set... File LayoutTests/svg/dom/set-class-attribute.html (right): https://codereview.chromium.org/174293005/diff/340001/LayoutTests/svg/dom/set... LayoutTests/svg/dom/set-class-attribute.html:4: Test for crbug.com/345178: This test passes if it does not crash<br/> On 2014/02/24 22:33:19, adamk wrote: > Actually, you'll also want to add an expected.txt for this test, and set > testRunner.dumpAsText() to avoid creating pixel results. Done.
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishtr@chromium.org/174293005/360001
The CQ bit was unchecked by adamk@chromium.org
The CQ bit was checked by adamk@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishtr@chromium.org/174293005/360001
The CQ bit was unchecked by adamk@chromium.org
https://codereview.chromium.org/174293005/diff/360001/LayoutTests/svg/dom/set... File LayoutTests/svg/dom/set-class-attribute.html (right): https://codereview.chromium.org/174293005/diff/360001/LayoutTests/svg/dom/set... LayoutTests/svg/dom/set-class-attribute.html:4: <script src="../../resources/js-test.js"></script> Including this means you don't need... https://codereview.chromium.org/174293005/diff/360001/LayoutTests/svg/dom/set... LayoutTests/svg/dom/set-class-attribute.html:7: testRunner.dumpAsText(); ...these two lines... https://codereview.chromium.org/174293005/diff/360001/LayoutTests/svg/dom/set... LayoutTests/svg/dom/set-class-attribute.html:11: Test for crbug.com/345178: This test passes if it does not crash<br/> but this text should be passed to description()
https://codereview.chromium.org/174293005/diff/360001/LayoutTests/svg/dom/set... File LayoutTests/svg/dom/set-class-attribute.html (right): https://codereview.chromium.org/174293005/diff/360001/LayoutTests/svg/dom/set... LayoutTests/svg/dom/set-class-attribute.html:4: <script src="../../resources/js-test.js"></script> On 2014/02/24 22:58:48, adamk wrote: > Including this means you don't need... Done. https://codereview.chromium.org/174293005/diff/360001/LayoutTests/svg/dom/set... LayoutTests/svg/dom/set-class-attribute.html:7: testRunner.dumpAsText(); On 2014/02/24 22:58:48, adamk wrote: > ...these two lines... Done. https://codereview.chromium.org/174293005/diff/360001/LayoutTests/svg/dom/set... LayoutTests/svg/dom/set-class-attribute.html:11: Test for crbug.com/345178: This test passes if it does not crash<br/> On 2014/02/24 22:58:48, adamk wrote: > but this text should be passed to description() Done.
still lgtm (hopefully esprehn is ok with this; it definitely seems like there's some refactoring to be done in SVG)
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishtr@chromium.org/174293005/380001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishtr@chromium.org/174293005/380001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishtr@chromium.org/174293005/400001
Message was sent while issue was closed.
Change committed as 167817 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
