Closed Bug 1062904 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect, critical)

All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 35
Tracking Status
firefox32 --- wontfix
firefox33 + fixed
firefox34 + fixed
firefox35 + fixed
fennec 33+ ---

People

(Reporter: cos_flaviu, Assigned: domivinc)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 2 obsolete files)

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.
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 +)'
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." ?
Suggestions?
Blocks: 886996
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
Attached patch patch_1062904.diff (obsolete) — Splinter Review
Toast message added:  "Unable to save the image"
Attachment #8486446 - Flags: review?(aaron.train)
Comment on attachment 8486446 [details] [diff] [review]
patch_1062904.diff

switching reviewers
Attachment #8486446 - Flags: review?(aaron.train) → review?(mark.finkle)
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
Attachment #8486446 - Flags: review?(mark.finkle) → review+
New patch with reviewer changes.
Attachment #8486446 - Attachment is obsolete: true
Attachment #8486504 - Flags: review?(mark.finkle)
Attachment #8486504 - Flags: review?(mark.finkle) → review+
Hi, I don't know the procedure to get this patch tested on the try server. Should I change the status of the bug?
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/
[Tracking Requested - why for this release]: #19 top crash for release.
tracking-fennec: --- → ?
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.
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
The Try run looks green enough. Requesting checkin.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/870723c4c3a5
Assignee: nobody → domivinc
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
>  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!
https://hg.mozilla.org/mozilla-central/rev/870723c4c3a5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Can we get a patch for Fx34 and Fx33 that does not add any strings?
tracking-fennec: ? → 33+
Attached patch patch_1062904_3.diff (obsolete) — Splinter Review
New patch that does not add any strings.
Attachment #8491832 - Flags: review?(mark.finkle)
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.
Attachment #8491832 - Flags: review?(mark.finkle) → review-
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
Attachment #8491832 - Attachment is obsolete: true
Attachment #8492136 - Flags: review+
Attachment #8492136 - Flags: approval-mozilla-beta?
Attachment #8492136 - Flags: approval-mozilla-aurora?
Comment on attachment 8492136 [details] [diff] [review]
saveimage-nullcheck v0.1

Beta+
Aurora+
Attachment #8492136 - Flags: approval-mozilla-beta?
Attachment #8492136 - Flags: approval-mozilla-beta+
Attachment #8492136 - Flags: approval-mozilla-aurora?
Attachment #8492136 - Flags: approval-mozilla-aurora+
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)
(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.
You need to log in before you can comment on or make changes to this bug.