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

RESOLVED FIXED in mozilla29

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: birtles)

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.)
(Reporter)

Updated

5 years ago
Flags: needinfo?(birtles)
(Reporter)

Comment 1

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

Comment 3

5 years ago
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.
Flags: needinfo?(birtles)
(Assignee)

Updated

5 years ago
Flags: needinfo?(birtles)
(Assignee)

Comment 4

5 years ago
(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.
Flags: needinfo?(birtles)
Please wait for bug 937614 to land Brian.
(Reporter)

Comment 6

5 years ago
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. :)).
Depends on: 937614
(Assignee)

Updated

5 years ago
Assignee: nobody → birtles
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 948245
(Assignee)

Comment 7

5 years ago
Created attachment 8345172 [details] [diff] [review]
Patch v1a

This patch depends on the patches for bug 948245
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.
(Assignee)

Comment 9

5 years ago
(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.
I think it is worth doing please if that's ok Brian :)
(Assignee)

Comment 11

5 years ago
Created attachment 8346285 [details] [diff] [review]
Patch v1b

Rewritten using a stack-based helper class.
Attachment #8345172 - Attachment is obsolete: true
Attachment #8346285 - Flags: review?(longsonr)
(Assignee)

Comment 13

5 years ago
These patches, by the way, depend on those from bug 948245 since otherwise the test case for 'min' will fail.
(Assignee)

Comment 14

5 years ago
Created attachment 8346288 [details] [diff] [review]
Patch v1c

Just a quick update to the comments to remove reference to RAII since that may be confusing.
Attachment #8346285 - Attachment is obsolete: true
Attachment #8346285 - Flags: review?(longsonr)
Attachment #8346288 - Flags: review?(longsonr)
(Assignee)

Updated

5 years ago
Attachment #8346288 - Attachment description: makeInvalidValuesUpdateTheModel → Patch v1c
Comment on attachment 8346288 [details] [diff] [review]
Patch v1c

Lovely. Thanks Brian.
Attachment #8346288 - Flags: review?(longsonr) → review+
(Assignee)

Comment 16

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/00218997103b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.