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

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: aaronmt, Assigned: Margaret)

Tracking

({crash})

35 Branch
Firefox 37
All
Android
crash
Points:
---

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed, firefox37 fixed, fennec35+)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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)
See Also: → bug 1105290
(Reporter)

Comment 2

4 years ago
Couple reports coming in on crash-stats for 35. Possibly easy to reproduce?
(Reporter)

Comment 3

4 years ago
(36 too)
status-firefox36: --- → affected
(Assignee)

Comment 4

4 years ago
Created attachment 8533921 [details]
MozReview Request: bz://1105316/margaret
Attachment #8533921 - Flags: review?(rnewman)
(Assignee)

Comment 5

4 years ago
/r/1369 - Bug 1105316 - Add null check in createEngineFromLocale

Pull down this commit:

hg pull review -r c70eed8f11bd712f8a8bdf84082678e6c30ae208
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?
(Assignee)

Comment 7

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

Comment 8

4 years ago
/r/1369 - Bug 1105316 - Add null check in createEngineFromLocale

Pull down this commit:

hg pull review -r 7bb2502adf56db1012fa91eedf4fdef041c1a05b
> 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.
(Assignee)

Updated

4 years ago
tracking-fennec: ? → 35+
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.
(Assignee)

Comment 11

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

Pull down this commit:

hg pull review -r 7b9a350d5c85e5c92697e660b9bde00357d2f748
(Assignee)

Comment 12

4 years ago
I updated my patch based on the latest comments here. If there's a less hacky way to do this, please let me know!
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 14

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

Comment 15

4 years ago
/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
(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.
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.)
(Assignee)

Comment 18

4 years ago
(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?
(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.
(Assignee)

Comment 20

4 years ago
/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
Attachment #8533921 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/27e8bbeb5d2b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
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.
Attachment #8533921 - Flags: approval-mozilla-beta?
Attachment #8533921 - Flags: approval-mozilla-aurora?
status-firefox37: --- → fixed
Attachment #8533921 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8533921 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox36: affected → fixed
(Assignee)

Comment 27

3 years ago
Comment on attachment 8533921 [details]
MozReview Request: bz://1105316/margaret
Attachment #8533921 - Attachment is obsolete: true
Attachment #8618732 - Flags: review+
(Assignee)

Comment 28

3 years ago
Created attachment 8618732 [details]
MozReview Request: Bug 1105316 - Look in Gecko chrome registry for fallback locale in search activity. r=rnewman
You need to log in before you can comment on or make changes to this bug.