Bug 61600 – SVGRadialGradientElement::selfHasRelativeLengths() doesn't consider if cx() is relative

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Ryan Sleevi]

This was reported downstream in Chromium by a user running a static analyzer over the Chromium sources, which include WebKit.

The text of the original report (from PVS studio) is:

V501 There are identical sub-expressions 'cy ().isRelative ()' to the left and to the right of the '||' operator. webcore_svg svgradialgradientelement.cpp 253

bool SVGRadialGradientElement::selfHasRelativeLengths() const
{
  return cy().isRelative()
      || cy().isRelative()
      || r().isRelative()
      || fx().isRelative()
      || fy().isRelative();
}

The logic error is that cy().isRelative() is checked twice, rather than checking cx() and then cy()

[Ryan Sleevi]

Created attachment 95104 [details]
Patch 1

Patch to fix the above. As this is my first WebKit patch, please let me know if I'm incorrect in adding r? and cq?

[WebKit Review Bot]

Attachment 95104 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/svg/SVGRadial..." exit_code: 1

ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryan Sleevi]

Created attachment 95105 [details]
Patch 2

[WebKit Review Bot]

Attachment 95105 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/svg/SVGRadial..." exit_code: 1

ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryan Sleevi]

Created attachment 95106 [details]
Attempt to fix style errors

[Eric Seidel (no email)]

Comment on attachment 95106 [details]
Attempt to fix style errors

How do we test this?

[Ryan Sleevi]

(In reply to comment #6)
> (From update of attachment 95106 [details])
> How do we test this?

Unfortunately, I'm not sure. I'm not familiar with the WebKit code to know what may cause this path to be hit, just that it was "clearly" wrong. Either one of the calls to cy.isRelative() should be removed (since it is redundant) or the more likely answer, that because isRelative() has no side-effects/is const, the likely intent was for cx() and cy() to be checked, just as fx() and fy() are checked.

For what it's worth, the downstream report is at http://code.google.com/p/chromium/issues/detail?id=84141 (originally http://code.google.com/p/chromium/issues/detail?id=83873#c2 )

[Nikolas Zimmermann]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 95106 [details] [details])
> > How do we test this?
> 
> Unfortunately, I'm not sure. I'm not familiar with the WebKit code to know what may cause this path to be hit, just that it was "clearly" wrong. Either one of the calls to cy.isRelative() should be removed (since it is redundant) or the more likely answer, that because isRelative() has no side-effects/is const, the likely intent was for cx() and cy() to be checked, just as fx() and fy() are checked.
> 
> For what it's worth, the downstream report is at http://code.google.com/p/chromium/issues/detail?id=84141 (originally http://code.google.com/p/chromium/issues/detail?id=83873#c2 )

I have a suggestion for a test:
Use LayoutTests/svg/custom/relative-sized-content.xhtml  as template.

The hasRelativeLength() stuff is relevant, when you eg. apply your radialGradient with cx/cy in percentages to a target rectangle, which itself is relatively postionioned. Currently when resizing the window you won't see any changes of 'cx' (if it is relative, and 'cy' is absolute).

You can "mimic" window size changes, by embedding your <svg> in an outer <div> and resize that, this is what the test example I cited above does.

To summarize:
You'll need a svg with width/height 100%, a rect with w/h="100%" a radGrad with cy="50" (sth. absolute) and a percentage based cx. That should expose the bug....

Good catch, btw...

[Tony Chang]

@zimmermann: Do you think you can write a test for this?  This bug was caught by a static code analysis tool and Ryan was just trying to be helpful by writing the patch to fix the bug.  I think this is his first webkit patch so it might be quicker if you can write the layout test.

[Nikolas Zimmermann]

CC'ing Leo/Dirk/Rob - does any of you have some time for this? I'm still blocked by the CSS & SVG Font patches.

[Rob Buis]

Hi Niko,

