1126240 - crash in java.lang.NullPointerException: lock == null at java.io.Reader.<init>(Reader.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Flaviu Cos]

This bug was filed from the Socorro interface and is 
report bp-c49f8300-f060-49a3-9065-b1e432150127.
=============================================================

Environment: 
Device: Lenovo Yoga Tab 10 (Android 4.4.2);
Build: Firefox 36 Beta 4;

Steps to reproduce:
1. Launch Search Activity from the widget.

Expected result:
Search Activity is launched and works correctly.

Actual result:
Firefox crashes.

Stack Trace:

java.lang.NullPointerException: lock == null
	at java.io.Reader.<init>(Reader.java:64)
	at java.io.InputStreamReader.<init>(InputStreamReader.java:77)
	at org.mozilla.search.providers.SearchEngineManager.getBufferedReader(SearchEngineManager.java:497)
	at org.mozilla.search.providers.SearchEngineManager.getFallbackLocale(SearchEngineManager.java:455)
	at org.mozilla.search.providers.SearchEngineManager.getInputStreamFromSearchPluginsJar(SearchEngineManager.java:439)
	at org.mozilla.search.providers.SearchEngineManager.createEngineFromLocale(SearchEngineManager.java:316)
	at org.mozilla.search.providers.SearchEngineManager.access$500(SearchEngineManager.java:39)
	at org.mozilla.search.providers.SearchEngineManager$2.run(SearchEngineManager.java:170)
	at android.os.Handler.handleCallback(Handler.java:808)
	at android.os.Handler.dispatchMessage(Handler.java:103)
	at android.os.Looper.loop(Looper.java:193)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43)

Comment 1

[Aaron Train [:aaronmt]]

[Tracking Requested - why for this release]:

Comment 2

[Flaviu Cos]

After further investigation I found that the crash is reproducible if the Search Activity widget is added to homescreen then fennec is moved to SD card. See bug 1017135.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Topcrash, tracking.
Mark, do you know who could help on this? Thanks

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Margaret probably has an idea of what's going wrong. If this crash is low enough in frequency, we might be able to mentor it to a contributor.

Comment 5

[:Margaret Leibovic]

I can take a look at this.

Comment 6

[:Margaret Leibovic]

rnewman reviewed these changes, so let's see if he wants to get involved, too :)

Comment 7

[Richard Newman [:rnewman]]

        final InputStream in = GeckoJarReader.getStream(getJarURL("!/chrome/chrome.manifest"));
        final BufferedReader br = getBufferedReader(in);

getStream can return null. We should have a null check here.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Recover gracefully if we can't read chrome.manifest when fetching fallback locale. v1

Flagging two reviewers in case Margaret doesn't work after dinner.

This can only happen if GeckoJarReader.getStream returns null. That'll happen if we can't read chrome.manifest from the JAR. In this case, we just return en-US and hope for the best; presumably we'll fall over later for a different reason.

Comment 9

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

