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

Status: RESOLVED FIXED

(Toolkit :: General, defect, P3)

Description

[Robert Bartlensky [:rbartlensky]]

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?

Comment 1

[Robert Bartlensky [:rbartlensky]]

Attachment: Bug 1475283: Fix DEAD_STORE reported by infer in nsTypeAheadFind.cpp.

Review commit: https://reviewboard.mozilla.org/r/257284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257284/

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

[Robert Bartlensky [:rbartlensky]]

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.

Comment 4

[Eliza Balazs [:ebalazs_]]

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.

Comment 5

[Robert Bartlensky [: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.

Comment 6

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8992407 [details]
Bug 1475283: Fix DEAD_STORE reported by infer in nsTypeAheadFind.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257284/diff/1-2/

Comment 7

[Robert Bartlensky [:rbartlensky]]

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

Comment 8

[Nathan Froyd [:froydnj]]

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

Comment 9

[Robert Bartlensky [:rbartlensky]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9649aefe9a6b
Fix DEAD_STORE reported by infer in nsTypeAheadFind.cpp. r=froydnj

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9649aefe9a6b