(In reply to comment #10)
> CC'ing Leo/Dirk/Rob - does any of you have some time for this? I'm still blocked by the CSS & SVG Font patches.

Yes I'll have a look.
Cheers,

Rob.

[Rob Buis]

I looked into this but I am not sure whether these relative values are used at all, or at least when having the viewport width changed. Basically once the viewport changes the client for the gradient resource is removed from dictionary of clients, then in applyResource it is newly calculated and the percentage calculation will be fine. Let me know if I am missing anything...
Cheers,

Rob.

[Nikolas Zimmermann]

(In reply to comment #12)
> I looked into this but I am not sure whether these relative values are used at all, or at least when having the viewport width changed. Basically once the viewport changes the client for the gradient resource is removed from dictionary of clients, then in applyResource it is newly calculated and the percentage calculation will be fine. Let me know if I am missing anything...
> Cheers,
> 
> Rob.


Rob is right, we discussed this on IRC.
When assigning absolute values to the <rect> that uses the gradient, it still works.

This is the reason:

Breakpoint 8, WebCore::RenderSVGResourceGradient::removeClientFromCache (this=0x11b48cc98, client=0x11b43cc08, markForInvalidation=false) at RenderSVGResourceGradient.cpp:68
68	    ASSERT(client);
(gdb) bt
#0  WebCore::RenderSVGResourceGradient::removeClientFromCache (this=0x11b48cc98, client=0x11b43cc08, markForInvalidation=false) at RenderSVGResourceGradient.cpp:68
#1  0x00000001021e33a0 in WebCore::SVGResources::removeClientFromCache (this=0x11b43cd20, object=0x11b43cc08, markForInvalidation=false) at SVGResources.cpp:316
#2  0x00000001021ee8c3 in WebCore::invalidateResourcesOfChildren (start=0x11b43cc08) at SVGRenderSupport.cpp:210
#3  0x00000001021f10b8 in WebCore::SVGRenderSupport::layoutChildren (start=0x11b442738, selfNeedsLayout=false) at SVGRenderSupport.cpp:258
#4  0x00000001021f825f in WebCore::RenderSVGRoot::layout (this=0x11b442738) at RenderSVGRoot.cpp:240
#5  0x000000010220b10b in WebCore::RenderObject::layoutIfNeeded (this=0x11b442738) at RenderObject.h:539
#6  0x000000010212ba60 in WebCore::RenderBlock::layoutInlineChildren (this=0x11b449278, relayoutChildren=true, repaintLogicalTop=@0x7fff5fbfd8a8, repaintLogicalBottom=@0x7fff5fbfd8a4) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlockLineLayout.cpp:1175
#7  0x000000010210869b in WebCore::RenderBlock::layoutBlock (this=0x11b449278, relayoutChildren=true, pageLogicalHeight=0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1236
#8  0x000000010210026c in WebCore::RenderBlock::layout (this=0x11b449278) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1134
#9  0x0000000102107067 in WebCore::RenderBlock::layoutBlockChild (this=0x1081d7a38, child=0x11b449278, marginInfo=@0x7fff5fbfda70, previousFloatLogicalBottom=@0x7fff5fbfdae4, maxFloatLogicalBottom=@0x7fff5fbfdc80) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1977
#10 0x0000000102108046 in WebCore::RenderBlock::layoutBlockChildren (this=0x1081d7a38, relayoutChildren=false, maxFloatLogicalBottom=@0x7fff5fbfdc80) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1915
#11 0x00000001021086b4 in WebCore::RenderBlock::layoutBlock (this=0x1081d7a38, relayoutChildren=false, pageLogicalHeight=0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1238
#12 0x000000010210026c in WebCore::RenderBlock::layout (this=0x1081d7a38) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1134
#13 0x0000000102107067 in WebCore::RenderBlock::layoutBlockChild (this=0x1081ecd88, child=0x1081d7a38, marginInfo=@0x7fff5fbfde50, previousFloatLogicalBottom=@0x7fff5fbfdec4, maxFloatLogicalBottom=@0x7fff5fbfe060) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1977
#14 0x0000000102108046 in WebCore::RenderBlock::layoutBlockChildren (this=0x1081ecd88, relayoutChildren=false, maxFloatLogicalBottom=@0x7fff5fbfe060) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1915
#15 0x00000001021086b4 in WebCore::RenderBlock::layoutBlock (this=0x1081ecd88, relayoutChildren=false, pageLogicalHeight=0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1238
#16 0x000000010210026c in WebCore::RenderBlock::layout (this=0x1081ecd88) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1134
#17 0x0000000102107067 in WebCore::RenderBlock::layoutBlockChild (this=0x106da8178, child=0x1081ecd88, marginInfo=@0x7fff5fbfe230, previousFloatLogicalBottom=@0x7fff5fbfe2a4, maxFloatLogicalBottom=@0x7fff5fbfe440) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1977
#18 0x0000000102108046 in WebCore::RenderBlock::layoutBlockChildren (this=0x106da8178, relayoutChildren=false, maxFloatLogicalBottom=@0x7fff5fbfe440) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1915
#19 0x00000001021086b4 in WebCore::RenderBlock::layoutBlock (this=0x106da8178, relayoutChildren=false, pageLogicalHeight=0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1238
#20 0x000000010210026c in WebCore::RenderBlock::layout (this=0x106da8178) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1134
#21 0x000000010226fad9 in WebCore::RenderView::layout (this=0x106da8178) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderView.cpp:130
#22 0x0000000101aa0617 in WebCore::FrameView::layout (this=0x11a659690, allowSubtree=true) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/FrameView.cpp:1000
#23 0x00000001018c3bad in WebCore::Document::updateLayout (this=0x107119000) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/dom/Document.cpp:1612
#24 0x0000000102189d69 in WebCore::RenderLayer::hitTest (this=0x106deaf28, request=@0x7fff5fbfeab0, result=@0x7fff5fbfe850) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/rendering/RenderLayer.cpp:2846
#25 0x00000001018c2f1b in WebCore::Document::prepareMouseEvent (this=0x107119000, request=@0x7fff5fbfeab0, documentPoint=@0x7fff5fbfe940, event=@0x7fff5fbfebd0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/dom/Document.cpp:2625
#26 0x0000000101a00529 in WebCore::EventHandler::prepareMouseEvent (this=0x1070d8818, request=@0x7fff5fbfeab0, mev=@0x7fff5fbfebd0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/EventHandler.cpp:1930
#27 0x0000000101a07d90 in WebCore::EventHandler::handleMouseMoveEvent (this=0x1070d8818, mouseEvent=@0x7fff5fbfebd0, hoveredNode=0x7fff5fbfeb00) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/EventHandler.cpp:1610
#28 0x0000000101a08333 in WebCore::EventHandler::mouseMoved (this=0x1070d8818, event=@0x7fff5fbfebd0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/EventHandler.cpp:1542
#29 0x0000000101a0e306 in WebCore::EventHandler::mouseMoved (this=0x1070d8818, event=0x11b473fc0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebCore/page/mac/EventHandlerMac.mm:606
#30 0x00000001010de126 in -[WebHTMLView(WebPrivate) _updateMouseoverWithEvent:] (self=0x106dee300, _cmd=0x7fff8481d2b5, event=0x11b473fc0) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:1619
#31 0x00000001010c9b21 in -[WebHTMLView(WebPrivate) _updateMouseoverWithFakeEvent] (self=0x106dee300, _cmd=0x7fff8483af0e) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:1201
#32 0x00000001010d4188 in -[WebHTMLView mouseUp:] (self=0x106dee300, _cmd=0x7fff82b1368c, event=0x11b40b610) at /Users/nikolaszimmermann/Coding/WebKit/Source/WebKit/mac/WebView/WebHTMLView.mm:3665
#33 0x00007fff8250d3d9 in -[NSWindow sendEvent:] ()
#34 0x00000001000421b5 in ?? ()
#35 0x0000000100042142 in ?? ()
#36 0x00007fff82442a86 in -[NSApplication sendEvent:] ()
#37 0x0000000100038e72 in ?? ()
#38 0x00007fff823d94da in -[NSApplication run] ()
#39 0x00007fff823d21a8 in NSApplicationMain ()
#40 0x0000000100009f18 in ?? ()
(gdb)

[Nico Weber]

So what's the way forward here? Accept the patch as-is?

[Dirk Schulze]

Comment on attachment 95106 [details]
Attempt to fix style errors

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

r=m, but please fix the changelog. Not sure if you have commit rights. If not, please upload a new patch and set 'r' and 'cq' to '?'. I'll review it again.

> ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Correct the logic for checking for relative lengths in an SVGRadialGradientElement to consider cx() in addition to cy()
> +        https://bugs.webkit.org/show_bug.cgi?id=61600.
> +
> +        * Source/WebCore/svg/SVGRadialGradientElement.cpp:

This should be:

Reviewed by NOBODY (OOPS!).

SVGRadialGradientElement::selfHasRelativeLengths() doesn't consider if cx() is relative
https://bugs.webkit.org/show_bug.cgi?id=61600.

Correct the logic for checking for relative lengths in an SVGRadialGradientElement to consider cx() in addition to cy()

...

(With correct indention of course)
Please add a comment that there is no way to test it and why it can't be tested as well.

[Ryan Sleevi]

Created attachment 99050 [details]
Patch

[Dirk Schulze]

Comment on attachment 99050 [details]
Patch

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

r- because you're not a commiter yet and can't correct the patch before landing it.

> Source/WebCore/ChangeLog:9
> +        No new tests, as there is no way to currently test this. With the bug present, a side-effect is causing the expected/correct image to be rendered. This is because the cached SVG resource is getting invalided through a different path, forcing a repaint. The repaint takes into consideration the proper dimensions, hiding the bug.

Just a snippet, can you please break the line? This comment is too long to stay in one line. Lines shouldn't be longer than the line with the sentence "Correct the logic....".

[Ryan Sleevi]

Created attachment 99051 [details]
Patch

[Ryan Sleevi]

Comment on attachment 99051 [details]
Patch

Updated the ChangeLog per your request.

Since this has happened with both patches that I've submitted - where they didn't conform to certain styles/expectations - is there any documentation about this on webkit.org that I might read, saving cycles for both myself and reviewer?

I've read much of the documentation, technical articles, and read perhaps way too much on the Wiki, but it seems these minor things keep biting me.

[Dirk Schulze]

(In reply to comment #19)
> (From update of attachment 99051 [details])
> Updated the ChangeLog per your request.
> 
> Since this has happened with both patches that I've submitted - where they didn't conform to certain styles/expectations - is there any documentation about this on webkit.org that I might read, saving cycles for both myself and reviewer?
> 
> I've read much of the documentation, technical articles, and read perhaps way too much on the Wiki, but it seems these minor things keep biting me.

You can use the script:


Tools/Scripts/prepare-ChangeLog --bug <bug number> --email=<your email>

It will create the ChangeLogs for you. You just have to fill the description.

[Ryan Sleevi]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 99051 [details] [details])
> > Updated the ChangeLog per your request.
> > 
> > Since this has happened with both patches that I've submitted - where they didn't conform to certain styles/expectations - is there any documentation about this on webkit.org that I might read, saving cycles for both myself and reviewer?
> > 
> > I've read much of the documentation, technical articles, and read perhaps way too much on the Wiki, but it seems these minor things keep biting me.
> 
> You can use the script:
> 
> 
> Tools/Scripts/prepare-ChangeLog --bug <bug number> --email=<your email>
> 
> It will create the ChangeLogs for you. You just have to fill the description.

Thanks. Unfortunately, that's exactly the tool I was using, which didn't help with this particular style issue.

I suppose I expected it to complain about the line length (since check-webkit-style is so good about such issues), or, failing that, I was hoping to find documentation somewhere as far as expectations for comment style, tests format, etc, similar to the WebKit coding style documentation.

While less of an issue with this bug, compared to the other bug I filed, I wanted to try to be a bit more proactive in understanding "The WebKit Way" for such contributions. Thanks for the r+/cq+.

[WebKit Review Bot]

Comment on attachment 99051 [details]
Patch

Clearing flags on attachment: 99051

Committed r90003: <http://trac.webkit.org/changeset/90003>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.