Downloading an APK crashes fennec

VERIFIED FIXED in Firefox 27

Status

()

Firefox for Android
General
--
critical
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: dougt, Assigned: Federico Paolinelli)

Tracking

({crash, reproducible})

unspecified
Firefox 28
All
Android
crash, reproducible
Points:
---

Firefox Tracking Flags

(firefox27 verified, firefox28 verified, fennec28+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
E/AndroidRuntime(29018): FATAL EXCEPTION: main
E/AndroidRuntime(29018): java.lang.RuntimeException: Error receiving broadcast Intent { act=helperBroadcastAction dat=moz-notification:?eventType=notification-clicked&id={c1340fa0-61cb-4925-b0b0-e33d7e41d1c8} flg=0x10 bnds=[0,102][768,230] (has extras) } in org.mozilla.gecko.NotificationHelper$1@41e4eb58
E/AndroidRuntime(29018): 	at android.app.LoadedApk$ReceiverDispatcher$Args.run(LoadedApk.java:773)
E/AndroidRuntime(29018): 	at android.os.Handler.handleCallback(Handler.java:730)
E/AndroidRuntime(29018): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/AndroidRuntime(29018): 	at android.os.Looper.loop(Looper.java:137)
E/AndroidRuntime(29018): 	at android.app.ActivityThread.main(ActivityThread.java:5103)
E/AndroidRuntime(29018): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/AndroidRuntime(29018): 	at java.lang.reflect.Method.invoke(Method.java:525)
E/AndroidRuntime(29018): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
E/AndroidRuntime(29018): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
E/AndroidRuntime(29018): 	at dalvik.system.NativeStart.main(Native Method)
E/AndroidRuntime(29018): Caused by: java.lang.NullPointerException
E/AndroidRuntime(29018): 	at org.mozilla.gecko.NotificationHelper.hideNotification(NotificationHelper.java:318)
E/AndroidRuntime(29018): 	at org.mozilla.gecko.NotificationHelper.handleNotificationIntent(NotificationHelper.java:152)
E/AndroidRuntime(29018): 	at org.mozilla.gecko.NotificationHelper.access$100(NotificationHelper.java:29)
E/AndroidRuntime(29018): 	at org.mozilla.gecko.NotificationHelper$1.onReceive(NotificationHelper.java:106)
E/AndroidRuntime(29018): 	at android.app.LoadedApk$ReceiverDispatcher$Args.run(LoadedApk.java:763)
E/AndroidRuntime(29018): 	... 9 more
I/ActivityManager(  526): START u0 {act=android.intent.action.VIEW dat=file:///storage/emulated/0/Download/MozStumbler-v0.7.3.apk typ=application/vnd.android.package-archive flg=0x4000000 cmp=com.android.packageinstaller/.PackageInstallerActivity} from pid 28169



i tried to download: https://github.com/dougt/MozStumbler/releases/download/v0.7.3/MozStumbler-v0.7.3.apk
Wes and Federico - I am assuming this is a download notification issue. Is it a dupe of a known bug?
(Assignee)

Comment 2

4 years ago
Not any I am aware of :-(
(Assignee)

Comment 3

4 years ago
Will look into it this evening

Updated

4 years ago
tracking-fennec: --- → ?
Reproducible on tapping the notification after a download (Nightly 11/04)
Severity: normal → critical
Keywords: crash, reproducible
OS: All → Android
(Assignee)

Comment 5

4 years ago
The problem here is related to having multiple installs of fennec on the same device.
The click events are trapped by a broadcast receiver. It turns out that clicking on the notification triggers not only the broadcast receiver of the instance that started the download, but also the one of other installs of fennec.

This is confirmed by the fact that I was able to reproduce it only after installing nightly together with my build.
Moreover, if we look at the stacktrace:
I/ActivityManager(  526): START u0 {act=android.intent.action.VIEW dat=file:///storage/emulated/0/Download/MozStumbler-v0.7.3.apk typ=application/vnd.android.package-archive flg=0x4000000 cmp=com.android.packageinstaller/.PackageInstallerActivity} from pid 28169

The apk is getting opened by pid 28169 
BUT
The crash happens in pid 29018
E/AndroidRuntime(29018): Caused by: java.lang.NullPointerException

We need to figure out a way to filter our own broadcasts. Anyway this makes this bug a bit less severe I suppose.
(Reporter)

Comment 6

4 years ago
> The problem here is related to having multiple installs of fennec on the same device.


No.  I only have one install of fennec (nightly) on my device.
(Assignee)

Comment 7

4 years ago
Ok, I think I found what is going on.
It's a nightly related problem, or I should say, it happens when more than one process are running.
We are installing the NotificationHelper as a singleton bound to the application context.
The problem is, if we have more than one process (which is the case with nightly, because of updateservice), we also have more than one Applications objects, so two helpers and two receivers.
The receiver in the UpdateService process gets triggered, but in that case the notification client is not created because there is no GeckoActivity, which ends in that sad NPE.

TL;DR: It crashes only if more than one processes are active at the same time.

Patch is coming soon.
Duplicate of this bug: 936080
Created attachment 828770 [details] [diff] [review]
Untested patch. v1

We should be specifying a component for the notification intent. This shouldn't be a broadcast to which every running Fennec binds!

This patch does so.

See also the Part 1 in Bug 936080, which makes us check for things that are null or invalid in the first place.
N.B., doing it this way should ensure that (a) only this Fennec catches the intent (package-scoped), and (b) only NotificationHelper catches the intent (class is set).

This isn't total protection from other apps catching the intent if they want to, but it should solve this bug.
This will still not fix the problem of the UpdateService picking up the broadcast. But actually, maybe we should just use the LocalBroadcastManager? https://developer.android.com/reference/android/support/v4/content/LocalBroadcastManager.html
(Assignee)

Comment 12

4 years ago
This solves only part of the problem. As I wrote in comment 7 , the problem here is also that having a couple of processes in the same app (one for the app, the other one for the updateservice), results in having two different application objects and then two NotificationHelper "singletons".

See also https://groups.google.com/d/msg/android-developers/tI8Po3hPjic/z2IJagJWwvIJ , the interesting part is "If you use android:process to have an .apk run in multiple processes, each process will get its own Application instance". 

The crash is due to the fact that the Helper initialized into the Application object related to the NotificationService's process tries to access a field that would have been initialized if the activity was created.
(Assignee)

Comment 13

4 years ago
@Wesj: LocalBroadcasts can't be tied to pending intents, if I remember correctly.

Check: http://developer.android.com/reference/android/app/PendingIntent.html#getBroadcast(android.content.Context, int, android.content.Intent, int)
(Assignee)

Comment 14

4 years ago
Not at home now, I'll post a patch this evening or tomorrow evening
(Assignee)

Comment 15

4 years ago
And I guess it would be healthy to open a new bug to remove singletons creation from Application's onCreate.
tracking-fennec: ? → 28+
Assignee: nobody → fedepaol
(Assignee)

Comment 16

4 years ago
Created attachment 829535 [details] [diff] [review]
bug-934345-fix

I also tried to make rnewman's patch work, but it looks like explicit intents can't launch dinamically registered broadcast receivers, so there is no way to tell the intent that we want to trigger our receiver.
I also found this nice article about that http://onemikro2nd.blogspot.com/2013/09/darker-corners-of-android.html .

Said that, I do agree that it would be nice to avoid having a notification intent trigger several instance of fennec (even though it would be more a problem for testers / developer than real users). What we can do is to make the intent action dependent to the package name (concatenating with it / with a hash of it). This last part is not in the patch yet but it would be super easy to implement: something like
    private static final String HELPER_BROADCAST_ACTION = "helperBroadcastAction" + AppConstants.ANDROID_PACKAGE_NAME;
Attachment #828770 - Attachment is obsolete: true
Attachment #829535 - Flags: review?(wjohnston)
Comment on attachment 829535 [details] [diff] [review]
bug-934345-fix

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

::: mobile/android/base/GeckoApp.java
@@ +1292,5 @@
>              }
>          });
>  
>          GeckoAppShell.setNotificationClient(makeNotificationClient());
> +        NotificationHelper.init(getApplicationContext());

I want to be careful and make sure we're only initializing once (or we're bailing if we're already initialized). It looks like NotificationHelper does that check though. I wonder if it should return if its already initialized....
Attachment #829535 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 18

4 years ago
Created attachment 829575 [details] [diff] [review]
bug-934345-fix

Same as before, but as per my previous comment I replaced the intent action with something like PackageName + the action itself.

I also made the init method return if the instance is already initialized.
Attachment #829535 - Attachment is obsolete: true
Attachment #829575 - Flags: review?(wjohnston)
Attachment #829575 - Flags: review?(wjohnston) → review+
Comment on attachment 829575 [details] [diff] [review]
bug-934345-fix

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

::: mobile/android/base/NotificationHelper.java
@@ +29,5 @@
>  public final class NotificationHelper implements GeckoEventListener {
>      public static final String NOTIFICATION_ID = "NotificationHelper_ID";
>      private static final String LOGTAG = "GeckoNotificationManager";
>      private static final String HELPER_NOTIFICATION = "helperNotif";
> +    private static final String HELPER_BROADCAST_ACTION = AppConstants.ANDROID_PACKAGE_NAME + "helperBroadcastAction";

+ ".helperBroadcastAction";, please.
(Assignee)

Comment 20

4 years ago
Created attachment 829585 [details] [diff] [review]
bug-934345-fix

Here we go :-)
Attachment #829575 - Attachment is obsolete: true
Attachment #829585 - Flags: review+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2a399012c3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4a2a399012c3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
(Assignee)

Comment 23

4 years ago
While reviewing uplift for bug 932816, I did notice that aurora too is affected by this bug since we are initializing NotificationHelper into the application object: http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/GeckoApplication.java#101


I guess it is safe to uplift this as well (?)
Flags: needinfo?(wjohnston)

Updated

4 years ago
status-firefox27: --- → affected
Comment on attachment 829585 [details] [diff] [review]
bug-934345-fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 815202
User impact if declined: Crash if multiple fennecs are installed
Testing completed (on m-c, etc.): Landed long ago. Seems good
Risk to taking this patch (and alternatives if risky): Low risk. Just moves initialization and kills renames a message. Alternative is to back out 815202 (and a few things that went in on top of it).
String or IDL/UUID changes made by this patch: None.
Attachment #829585 - Flags: approval-mozilla-aurora?
Comment on attachment 829585 [details] [diff] [review]
bug-934345-fix

Given the low risk called out, approving on aurora.
Attachment #829585 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Keywords: verifyme
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ebf8f6c2246
status-firefox27: affected → fixed
status-firefox28: --- → fixed
Flags: needinfo?(wjohnston)

Comment 27

4 years ago
Verified as fixed in builds: 27 and 28 beta 4;
Device: Asus Transformer Tab (Android 4.0.3).
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
status-firefox28: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.