Comment on attachment 8555968 [details] [diff] [review]
Recover gracefully if we can't read chrome.manifest when fetching fallback locale. v1

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

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +484,5 @@
>          }
>  
>          final InputStream in = GeckoJarReader.getStream(getJarURL("!/chrome/chrome.manifest"));
> +        if (in == null) {
> +            Log.wtf(LOG_TAG, "Unable to read chrome.manifest. This should never happen.");

Why should this never happen? It's apparently happened before!

Seems like a fine fallback mechanism but I don't think we're fixing the actual issue here. Can we dive a bit deeper to potentially find out what's going on with the JAR/file path/chrome.manifest?

Comment 10

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #9)
> Comment on attachment 8555968 [details] [diff] [review]
> Recover gracefully if we can't read chrome.manifest when fetching fallback
> locale. v1
> 
> Review of attachment 8555968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> @@ +484,5 @@
> >          }
> >  
> >          final InputStream in = GeckoJarReader.getStream(getJarURL("!/chrome/chrome.manifest"));
> > +        if (in == null) {
> > +            Log.wtf(LOG_TAG, "Unable to read chrome.manifest. This should never happen.");
> 
> Why should this never happen? It's apparently happened before!
> 
> Seems like a fine fallback mechanism but I don't think we're fixing the
> actual issue here. Can we dive a bit deeper to potentially find out what's
> going on with the JAR/file path/chrome.manifest?

Looking at the crash reports, it seems like over half of them are from these Lenovo devices. Do we have one in the SF office to try to reproduce and figure out where things are going wrong with GeckoJarReader?

It looks like there are places where we just eat exceptions and return null, e.g.:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/GeckoJarReader.java#130
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/GeckoJarReader.java#113

Also, if we're having problems reading the chrome.manifest, are we also going to have problems down the road trying to read the search plugin out of the jar as well?

I would be ok with landing a band-aid, but we would need to look out for these potential new crashes.

Comment 11

[:Margaret Leibovic]

Flaviu reminded me on IRC that this crash is only reproducible after moving Fennec to the SD card and back (comment 2).

What affect would moving the app around like this have on poking into the jar files? Would that lead to issues reading out of the jar?

Comment 12

[Flaviu Cos]

Revised steps to reproduce:

1. Install fennec;
2. Add Search Activity widget to homescreen;
3. Go to Device settings -> Apps -> fennec;
4. Move storage to SD card;
5. Go back to homescreen;
6. Tap on Search Activity widget to launch it.

Can not reproduce on Samsung Galaxy S4 (Android 4.4.2), or Samsung Galaxy R (Android 2.3.4) because after moving the app to SD card the widget is automatically removed from the homescreen.

Comment 13

[Richard Newman [:rnewman]]

That implies that:

     private String getJarURL(String path) {
         return "jar:jar:file://" + context.getPackageResourcePath() + "!/" + AppConstants.OMNIJAR_NAME + path;
     }

does not return a correct path when we're installed on the SD card.

Comment 14

[Nick Alexander :nalexander [he/him]]

Can we extract this jar:jar:file:// nonsense into a helper, and unify access throughout the codebase?  Perhaps this has already happened in this patch.

Comment 15

[Richard Newman [:rnewman]]

My speculation (while I wait to find a device that'll let me repro) is that getPackageResourcePath is the wrong method to use, and we should be using one of

context.getPackageCodePath()

or one of the appinfo fields:

context.getApplicationInfo().sourceDir
context.getApplicationInfo().dataDir
context.getApplicationInfo().nativeLibraryDir

or similar. One of those will match the location of the extracted JAR, which is probably treated as an asset, not a resource.

Comment 16

[Richard Newman [:rnewman]]

This is pretty annoying.

When we're moved to the SD card, our APK moves from

/data/app/org.mozilla.fennec_rnewman-1.apk

to

/mnt/asec/org.mozilla.fennec_rnewman-2/pkg.apk

In that -2 dir is *only* /lib and pkg.apk. We don't have a ".apk!/".

context.getAssets().open("omni.ja") does give us omni.ja, but that gives us an AssetInputStream directly. Which probably isn't what we want.

Comment 17

[Richard Newman [:rnewman]]

I can't reproduce this.

On my 2.3.5 device the widget sticks around, but tapping it does nothing, so I can't get a crash.

I can't even get a crash if I launch the activity with `am start` -- it'll successfully find the fallback locale.

So my speculation is that the APK doesn't contain omni.ja, but I have no way of finding out. We could make this crash more descriptively, we could add logs about the failure and ask Flaviu to repro with logs, or we could band-aid it blind.

Comment 18

[Richard Newman [:rnewman]]

Flaviu: can we get the adb logs from before the crash?

Comment 19

[Flaviu Cos]

Attachment: logs.txt

I attached the logs.

Comment 20

[:Margaret Leibovic]

Comment on attachment 8555968 [details] [diff] [review]
Recover gracefully if we can't read chrome.manifest when fetching fallback locale. v1

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

r-, since this band-aid is gonna fail when we try to actually get the search plugin.

Comment 21

[Richard Newman [:rnewman]]

Weeelllll, that'll do it.

Exception getting input stream from jar URL: jar:jar:file:///mnt/asec2/[2]org.mozilla.fennec-1/pkg.apk!/assets/omni.ja!/chrome/chrome.manifest
java.net.URISyntaxException: Illegal character in path at index 18: file:///mnt/asec2/[2]org.mozilla.fennec-1/pkg.apk

Comment 22

[Richard Newman [:rnewman]]

Every call site:

Exception getting input stream from jar URL: jar:jar:file:///mnt/asec2/[2]org.mozilla.fennec-1/pkg.apk!/assets/omni.ja!/chrome/en-US/locale/en-US/browser/searchplugins/list.txt
java.net.URISyntaxException: Illegal character in path at index 18: file:///mnt/asec2/[2]org.mozilla.fennec-1/pkg.apk
	at libcore.net.UriCodec.validate(UriCodec.java:63)
	at java.net.URI.parseURI(URI.java:402)
	at java.net.URI.<init>(URI.java:204)
	at org.mozilla.gecko.util.GeckoJarReader.getZipFile(GeckoJarReader.java:99)
	at org.mozilla.gecko.util.GeckoJarReader.getStream(GeckoJarReader.java:107)
	at org.mozilla.search.providers.SearchEngineManager.getInputStreamFromSearchPluginsJar(SearchEngineManager.java:455)
	at org.mozilla.search.providers.SearchEngineManager.createEngineFromLocale(SearchEngineManager.java:348)
	at org.mozilla.search.providers.SearchEngineManager.access$400(SearchEngineManager.java:39)
	at org.mozilla.search.providers.SearchEngineManager$2.defaultBehavior(SearchEngineManager.java:202)
	at org.mozilla.search.providers.SearchEngineManager$2.distributionNotFound(SearchEngineManager.java:147)
	at org.mozilla.gecko.distribution.Distribution$4.run(Distribution.java:880)
	at android.os.Handler.handleCallback(Handler.java:808)
	at android.os.Handler.dispatchMessage(Handler.java:103)
	at android.os.Looper.loop(Looper.java:193)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43)
Exception getting input stream from jar URL: jar:jar:file:///mnt/asec2/[2]org.mozilla.fennec-1/pkg.apk!/assets/omni.ja!/chrome/en/locale/en/browser/searchplugins/list.txt
java.net.URISyntaxException: Illegal character in path at index 18: file:///mnt/asec2/[2]org.mozilla.fennec-1/pkg.apk
	at libcore.net.UriCodec.validate(UriCodec.java:63)
	at java.net.URI.parseURI(URI.java:402)
	at java.net.URI.<init>(URI.java:204)
	at org.mozilla.gecko.util.GeckoJarReader.getZipFile(GeckoJarReader.java:99)
	at org.mozilla.gecko.util.GeckoJarReader.getStream(GeckoJarReader.java:107)
	at org.mozilla.search.providers.SearchEngineManager.getInputStreamFromSearchPluginsJar(SearchEngineManager.java:464)
	at org.mozilla.search.providers.SearchEngineManager.createEngineFromLocale(SearchEngineManager.java:348)
	at org.mozilla.search.providers.SearchEngineManager.access$400(SearchEngineManager.java:39)
	at org.mozilla.search.providers.SearchEngineManager$2.defaultBehavior(SearchEngineManager.java:202)
	at org.mozilla.search.providers.SearchEngineManager$2.distributionNotFound(SearchEngineManager.java:147)
	at org.mozilla.gecko.distribution.Distribution$4.run(Distribution.java:880)
	at android.os.Handler.handleCallback(Handler.java:808)
	at android.os.Handler.dispatchMessage(Handler.java:103)
	at android.os.Looper.loop(Looper.java:193)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43)
