1105316 - crash in java.lang.NullPointerException: at org.mozilla.search.providers.SearchEngineManager.createEngineFromLocale(SearchEngineManager.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-381813bf-9131-4f99-a1bf-42ac32141123.
=============================================================

java.lang.NullPointerException
	at org.mozilla.search.providers.SearchEngineManager.createEngineFromLocale(SearchEngineManager.java:332)
	at org.mozilla.search.providers.SearchEngineManager.access$500(SearchEngineManager.java:38)
	at org.mozilla.search.providers.SearchEngineManager$2.run(SearchEngineManager.java:165)
	at android.os.Handler.handleCallback(Handler.java:733)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:136)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43)

Comment 1

[:Margaret Leibovic]

Looks like `in` can be null here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java#332

Comment 2

[Aaron Train [:aaronmt]]

Couple reports coming in on crash-stats for 35. Possibly easy to reproduce?

Comment 3

[Aaron Train [:aaronmt]]

(36 too)

Comment 4

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1105316/margaret

Comment 5

[:Margaret Leibovic]

/r/1369 - Bug 1105316 - Add null check in createEngineFromLocale

Pull down this commit:

hg pull review -r c70eed8f11bd712f8a8bdf84082678e6c30ae208

Comment 6

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/1367/#review799

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
(Diff revision 1)
>          final InputStream in = getInputStreamFromSearchPluginsJar("list.txt");

I'm a little confused as to why this is ever returning null; worst case it should return the en-US list:

```

        // Finally, fall back to en-US.
        url = getSearchPluginsJarURL("en-US", fileName);
        return GeckoJarReader.getStream(url);
```

So... sure, add a null check here, but (a) log, and (b) figure out how it's possible that we don't even have en-US searchplugins?

Comment 7

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #6)
> https://reviewboard.mozilla.org/r/1367/#review799
> 
> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> (Diff revision 1)
> >          final InputStream in = getInputStreamFromSearchPluginsJar("list.txt");
> 
> I'm a little confused as to why this is ever returning null; worst case it
> should return the en-US list:
> 
> ```
> 
>         // Finally, fall back to en-US.
>         url = getSearchPluginsJarURL("en-US", fileName);
>         return GeckoJarReader.getStream(url);
> ```
> 
> So... sure, add a null check here, but (a) log, and (b) figure out how it's
> possible that we don't even have en-US searchplugins?

You know more than I do about the different types of locale situations we ship. Is it possible we ship builds without an en-US locale?

It also looks like GeckoJarReader.getStream will just eat exceptions and return null:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/GeckoJarReader.java#110

So maybe that's what we're seeing? I could also add a log statement in there.

Similarly to bug 1105290, catching this exception will prevent the activity from crashing, but I'm worried it will leave the search activity in a busted state if no search engine can be found.

Comment 8

[:Margaret Leibovic]

/r/1369 - Bug 1105316 - Add null check in createEngineFromLocale

Pull down this commit:

hg pull review -r 7bb2502adf56db1012fa91eedf4fdef041c1a05b

Comment 9

[Richard Newman [:rnewman]]

> You know more than I do about the different types of locale situations we
> ship. Is it possible we ship builds without an en-US locale?

Single-locale repacks.

E.g.,

$ jar tf omni.ja| fgrep searchplugins/list
chrome/th/locale/th/browser/searchplugins/list.txt

What you really want to do is use the default locale, not assume that the default === en-US.

I'm not sure how you're supposed to do that, but it's in chrome/chrome.manifest:

locale global th th/locale/th/global/


> Similarly to bug 1105290, catching this exception will prevent the activity
> from crashing, but I'm worried it will leave the search activity in a busted
> state if no search engine can be found.

Aye. Best not to rush into it.

Comment 10

[Richard Newman [:rnewman]]

Notes from IRC:

09:49 <@rnewman> I got the bug comment (8), but nowt else
09:49 <@rnewman> I think your two choices are (a) do nothing, or (b) figure out how to find out which locales to search
09:49 <@rnewman> it *should* be in some AB_CD var somewhere
09:50 <@rnewman> if you can find that out, you can use that in place of en-US, and things should Just Work
09:55 < Pike> single locale builds wouldn't have en-US, I think?
09:56 < Pike> margaret: getting the selected locale from the gecko chrome registry for browser should get you the right locale value there
09:56 < Pike> and yay, yikes, java-gecko-yippiditpity
09:57 < margaret> Pike: yeah, the issue is we're spelunking in the jar from java
09:57 < margaret> this code is designed to work when gecko isn't running
09:58 < Pike> margaret: in that case, you probably want to do a fallback chain from th-TH to th, etc


So yeah, I think the answer is to not assume en-US, and instead to read chrome/chrome.manifest, just as the chrome registry would.

Comment 11

[:Margaret Leibovic]

/r/1369 - Bug 1105316 - Look in Gecko chrome registry for fallback locale in search activity

Pull down this commit:

hg pull review -r 7b9a350d5c85e5c92697e660b9bde00357d2f748

Comment 12

[:Margaret Leibovic]

I updated my patch based on the latest comments here. If there's a less hacky way to do this, please let me know!

Comment 13

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/1367/#review899

High-level logic looks good to me; I grabbed a proper multilocale APK and checked that it has only one locale registration for `global`, so we should be good to go in every context.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
(Diff revision 3)
> +                // We're looking for a line like "locale global en-US en-US/locale/en-US/global/"

N.B., this matches

https://developer.mozilla.org/en/docs/Chrome_Registration#locale

Might be worth commenting to that effect.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
(Diff revision 3)
> +                    fallbackLocale = line.split(" ")[2];

I think you can safely call

```
line.split(" ", 4)[2];
```

