Bug 46499 – REGRESSION(63307): SVG elements fail to re-render

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

[Evan Adams]

This is recent regression.  I'm seeing this in Chrome on both MacOS and Linux.

I have an SVG shape that blinks.  The first time it displays is fine and the first time it is hidden is also fine.  After that it never displays again.

I've attached a simple reproducible test case.  It involves having two shapes in a group.  Removing the non-blinking shape makes things work.  Commenting out the stroke attribute of the non-blinking shape makes things work, although, that appears to be a red-herring.  Changing the size of the non-blinking shape makes it so commenting out the stroke attribute no long fixes things.

[Evan Adams]

Created attachment 68734 [details]
Reproduces the bug; has two SVG shapes and a timer to blink one of them.

[Simon Fraser (smfr)]

What's the regression window?

[James Robinson]

Created attachment 68965 [details]
smaller reduction

Slightly smaller reduction.  Looks like the path isn't generating a repaint invalidation.  Passes in safari 5.0.1

[James Robinson]

Comment on attachment 68734 [details]
Reproduces the bug; has two SVG shapes and a timer to blink one of them.

This testcase is still useful - the one I posted requires the HTML5 tree builder as it uses inline <svg> and doesn't work in older WebKits.

[James Robinson]

Regression range from the nightlies is r63285 - 63355.  I strongly suspect that http://trac.webkit.org/changeset/63307 is the culprit here.

[James Robinson]

Local revert confirms that patch is wrong.  Tsk, tsk.  Will revert and add a new repaint test.

[James Robinson]

Created attachment 68989 [details]
Patch

[James Robinson]

Comment on attachment 68989 [details]
Patch

Clearing flags on attachment: 68989

Committed r68444: <http://trac.webkit.org/changeset/68444>

[James Robinson]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/68444 might have broken Leopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/68441
http://trac.webkit.org/changeset/68442
http://trac.webkit.org/changeset/68443
http://trac.webkit.org/changeset/68444
http://trac.webkit.org/changeset/68445

[Csaba Osztrogonác]

(In reply to comment #10)
> http://trac.webkit.org/changeset/68444 might have broken Leopard Intel Release (Tests)
> The following changes are on the blame list:
> http://trac.webkit.org/changeset/68441
> http://trac.webkit.org/changeset/68442
> http://trac.webkit.org/changeset/68443
> http://trac.webkit.org/changeset/68444
> http://trac.webkit.org/changeset/68445

And Qt bot of course... 
Qt specific expected file updated in http://trac.webkit.org/changeset/68489

[Nikolas Zimmermann]

(In reply to comment #6)
> Local revert confirms that patch is wrong.  Tsk, tsk.  Will revert and add a new repaint test.

I'd strongly prefer if you could give the orignal patch author a chance to answer, before reverting.
The patch is still correct, it just means that display changes aren't causing relayouts anymore.
It should be investigated, rather than reverting the patch.

:(

[Eric Seidel (no email)]

In defense of James's actions (which I was not involved with): it's pretty standard policy to revert regressions.  It was unfortunate he hadn't updated the original bug, but it's also possible he was about to and I just beat him to it.

I think it was pretty awesome that he landed a layout test at the same time to prevent any possible future regression.

You should feel free to roll back in your patch as soon as it doesn't regress.  Not repainting SVGs is a pretty major regression and I'm glad this was resolved promptly, rollout or otherwise.

Looking forward to seeing your original patch land again!

[Dirk Schulze]

(In reply to comment #13)
> In defense of James's actions (which I was not involved with): it's pretty standard policy to revert regressions.  It was unfortunate he hadn't updated the original bug, but it's also possible he was about to and I just beat him to it.

I know this policy, but sometime it can be better to fix it with a second patch instead of rolling out everything. Sure, this patch caused a repaint regression, but now we have a performance regression. And it was also the policy to inform the original author before rolling out anything, to give the author the time to fix it. There was no chance for Niko to answer to James comment because of the time shift.

[Nikolas Zimmermann]

(In reply to comment #13)
> In defense of James's actions (which I was not involved with): it's pretty standard policy to revert regressions.  It was unfortunate he hadn't updated the original bug, but it's also possible he was about to and I just beat him to it.
Sure, but when planning to revert a patch, the author should be given a chance to comment first.
It's not that we're releasing a new Safari/Chrome tomorrow, then it would have been fine IMHO.

> 
> I think it was pretty awesome that he landed a layout test at the same time to prevent any possible future regression.
The layout test is flawed. It's timing dependant: a test with two 50ms timeouts, always cries for a better solution. It's also in the wrong directory: svg/in-html would be the place to put new HTML5+SVG regression tests.
 
> You should feel free to roll back in your patch as soon as it doesn't regress.  Not repainting SVGs is a pretty major regression and I'm glad this was resolved promptly, rollout or otherwise.
It's only not repainting display changes, there's not a general repaint problem (of course, not repainting display changes is a pretty major bug).

> 
> Looking forward to seeing your original patch land again!

Definately.

[James Robinson]

Hi folks,

Naturally, I always prefer to ping a patch author before reverting if possible, but in this case it wasn't.  Given that the original patch was an incorrect perf optimization that caused a serious correctness regression reverting was an easy choice.

As for the test the setTimeouts are less than ideal and you are welcome to improve the test.  It does test what it sets out to and I think fast/repaint is a fine place to put a repaint test.

[Nikolas Zimmermann]

(In reply to comment #16)
> Hi folks,
> 
> Naturally, I always prefer to ping a patch author before reverting if possible, but in this case it wasn't.  Given that the original patch was an incorrect perf optimization that caused a serious correctness regression reverting was an easy choice.

It was not incorrect. Your revert hides the fact, that we're not dealing display attribute changes correct for SVG renderers.
 
> As for the test the setTimeouts are less than ideal and you are welcome to improve the test.  It does test what it sets out to and I think fast/repaint is a fine place to put a repaint test.

I prefer to give that kind of feedback as review feedback, and you could improve your test, before landing.

To summarize: I don't like the attitude that's being shown here.

[Eric Seidel (no email)]

I think WildFox and jamesr need to go hug in #webkit.  krit can join the love-fest if he so choses. :)

[James Robinson]

I'm sorry you feel that way.  The revision this patch reverted added no regression test, which usually means that no change in behavior is intended.  The layout test portion was reviewed by Simon who I think is more than qualified to review repaint tests.  I think the best overall way to reduce issues like this is to increase test coverage so that regressions are noticed sooner (preferably during development and before landing).

[Nikolas Zimmermann]

Created attachment 69571 [details]
Patch

The real problem with RenderStyle::diff() is fixed in trunk, revert james workaround.
His patch remains, and works with this patch as well.

[Dirk Schulze]

Comment on attachment 69571 [details]
Patch

Found another regression in http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/animate-elem-31-t.html with this patch. Niko investigates...

[Alexey Proskuryakov]

What's the status of this bug? When opening "smaller reduction", I see a green square in ToT, but a blank page in Safari 5 and in Firefox. What's right?

[Dirk Schulze]

(In reply to comment #22)
> What's the status of this bug? When opening "smaller reduction", I see a green square in ToT, but a blank page in Safari 5 and in Firefox. What's right?

The original regression is fixed. The result on Safari 5 and Opera is blank, because both do not support HTML5. And Firefox has the same behavior like WebKit trunk, a green rect at the end of the animation.