934345 - Downloading an APK crashes fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Doug Turner (:dougt)]

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

Comment 1

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

Wes and Federico - I am assuming this is a download notification issue. Is it a dupe of a known bug?

Comment 2

[Federico Paolinelli]

Not any I am aware of :-(

Comment 3

[Federico Paolinelli]

Will look into it this evening

Comment 4

[Aaron Train [:aaronmt]]

Reproducible on tapping the notification after a download (Nightly 11/04)

Comment 5

[Federico Paolinelli]

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.

Comment 6

[Doug Turner (:dougt)]

> 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.

Comment 7

[Federico Paolinelli]

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.

Comment 9

[Richard Newman [:rnewman]]

Attachment: 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.

Comment 10

[Richard Newman [:rnewman]]

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.

Comment 11

[Wesley Johnston (:wesj)]

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

Comment 12

[Federico Paolinelli]

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.

Comment 13

[Federico Paolinelli]

@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)

Comment 14

[Federico Paolinelli]

Not at home now, I'll post a patch this evening or tomorrow evening

Comment 15

[Federico Paolinelli]

And I guess it would be healthy to open a new bug to remove singletons creation from Application's onCreate.

Comment 16

[Federico Paolinelli]

Attachment: 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;

Comment 17

[Wesley Johnston (:wesj)]

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....

Comment 18

[Federico Paolinelli]

Attachment: 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.

Comment 19

[Richard Newman [:rnewman]]

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.

Comment 20

[Federico Paolinelli]

Attachment: bug-934345-fix

Here we go :-)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2a399012c3

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a2a399012c3

Comment 23

[Federico Paolinelli]

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 (?)

Comment 24

[Wesley Johnston (:wesj)]

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.

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 829585 [details] [diff] [review]
bug-934345-fix

Given the low risk called out, approving on aurora.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ebf8f6c2246

Comment 27

[Flaviu Cos]

Verified as fixed in builds: 27 and 28 beta 4;
Device: Asus Transformer Tab (Android 4.0.3).