Closed Bug 761706 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect, critical)

ARM
Android
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 18
Tracking Status
firefox14 --- affected
firefox15 --- affected
firefox16 --- affected
firefox17 --- affected
firefox18 --- fixed
blocking-fennec1.0 --- -
fennec 17+ ---

People

(Reporter: mfinkle, Assigned: kats)

References

Details

(Keywords: crash)

Crash Data

Attachments

(7 files, 3 obsolete files)

46.02 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
4.43 KB, text/plain
Details
4.98 KB, patch
kats
: review+
Details | Diff | Splinter Review
7.32 KB, patch
kats
: review+
Details | Diff | Splinter Review
5.92 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
12.31 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
1.49 KB, patch
mounir
: review+
Details | Diff | Splinter Review
+++ 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.
tracking-fennec: --- → 16+
blocking-fennec1.0: ? → -
Is this showing up in crash stats?
(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.
Assignee: nobody → sriram
tracking-fennec: 16+ → 17+
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.
Assignee: sriram → bugmail.mozilla
Blocks: 760392
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.
Attachment #660085 - Flags: feedback?(mark.finkle)
Attachment #660082 - Flags: review?(mark.finkle) → review+
Attachment #660083 - Flags: review?(mark.finkle) → review+
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?
(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?
(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.
Whitespace changes only (and added one set of braces around a single-line if). I'll attach an ignore-ws patch as well.
Attachment #660965 - Flags: review?(mark.finkle)
Oh I also added LOGTAG to GeckoNetworkManager
Rebased the old patch on top of the whitespace fixing; carrying r+
Attachment #660082 - Attachment is obsolete: true
Attachment #660969 - Flags: review+
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.
Attachment #660083 - Attachment is obsolete: true
Attachment #660970 - Flags: review+
Comment on attachment 660965 [details] [diff] [review]
(1/6) Fix indenting

Thanks for the ignore-ws patch
Attachment #660965 - Flags: review?(mark.finkle) → review+
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.
Attachment #660085 - Attachment is obsolete: true
Attachment #660085 - Flags: feedback?(mark.finkle)
Attachment #660975 - Flags: review?(mark.finkle)
Inner classes in Java can access private functions of the outer class just fine.
Attachment #660978 - Flags: review?(mounir)
Attachment #660971 - Flags: review?(mark.finkle) → review+
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
Attachment #660975 - Flags: review?(mark.finkle) → review+
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.
Attachment #660978 - Flags: review?(mounir) → review+
(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.
(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.
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.
There's one crash in 18.0a1/20120921: bp-3b7dc71f-1bb5-47bf-b318-06b852120921.
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.
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.
Verified fixed by crash stats in Firefox 18.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.