1062904 - crash in java.lang.NullPointerException: uriString at android.net.Uri$StringUri.<init>(Uri.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Flaviu Cos]

This bug was filed from the Socorro interface and is 
report bp-76b0c54f-842b-4710-9c86-43b032140904.
=============================================================
Environment: 
Device: Google Nexus 7 (Android 4.4.4);
Build: Nightly 34.0a1 (2014-09-04);

Steps to reproduce:
1. Load a website which contains images (e.g. mozilla.org);
2. Long tap on a image;
3. Select image from the context menu;
4. Tap on 'Set image as'.

Expected result:
A list with the available options is displayed.

Actual result:
Firefox crashes.

Comment 1

[Aaron Train [:aaronmt]]

java.lang.NullPointerException: uriString
	at android.net.Uri$StringUri.<init>(Uri.java:467)
	at android.net.Uri$StringUri.<init>(Uri.java:457)
	at android.net.Uri.parse(Uri.java:429)
	at org.mozilla.gecko.GeckoApp.setImageAs(GeckoApp.java:971)
	at org.mozilla.gecko.GeckoApp.handleMessage(GeckoApp.java:591)
	at org.mozilla.gecko.BrowserApp.handleMessage(BrowserApp.java:1329)
	at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:163)
	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2323)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:356)
	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:176)

I'm not able to reproduce on my Nexus 5, I get a picker with 'Contact Photo, Wallpaper (Gallery), Wallpaper (Google +)'

Comment 2

[Dominique Vincent [:domivinc]]

Media.insertImage is called to save the image in a local file before the call to the chooser activity.
The returned value (path) is not tested in the code. It could be null if there is no SD card in your phone or if your SD card access is blocked ( USB connection will block SD card in Mass storage mode).
If the path is null you get the exception.

I could make a patch to fix that. I could add a toast message when the path is null: "Unable to save the image to your SD Card." ?

Comment 3

[Aaron Train [:aaronmt]]

Suggestions?

Comment 4

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

