Closed Bug 1316013 Opened 3 years ago Closed 3 years ago

[findbugs] [DLS] Dead store

Categories

(Firefox for Android :: General, defect, P3)

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: sebastian, Assigned: swaroop.rao, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file)

* Dead store to strSessionId in org.mozilla.gecko.media.GeckoMediaDrmBridgeV21.createSession(int, int, String, byte[])

* Dead store to strCryptoSessionId in org.mozilla.gecko.media.GeckoMediaDrmBridgeV21.ensureMediaCryptoCreated()

* Dead store to sessionId in org.mozilla.gecko.media.GeckoMediaDrmBridgeV21$MediaDrmListener.onEvent(MediaDrm, byte[], int, int, byte[])

"This instruction assigns a value to a local variable, but the value is not read or used in any subsequent instruction. Often, this indicates an error, because the value computed is never used.

Note that Sun's javac compiler often generates dead stores for final local variables. Because FindBugs is a bytecode-based tool, there is no easy way to eliminate these false positives."
To start, set up a build environment - you can see the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Then, you'll need to upload a patch - see:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "sebastian" and you can find me and other helpful folks in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC
I looked into this quickly and it looks like, in all three cases listed above in the description, the variable ends up getting used in a Log.* statement. An example is shown below:

if (DEBUG) Log.d(LOGTAG, "MediaDrm.EVENT_KEY_EXPIRED, sessionId=" + sessionId);

Does this still need to be fixed?
Flags: needinfo?(s.kaspari)
I should have posted the line numbers too: 

> Dead store to strSessionId in org.mozilla.gecko.media.GeckoMediaDrmBridgeV21.createSession(int, int, String, byte[])
> 
> Bug type DLS_DEAD_LOCAL_STORE (click for details)
> In class org.mozilla.gecko.media.GeckoMediaDrmBridgeV21
> In method org.mozilla.gecko.media.GeckoMediaDrmBridgeV21.createSession(int, int, String, byte[])
> Local variable named strSessionId
> At GeckoMediaDrmBridgeV21.java:[line 173]

This looks a bit like a false positive because DEBUG is "always" false. However in this case it doesn't seem like we need to define strSessionId outside of this block and could even avoid it completely by just building the string as part of the log statement (without any reference).

> Dead store to strCryptoSessionId in org.mozilla.gecko.media.GeckoMediaDrmBridgeV21.ensureMediaCryptoCreated()
> 
> Bug type DLS_DEAD_LOCAL_STORE (click for details)
> In class org.mozilla.gecko.media.GeckoMediaDrmBridgeV21
> In method org.mozilla.gecko.media.GeckoMediaDrmBridgeV21.ensureMediaCryptoCreated()
> Local variable named strCryptoSessionId
> At GeckoMediaDrmBridgeV21.java:[line 595]

Same here. Let's avoid building strSessionId if we do not log it anyways (DEBUG=false).

> Dead store to sessionId in org.mozilla.gecko.media.GeckoMediaDrmBridgeV21$MediaDrmListener.onEvent(MediaDrm, byte[], int, int, byte[])
> 
> Bug type DLS_DEAD_LOCAL_STORE (click for details)
> In class org.mozilla.gecko.media.GeckoMediaDrmBridgeV21$MediaDrmListener
> In method org.mozilla.gecko.media.GeckoMediaDrmBridgeV21$MediaDrmListener.onEvent(MediaDrm, byte[], int, int, byte[])
> Local variable named sessionId
> At GeckoMediaDrmBridgeV21.java:[line 374]

And here we also only need this string for logging. So let's not build it if we do not need it.
Flags: needinfo?(s.kaspari)
Done. I have made the suggested changes and sent it out for review. I hadn't realized that DEBUG is hard-coded to FALSE. For some reason, I just assumed that there's a System.getenv() call that determines its value from an environment variable.
Assignee: nobody → swaroop.rao
Status: NEW → ASSIGNED
Comment on attachment 8811245 [details]
Bug 1316013 - Fixed by removing variables and building them as part of the Log statement (which never gets called because DEBUG is set to FALSE).

https://reviewboard.mozilla.org/r/93430/#review93702

LGTM.
Attachment #8811245 - Flags: review?(s.kaspari) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f0f2dbc18d18
Fixed by removing variables and building them as part of the Log statement (which never gets called because DEBUG is set to FALSE). r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f0f2dbc18d18
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.