[findbugs] [RCN] Redundant nullcheck

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: sebastian, Assigned: Hamel Joshi, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
All
Android
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

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

Attachments

(1 attachment)

* Redundant nullcheck of profileName, which is known to be non-null in org.mozilla.gecko.GeckoService.handleIntent(Intent, int)

* Redundant nullcheck of callback, which is known to be non-null in org.mozilla.gecko.push.PushService.handleMessage(String, Bundle, EventCallback)

"This method contains a redundant check of a known non-null value against the constant null."
Priority: -- → P3
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

8 months ago
Hi Sebastian, I'm new to Mozilla and would like to work on this bug as my first bug item. What is required for this bug to be fixed in brief?
Hi! Great! To fix this bug you will need to remove the redundant null checks in GeckoService and PushService:

GeckoService (We do not need to check for null here anymore, there is already a null check some lines up where we throw an exception):
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoService.java?q=path%3AGeckoService&redirect_type=single#151-152

PushService (There's an unneeded check whether callback is null. But it can't be, we throw an exception at the beginning of the method in this case):
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/push/PushService.java#398-400

Does this help?
(Assignee)

Comment 4

8 months ago
Hi Sebastian, thanks very much for getting back to me so quickly!

Yes, this certainly has helped very much!

Just to make sure that I'm on the right lines, is it just a simple case of removing the highlighted if statements and returning the log calls. In the particular case of PushService but GeckoService will still require the if statement just not for checking if its null?
In PushService you can remove this block, it will never be executed because of the check we do earlier:

>                if (callback == null) {
>                    Log.e(LOG_TAG, "callback must not be null in " + event);
>                    return;
>                }

In GeckoService profileName is never null at this point, so we can simplify the if statement to:

>        if (!GeckoThread.initWithProfile(profileName,
>                                         profileDir != null ? new File(profileDir) : null)) {
(Assignee)

Comment 6

8 months ago
Oh ok, thanks very much for clarifying that up for me! I thought so that for PushService just wasn't sure about GeckoService.

I've got the development environment setup so it shouldn't be that long to commit the changes into a patch. Just a final question, is there any testing required before committing the changes in?
(In reply to Hamel Joshi from comment #6)
> I've got the development environment setup so it shouldn't be that long to
> commit the changes into a patch. Just a final question, is there any testing
> required before committing the changes in?

After you upload a patch I'll push it to our test servers and run our automated tests. I'll also check that the warnings are gone in findbugs. This should be all that is needed here as this is not a user facing bug.
(Assignee)

Comment 8

8 months ago
Hi again! What is the normal procedure of creating and committing patches? Is it ok to commit the changes in just one patch as the changes are very small or is it best to commit the changes individually in separate patches.
(Assignee)

Comment 9

7 months ago
Created attachment 8811346 [details] [diff] [review]
Bug 1316017 - Fixed by removing redundant nullcheck calls.
(Assignee)

Updated

7 months ago
Attachment #8811346 - Attachment description: Bug 1316017 - Fixed by removing redundant nullcheck calls. r?sebastian → Bug 1316017 - Fixed by removing redundant nullcheck calls.
(Assignee)

Comment 10

7 months ago
Hi, I have created a patch for this bug? When can this be reviewed?

Many thanks
Comment on attachment 8811346 [details] [diff] [review]
Bug 1316017 - Fixed by removing redundant nullcheck calls.

You'll need to set the "review?" flag on the attachment. But I just did and will review your patch. :)
Attachment #8811346 - Flags: review?(s.kaspari)
Assignee: nobody → joshihamel
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d84aba232f6f
Comment on attachment 8811346 [details] [diff] [review]
Bug 1316017 - Fixed by removing redundant nullcheck calls.

Review of attachment 8811346 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=631e2abef16071ac1354ab980f8a5d60298f974b

If this passes all tests then we can land this patch by adding the "checkin-needed" keyword to the bug.
Attachment #8811346 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 14

7 months ago
Hi, I can see from the above link that the test has now been completed with one highlighted area described as test failed. What is the next procedure?
rc3 failed once but the test run has been re-triggered and the second run of rc3 succeeded. Everything's fine. We can land this. I'll add the keyword. :)

Thank you for your patch!
Keywords: checkin-needed
(Assignee)

Comment 16

7 months ago
Oh ok, that's great!

Thank you very much for all your help and time much appreciated :)
Thanks for the patch! For future reference, please make sure that your patches contain the necessary commit information so they're more ready to land. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 18

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49351d35d695
Remove redundant nullcheck calls. r=sebastian
Keywords: checkin-needed

Comment 19

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49351d35d695
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months 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.