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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Andi [:andi]]

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.

Comment 1

[Andi [:andi]]

Attachment: 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/

Comment 2

[Andi [:andi]]

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/

Comment 3

[Andi [:andi]]

Attachment: 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/

Comment 4

[Cameron McCormack (:heycam)]

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?

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.

Comment 6

[Cameron McCormack (:heycam)]

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?

Comment 7

[Andi [:andi]]

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/

Comment 8

[Cameron McCormack (:heycam)]

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!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0727ff1bef

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed0727ff1bef