1261985 - [infer] Errors in SearchEngine

Status: RESOLVED INACTIVE

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

/var/lib/jenkins/workspace/fennec-infer/mobile/android/base/java/org/mozilla/gecko/search/SearchEngine.java:232: error: NULL_DEREFERENCE
  object resultsUri last assigned on line 229 could be null and is dereferenced at line 232

Comment 1

[Shane Dharan]

Hi can I work on this bug? Seems like I should just check for null before iterating through the set?

Comment 2

[Shane Dharan]

How would I recreate the error?

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Shane Dharan from comment #2)
> How would I recreate the error?

Unfortunately, I don't know how to run Infer locally. We run it in automation and the results are posted to [1] (daily?) so we can just make your change, land it, and ensure it's fixed via [1].

[1]: https://people.mozilla.org/~sledru/reports/fennec-infer/bugs.txt

Comment 4

[Shane Dharan]

Attachment: nullSearchPatch.diff

Added a null check for empty searches.

If you find any issues let me know :)

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8778095 [details] [diff] [review]
nullSearchPatch.diff

By the way, instead of NI requests for code reviews, you can set a review flag by clicking into "Details", selecting the field next to review, changing it to "?", and selecting me as a reviewer.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8778095 [details] [diff] [review]
nullSearchPatch.diff

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

Looks good once the style nit is fixed! Please post another patch & flag me for review when you're ready.

To test functionality, I made a push to our try test server (above). If it's all green, we'll be able to land once the new patch is reviewed.

::: mobile/android/base/java/org/mozilla/gecko/search/SearchEngine.java
@@ +227,4 @@
>       */
>      public String queryForResultsUrl(String url) {
>          final Uri resultsUri = getResultsUri();
> +        if(resultsUri == null) {

nit: space after if, i.e. `if (`

If you run checkstyle (via `./mach gradle app:checkstyle`), it should catch issues like this.

Comment 8

[Shane Dharan]

Attachment: nullSearchPatch.diff

Changed the coding style :)

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8778711 [details] [diff] [review]
nullSearchPatch.diff

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

Looks good and thanks for the patch. Sorry about that delay.

You can add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Your patch has a few intermittent errors but they should be fine for checkin.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 10

[Sylvestre Ledru [:Sylvestre]]

No action for a while, closing.