Exception getting input stream from jar URL: jar:jar:file:///mnt/asec2/[2]org.mozilla.fennec-1/pkg.apk!/assets/omni.ja!/chrome/chrome.manifest
java.net.URISyntaxException: Illegal character in path at index 18: file:///mnt/asec2/[2]org.mozilla.fennec-1/pkg.apk
	at libcore.net.UriCodec.validate(UriCodec.java:63)
	at java.net.URI.parseURI(URI.java:402)
	at java.net.URI.<init>(URI.java:204)
	at org.mozilla.gecko.util.GeckoJarReader.getZipFile(GeckoJarReader.java:99)
	at org.mozilla.gecko.util.GeckoJarReader.getStream(GeckoJarReader.java:107)
	at org.mozilla.search.providers.SearchEngineManager.getFallbackLocale(SearchEngineManager.java:486)
	at org.mozilla.search.providers.SearchEngineManager.getInputStreamFromSearchPluginsJar(SearchEngineManager.java:471)
	at org.mozilla.search.providers.SearchEngineManager.createEngineFromLocale(SearchEngineManager.java:348)
	at org.mozilla.search.providers.SearchEngineManager.access$400(SearchEngineManager.java:39)
	at org.mozilla.search.providers.SearchEngineManager$2.defaultBehavior(SearchEngineManager.java:202)
	at org.mozilla.search.providers.SearchEngineManager$2.distributionNotFound(SearchEngineManager.java:147)
	at org.mozilla.gecko.distribution.Distribution$4.run(Distribution.java:880)

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Richard, do you have a clue for this bug? Thanks

