Bug 12073

The SVG spec allows one to reset the clock on computer that the "browser" is running on? This seems like it could (at the very least) create a denial-of-service attack on time-sensitive security exchanges (like Kerberos). In the worst case, it might even allow replay attacks. Am I misunderstanding what setCurrentTime() would do?

(In reply to comment #1) > The SVG spec allows one to reset the clock on computer that the "browser" is > running on? Nah. That definitely would be a security hole. :) Especially since on OS X it normally requires administrator access to change the clock. :) From the spec: setCurrentTime Adjusts the clock for this SVG document fragment, establishing a new current time. Parameters: in float seconds. The new current time in seconds relative to the start time for the current SVG document fragment. I expect that this basically only pertains to the animation timeline. I don't know how one would make this work for JavaScript (nor to I expect it's supposed to). It's important (at least for editing) to be able to change the animation timeline's current position. There are also cases during display where such is useful. That's exactly what this method allows. If you set the time to 1000, the document suddenly fast-forwards (or rewinds) to exactly how it should look 1000 seconds after load time (with all animations in their proper state, etc). Any JavaScript timers/animation I expect would be unaffected. Certainly other pages, the browser's sense of time, and the computer clock should all remain unaffected. :)

The SMIL animation tests that the SVG WG is writing depend on the functionality provided by setCurrentTime() and pauseAnimations(). Is there any chance of getting this bug fixed any time soon so that we can run the tests against webkit too?

Created attachment 56872 [details] testcase Here's a testcase. When loaded, the content area of the window should be filled with green for all eternity. You should never see any red.

I should probably have mentioned - only supporting moving *forward* along the timeline with setCurrentTime() would be sufficient for the WG's testing framework. Moving *backwards* along the timeline is harder, so we'd be perfectly happy to see that come later.

Created attachment 67423 [details] Patch to fix this bug. This patch works for me (in the Qt version of WebKit); but I did not run it through any tests. Anyone care to apply the patch and run the tests?

Comment on attachment 67423 [details] Patch to fix this bug. Hi Chris, > +void SMILTimeContainer::setElapsed(SMILTime time) > +{ > + m_beginTime = currentTime() - time.value(); > + m_accumulatedPauseTime = 0; > + if (isPaused()) { > + SMILTime elapsed = this->elapsed(); > + updateAnimations(elapsed); > + } No need for a local variable, just use: if (isPaused()) updateAnimations(elapsed()); You have to write tests and a ChangeLog, please use prepare-ChangeLog in your root WebKit directory to generate templates. r-, until these issues are resolved. The patch itself looks just fine, but you'll need to write several testcases, ideally using the existing framework in LayoutTests/svg/animations. Test it using accumulative animations, normal ones, etc. Thanks in advance!

(In reply to comment #7) > (From update of attachment 67423 [details]) > Hi Chris, > > > +void SMILTimeContainer::setElapsed(SMILTime time) > > +{ > > + m_beginTime = currentTime() - time.value(); > > + m_accumulatedPauseTime = 0; > > + if (isPaused()) { > > + SMILTime elapsed = this->elapsed(); > > + updateAnimations(elapsed); > > + } > No need for a local variable, just use: > if (isPaused()) > updateAnimations(elapsed()); > > You have to write tests and a ChangeLog, please use prepare-ChangeLog in your root WebKit directory to generate templates. r-, until these issues are resolved. The patch itself looks just fine, but you'll need to write several testcases, ideally using the existing framework in LayoutTests/svg/animations. > Test it using accumulative animations, normal ones, etc. > > Thanks in advance! It's pretty hard to test is within our current animation DRT api, since the hack for this framework tries to do the same thing. So we can't test setCurrentTime that stes the current time as well. I'd suggest to remove the current hack and use setCurrentTime instead. This way our animation framework would get browser independent and we have dozens of tests for setCurrentTime. What do you think Niko?

When can we have setCurrentTime implemented. It is so important for our project

I'll upload a my current patch soon. But it is not for review and may fail to apply. I had bigger problems with our asynchrony timer handling. Problem: the time get's not set immediately. I won't have time to look at this in the next weeks. I hope it helps others to find a better fix. I may come back to the bug if it did not get fixed before I have more time.

Any updates on this BUG, This is supported on Firefox and Opera but not WebKit based browsers which is preventing lot of interactivity for WebKit based browsers. Can anyone take up this BUG please ?

@Dirk, Nikolas: If no one has any objection, I'll try to finish this fix and get it landed. I think there are some edge-cases not being dealt with yet, but it doesn't look too difficult to fix.

(In reply to comment #12) > @Dirk, Nikolas: If no one has any objection, I'll try to finish this fix and get it landed. I think there are some edge-cases not being dealt with yet, but it doesn't look too difficult to fix. You'll see some difficulties with asynchronous timers in the SVG implementation once you write some test cases for setCurrentTime IIRC. But I have no objections if you want to work on it.

Created attachment 116084 [details] Patch

This patch is incomplete -- I'm writing more tests right now. But I wanted to get feedback on the SVGSMILElement::reset() method, which makes me feel a bit squirrely. The underlying issue that makes the implementation tricky comes up when you have multiple <animate> elements associated with the same target element. When you just have a single <animate> in the Active state, progress()ing it backwards in time works fine. But when you have more than one, or the current one has become Inactive or Frozen, things get weird. To make matters worse, the TimeContainer removes Inactive SMILElements from m_scheduledAnimations, making it impossible to go backwards in time across the boundary between them. The only solution I could think of that seemed likely to not break in subtle ways was to - Not remove inactive SMILElements from m_scheduledAnimations, and - Implement SMILElement::reset() to take it back to a known-good state (with its parsed attributes preserved). The one thing that still worries me about this approach is that it looks like SMILElement::[m_beginTimes m_endTimes] might not be maintained correctly. I don't clear them in reset(), because they contain state that's initialized in insertedIntoDocument() and parseBeginOrEnd(). But it looks like they're also modified in handleConditionEvent(), which appears to happen post-initialization, and seems like it should be reverted in reset(). I'm open to any other approaches, of course, and if anyone can offer suggestions on test-cases that would trigger the handleConditionEvent() case, it would be greatly appreciated.

Created attachment 116137 [details] Patch

That last patch adds slightly better tests, and deals with a few timing edge-cases. Any feedback on the general approach and other test cases that would be appreciated.

Comment on attachment 116137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116137&action=review > Source/WebCore/svg/SVGSVGElement.cpp:534 > + if (seconds < 0) > + seconds = 0; Can this and the isnan check just be rolled into a seconds = max(seconds, 0); line? > Source/WebCore/svg/SVGSVGElement.cpp:536 > + else if (isnan(seconds)) > + return; Should this throw an exception? Do we have tests for hte behavior of element.setCurrentTime("foo")? Seems we should. > Source/WebCore/svg/animation/SMILTimeContainer.cpp:138 > + Vector<SVGSMILElement*> toReset; > + copyToVector(m_scheduledAnimations, toReset); > + for (unsigned n = 0; n < toReset.size(); ++n) > + toReset[n]->reset(); Why do we need to copy them? Can reset() cause arbitrary dom modifications? > Source/WebCore/svg/animation/SMILTimeContainer.cpp:-284 > - else if (!animation->isContributing(elapsed)) { > - m_scheduledAnimations.remove(animation); > - if (m_scheduledAnimations.isEmpty()) > - m_savedBaseValues.clear(); > - } You should explain this (and other changes) in your ChangeLog. > Source/WebCore/svg/animation/SVGSMILElement.cpp:182 > +void SVGSMILElement::reset() > +{ > + m_activeState = Inactive; > + m_isWaitingForFirstInterval = true; > + m_intervalBegin = SMILTime::unresolved(); > + m_intervalEnd = SMILTime::unresolved(); > + m_previousIntervalBegin = SMILTime::unresolved(); > + m_lastPercent = 0; > + m_lastRepeat = 0; > + m_nextProgressTime = 0; > + > + resolveFirstInterval(); > +} We should consider using this in the constructor so that the constructor and reset() are known to take the same codepaths.

Instead of patching it, I wonder if we could wipe out the SVG animation support. It is a terrible standard. It is requires lots of complicated code and support infrastructure. A significant amount of work would be need to be done to make it respectable (it modifies dom etc). Other engines do fine without. Hixie removed animations from the Acid3 (the original motivation for implementing them). Are there any real uses for it anywhere?

(In reply to comment #19) > Instead of patching it, I wonder if we could wipe out the SVG animation support. It is a terrible standard. It is requires lots of complicated code and support infrastructure. A significant amount of work would be need to be done to make it respectable (it modifies dom etc). Other engines do fine without. Hixie removed animations from the Acid3 (the original motivation for implementing them). > > Are there any real uses for it anywhere? Yes there are a lot of use cases. And before anyone can remove SVG Animation, we need a reasonable replacement for it. JS is of course one way, but maybe not the best. CSS Animations/Transitions is another way but it is not possible to animate SVG attributes with it (the general use case). The SVG WG is working on finding a replacement, but that no other engine is supporting it is absolutely wrong. Rather there is just one viewer not supporting SVG Animations: IE. And I also don't understand why it is a terrible standard? You can do a lot more with SVG Animation than with CSS Animations/Transitions, with just one line in the SVG document. It supports the sandwich model and animation of XML attributes as well as CSS properties. It is well defined (at least if you look at SMIL Animations 3.0) with a description of all edge cases. The work on the DOM changes (animVal/baseVal) is one problem. Neither Niko nor me had enough time to implement it completely. I had no time to look at the patch yet, but a missing implementation of setCurrentTime() is no reason to remove SVG Animation at this point of time. In general I do not oppose to push CSS Animations for SVG, but it is still a lot to do before we can think about removing SVG Animation. And even at this time, we will break a lot of content out there.

(In reply to comment #19) > Instead of patching it, I wonder if we could wipe out the SVG animation support. It is a terrible standard. It is requires lots of complicated code and support infrastructure. A significant amount of work would be need to be done to make it respectable (it modifies dom etc). That's not correct. Dirk & me spent a lot of time last year to prepare animVal support, and to move animations away from modifying the DOM. It's all prepared, I have a patch <10k that patches SVGLength animations to work without any dOM modifications, while exposing animVal/baseVal correctly to JS. I have other blocking issues atm, but I want to come back to it... or at least find someone to take over the work. It's really all prepared for DOM attributes, CSS property animations are another topic.

Comment on attachment 116137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116137&action=review I fully agree with Erics review comments. I have some more issues with testing: > LayoutTests/svg/animations/script-tests/animate-setcurrenttime.js:31 > +animate1.setAttribute("to", "200"); > +animate1.setAttribute("fill", "freeze"); We need lots more tests, as you can guess :-) * Accumulating animations need to be tested * Several animations running in parallel * Nested time containers, test modifying timeline in a inner <svg> element. <svg>some content <svg width="50" height="50"> rect-with-anim-in-inner-svg... </svg> </svg> Test moving time in outer <svg>, should it affect the inner <svg> timeline? * Use pure SVG/HTML5 self-contained tests, not using script-tests/ framework as well ... I'm sure I could come up with more, once I have some more time to think about that. Do you also test your testcases in eg. Opera? Would be nice to have them pass in Opera fully as well.

Eric, Nikolas: Thanks for the prompt and detailed review. I probably wasn't as clear on this as I should have been, but this patch wasn't intended to be committed quite yet. I was hoping to get feedback on the specific mechanisms called out in my first comment (SMILElement::reset() and the fact that inactive animations are no longer removed from SMILTimeContainer::m_scheduledAnimations). If no one sees any serious problem with this approach, I'll continue cleaning it up in light of the review feedback. I'm also in the midst of writing more tests, as Nikolas suggests (thanks for the specific test suggestions), and I should have something ready later today. I've been testing them on Firefox, and will check Opera as well.

Please dont even consider removing SVG SMIL animation. Its far far better than CSS animations and JS as editing it does not require much of programming expertise. Its far more readable as it is more structured than CSS animations. Its just a matter of time SVG picks up. animateMotion in SVG is something unique not possible in CSS animations and can be done in 1 line in SVG

Created attachment 116348 [details] Patch

(In reply to comment #18) > (From update of attachment 116137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116137&action=review > > > Source/WebCore/svg/SVGSVGElement.cpp:534 > > + if (seconds < 0) > > + seconds = 0; > > Can this and the isnan check just be rolled into a seconds = max(seconds, 0); line? I changed the <0 check to simply use max. That doesn't work properly for the NaN case (see next comment). > > Source/WebCore/svg/SVGSVGElement.cpp:536 > > + else if (isnan(seconds)) > > + return; > > Should this throw an exception? Do we have tests for hte behavior of element.setCurrentTime("foo")? Seems we should. The spec appears to be silent on what non-numeric values should do. Throwing an exception would seem reasonable (and Mozilla does indeed throw), but the spec includes no 'raises' clause for this method (apparently not an oversight -- plenty of other methods are spec'd to raise exceptions). If you think it's worthwhile to change this in WebKit, I'm glad to do so -- I'll need to change the IDL so that I get an ExceptionCode parameter in SVGSVGElement::setCurrentTime() (unless there's another way to throw a DOM exception that I'm simply unaware of). As I've expanded the tests, I added a couple that deal with values like NaN and "blah". They explicitly test that these have no effect, and if we decide to throw instead I'll obviously need to update the test. > > Source/WebCore/svg/animation/SMILTimeContainer.cpp:138 > > + Vector<SVGSMILElement*> toReset; > > + copyToVector(m_scheduledAnimations, toReset); > > + for (unsigned n = 0; n < toReset.size(); ++n) > > + toReset[n]->reset(); > > Why do we need to copy them? Can reset() cause arbitrary dom modifications? Yes, SVGSMILElement::reset() can cause it to reschedule itself (in resolveFirstElement()), which modifies m_scheduledAnimations. > > Source/WebCore/svg/animation/SMILTimeContainer.cpp:-284 > > - else if (!animation->isContributing(elapsed)) { > > - m_scheduledAnimations.remove(animation); > > - if (m_scheduledAnimations.isEmpty()) > > - m_savedBaseValues.clear(); > > - } > > You should explain this (and other changes) in your ChangeLog. I've added a couple of comments in the ChangeLog. Please let me know if they're insufficient -- I'm still acclimating to WebKit norms on this front. > > Source/WebCore/svg/animation/SVGSMILElement.cpp:182 > > +void SVGSMILElement::reset() > > +{ > > + m_activeState = Inactive; > > + m_isWaitingForFirstInterval = true; > > + m_intervalBegin = SMILTime::unresolved(); > > + m_intervalEnd = SMILTime::unresolved(); > > + m_previousIntervalBegin = SMILTime::unresolved(); > > + m_lastPercent = 0; > > + m_lastRepeat = 0; > > + m_nextProgressTime = 0; > > + > > + resolveFirstInterval(); > > +} > > We should consider using this in the constructor so that the constructor and reset() are known to take the same codepaths. I've added an explicit reset() call in the ctor. I left the initializers alone (IOW, some values get doubly-initialized), because much of the other code I've seen in WebKit seems to prefer setting all members in initializer expressions. It feels a bit weird this way, so if you prefer I'd be glad to be more surgical in initializing these fields.

(In reply to comment #22) > (From update of attachment 116137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116137&action=review > > I fully agree with Erics review comments. I have some more issues with testing: > > > LayoutTests/svg/animations/script-tests/animate-setcurrenttime.js:31 > > +animate1.setAttribute("to", "200"); > > +animate1.setAttribute("fill", "freeze"); > > We need lots more tests, as you can guess :-) > * Accumulating animations need to be tested > * Several animations running in parallel > * Nested time containers, test modifying timeline in a inner <svg> element. > <svg>some content <svg width="50" height="50"> rect-with-anim-in-inner-svg... </svg> </svg> > Test moving time in outer <svg>, should it affect the inner <svg> timeline? > * Use pure SVG/HTML5 self-contained tests, not using script-tests/ framework as well > ... > I'm sure I could come up with more, once I have some more time to think about that. > Do you also test your testcases in eg. Opera? Would be nice to have them pass in Opera fully as well. The most recent patch has a test that I believe covers these cases. I'm still becoming familiar with the "dark corners" of the animation spec, so it's entirely possible that I've missed some cases. Please feel free to throw out any cases that come to mind, and I'll add them. As for the script-tests/ framework question -- I've modified the test to be more self-contained, and it now uses only "fast/js/resources/js-test-pre.js" (for shouldBe() and the PASS/FAIL stuff), and otherwise uses the layoutTestController directly. I'm not sure if this is precisely what you had in mind. I chose not to do pixel tests because I felt it more important to cover all the intermediate states at various values of setCurrentTime(). But if you feel a pixel test would be helpful, I'm glad to add one. Also, I jammed all the cases together into one big test-case. If you feel it would be better to split them up, again that's fine by me. (I also just noticed that I left a couple of commented-out script includes in there -- I'll remove those on the next patch)

(In reply to comment #22) > I'm sure I could come up with more, once I have some more time to think about that. > Do you also test your testcases in eg. Opera? Would be nice to have them pass in Opera fully as well. Forgot to respond to the Opera question -- I've checked these tests on Mozilla and Opera, and everything passes except for the handling of nested time containers. If I understand it correctly, WebKit's behavior is the correct one -- the currentTime of the outer/inner containers should be independent of one-another. But they seem to interact with each other on Opera and Mozilla (different seemingly-wrong behavior on each).

Ping? Finally back from vacation. If either of you has a chance to look at the updated patch, it would be greatly appreciated.

Comment on attachment 116348 [details] Patch This looks reasonable, but you really need Antti or Niko to review, since they are much more familiar with our SVG Animation implementation than I am.

Comment on attachment 116348 [details] Patch This really looks great, now if we just already had my animal patch in .... would you be interested to implement that as well? Everything is in place for animal, we just need to hook it in.

(In reply to comment #31) > (From update of attachment 116348 [details]) > This really looks great, now if we just already had my animal patch in .... would you be interested to implement that as well? Everything is in place for animal, we just need to hook it in. Many thanks for the review. I'm unfamiliar with the "animal patch", but if you can point me in the right direction, I'll have a look.

(In reply to comment #32) > (In reply to comment #31) > > (From update of attachment 116348 [details] [details]) > > This really looks great, now if we just already had my animal patch in .... would you be interested to implement that as well? Everything is in place for animal, we just need to hook it in. > > Many thanks for the review. I'm unfamiliar with the "animal patch", but if you can point me in the right direction, I'll have a look. Ouch, I meant to say "animVal" patch :-) Say you <animate> a <rect> width from "100" to "200", in 1s. at 0s, rect.width.baseVal.value == rect.width.animVal.value, when the anim runs, it should differ and return eg. 150 at 0.5s with a linear animation progress. The whole animVal/baseVal distinction is prepared since mid of last year, but we couldn't yet take the time and turn it on. Bug 12437 is discussing it.

Created attachment 122026 [details] Patch

Comment on attachment 122026 [details] Patch Rejecting attachment 122026 [details] from commit-queue. jgw@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Comment on attachment 122026 [details] Patch Clearing flags on attachment: 122026 Committed r104719: <http://trac.webkit.org/changeset/104719>

All reviewed patches have been landed. Closing bug.