761706 - Better fix for: java.lang.IllegalArgumentException (Receiver not registered)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

+++ This bug was initially created as a clone of Bug #744850 +++

Bug 744850 is currently used to find any way to stop the bleeding/crashing. We need to look for other ways to fix the bug in a more correct manner.

Comment 1

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

Is this showing up in crash stats?

Comment 2

[Scoobidiver (away)]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Is this showing up in crash stats?
No because it's fixed by bug 744850, but bug 760392 has a close stack trace.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

This problem is trivial to reproduce if you turn on the "don't keep activities" checkbox. The problem occurs because we do the registration/unregistration in onApplicationPause and onApplicationResume, which are independent of the GeckoApp activity's lifecycle. Specifically, you can open the awesomebar and android could kill GeckoApp without ever unregistering the connectivity listener.

There's a whole mess of bad code here. Patches coming.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/3) Bind GeckoNetworkManager to the global application context

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/3) Bind GeckoConnectivityReceiver to the global application context

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/3) Simplify background detection

This is the big scary one, but I'm pretty sure this is the right thing to be doing. The problem with the existing method is that gecko activities that have been paused by other gecko activities can be destroyed by android, and when the top activity is popped the old one is re-started from scratch. The hasStarted boolean is false at that point and screws things up.

Comment 7

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

Should we be doing the same thing for the GeckoBatteryManager? GeckoScreenOrientationListener seems similar too, but I'm not sure if it needs to be associated with an Activity or not.

Lastly, we seem to use slightly different init/start/stop APIs for each of these managers/listeners. Should we file a followup to unify them a bit?

Comment 8

[Sriram Ramasubramanian [:sriram]]

(In reply to Kartikaya Gupta (:kats) from comment #6)
> Created attachment 660085 [details] [diff] [review]
> (3/3) Simplify background detection
> 
> This is the big scary one, but I'm pretty sure this is the right thing to be
> doing. The problem with the existing method is that gecko activities that
> have been paused by other gecko activities can be destroyed by android, and
> when the top activity is popped the old one is re-started from scratch. The
> hasStarted boolean is false at that point and screws things up.

How do you detect if the new activity opened is owned by us, or some other activity via Share?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Sriram Ramasubramanian [:sriram] from comment #8)
> How do you detect if the new activity opened is owned by us, or some other
> activity via Share?

If the new activity is owned by us, it will extend from GeckoActivity, and so will call GeckoApplication.onActivityResume. If the new activity is not owned by us, then it won't extend from GeckoActivity, and GeckoApplication will not know about it. It will just know that the last activity (which *was* owned by us) got paused, so we are effectively in the background.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/6) Fix indenting

Whitespace changes only (and added one set of braces around a single-line if). I'll attach an ignore-ws patch as well.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/6) Fix indenting (patch with ignoring whitespace)

Oh I also added LOGTAG to GeckoNetworkManager

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/6) Bind GeckoNetworkManager to the global application context

Rebased the old patch on top of the whitespace fixing; carrying r+

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/6) Bind GeckoConnectivityReceiver to the global application context

Rebased the old patch on top of the whitespace changes, and renamed some of methods/staticness so that the interface to use GeckoConnectivityReceiver is the same as the other classes. Carrying r+ since the bulk of the change is the same as the last patch.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: (4/6) Bind GeckoBatteryManager to the global application context

Comment 15

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

Comment on attachment 660965 [details] [diff] [review]
(1/6) Fix indenting

Thanks for the ignore-ws patch

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: (5/6) Move application-level receivers from GeckoApp to GeckoApplication

Move the broadcast receivers into GeckoApplication, and get rid of the hacky GeckoActivity code to detect if one of our activities is on the top of the stack. Note that I keep the screen orientation listener in GeckoApp because it does seem to be an activity-specific thing and not an application-wide thing.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: (6/6) Make public function private

Inner classes in Java can access private functions of the outer class just fine.

Comment 18

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

Comment on attachment 660975 [details] [diff] [review]
(5/6) Move application-level receivers from GeckoApp to GeckoApplication

Let's just keep an eye open for any pause/resume regressions

Comment 19

[Mounir Lamouri (:mounir)]

Comment on attachment 660978 [details] [diff] [review]
(6/6) Make public function private

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

r=me, assuming this is working.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=acce06249686 showed no new errors, so pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2e7dc4182f
https://hg.mozilla.org/integration/mozilla-inbound/rev/338823aa94a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/29466c92fe44
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5e2949c8f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/967eec801160
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec6eab545501

This is marked as tracking 17+ but I don't really think it needs uplifting to 17 since there's no user-visible impact. I'd rather just let it ride the trains.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (:kats) from comment #20)
> This is marked as tracking 17+ but I don't really think it needs uplifting
> to 17 since there's no user-visible impact.

By this I mean no crashing. There is probably some user-visible behaviour with edge cases and such, but the exceptions get caught. So at worst Gecko wouldn't be getting some battery/network/screen change notifications.

Comment 22

[Scoobidiver (away)]

(In reply to Kartikaya Gupta (:kats) from comment #21) 
> By this I mean no crashing.
Bug 760392 is #22 top crasher in 15.0.1.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

I stand corrected. We should keep an eye on those crash reports and see if they still happen on 18 with this set of patches landed.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf2e7dc4182f
https://hg.mozilla.org/mozilla-central/rev/338823aa94a6
https://hg.mozilla.org/mozilla-central/rev/29466c92fe44
https://hg.mozilla.org/mozilla-central/rev/0a5e2949c8f3
https://hg.mozilla.org/mozilla-central/rev/967eec801160
https://hg.mozilla.org/mozilla-central/rev/ec6eab545501

Comment 25

[Scoobidiver (away)]

There's one crash in 18.0a1/20120921: bp-3b7dc71f-1bb5-47bf-b318-06b852120921.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this safe to uplift to Beta?  We're going to build on 17 Beta 4 tomorrow and would consider uplift here to help with stability if this is known to be low risk.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

I wouldn't consider these patches to be low-risk. There's a lot of changes that have the potential to interact badly with code on 17 and I wouldn't feel comfortable uplifting it this late.

Comment 28

[Kevin Brosnan [Ex-Mozilla]]

Verified fixed by crash stats in Firefox 18.