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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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()".
(Assignee)

Comment 1

9 years ago
Created attachment 458487 [details] [diff] [review]
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.
Attachment #458487 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #458487 - Flags: review? → review?(birtles)
(Assignee)

Updated

9 years ago
Blocks: 571016
Comment on attachment 458487 [details] [diff] [review]
fix

Looks good to me. Let's give that a try.
Attachment #458487 - Flags: review?(birtles) → review+
(Assignee)

Comment 3

9 years ago
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.)
Attachment #458487 - Flags: approval2.0?

Updated

9 years ago
blocking2.0: --- → final+
(Assignee)

Comment 4

9 years ago
Comment on attachment 458487 [details] [diff] [review]
fix

(canceling approval request, as this now has blocking+ and hence doesn't require approval)
Attachment #458487 - Flags: approval2.0?
(Assignee)

Comment 5

9 years ago
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
Blocks: 580077
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
OS: Linux → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.