580098 - Script that runs after SVGLoad but before first SMIL sample can trigger NS_ERROR_DOM_INVALID_STATE_ERR

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

See bug 571016 and Bug 580077 -- both are instances where we started triggering a randomorange bug with NS_ERROR_DOM_INVALID_STATE_ERR being thrown on a "getStartTime()" call.

This happens when script gets evaluated after SVGLoad has fired (so svg.animationsPaused() is true, and svg.getCurrentTime() is 0), but before our first sample has been triggered.  Since we haven't had any samples yet, the nsSMILTimedElement still has mElementState == STATE_STARTUP.  So when we receive a scripted call to getStartTime, nsSMILTimedElement ends up returning a just-initialized (unresolved) nsSMILTimeValue here:
>287 nsSMILTimeValue
>288 nsSMILTimedElement::GetStartTime() const
>289 {
>290   return mElementState == STATE_WAITING || mElementState == STATE_ACTIVE
>291          ? mCurrentInterval->Begin()->Time()
>292          : nsSMILTimeValue();
>293 }
>294 
...which makes nsSVGAnimationElement throw an exception here:
>209 nsSVGAnimationElement::GetStartTime(float* retval)
>210 {
>211   FlushAnimations();
>212 
>213   nsSMILTimeValue startTime = mTimedElement.GetStartTime();
>214   if (!startTime.IsResolved())
>215     return NS_ERROR_DOM_INVALID_STATE_ERR;


I also described this basic problem in bug 571016 comment 14 and bug 571016 comment 15.

I *think* a good fix would be to set mResampleNeeded to PR_TRUE on SVGLoad, so that any API calls before our first sample will trigger a synchronous sample, via "FlushAnimations()".

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

(In reply to comment #0)
> I *think* a good fix would be to set mResampleNeeded to PR_TRUE on SVGLoad, so
> that any API calls before our first sample will trigger a synchronous sample,
> via "FlushAnimations()".

This patch makes this change.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 458487 [details] [diff] [review]
fix

Looks good to me. Let's give that a try.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 458487 [details] [diff] [review]
fix

Requesting approval to land on trunk.  Justifications:
 - Small, well-understood correctness fix.
 - Should fix 2 randomorange bugs mentioned in comment 0. (We're currently using hacks in the tests to work around this problem, but we'd rather not.)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 458487 [details] [diff] [review]
fix

(canceling approval request, as this now has blocking+ and hence doesn't require approval)

Comment 5

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/b88ed3955c99
(sorry, forgot to include "a=blocking2.0+" in commit message)

Also, reverted the no-longer-needed workarounds that had been checked in to stop randomoranges bug 571016 and bug 580077:
http://hg.mozilla.org/mozilla-central/rev/2006b9fc09ec