Closed Bug 951547 Opened 6 years ago Closed 6 years ago

content/smil/nsSMILTimedElement.cpp: [-Wsometimes-uninitialized] variable 'activeTime' is used uninitialized whenever 'if' condition is false

Categories

(Core :: SVG, defect, minor)

x86
macOS
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch handle-STATE_STARTUP.patch (obsolete) — Splinter Review
Bug 948245 introduced this -Wsometimes-uninitialized warning because not all the if/else chain was not handling STATE_STARTUP.

Is it valid to call SampleFillValue() in the STATE_STARTUP state?
Attachment #8349240 - Flags: review?(dholbert)
(In reply to Chris Peterson (:cpeterson) from comment #0)
> Is it valid to call SampleFillValue() in the STATE_STARTUP state?

I'm pretty sure that should never happen. This patch is fine but it might be better just to make the second branch of the if block unconditional and add a MOZ_ASSERT that the mElementState is STATE_ACTIVE. That way we don't end up with a branch that is never tested. What do you think?
Comment on attachment 8349240 [details] [diff] [review]
handle-STATE_STARTUP.patch

Thanks for reporting this, Chris!

(In reply to Brian Birtles (:birtles) from comment #1)
> (In reply to Chris Peterson (:cpeterson) from comment #0)
> > Is it valid to call SampleFillValue() in the STATE_STARTUP state?
> 
> I'm pretty sure that should never happen.

Me too.

> This patch is fine but it might be
> better just to make the second branch of the if block unconditional and add
> a MOZ_ASSERT that the mElementState is STATE_ACTIVE. That way we don't end
> up with a branch that is never tested. What do you think?

I like that better, yeah. (So rather than convert to 'switch', we should just replace the "else if (mElementState == STATE_ACTIVE) {" with an unconditional "else" plus an assertion).

cpeterson, mind tweaking the patch to do that?
Attached patch patch-v2Splinter Review
That sounds like a simpler solution. :)

Patch v2 replace the STATE_ACTIVE check with an assertion. This fixes the -Wsometimes-uninitialized warning.
Attachment #8349240 - Attachment is obsolete: true
Attachment #8349240 - Flags: review?(dholbert)
Attachment #8349855 - Flags: review?(dholbert)
Comment on attachment 8349855 [details] [diff] [review]
patch-v2

>-  } else if (mElementState == STATE_ACTIVE) {
>+  } else {
>+    MOZ_ASSERT(mElementState == STATE_ACTIVE);

Add some assertion message -- maybe just something along the lines of:
  Unexpected state (probably STATE_STARTUP, but we shouldn't
  be sampling the fill value if we're in that state...)

r=me with that
Attachment #8349855 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/a7e1bd889822
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.