Closed Bug 1475283 Opened Last year Closed Last year

[Static Analysis] DEAD_STORE errors in toolkit/components/typeaheadfind/nsTypeAheadFind.cpp

Categories

(Toolkit :: General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: rbartlensky, Assigned: rbartlensky)

References

Details

Attachments

(1 file)

toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:118: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  116.       mDidAddObservers = true;
  117.       // ----------- Listen to prefs ------------------
  118. >     nsresult rv = prefInternal->AddObserver("accessibility.browsewithcaret", this, true);
  119.       rv = prefInternal->AddObserver("accessibility.typeaheadfind", this, true);
  120.       NS_ENSURE_SUCCESS(rv, rv);

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp?q=toolkit%2Fcomponents%2Ftypeaheadfind%2FnsTypeAheadFind.cpp%3A118&redirect_type=direct#117

First of all, the return value on 118 is not used, because it is overwritten on line 119. Second of all is line 120 correct?
Blocks: infer
Comment on attachment 8992407 [details]
Bug 1475283: Fix DEAD_STORE reported by infer in nsTypeAheadFind.cpp.

https://reviewboard.mozilla.org/r/257284/#review264166

::: commit-message-2ed15:1
(Diff revision 1)
> +Bug 1475283: Add additional check. r?froydnj

This commit message is not very descriptive.  Maybe "fix $BLAH issue reported by infer in nsTypeAheadFind"?

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:119
(Diff revision 1)
>  
>    if (!mDidAddObservers) {
>      mDidAddObservers = true;
>      // ----------- Listen to prefs ------------------
>      nsresult rv = prefInternal->AddObserver("accessibility.browsewithcaret", this, true);
> +    NS_ENSURE_SUCCESS(rv, rv);

What sort of problem is infer reporting here?
Attachment #8992407 - Flags: review?(nfroyd) → review+
Thank you for your review. The warning says that the return value of:        

prefInternal->AddObserver("accessibility.browsewithcaret", this, true);

is not used. Either use it, or do not store it. I noticed that the second addObserver return value is asserted, so I figured that in order to fix the error, the first return value should be asserted as well, and not dropped.
I couldn't land your patch. rbartlensky: Please set the issues opened by the reviewer as fixed by commit so review board allows to land them. Thank you.
Flags: needinfo?(rbartlensky)
Comment on attachment 8992407 [details]
Bug 1475283: Fix DEAD_STORE reported by infer in nsTypeAheadFind.cpp.

https://reviewboard.mozilla.org/r/257284/#review264166

> What sort of problem is infer reporting here?

Thank you for your review. The warning says that the return value of:        

`prefInternal->AddObserver("accessibility.browsewithcaret", this, true);`

is not used. Either use it, or do not store it. I noticed that the second addObserver return value is asserted, so I figured that in order to fix the error, the first return value should be asserted as well, and not dropped.
(In reply to Eliza Balazs [:ebalazs_] from comment #4)
> I couldn't land your patch. rbartlensky: Please set the issues opened by the
> reviewer as fixed by commit so review board allows to land them. Thank you.

I will as soon as I get an approval.
Flags: needinfo?(rbartlensky)
(In reply to Robert Bartlensky [:rbartlensky] from comment #7)
> (In reply to Eliza Balazs [:ebalazs_] from comment #4)
> > I couldn't land your patch. rbartlensky: Please set the issues opened by the
> > reviewer as fixed by commit so review board allows to land them. Thank you.
> 
> I will as soon as I get an approval.

For future reference, if you want an approval of some sort, it's good practice to needinfo the person or to set a review flag.

The change is fine, thanks!
(In reply to Nathan Froyd [:froydnj] from comment #8)
> (In reply to Robert Bartlensky [:rbartlensky] from comment #7)
> > (In reply to Eliza Balazs [:ebalazs_] from comment #4)
> > > I couldn't land your patch. rbartlensky: Please set the issues opened by the
> > > reviewer as fixed by commit so review board allows to land them. Thank you.
> > 
> > I will as soon as I get an approval.
> 
> For future reference, if you want an approval of some sort, it's good
> practice to needinfo the person or to set a review flag.
> 
> The change is fine, thanks!

Thank you for you help. I will do so!
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9649aefe9a6b
Fix DEAD_STORE reported by infer in nsTypeAheadFind.cpp. r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9649aefe9a6b
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.