1316013 - [findbugs] [DLS] Dead store

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

* 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."

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 2

[swaroop.rao]

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?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 4

[swaroop.rao]

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.

Comment 5

[swaroop.rao]

Attachment: 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).

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

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=059a01007eda

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Phil Ringnalda (:philor)]

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

Comment 10

[areya]

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