1501648 - Crash in java.lang.IllegalArgumentException: at android.app.LoadedApk.forgetReceiverDispatcher(LoadedApk.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-47a747a0-86da-440e-b278-4080d0181024.
=============================================================

Seen while looking at 63 crash stats for Fennec: https://bit.ly/2O0wffD and currently at #21 overall. This signature was present in 62.0.3, with about 500 crashes. As we progressed through 63 betas this signature increased. Comments mention crashing when "clicking through tabs" and when reading a news article. 

Full Java stack trace:

java.lang.IllegalArgumentException: Receiver not registered: org.mozilla.gecko.mma.PackageAddedReceiver@7e5db88
	at android.app.LoadedApk.forgetReceiverDispatcher(LoadedApk.java:1007)
	at android.app.ContextImpl.unregisterReceiver(ContextImpl.java:1330)
	at android.content.ContextWrapper.unregisterReceiver(ContextWrapper.java:608)
	at org.mozilla.gecko.mma.MmaDelegate.unregisterInstalledPackagesReceiver(MmaDelegate.java:301)
	at org.mozilla.gecko.mma.MmaDelegate.flushResources(MmaDelegate.java:124)
	at org.mozilla.gecko.BrowserApp.onDestroy(BrowserApp.java:1550)
	at android.app.Activity.performDestroy(Activity.java:6884)
	at android.app.Instrumentation.callActivityOnDestroy(Instrumentation.java:1153)
	at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:4191)
	at android.app.ActivityThread.handleDestroyActivity(ActivityThread.java:4222)
	at android.app.ActivityThread.-wrap6(ActivityThread.java)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1539)
	at android.os.Handler.dispatchMessage(Handler.java:102)
	at android.os.Looper.loop(Looper.java:154)
	at android.app.ActivityThread.main(ActivityThread.java:6121)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:889)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:779)

Comment 1

[David Durst [:ddurst]]

This appears to be new in 63, so it's probably related to something that changed for 63 (which should be a small set)?

Comment 2

[:sdaswani]

#21 sounds smaller so putting it on our medium list.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

The exceptions are because of trying to unregister a broadcast receiver which at that time is not registered.
Although the low crash rate doesn't indicate an inherent problem there are two receivers which appear in the majority of this reports:

HeadSetStateReceiver
https://crash-stats.mozilla.com/report/index/8d052c1c-038f-4c61-9685-cbc270181101
https://crash-stats.mozilla.com/report/index/75d05499-46bd-4cc7-831e-30f1d0181103

And PackageAddedReceiver
https://crash-stats.mozilla.com/report/index/f62bcb75-1071-4725-a8c2-504450181105
https://crash-stats.mozilla.com/report/index/613fa924-c189-4ca0-84f3-2dd760181106

For older versions of Fennec (v. ~30) there are also reports regarding the GeckoNetworkManager
https://crash-stats.mozilla.com/report/index/68f8c2f2-3215-4690-a9c1-399470181103
https://crash-stats.mozilla.com/report/index/90c0d04f-750f-43bd-bba7-fd9f80181102
but they don't appear anymore in the recent app versions.

There is no way to check if a receiver is registered before trying to unregister it so the simple solution to avoid this type of crashes would be to just wrap those `unregisterReceiver(..)` calls in a try-catch block.
But being that the reports refer mostly to two receivers there may be some hidden issue which needs to be investigated.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1501648 - Prevent crashes for unregistered receivers from MmaDelegate and GeckoMediaControlAgent; r?JanH

Although this two receivers are guarded by checks for if they were initialized
(and so registered) there are reports of crashes because of trying to unregister
them without them actually being registered.

While not an ideal solution, wrapping the unregister operations in a try-catch
saves the users from a crash and because the unregister is done when the app
is closed (for the MmaDelegate receiver) or when the app finished playing media
(for the GeckoMediaControlAgent receiver) the user doesn't loose any functionality.

Comment 5

[Jan Henning [:JanH]]

