941315 - nsSMILTimedElement calls UpdateCurrentInterval() inconsistently between error cases vs. success cases

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

While reviewing bug 937614, I noticed some error-handling cases where nsSMILTimedElement might be missing a call to UpdateCurrentInterval().

It looks like we try to call UpdateCurrentInterval every time we modify mSimpleDur, mMax, or mMin, but there are a few early-returns where we modify those (setting them to Indefinite) but we *don't* update the current interval.

e.g.
> 906 nsSMILTimedElement::SetSimpleDuration(const nsAString& aDurSpec)
> 907 {
> [...]
> 915   if (NS_FAILED(rv)) {
> 916     mSimpleDur.SetIndefinite();
> 917     return NS_ERROR_FAILURE;
> 918   }
[...]
> 929   if (isMedia)
> 930     duration.SetIndefinite();
[...]
> 937   mSimpleDur = duration;
> 938   UpdateCurrentInterval();

http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILTimedElement.cpp?rev=5f1f90c96c91#916

Here, the error-handling case and the "isMedia" case both end up setting mSimpleDur to indefinite, but we only call UpdateCurrentInterval() in one of those code-paths and not the other.

And there are similar inconsistencies in the SetMin() and SetMax() methods there.

Brian, do you know if these differences are intentional & OK, or should we really be calling UpdateCurrentInterval() in those cases as well?  (I don't remember this code well enough to have a good feel for when it's required & when it's not.)

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> It looks like we try to call UpdateCurrentInterval every time we modify
> mSimpleDur, mMax, or mMin, but there are a few early-returns where we modify
> those (setting them to Indefinite

(sorry -- "setting them to Indefinite" is only accurate for mSimpleDur & mMax. In contrast, the error-handling code for mMin calls SetMillis(0L), since that's the default value for the 'min' attribute).

Comment 2

[Daniel Holbert [:dholbert]]

SetRepeatDur() is another function that might need this:
http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILTimedElement.cpp?rev=5f1f90c96c91#1077

Comment 3

[Brian Birtles (:birtles)]

Sorry, I won't have a chance to look into this properly until next week. I don't remember doing this on purpose so it could well be a bug.

Comment 4

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> It looks like we try to call UpdateCurrentInterval every time we modify
> mSimpleDur, mMax, or mMin, but there are a few early-returns where we modify
> those (setting them to Indefinite) but we *don't* update the current
> interval.

Sorry for the delay here. Yes, I agree we should update the current interval in these four places (SetSimpleDuration, SetMin, SetMax, SetRepeatDur). It's been a long time since I worked on this code so I'm not 100% sure it's necessary but if (a) current tests pass with this change added, and (b) we can generate a test that fails without this added then I think we should be make this change.

I think we will probably rework large parts of this SVG/SMIL animation code on top of the Web Animations code when we do that, so it would be good to have regression tests for this.

I can do this next week if you don't have time.

Comment 5

[Robert Longson [:longsonr]]

Please wait for bug 937614 to land Brian.

Comment 6

[Daniel Holbert [:dholbert]]

I don't have cycles do do this at the moment, so I'll defer to you (I think you're also probably more likely to be able to achieve (b) in comment 4 than I am. :)).

Comment 7

[Brian Birtles (:birtles)]

Attachment: Patch v1a

This patch depends on the patches for bug 948245

Comment 8

[Robert Longson [:longsonr]]

I wonder if a stack class that caLls UpdateCurrentInterval in its destructor would be more robust? Just instantiate it and exit the method however you like.

Comment 9

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #8)
> I wonder if a stack class that caLls UpdateCurrentInterval in its destructor
> would be more robust? Just instantiate it and exit the method however you
> like.

Yes, it would be more robust. I am anticipating we rewrite all this code in the next 12 months when rebase on top of Web Animations components but perhaps its still worth doing anyway.

Comment 10

[Robert Longson [:longsonr]]

I think it is worth doing please if that's ok Brian :)

Comment 11

[Brian Birtles (:birtles)]

Attachment: Patch v1b

Rewritten using a stack-based helper class.

Comment 12

[Brian Birtles (:birtles)]

Try run is here: https://tbpl.mozilla.org/?tree=Try&rev=4164f6a6f786

Comment 13

[Brian Birtles (:birtles)]

These patches, by the way, depend on those from bug 948245 since otherwise the test case for 'min' will fail.

Comment 14

[Brian Birtles (:birtles)]

Attachment: Patch v1c

Just a quick update to the comments to remove reference to RAII since that may be confusing.

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 8346288 [details] [diff] [review]
Patch v1c

Lovely. Thanks Brian.

Comment 16

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #15)
> Lovely. Thanks Brian.

Thanks Robert!

I'll push this together with bug 948245 once its reviewed rather than prepare a separate version of this patch.

Comment 17

[Brian Birtles (:birtles)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00218997103b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00218997103b