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

VERIFIED FIXED in Firefox 36

Status

defect
--
critical
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: cos_flaviu, Assigned: rnewman)

Tracking

({crash})

36 Branch
Firefox 38
All
Android
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox35 wontfix, firefox36+ verified, firefox37+ verified, firefox38+ verified, fennec36+)

Details

(crash signature)

Attachments

(3 attachments)

Reporter

Description

4 years ago
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)
[Tracking Requested - why for this release]:
tracking-fennec: --- → ?
Reporter

Comment 2

4 years ago
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.
Topcrash, tracking.
Mark, do you know who could help on this? Thanks
Flags: needinfo?(mark.finkle)
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.
Flags: needinfo?(mark.finkle) → needinfo?(margaret.leibovic)
I can take a look at this.
Assignee: nobody → margaret.leibovic
Flags: needinfo?(margaret.leibovic)
rnewman reviewed these changes, so let's see if he wants to get involved, too :)
Flags: needinfo?(rnewman)
Assignee

Comment 7

4 years ago
        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.
Assignee: margaret.leibovic → rnewman
Status: NEW → ASSIGNED
Flags: needinfo?(rnewman)
Assignee

Comment 8

4 years ago
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.
Attachment #8555968 - Flags: review?(michael.l.comella)
Attachment #8555968 - Flags: review?(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]:
-----------------------------------------------------------------

::: 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?
Attachment #8555968 - Flags: review?(michael.l.comella) → review+
(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.
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?
Reporter

Comment 12

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

Comment 13

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

Comment 15

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

Comment 16

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

Comment 17

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

Comment 18

4 years ago
Flaviu: can we get the adb logs from before the crash?
Flags: needinfo?(flaviu.cos)
Reporter

Comment 19

4 years ago
Posted file logs.txt
I attached the logs.
Flags: needinfo?(flaviu.cos)
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.
Attachment #8555968 - Flags: review?(margaret.leibovic) → review-
Assignee

Comment 21

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

Comment 22

4 years ago
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)
Richard, do you have a clue for this bug? Thanks
Flags: needinfo?(rnewman)
Assignee

Comment 24

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

Comment 25

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

Comment 26

4 years ago
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.
Attachment #8562266 - Flags: review?(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.
Attachment #8562266 - Flags: review?(margaret.leibovic) → review+
Assignee

Comment 30

4 years ago
But but but I built and ran that test!
Flags: needinfo?(rnewman)
Assignee

Comment 32

4 years ago
Looks like someone didn't run mcMerge.

http://hg.mozilla.org/mozilla-central/rev/a803874de085
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Assignee

Comment 33

4 years ago
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.
Attachment #8562266 - Flags: approval-mozilla-beta?
Attachment #8562266 - Flags: approval-mozilla-aurora?
Attachment #8562266 - Flags: approval-mozilla-beta?
Attachment #8562266 - Flags: approval-mozilla-beta+
Attachment #8562266 - Flags: approval-mozilla-aurora?
Attachment #8562266 - Flags: approval-mozilla-aurora+
Reporter

Comment 36

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

Comment 37

4 years ago
Verified as fixed in Firefox 36 Beta 10;
Device: Lenovo Yoga Tab 10 (Android 4.4.2).
Status: RESOLVED → VERIFIED

Updated

a year ago
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.