974578 - 'java.lang.IllegalArgumentException: Receiver not registered' after installing a webapp

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P2)

Description

[krupa raj[:krupa]]

Android 4.0.4/ Firefox Mobile 30a1 (02/19) 
Samsung Galaxy

setup:
1) about:config
2) search for apk
3) Change browser.webapps.apkFactoryUrl to
https://apk-controller.stage.mozaws.net/application.apk

steps to reproduce:
1. Load marketplace-dev.allizom.org on Firefox mobile
2. Search for "Keeper web app"
3. Install the app

Logcat shows : 
-19 19:48:48.556 E/GeckoWebappEventListener(18237): error unregistering install receiver: 
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): java.lang.IllegalArgumentException: Receiver not registered: org.mozilla.gecko.webapp.InstallListener@413f4f10
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.app.LoadedApk.forgetReceiverDispatcher(LoadedApk.java:628)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.app.ContextImpl.unregisterReceiver(ContextImpl.java:1131)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.content.ContextWrapper.unregisterReceiver(ContextWrapper.java:361)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at org.mozilla.gecko.webapp.EventListener$2.onActivityResult(EventListener.java:234)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at org.mozilla.gecko.ActivityHandlerHelper.handleActivityResult(ActivityHandlerHelper.java:33)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at org.mozilla.gecko.GeckoApp.onActivityResult(GeckoApp.java:2324)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.app.Activity.dispatchActivityResult(Activity.java:4653)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.app.ActivityThread.deliverResults(ActivityThread.java:2988)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.app.ActivityThread.handleSendResult(ActivityThread.java:3035)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.app.ActivityThread.access$1100(ActivityThread.java:127)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1189)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.os.Handler.dispatchMessage(Handler.java:99)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.os.Looper.loop(Looper.java:137)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at android.app.ActivityThread.main(ActivityThread.java:4507)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at java.lang.reflect.Method.invokeNative(Native Method)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at java.lang.reflect.Method.invoke(Method.java:511)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:978)
02-19 19:48:48.556 E/GeckoWebappEventListener(18237): 	at com.android.internal.os.ZygoteInit.main(ZygoteIn

Comment 1

[krupa raj[:krupa]]

I'm not sure this is not an apk thing.

Comment 2

[Andy McKay]

Not sure either, ozten?

Comment 3

[Austin King [:ozten]]

For client pieces: I'm clueless, but jhugman, myk or mhaigh can help.

jhugman?

Comment 4

[Myk Melez [:myk] [@mykmelez]]

This is a client bug, so I'm moving it into the appropriate product/component.

Krupa: does the app still appear to install and run as expected?

Comment 5

[krupa raj[:krupa]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> This is a client bug, so I'm moving it into the appropriate
> product/component.
> 
> Krupa: does the app still appear to install and run as expected?

Install and launch works fine. I didn't try using the app.

Comment 6

[Aaron Train [:aaronmt]]

I'm unable to reproduce this with Nightly (03/09). I used the production release URL
https://controller.apk.firefox.com/application.apk (via manifest https://www.keepersecurity.com/vault/manifest.webapp)

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Hmm, this seems like a duplicate of bug 967211, which I did just reproduce on a tip build.

Comment 8

[krupa raj[:krupa]]

I hit this bug again a few seconds ago.

Comment 9

[krupa raj[:krupa]]

I keep hitting this bug quite often. I saw this issue again with the Nightly build from https://bugzilla.mozilla.org/show_bug.cgi?id=977704#c29 when installing the app Minimalist https://marketplace-dev.allizom.org/app/minimalist/

03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): error unregistering install receiver: 
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): java.lang.IllegalArgumentException: Receiver not registered: org.mozilla.gecko.webapp.InstallListener@42cf5438
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.app.LoadedApk.forgetReceiverDispatcher(LoadedApk.java:667)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.app.ContextImpl.unregisterReceiver(ContextImpl.java:1453)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.content.ContextWrapper.unregisterReceiver(ContextWrapper.java:489)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at org.mozilla.gecko.webapp.EventListener$2.onActivityResult(EventListener.java:234)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at org.mozilla.gecko.ActivityHandlerHelper.handleActivityResult(ActivityHandlerHelper.java:33)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at org.mozilla.gecko.GeckoApp.onActivityResult(GeckoApp.java:2396)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.app.Activity.dispatchActivityResult(Activity.java:5423)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.app.ActivityThread.deliverResults(ActivityThread.java:3361)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.app.ActivityThread.handleSendResult(ActivityThread.java:3408)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.app.ActivityThread.access$1300(ActivityThread.java:135)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1244)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.os.Handler.dispatchMessage(Handler.java:102)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.os.Looper.loop(Looper.java:136)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at android.app.ActivityThread.main(ActivityThread.java:5017)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at java.lang.reflect.Method.invokeNative(Native Method)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at java.lang.reflect.Method.invoke(Method.java:515)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:779)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:595)
03-20 19:07:53.722 E/GeckoWebappEventListener( 4652): 	at dalvik.system.NativeStart.main(Native Method)
03-20 19:07:53.722 W/GeckoBatteryManager(

I also noticed that the button status never changes to "Launch" even though the install was successful (even after reloading the page)

Comment 10

[Martyn Haigh (:mhaigh)]

Attachment: Prevent broadcast receiver from being unregistered twice

Comment 11

[Martyn Haigh (:mhaigh)]

https://tbpl.mozilla.org/?tree=Try&rev=7e5d170c9bb1

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8437683 [details] [diff] [review]
Prevent broadcast receiver from being unregistered twice

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

This is ok, but I would do it a bit differently.


Instead of calling Context.unregisterReceiver and InstallListener.cleanup in two different places—InstallListener.onReceive on success and EventListener.installApk on failure—I would call them unconditionally in EventListener.installApk, since its onActivityResult ActivityResultHandler gets called regardless.

Then I would still store hasBeenReceived in InstallListener, since we'll need it to determine if installation was successful.  But I would call it isReceived or simply "received", which are more common in Mozilla code.

Finally, I would move the InstallListener.cleanup code into installapk, since onActivityResult could have access to installApk's existing filePath String (if we made it final) or even its File object, so it wouldn't have to compute the filename like InstallListener.cleanup does (reproducing an algorithm that could get out-of-sync with its other occurrence).


Note that we can use ACTION_INSTALL_PACKAGE <http://developer.android.com/reference/android/content/Intent.html#ACTION_INSTALL_PACKAGE> with EXTRA_RETURN_RESULT <http://developer.android.com/reference/android/content/Intent.html#EXTRA_RETURN_RESULT> to get the actual result code of the install in onActivityResult on Android 4+.  But we still need a fallback for earlier versions of Android.

::: dom/apps/src/Webapps.jsm
@@ -2727,4 @@
>  
>      return Task.spawn((function*() {
>        yield this._ensureSufficientStorage(aNewApp);
> -

Nit: remove this extraneous change.

::: mobile/android/base/webapp/InstallListener.java
@@ +35,4 @@
>  
>      @Override
>      public void onReceive(Context context, Intent intent) {
> +        mReceived = true;

Assigning mReceived at the top of onReceived is fine if we can also limit reception to the specific package being installed.

Otherwise we have the problem that InstallListener receives intents for all packages being installed while it's registered.  So if multiple installs race each other (which can happen when we install multiple updates), then a listener can receive an intent for another package's installation.

Hence we have the code a bit later on that returns early in that case:

> String manifestUrl = apkResources.getManifestUrl();
> if (TextUtils.isEmpty(manifestUrl)) {
>     Log.i(LOGTAG, "No manifest URL present in metadata");
>     return;
> } else if (!isCorrectManifest(manifestUrl)) {
>     // This happens when the updater triggers installation of multiple
>     // APK updates simultaneously.  If we're the receiver for another
>     // update, then simply ignore this intent by returning early.
>     Log.i(LOGTAG, "Manifest URL is for a different install; ignoring");
>     return;
> }

So either we need to restrict the listener to the specific package, or mReceived needs to be assigned after that code.

Can we restrict the listener to the specific package?  We use an IntentFilter <http://developer.android.com/reference/android/content/IntentFilter.html> to register it to receive ACTION_PACKAGE_ADDED intents, and intent filters seem pretty powerful, so it seems likely that we can craft one specific enough.

And if we can, let's do it!  Then we can leave this statement here and also rip out the code quoted above.

@@ +86,5 @@
>      }
>  
> +    public boolean hasBeenReceived() {
> +        return mReceived;
> +    }

Nit: I would put this up near the mReceived declaration.

Comment 13

[Martyn Haigh (:mhaigh)]

Attachment: Prevent broadcast receiver from being unregistered twice

Unfortunately we can't specify a package name within an intent filter, so we have to either be listening, or not.  

There's a bit of ugly code which we can't work around because of limitations of Java.  We can't assign a final variable within a try-catch block, there are workarounds, and assigning a temp variable is the least ugly one.  Still not very nice though.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8438503 [details] [diff] [review]
Prevent broadcast receiver from being unregistered twice

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

(In reply to Martyn Haigh (:mhaigh) from comment #13)
> Created attachment 8438503 [details] [diff] [review]
> Prevent broadcast receiver from being unregistered twice
> 
> Unfortunately we can't specify a package name within an intent filter, so we
> have to either be listening, or not.  

Indeed, it looks like we could use IntentFilter.addDataSchemeSpecificPart to do this, but only from API level 19.  Perhaps add a comment explaining that, for the far distant day when we can drop support for older Androids?


> There's a bit of ugly code which we can't work around because of limitations
> of Java.  We can't assign a final variable within a try-catch block, there
> are workarounds, and assigning a temp variable is the least ugly one.  Still
> not very nice though.

We can declare it as a blank final and then assign it within the try/catch block:

        final String filePath;

        try {
            filePath = message.getString("filePath");

But we don't even need to do that, since filePath is just an intermediate variable that we use to initialize *file*, which is what we really want to access in the cleanup handler.  And *file* isn't assigned within a try/catch block, so we can simply make it final and then pass it to the cleanup handler:

        final File file = new File(filePath);
        …
        ActivityHandlerHelper.startIntentForActivity(context, intent, new ActivityResultHandler() {
            @Override
            public void onActivityResult(int resultCode, Intent data) {
            …
                try {
                    …
                    cleanup(file);
    …
    public static void cleanup(File apkFile) {
        if (apkFile.exists()) {
            apkFile.delete();
            Log.i(LOGTAG, "Downloaded APK file deleted");
        }
    }

And that makes the cleanup handler so simple that we might as well inline its code into onActivityResult:

        final File file = new File(filePath);
        …
        ActivityHandlerHelper.startIntentForActivity(context, intent, new ActivityResultHandler() {
            @Override
            public void onActivityResult(int resultCode, Intent data) {
            …
                if (file.exists()) {
                    file.delete();
                }

(We still have to check that the file exists, because it might have been deleted in the interim!)

::: mobile/android/base/webapp/EventListener.java
@@ +178,5 @@
>              public void onActivityResult(int resultCode, Intent data) {
>                  // The InstallListener will catch the case where the user pressed install.
>                  // Now deal with if the user pressed cancel.
> +                // The DONE button also returns a resultCode of 0, so we have to check to see if
> +                // the receiver has already been activated.

These comments are no longer accurate, since here we handle both success and failure.

@@ +179,5 @@
>                  // The InstallListener will catch the case where the user pressed install.
>                  // Now deal with if the user pressed cancel.
> +                // The DONE button also returns a resultCode of 0, so we have to check to see if
> +                // the receiver has already been activated.
> +                if (resultCode == Activity.RESULT_CANCELED && !receiver.isReceived()) {

resultCode always equals Activity.RESULT_CANCELED here, but equating them is misleading, since they just happen to match.  Thus we should only check !receiver.isReceived() in this conditional expression.

@@ +185,5 @@
>                  }
> +                try {
> +                    context.unregisterReceiver(receiver);
> +                    cleanup(manifestUrl);
> +                } catch (java.lang.IllegalArgumentException e) {

Now that this is the only place where we unregister the receiver, it should no longer raise an exception, so we shouldn't have to catch it.  (I added the try/catch block back when I didn't understand this code well enough!  Otherwise I would have consolidated unregistration back then instead of catching the exception.)

::: mobile/android/base/webapp/InstallListener.java
@@ +39,3 @@
>      @Override
>      public void onReceive(Context context, Intent intent) {
> +

Nit: remove extraneous line of whitespace.

Comment 15

[Martyn Haigh (:mhaigh)]

Attachment: Prevent broadcast receiver from being unregistered twice

I've kept in the unregisterReceiver call in the InstallListener as this is the only path a successful install (user presses buttons 'Install' then 'Open') will follow, and have adjusted the flow within the onActivityResult in the EventListener appropriately. 

https://tbpl.mozilla.org/?tree=Try&rev=ceea311e3658

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8439907 [details] [diff] [review]
Prevent broadcast receiver from being unregistered twice

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

(In reply to Martyn Haigh (:mhaigh) from comment #15)
> I've kept in the unregisterReceiver call in the InstallListener as this is
> the only path a successful install (user presses buttons 'Install' then
> 'Open') will follow, and have adjusted the flow within the onActivityResult
> in the EventListener appropriately. 

Nice catch!  onActivityResult might still get called, if the user subsequently returns to Fennec.  But if Fennec gets killed in the meantime, then it won't.

Of course in that case it doesn't matter whether we unregister the receiver, but it's still better to do so once InstallListener has received the message, which ensures it happens as soon as it's possible.

::: mobile/android/base/webapp/EventListener.java
@@ +200,1 @@
>                  }

Ooh, I like this way of deleting the file even better, as it's simpler while functionally equivalent!  (It might also be marginally more performant, although that doesn't matter in this case.)

Erm, but if this callback might not get called, then we should try deleting the file in InstallReceiver after all!

That doesn't mean reintroducing cleanup(), though.  Instead, define *file* before *installReceiver* here in installApk(), then pass *file* to the InstallListener constructor as a new third argument that InstallListener stores in a member variable, so it can use that File object to try to delete the file after receiving the intent (which it too can do by unconditionally calling File.delete).

(Also: return before defining *installReceiver* if the file doesn't exist, which will solve another problem: currently, we do that after registering the receiver with the context; so if the file doesn't exist, then presumably we end up leaking the receiver.)

Comment 17

[Martyn Haigh (:mhaigh)]

Attachment: Prevent broadcast receiver from being unregistered twice

Added file deletion on happy path and moved some code around as per review.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8442352 [details] [diff] [review]
Prevent broadcast receiver from being unregistered twice

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

::: mobile/android/base/webapp/EventListener.java
@@ +190,5 @@
>          ActivityHandlerHelper.startIntentForActivity(context, intent, new ActivityResultHandler() {
>              @Override
>              public void onActivityResult(int resultCode, Intent data) {
> +                // Invoked if the user cancels installation or presses the 'Done'
> +                // button once the app has been successfully installed.

Nit: it's also invoked if the user presses the Open button to launch the app and then returns to Fennec.  Basically, it always happens when the user returns to Fennec, unless Fennec has been killed in the meantime.

Also, it might be better to put this comment on the onActivityResult declaration on the line above.  Here, it looks like it comments on the !receiver.isReceived() conditional, which isn't the case.

@@ +196,4 @@
>                      callback.sendError("APK installation cancelled by user");
> +                    context.unregisterReceiver(receiver);
> +                }
> +                if (file.delete()) {

Presumably we would only need to do this if !receiver.isReceived(), although it doesn't hurt to do it unconditionally.

Comment 19

[Martyn Haigh (:mhaigh)]

Attachment: Prevent broadcast receiver from being unregistered twice

Myk: I can't replicate this code being called when the user presses the 'Open' button and then returns to Fennec.  In my testing Fennec didn't appear to have been killed.  

https://tbpl.mozilla.org/?tree=Try&rev=cfef0daa96d4

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8444616 [details] [diff] [review]
Prevent broadcast receiver from being unregistered twice

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

(In reply to Martyn Haigh (:mhaigh) from comment #19)
> Myk: I can't replicate this code being called when the user presses the
> 'Open' button and then returns to Fennec.  In my testing Fennec didn't
> appear to have been killed.  

Hmm, I wonder why this varies between our devices.  In any case, this code should work correctly either way.  It's worth adding a note that onActivityResult may also be called when the user presses Open and then returns to Fennec.

Comment 21

[Martyn Haigh (:mhaigh)]

Attachment: Prevent broadcast receiver from being unregistered twice

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c44f8dcd20df