Comment 24

[Richard Newman [:rnewman]]

Yes, just haven't cycled around to fix it yet. Enough details are in the bug that someone with more time could pick this up -- I suggest one of three solutions:

* Correct escaping, if that allows [] in the URI.
* Relative JAR URLs, so we don't need to include the APK path in the URI.
* Not using JAR URLs that refer to the APK.

And on top: refactor JAR access so that some shared logic isn't duplicated. See also favicon loading.

Comment 25

[Richard Newman [:rnewman]]

Yup, those should be escaped, and if they are we eventually get the right file path.

Comment 26

[Richard Newman [:rnewman]]

Attachment: Correctly encode APK paths in SearchEngineManager. v1

This is the approach we already take everywhere else we make a jar:jar: URI.

I've unified those places into GeckoJarReader, cleaned up imports, fixed a
typo, and wrote a trivial test for this case.

I made a few utility methods static to facilitate testing and future refactoring.

Comment 27

[:Margaret Leibovic]

Comment on attachment 8562266 [details] [diff] [review]
Correctly encode APK paths in SearchEngineManager. v1

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

Nice, this is a good improvment.

::: mobile/android/base/favicons/Favicons.java
@@ -500,5 @@
>      private static String getBrandingBitmapPath(Context context, String name) {
> -        final String apkPath = context.getPackageResourcePath();
> -        return "jar:jar:" + new File(apkPath).toURI() + "!/" +
> -               AppConstants.OMNIJAR_NAME + "!/" +
> -               "chrome/chrome/content/branding/" + name;

Nice to see this pattern get factored out.

::: mobile/android/base/util/GeckoJarReader.java
@@ +174,5 @@
>              return results;
>          }
>      }
> +
> +    public static String getJarURL(Context context, String path) {

Document this to explain what 'path' is supposed to be.

Comment 28

[Richard Newman [:rnewman]]

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

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/bcc03debfb8f for android build failures:


https://treeherder.mozilla.org/logviewer.html#?job_id=1958597&repo=fx-team

Comment 30

[Richard Newman [:rnewman]]

But but but I built and ran that test!

Comment 31

[Richard Newman [:rnewman]]

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

Comment 32

[Richard Newman [:rnewman]]

Looks like someone didn't run mcMerge.

http://hg.mozilla.org/mozilla-central/rev/a803874de085

Comment 33

[Richard Newman [:rnewman]]

Comment on attachment 8562266 [details] [diff] [review]
Correctly encode APK paths in SearchEngineManager. v1

Approval Request Comment
[Feature/regressing bug #]:
  Search activity.

[User impact if declined]:
  Crashes if your device is unlucky enough to have an unexpected character in the path it allocates to Firefox. This should only affect users that have moved Firefox to their SD card, but it will reliably crash.

[Describe test coverage new/current, TreeHerder]:
  New simple test.

[Risks and why]: 
  Should be low risk: collects multiple identical implementations together that don't suffer from the bug, and then uses that implementation instead of the buggy one.

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d83c055e2a9

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Flaviu Cos]

Verified as fixed in builds:
- 38.0a1 2015-02-16;
- 37.0a2 2015-02-16;
Device: Lenovo Yoga Tab 10 (Android 4.4.2).

Comment 37

[Flaviu Cos]

Verified as fixed in Firefox 36 Beta 10;
Device: Lenovo Yoga Tab 10 (Android 4.4.2).