[Static Analysis][Uninitialized scalar field] In function ElementPropertyTransition(...) from nsTransitionManager.h

RESOLVED FIXED in Firefox 46

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla46
coverity
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: CID 1348596 )

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
The Static Analysis tool Coverity added that variable mReversePortion it's not initialized in the constructor nor in any function that it calls.

Variable mReversePortion is used in function 

>>void
>>nsTransitionManager::ConsiderStartingTransition(
>>  nsCSSProperty aProperty,
>>  const StyleTransition& aTransition,
>>  dom::Element* aElement,
>>  AnimationCollection*& aElementTransitions,
>>  nsStyleContext* aOldStyleContext,
>>  nsStyleContext* aNewStyleContext,
>>  bool* aStartedAny,
>>  nsCSSPropertySet* aWhichStarted)

>>    // Compute the appropriate negative transition-delay such that right
>>    // now we'd end up at the current position.
>>    double valuePortion =
>>      oldPT->CurrentValuePortion() * oldPT->mReversePortion +
>>      (1.0 - oldPT->mReversePortion);

and it's attributed a value later on:

>>  RefPtr<ElementPropertyTransition> pt =
>>    new ElementPropertyTransition(aElement->OwnerDoc(), aElement,
>>                                  aNewStyleContext->GetPseudoType(), timing);
>>  pt->mStartForReversingTest = startForReversingTest;
>>  pt->mReversePortion = reversePortion;

The variable is used only if this will be valid:

>>  if (haveCurrentTransition &&
>>      aElementTransitions->mAnimations[currentIndex]->HasCurrentEffect() &&
>>      oldPT->mStartForReversingTest == endValue) 

Now i can't say for sure that the first time when functuion valls the above if will not be validated, and only the assignment will be called. I don't think that will hurt to initialize this variable in the constructor.
(Assignee)

Comment 1

2 years ago
Created attachment 8708966 [details]
MozReview Request: Bug 1240479 - intialize mReversePortion in constructor. r?heycam

Review commit: https://reviewboard.mozilla.org/r/31199/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31199/
(Assignee)

Comment 2

2 years ago
Comment on attachment 8708966 [details]
MozReview Request: Bug 1240479 - intialize mReversePortion in constructor. r?heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31199/diff/1-2/
Attachment #8708966 - Attachment description: MozReview Request: Bug 1240479 - intialize mReversePortion in constructor. r?bzbarsky@mit.edu → MozReview Request: Bug 1240479 - intialize mReversePortion in constructor. r?heycam
Attachment #8708966 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8708966 - Attachment is obsolete: true
Attachment #8708966 - Flags: review?(cam)
(Assignee)

Comment 3

2 years ago
Created attachment 8708988 [details]
MozReview Request: Bug 1240479 - pass values for mReversePortion and mStartForReversingTestin through constructor. r?heycam

Review commit: https://reviewboard.mozilla.org/r/31221/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31221/
Attachment #8708988 - Flags: review?(cam)
I don't think we're doing anything unsafe here, since the variable is initialized immediately after the object is constructed.  But maybe we want to initialize both mStartForReversingTest and mReversePortion based on values passed into the constructor instead.  That would save initializing those two variables with values that are immediately written over.  Brian?
Flags: needinfo?(bbirtles)
(In reply to Cameron McCormack (:heycam) from comment #4)
> I don't think we're doing anything unsafe here, since the variable is
> initialized immediately after the object is constructed.  But maybe we want
> to initialize both mStartForReversingTest and mReversePortion based on
> values passed into the constructor instead.  That would save initializing
> those two variables with values that are immediately written over.  Brian?

That's dbaron's code so he would know best but I think passing those values into the constructor sounds good.
Flags: needinfo?(bbirtles)
Comment on attachment 8708988 [details]
MozReview Request: Bug 1240479 - pass values for mReversePortion and mStartForReversingTestin through constructor. r?heycam

https://reviewboard.mozilla.org/r/31221/#review28107

Bogdan, could you change the constructor to take these two values instead?
Attachment #8708988 - Flags: review?(cam)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8708988 [details]
MozReview Request: Bug 1240479 - pass values for mReversePortion and mStartForReversingTestin through constructor. r?heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31221/diff/1-2/
Attachment #8708988 - Attachment description: MozReview Request: Bug 1240479 - intialize mReversePortion in constructor. r?heycam → MozReview Request: Bug 1240479 - pass values for mReversePortion and mStartForReversingTestin through constructor. r?heycam
Attachment #8708988 - Flags: review?(cam)
Comment on attachment 8708988 [details]
MozReview Request: Bug 1240479 - pass values for mReversePortion and mStartForReversingTestin through constructor. r?heycam

https://reviewboard.mozilla.org/r/31221/#review28113

Looks good, thanks!
Attachment #8708988 - Flags: review?(cam) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ed0727ff1bef
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.