Refactoring SVG properties/tear-offs

[Kouhei Ueno]

Hi blink-dev,

I would like to refactor SVG properties/tear-offs implementation.

The current SVG properties/tear-offs implementation is known to have many problems. It has caused many UAFs and leaks. There are a lot of hacks in bindings code which is making codegen rewrite and oilpan hard. Last but not least, the code is very hard to read with C macros and C++ templates.

I have been working on a new implementation of SVG property/tear-offs without those problems, and I now have a basic prototype working passing LayoutTests.

Here is the design doc for the refactor: https://docs.google.com/document/d/1bg7CUyUszqdwmENY3JX6_PoQD6uHRCNcRPJMlC4qlkw/edit?usp=sharing

I'd appreciate your feedback on this.

Thanks,

Kouhei Ueno

[Philip Rogers]

+100! (that's factorial) to refactoring this.

We will be supporting this new infrastructure for a long time so I'm interested in keeping it as understandable as possible. I think this proposal is exactly that.

During this refactoring we should continue to keep an eye on the SVG WG. There's a proposal for a new SVG DOM (http://dev.w3.org/SVG/proposals/improving-svg-dom/) and this may end up with implementations supporting two parallel SVG DOM implementations (http://lists.w3.org/Archives/Public/www-svg/2013Nov/0121.html).

Philip

[Hajime Morrita]

Talked with Kouhei offline, and I learned that one reason that makes
this refactoring difficult (and forces him write this long document)
is to make it perfectly working during the refactoring.

I wonder if it is worth breaking it "partially", in harmless way. One
particularly tricky bit is the notion of "tear-off", and I'm wondering
how wild web depends on it. If the hard-to-handle edge cases are
purely theoretical and wild web barely depends on it, we can possibly
just remove it as a simplification. This can be reasonable especially
because there are better proposals like @pdr mentioned.

I don't think we can totally get rid of the tear-off API considering
its adoption, but if subsetting it helps code simplicity and
robustness, it will be worth considering.

--
morrita

[Eric Seidel]

You should feel free to break any behavior which FF and IE don't have
without a doubt.

I'm a bit more scared to break behaviors which both IE and FF agree with.

I'm less scared to break behaviors that IE has than ones that FF has
since IE's SVG has been around a lot less time.

[Philip Rogers]

Unfortunately we can't break declarative SVG animations (SMIL a.k.a. frown) even though no version of IE support them. Breaking SMIL would greatly simplify this project but our UseCounter data show reasonably high numbers for <animate>.

IE11 does have basic SVGDOM support, including tearoffs.

[Kouhei Ueno]

Thanks for your feedback!

I'm pretty sure that I'm currently indoctrinated to the way the current implementation works, so I'd really appreciate your feedback on the design. "Why does this part complicated?" questions are highly welcomed. :)

Regarding the new SVG DOM, I hope we can only support the new one. The API looks way better than the current one. It seems to basically get rid of all complex part of the current tear-off spec.

On the other hand, supporting two parallel SVG DOM implementations sounds like a nightmare to me. It should be theoretically possible, but I'm sure it would complex the implementation even more.

As for breaking the current implementation partially, the only thing I have atm is to allow discarding of tear-offs with expandos. We currently discard expandos of tear-offs at GC. "rect.x.expando = 1234;" would disappear after GC. I made this retain in the new design, but this may not be needed.

If we don't need to retain expandos, we can always make tear-off objects on-the-fly, and never keep reference to them from SVGProperty implementation. This would remove the need for [KeepAttribute] hacks in the V8 bindings side, and we can remove few edges in the Blink side.

Below are other ideas I had, but seems not worth it:

* No dynamically updating tear-offs

In the current implementation, when we reference a tear-off at some point like "var x = rect.x.animVal;" The var x is NOT a snapshot at that point. It will be dynamically updated while animation.

I considered dropping support for this because I couldn't find any website which uses this. It seemed to me at first that "always snapshotting" would allow us to remove tear-offs at all.

However, given that we still have to support "write" operations like "rect.x.baseVal.value = 5;" It seems we still need to keep tear-offs as references instead of returning a snapshot value.

Also we need to keep a flag somewhere to tell if we are referencing the value in readonly mode or not, and it seems it is most simple to have it as a flag inside tear-off.

* Have baseVal == animVal all the time, even during animation.

This will badly break the spec, but not much website will be affected. The new spec will be something similar to this; It doesn't have baseVal/animVal only old baseVal.

However, this will not simplify the design much as we still have to keep the value snapshot before animation start so that we can restore the value after animation ended.

2013/11/22 Hajime Morrita <mor...@chromium.org>

--

Kouhei Ueno

[Adam Barth]

== Questions ==

Figure 2a: I don't fully understand this diagram.  From the discussion in the Background section, I was expecting to see three objects on the V8 side of the diagram: one corresponding to the SVGElement (black), one corresponding to the SVGAnimatedPropertyTearOff (red), and one corresponding to the SVGPropertyTearOff (purple).  Maybe the SVGAnimatedPropertyTearOff object has been omitted from the diagram?  It does seem to be in Figure 3a, which makes is hard to compare the new design to the existing design.

Figure 3a: I don't fully understand why there isn't a [SetReference] arrow from V8NewSVGAnimatedLength to V8SVGElement.  If a script holds a V8NewSVGLength object and drops all other references, the V8SVGElement (and its whole document) will be retained.  However, if a script holds just a V8NewSVGAnimatedLength object (and if the V8NewSVGLength is created lazily), then the V8SVGElement object will not be retained.  Perhaps these objects are created eagerly?

Figure 5: In SVGAnimatedType, this figure sure a union that contains a RefPtr<NewSVGProperty>.  I don't think it's possible in C++ to store types with non-trivial destructors in a union.  RefPtr<T> has a non-trivial destructor that calls deref() on the pointer it stores.  The text says that we'll use RefPtr<NewSVGPropertyBase> instead of SVGAnimatedType.  Perhaps the diagram is out of date with respect to the text?

Thanks!

Adam

[Kouhei Ueno]

Hi Adam,

Thanks for your comments!

 

Figure 2a: I don't fully understand this diagram.  From the discussion in the Background section, I was expecting to see three objects on the V8 side of the diagram: one corresponding to the SVGElement (black), one corresponding to the SVGAnimatedPropertyTearOff (red), and one corresponding to the SVGPropertyTearOff (purple).  Maybe the SVGAnimatedPropertyTearOff object has been omitted from the diagram?  It does seem to be in Figure 3a, which makes is hard to compare the new design to the existing design.

Added missing V8 wrappers to the diagram.

 

Figure 3a: I don't fully understand why there isn't a [SetReference] arrow from V8NewSVGAnimatedLength to V8SVGElement.  If a script holds a V8NewSVGLength object and drops all other references, the V8SVGElement (and its whole document) will be retained.  However, if a script holds just a V8NewSVGAnimatedLength object (and if the V8NewSVGLength is created lazily), then the V8SVGElement object will not be retained.  Perhaps these objects are created eagerly?

You're right. [SetReference] is needed to retain V8SVGElement. Added to the diagram.

 

Figure 5: In SVGAnimatedType, this figure sure a union that contains a RefPtr<NewSVGProperty>.  I don't think it's possible in C++ to store types with non-trivial destructors in a union.  RefPtr<T> has a non-trivial destructor that calls deref() on the pointer it stores.  The text says that we'll use RefPtr<NewSVGPropertyBase> instead of SVGAnimatedType.  Perhaps the diagram is out of date with respect to the text?

RefPtr<T> should be outside of union. I fixed the diagram.

For SVGAnimatedType, I would like to remove it after the migration is complete. During the migration process, we will have two implementation of SVG property. I would like to temporarily introduce SVGAnimatedNewPropertyAnimator as a compatibility layer so SVGAnimator impl. can handle both implementations.

Thanks,

Kouhei

2013/11/27 Adam Barth <aba...@chromium.org>

--

Kouhei Ueno

[Kouhei Ueno]

Hi,

Any objections on discarding expandos of SVGTearOffs? I would like to go forward with this because it would greatly simplify the design.

Expandos on tear-offs are not stated in the spec, and has been discarded in the previous implementation, so this will not be a regression issue.

I also could not find a real-world svg library depending on this behavior.

The only concern is that IE and Firefox do seem to hold expandos.

2013/11/28 Kouhei Ueno <kou...@google.com>

--

Kouhei Ueno

[Dirk Schulze]

On Nov 22, 2013, at 8:31 AM, Kouhei Ueno <kou...@google.com> wrote:

>
> Below are other ideas I had, but seems not worth it:
> * No dynamically updating tear-offs
> In the current implementation, when we reference a tear-off at some point like "var x = rect.x.animVal;" The var x is NOT a snapshot at that point. It will be dynamically updated while animation.
>
> I considered dropping support for this because I couldn't find any website which uses this. It seemed to me at first that "always snapshotting" would allow us to remove tear-offs at all.
> However, given that we still have to support "write" operations like "rect.x.baseVal.value = 5;" It seems we still need to keep tear-offs as references instead of returning a snapshot value.
>
> Also we need to keep a flag somewhere to tell if we are referencing the value in readonly mode or not, and it seems it is most simple to have it as a flag inside tear-off.

Yes, I am supportive for this option. And it is part of my suggestion to move on and deprecate SVG DOM step by step in the email the Philip mentioned. animVal is currently supported by Firefox but with some bugs. IIRC WebKit and Blink have the best support for animVal but with a hight price of complexity.

A bit off topic: I disagree with Philip that SMIL animations cause a lot of code complexity. Firefox has an interesting concept where they do not differ between SVG Animations and CSS Animations during the processing at all. This could be adapted to Blink and WebKit and could be part of the WebAnimations approaches.

>
> * Have baseVal == animVal all the time, even during animation.
> This will badly break the spec, but not much website will be affected. The new spec will be something similar to this; It doesn't have baseVal/animVal only old baseVal.

This is exactly the behavior of IE 11 as far as I know. I don’t think that we can remove animVal, but aliasing it baseVal should be ok most of the time. This already should remove a huge part of the complexity. In fact Niko added all these templates and markros just to support animVal. We support animVal for nearly two years. I doubt that much content adapted to animVal already.

Greetings,
Dirk

[Erik Dahlström]

On Tue, 03 Dec 2013 16:54:07 +0100, Dirk Schulze <dsch...@chromium.org>
wrote:

> A bit off topic: I disagree with Philip that SMIL animations cause a lot
> of code complexity. Firefox has an interesting concept where they do not
> differ between SVG Animations and CSS Animations during the processing
> at all. This could be adapted to Blink and WebKit and could be part of
> the WebAnimations approaches.

Transitioning the existing SMIL engine over to use Web Animations as the
foundation sounds great to me.

--
Erik Dahlstrom, Web Technology Developer, Opera Software
Co-Chair, W3C SVG Working Group

[Kouhei Ueno]

Thanks all for comments!

For SVG Animations, my current plan is to continue using the existing implementation for a short term by creating a bridge.

Replacing SVG Animations and SVG Properties at a same time is risky, so I plan to use the current implementation of SVG Animations while replacing SVG Properties implementation.

After the transition is done, I think it would be great for WebAnimations to handle SMIL animations without the bridge.

2013/12/4 Erik Dahlström <e...@opera.com>

--

Kouhei Ueno

[Philip Rogers]

I was asked off-list to quantify "reasonably high numbers for <animate>". Right at half of all pages containing SVG trigger for having <animate> on dev channel. We should consider this with a grain of salt, as we don't yet have data from the stable channel, but this was surprising to me.

+1 to Kouhei's plan to construct a bridge for SMIL and not rewrite both animations and tearoffs at the same time :)

[Elliott Sprehn]

You're saying 50% of all pages containing SVG use <animate> ?

[Erik Dahlström]

How is this measured?

I find it hard to believe that 50% of all SVG content actually uses SMIL
for animations. Perhaps the figure is that high because of script
libraries testing for SMIL support?

E.g from modernizr:

tests['smil'] = function() {
return !!document.createElementNS &&
/SVGAnimate/.test(toString.call(document.createElementNS(ns.svg,
'animate')));
};

Would it be possible to instead measure only parsed <animate> tags, that
is, ignoring any <animate> elements created by scripts? That would most
likely give a more accurate figure. Or perhaps measuring the number of
SMIL timelines started containing more than a couple of animation
elements? The latter would also get a reading for script-created SMIL
elements, so might be slightly more accurate than just looking at the
markup.


On Wed, 11 Dec 2013 20:46:57 +0100, Elliott Sprehn <esp...@chromium.org>
wrote:

> To unsubscribe from this group and stop receiving emails from it, send
> an email to blink-dev+...@chromium.org.

[Eric Seidel]

Yes, I think modernizr wounds like it would trigger it:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/svg/SVGAnimationElement.cpp&q=SVGAnimation&sq=package:chromium&l=63

[Kouhei Ueno]

Just a heads up.
I already landed the new base classes as
Source/core/svg/properties/New*, and now I'm going to land migrations
starting from SVGLength{,List}:
https://codereview.chromium.org/112003003/

inferno: I tried my best to test this patch on local ASAN build
against LayoutTests and past failure cases, but if you notice any
regressions from SVG DOM, please contact me.

2013/12/13 Eric Seidel <ese...@chromium.org>:

--
Kouhei Ueno

[Kouhei Ueno]

The migration is now completely done in r169737. The old SVG properties implementations are completely removed, and new SVG properties implementations are all landed.

I hope this work have made working on SVG properties code easier.

Thanks haraken, pdr, and fs for reviewing many giant patches :)

--

Kouhei Ueno

[Kentaro Hara]

Awesome work!

Just adding a history of this work:

(1) In the good old days (several years ago), SVG binding was generated by a code generator. All other bindings were hand-written in custom bindings.

(2) People found that SVG's code generator was awesome, so they started integrating other custom bindings into SVG's code generator. However, SVG was a bit too hard to understand, so they used "if (SVG) { generateSVGBinding(); } else { generateNormalBinding() }" in a lot of places of the code generator.

(3) Consequently, the code generator was full of "if (SVG)".

(4) kouhei@ refactored SVG code so that SVG's architecture aligns with other DOM architecture, and then removed all "if (SVG)" from the code generator. (<---- We're now here.)

--

Kouhei Ueno

--
Kentaro Hara, Tokyo, Japan