[findbugs] [DLS] Dead store

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

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

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
All
Android

Firefox Tracking Flags

(firefox53 fixed)

Details

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

Attachments

(1 attachment)

* 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
(Assignee)

Comment 2

2 years ago
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)
(Assignee)

Comment 4

2 years ago
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 hidden (mozreview-request)
Assignee: nobody → swaroop.rao
Status: NEW → ASSIGNED
(Reporter)

Comment 6

2 years ago
mozreview-review
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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 8

2 years ago
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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0f2dbc18d18
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.