here -- if you're never accessing more than the 3rd element (index 2), you just want to accumulate junk in index 3, and thus can pass a limit.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
(Diff revision 3)
> +            isr = new InputStreamReader(in);

This constructor should never throw, so consider moving it outside of the `try` and saving yourself some lines in the `finally`.

Oh, and you should specify UTF-8.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
(Diff revision 3)
> +            BufferedReader br = new BufferedReader(isr);

You should be closing the `BufferedReader` instead of the `ISR`. It'll take care of closing the stream for you.

Take a look at this and kill a bunch of lines!

http://stackoverflow.com/questions/12199142/closing-bufferedreader-and-inputstreamreader

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
(Diff revision 3)
> +    private String getFallbackLocale() {

Make a note that this isn't totally thread-safe, and which thread it should be accessed from.

I believe the failure mode will be simply excess work, but that might change in the future.

Comment 14

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #13)

> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> (Diff revision 3)
> > +            isr = new InputStreamReader(in);
> 
> This constructor should never throw, so consider moving it outside of the
> `try` and saving yourself some lines in the `finally`.
> 
> Oh, and you should specify UTF-8.
> 
> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> (Diff revision 3)
> > +            BufferedReader br = new BufferedReader(isr);
> 
> You should be closing the `BufferedReader` instead of the `ISR`. It'll take
> care of closing the stream for you.
> 
> Take a look at this and kill a bunch of lines!
> 
> http://stackoverflow.com/questions/12199142/closing-bufferedreader-and-
> inputstreamreader

I believe I initially modeled all this logic after this sync code:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/Utils.java#508

Do you want to file a mentor bug to fix that up as well?

Comment 15

[:Margaret Leibovic]

/r/1369 - Bug 1105316 - Look in Gecko chrome registry for fallback locale in search activity. r=rnewman

Pull down this commit:

hg pull review -r 1ec1f99b0c31bb00264a84bf1ca9aa28a422e542

Comment 16

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #14)

> Do you want to file a mentor bug to fix that up as well?

Yes please!

Will get to this review today.

Comment 17

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/1367/#review981

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
(Diff revision 4)
> -            isr = new InputStreamReader(in);
> +            br = new BufferedReader(new InputStreamReader(in, "UTF-8"));

Lift this out, too. Without br's constructor succeeding you'll fail to close `in`, and neither BR nor ISR are documented to fail.

So this can simplify down to:

```
final BufferedReader br = new BufferedReader(new InputStreamReader(getInputStreamFromSearchPluginsJar("list.txt"), "UTF-8"));
try {
  ...
} finally {
  br.close();
}
```

Strictly speaking, ISR can throw an UnsupportedEncodingException, but this provably cannot occur when the input is "UTF-8".

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
(Diff revision 4)
> +            br = new BufferedReader(new InputStreamReader(in, "UTF-8"));

Same here.

(You might consider lifting out the InputStream -> BR logic into a tiny helper method.)

Comment 18

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #17)
> https://reviewboard.mozilla.org/r/1367/#review981
> 
> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> (Diff revision 4)
> > -            isr = new InputStreamReader(in);
> > +            br = new BufferedReader(new InputStreamReader(in, "UTF-8"));
> 
> Lift this out, too. Without br's constructor succeeding you'll fail to close
> `in`, and neither BR nor ISR are documented to fail.
> 
> So this can simplify down to:
> 
> ```
> final BufferedReader br = new BufferedReader(new
> InputStreamReader(getInputStreamFromSearchPluginsJar("list.txt"), "UTF-8"));
> try {
>   ...
> } finally {
>   br.close();
> }
> ```
> 
> Strictly speaking, ISR can throw an UnsupportedEncodingException, but this
> provably cannot occur when the input is "UTF-8".

Sure, *we* know that, but the compiler doesn't, so it complained about the exception, which is why I moved it inside the try statement. Is there a way to work around that?

Comment 19

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #18)

> Sure, *we* know that, but the compiler doesn't, so it complained about the
> exception, which is why I moved it inside the try statement. Is there a way
> to work around that?

One of these options:

* Mark this method as `throws UnsupportedEncodingException`, and allow a caller to muffle it.

* Use immediate assign to a final:

        final BufferedReader br;
        try {
            final InputStream in = getInputStreamFromSearchPluginsJar("list.txt");
            br = new BufferedReader(new InputStreamReader(in, "UTF-8"));
        } catch (UnsupportedEncodingException e) {
            // Cannot happen.
            return null;
        }

        // br will hereafter never be null.

* Refactor something like that into a method that'll give you a BufferedReader from an InputStream without throwing UEE.

Comment 20

[:Margaret Leibovic]

/r/1369 - Bug 1105316 - Look in Gecko chrome registry for fallback locale in search activity. r=rnewman

Pull down this commit:

hg pull review -r 42e908cf3c6d08cb8a108d6f7b11b169ef3ab0dd

Comment 21

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/1367/#review987

Ship It!

Comment 22

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/27e8bbeb5d2b

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27e8bbeb5d2b

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8533921 [details]
MozReview Request: bz://1105316/margaret

Approval Request Comment
[Feature/regressing bug #]:
  Search activity work in the past few releases.

[User impact if declined]:
  It's possible for Fennec to crash when it tries to discover the user's default search engine. This is most likely confined to single-locale repacks.

[Describe test coverage new/current, TBPL]:
  None; we can't perform automated testing of single-locale builds. See Bug 946058.

[Risks and why]: 
  Tracking 35. This is a fairly minimal patch: it adds logic to compute the global locale by reading chrome.manifest, rather than hard-coding "en-US".

[String/UUID change made/needed]:
  None.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d98e26a6fb83

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1c97f07f4f7d

Comment 28

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1105316 - Look in Gecko chrome registry for fallback locale in search activity. r=rnewman