(In reply to Petru-Mugurel Lingurar[:petru] from comment #3)
> There is no way to check if a receiver is registered before trying to
> unregister it so the simple solution to avoid this type of crashes would be
> to just wrap those `unregisterReceiver(..)` calls in a try-catch block.
> But being that the reports refer mostly to two receivers there may be some
> hidden issue which needs to be investigated.

Not "may", there *is* a lifecycle issue somewhere in our implementation, and those crashes happen intentionally so you as a developers notice and then fix them [1]. Just ignoring them definitively feels wrong, although it may be acceptable as a short-term fix for our users. Maybe we should have ignored the exception only on Beta/Release...


[1] Our event dispatcher implementation has diagnostic crashes on Nightly for the same reason if you commit the same kind of errors by registering twice, or unregistering without having previously registered something, etc.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

(In reply to Jan Henning [:JanH] from comment #5)
> (In reply to Petru-Mugurel Lingurar[:petru] from comment #3)
> > There is no way to check if a receiver is registered before trying to
> > unregister it so the simple solution to avoid this type of crashes would be
> > to just wrap those `unregisterReceiver(..)` calls in a try-catch block.
> > But being that the reports refer mostly to two receivers there may be some
> > hidden issue which needs to be investigated.
> 
> Not "may", there *is* a lifecycle issue somewhere in our implementation, and
> those crashes happen intentionally so you as a developers notice and then
> fix them [1]. Just ignoring them definitively feels wrong, although it may
> be acceptable as a short-term fix for our users. Maybe we should have
> ignored the exception only on Beta/Release...
> 
> 
> [1] Our event dispatcher implementation has diagnostic crashes on Nightly
> for the same reason if you commit the same kind of errors by registering
> twice, or unregistering without having previously registered something, etc.

Thanks Jan!
I've created bug 1505685 to continue investigating the issue.
Indeed I went with wrapping those `unregisterReceiver()` in a try-catch as a last-resort solution for stopping the crashes for the majority of users - release and beta channels.
In manual testing, with logs I wan't able to reproduce it. And in the code, before actually calling `unregisterReceiver()` there are checks to make sure the `registerReceiver()` code block happened. That is why, without finding anything wrong and because this are terminal events which doesn't affect the functionality I went with the try-catch.

Comment 7

[Petru-Mugurel Lingurar [:petru]]

Ryan, the patch for this ticket stops the crashes without introducing any side-effects for the users but does not actually fix the underlying issue. Will continue to investigate it.

If you think this would need an uplift to beta/release now, let me know.

Comment 8

[Jan Henning [:JanH]]

@Petru: So we don't have to continually re-uplift in case the investigation takes longer than expected, you could simply make the catch conditional on "BuildConfig.RELEASE_OR_BETA" - it it's true, just swallow the exception (maybe log it, though), if it's false (i.e. Nightly) rethrow it.

Comment 9

[Eliza Balazs [:ebalazs_]]

Petru: Hi, Ehen trying to land I get the following warnings:
This revision is not associated with a repository. In order to land, a revision must be associated with a repository on Phabricator. 
This diff does not have the proper author information uploaded to Phabricator. This can happen if the diff was created using the web UI, or a non standard client. The author should re-upload the diff to Phabricator using the "arc diff" command. 

Please update the patch. Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f8d41b9bb67
Prevent crashes for unregistered receivers from MmaDelegate and GeckoMediaControlAgent; r=jchen,JanH

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment 7 is better directed at Julien since 64 is his release.

Comment 13

[Julien Cristau [:jcristau]]

Probably good to uplift to at least 64 to stop the bleeding while we continue to investigate.  Thanks Petru.

Comment 14

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9023293 [details]
Bug 1501648 - Prevent crashes for unregistered receivers from MmaDelegate and GeckoMediaControlAgent; r?JanH

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: App crashes

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: No STR

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Not risky because it just suppresses an error which would otherwise crash the app.

String changes made/needed:

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9023293 [details]
Bug 1501648 - Prevent crashes for unregistered receivers from MmaDelegate and GeckoMediaControlAgent; r?JanH

paper over crashes, approved for 64.0b11

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e437c76c270