(In reply to Dominique Vincent [:domivinc] from comment #2)
> Media.insertImage is called to save the image in a local file before the
> call to the chooser activity.
> The returned value (path) is not tested in the code. It could be null if
> there is no SD card in your phone or if your SD card access is blocked ( USB
> connection will block SD card in Mass storage mode).
> If the path is null you get the exception.
> 
> I could make a patch to fix that. I could add a toast message when the path
> is null: "Unable to save the image to your SD Card." ?

I worry about using "SD Card" language since the majority of people likely have no idea what that means. Let's start with "Unable to save the image" and see how it goes.

Comment 5

[Dominique Vincent [:domivinc]]

Attachment: patch_1062904.diff

Toast message added:  "Unable to save the image"

Comment 6

[Aaron Train [:aaronmt]]

Comment on attachment 8486446 [details] [diff] [review]
patch_1062904.diff

switching reviewers

Comment 7

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

Comment on attachment 8486446 [details] [diff] [review]
patch_1062904.diff

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>             if (image != null) {
>                 String path = Media.insertImage(getContentResolver(),image, null, null);
>-                final Intent intent = new Intent(Intent.ACTION_ATTACH_DATA);
>-                intent.addCategory(Intent.CATEGORY_DEFAULT);
>-                intent.setData(Uri.parse(path));
>-
>-                // Removes the image from storage once the chooser activity ends.
>-                Intent chooser = Intent.createChooser(intent, getString(R.string.set_image_chooser_title));
>-                ActivityResultHandler handler = new ActivityResultHandler() {
>-                    @Override
>-                    public void onActivityResult (int resultCode, Intent data) {
>-                        getContentResolver().delete(intent.getData(), null, null);
>-                    }
>-                };
>-                ActivityHandlerHelper.startIntentForActivity(this, chooser, handler);
>+                if (path != null) {
>+                    final Intent intent = new Intent(Intent.ACTION_ATTACH_DATA);
>+                    intent.addCategory(Intent.CATEGORY_DEFAULT);
>+                    intent.setData(Uri.parse(path));
>+
>+                    // Removes the image from storage once the chooser activity ends.
>+                    Intent chooser = Intent.createChooser(intent, getString(R.string.set_image_chooser_title));
>+                    ActivityResultHandler handler = new ActivityResultHandler() {
>+                        @Override
>+                        public void onActivityResult (int resultCode, Intent data) {
>+                            getContentResolver().delete(intent.getData(), null, null);
>+                        }
>+                    };
>+                    ActivityHandlerHelper.startIntentForActivity(this, chooser, handler);
>+                } else {
>+                    Toast.makeText((Context) this, R.string.set_image_path_fail, Toast.LENGTH_SHORT).show();
>+                }

This code is almost indented to the point where I'd suggest using early returns, but we can use this.

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

> <!ENTITY set_image_fail "Unable to set image">
>+<!ENTITY set_image_path_fail "Unable to save the image">

Given the "Unable to set image" (no 'the') let's be consistent and use: "Unable to save image"

r+ but put up a new patch so we can check it in with the string change

Comment 8

[Dominique Vincent [:domivinc]]

Attachment: patch_1062904_2.diff

New patch with reviewer changes.

Comment 9

[Dominique Vincent [:domivinc]]

Hi, I don't know the procedure to get this patch tested on the try server. Should I change the status of the bug?

Comment 10

[Kevin Brosnan [:kbrosnan]]

Looks like this slipped by everyone. Here is a try push of Android and B2G which should satisfy the sheriffs. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=01a2a9651cbb

Looks like you have fixed a couple bugs, when you have a moment requesting level 1 access will allow you to do this yourself. https://www.mozilla.org/hacking/commit-access-policy/

Comment 11

[Kevin Brosnan [:kbrosnan]]

[Tracking Requested - why for this release]: #19 top crash for release.

Comment 12

[Kevin Brosnan [:kbrosnan]]

How do you feel about a patch to Beta 33 and Aurora 34 that silently fails to set the background image rather than crashing? Noticed that there are string changes here and those need to ride the trains.

Comment 13

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

(In reply to Kevin Brosnan [:kbrosnan] from comment #12)
> How do you feel about a patch to Beta 33 and Aurora 34 that silently fails
> to set the background image rather than crashing? Noticed that there are
> string changes here and those need to ride the trains.

I can live with silent fails instead of crashes.

Comment 14

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

The Try run looks green enough. Requesting checkin.

Comment 15

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

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

Comment 16

[Daniel Holbert [:dholbert]]

>  Bug 1062904 - crash in java.lang.NullPointerException: uriString at android.net.Uri.<init>(Uri.java) r=mfinkle

Just a heads-up, for future bugs that you work on: in Mozilla code, commit messages are generally supposed to describe the change, rather than describing the bug (or being the copypasted bug summary).

For reference, see:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Thanks!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/870723c4c3a5

Comment 18

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

Can we get a patch for Fx34 and Fx33 that does not add any strings?

Comment 19

[Dominique Vincent [:domivinc]]

Attachment: patch_1062904_3.diff

New patch that does not add any strings.

Comment 20

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

Comment on attachment 8491832 [details] [diff] [review]
patch_1062904_3.diff

You made this patch on top of the previous work which has landed on mozilla-central. Instead, we need to put the null check code on mozilla-aurora, without any strings.

I can make one using your original patch.

Comment 21

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

Attachment: saveimage-nullcheck v0.1

This patch is a "no strings" version of what landed on Nightly. This patch is for Aurora and Beta.

Approval Request Comment
[Feature/regressing bug #]: Bug 886996
[User impact if declined]: Crashes on some devices when saving an image as a background
[Describe test coverage new/current, TBPL]: Manual
[Risks and why]: Low risks
[String/UUID change made/needed]: none

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8492136 [details] [diff] [review]
saveimage-nullcheck v0.1

Beta+
Aurora+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01483c7e8fe9
https://hg.mozilla.org/releases/mozilla-beta/rev/4bfa8b78669c

Comment 24

[Flaviu Cos]

The crash is no longer reproducible, but still can not use 'Set image as' option.

Tapping on 'Set image as' throws the following exception in logcat:
Failed to insert image
java.io.FileNotFoundException: No such file or directory
at android.database.DatabaseUtils.readExceptionWithFileNotFoundExceptionFromParcel(DatabaseUtils.java:146)
at android.content.ContentProviderProxy.openAssetFile(ContentProviderNative.java:611)
at android.content.ContentResolver.openAssetFileDescriptor(ContentResolver.java:922)
at android.content.ContentResolver.openOutputStream(ContentResolver.java:669)
at android.content.ContentResolver.openOutputStream(ContentResolver.java:645)
at android.provider.MediaStore$Images$Media.insertImage(MediaStore.java:902)
at org.mozilla.gecko.GeckoApp.setImageAs(GeckoApp.java:967)
at org.mozilla.gecko.GeckoApp.handleMessage(GeckoApp.java:595)
at org.mozilla.gecko.BrowserApp.handleMessage(BrowserApp.java:1482)
at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:168)
at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2327)
at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:365)
at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:186)

Comment 25

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

(In reply to Flaviu Cos, QA [:flaviu] from comment #24)
> The crash is no longer reproducible, but still can not use 'Set image as'
> option.
> 
> Tapping on 'Set image as' throws the following exception in logcat:
> Failed to insert image
> java.io.FileNotFoundException: No such file or directory

This seems like the expected result now.

Comment 26

[u279076]

I'm still seeing crashes with Fennec 33/34 being reported, although volume is pretty low. Can this truly be called fixed? Is a follow-up bug required?

Source:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+uriString+at+android.net.Uri%24StringUri.%3Cinit%3E%28Uri.java%29&product=FennecAndroid&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2014-10-30+18%3A00%3A00&range_value=1#reports