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

RESOLVED FIXED in Firefox 64

Status

()

defect
P1
critical
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: marcia, Assigned: petru)

Tracking

({crash, regression})

Firefox 63
Firefox 65
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 fixed, firefox65 fixed)

Details

(Whiteboard: [priority:medium], crash signature)

Attachments

(1 attachment)

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)
This appears to be new in 63, so it's probably related to something that changed for 63 (which should be a small set)?
Flags: needinfo?(sdaswani)

Comment 2

7 months ago
#21 sounds smaller so putting it on our medium list.
Flags: needinfo?(sdaswani)
Whiteboard: [priority:medium]

Updated

7 months ago
Priority: -- → P1
Assignee

Updated

7 months ago
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Assignee

Comment 3

7 months ago
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.
Assignee

Comment 4

7 months ago
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.
Assignee

Updated

7 months ago
Keywords: checkin-needed
(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.
Assignee

Updated

7 months ago
Keywords: checkin-needed
Assignee

Comment 6

7 months ago
(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.
Assignee

Comment 7

7 months ago
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.
Flags: needinfo?(ryanvm)
Assignee

Updated

7 months ago
Depends on: 1505685
@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.
Attachment #9023293 - Attachment description: Bug 1501648 - Prevent crashes for unregistered receivers from MmaDelegate and GeckoMediaControlAgent; r?sdaswani → Bug 1501648 - Prevent crashes for unregistered receivers from MmaDelegate and GeckoMediaControlAgent; r?JanH
Assignee

Updated

7 months ago
Keywords: checkin-needed
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!
Flags: needinfo?(petru.lingurar)
Assignee

Updated

7 months ago
Flags: needinfo?(petru.lingurar)
Keywords: checkin-needed

Comment 10

7 months ago
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f8d41b9bb67
Prevent crashes for unregistered receivers from MmaDelegate and GeckoMediaControlAgent; r=jchen,JanH
Keywords: checkin-needed

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f8d41b9bb67
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Comment 7 is better directed at Julien since 64 is his release.
Flags: needinfo?(ryanvm) → needinfo?(jcristau)
Probably good to uplift to at least 64 to stop the bleeding while we continue to investigate.  Thanks Petru.
Flags: needinfo?(jcristau)
Assignee

Comment 14

6 months ago
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:
Attachment #9023293 - Flags: approval-mozilla-beta?
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
Attachment #9023293 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.