1316017 - [findbugs] [RCN] Redundant nullcheck

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

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

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

[Hamel Joshi]

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?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 4

[Hamel Joshi]

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?

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

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)) {

Comment 6

[Hamel Joshi]

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?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 8

[Hamel Joshi]

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.

Comment 9

[Hamel Joshi]

Attachment: Bug 1316017 - Fixed by removing redundant nullcheck calls.

Comment 10

[Hamel Joshi]

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

Many thanks

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 14

[Hamel Joshi]

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?

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

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!

Comment 16

[Hamel Joshi]

Oh ok, that's great!

Thank you very much for all your help and time much appreciated :)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49351d35d695
Remove redundant nullcheck calls. r=sebastian

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49351d35d695