794642 - New fennec UpdateService service creates unnecessary components

Status: RESOLVED FIXED

(Firefox for Android :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Attachment: /data/anr/traces from UpdateService getting killed

Right now the UpdateService service runs in a separate process from Fennec. This means that it instantiates its own copy of GeckoApplication, including all of the things that GeckoApplication instantiates (i.e. GeckoConnectivityReceiver, GeckoBatteryManager, GeckoNetworkManager, and MemoryMonitor). It might also instantiate other things that I'm not aware of.

This seems unnecessary, and in fact can cause the UpdateService to get killed by ANR in the following way:
1) Low memory notification fires
2) MemoryMonitor catches it and tries to send a low-memory event to gecko
3) MemoryMonitor calls GeckoAppShell.geckoEventSync() to wait for the response from gecko
4) No response is coming (since gecko isn't actually running in this process) and so android kills the UpdateService for getting locked up.

I'm attaching an /data/anr/traces.txt where I saw this happen. Note that the "Cmd line" at the top is that of the UpdateService, but the first stack trace clearly shows the MemoryMonitor blocked on gecko.

Comment 1

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Attachment: Add missing else

Also while I was poking around in UpdateService I noticed a (benign) missing else. See attached.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Avoid unnecessary GeckoApplication initialization in services

Comment 3

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Is the mInited guard needed? According to the pretty picture at http://developer.android.com/reference/android/app/Activity.html#ActivityLifecycle the onCreate should ever be called once.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Kartikaya Gupta (:kats) (away sep28-oct08) from comment #3)
> Is the mInited guard needed? According to the pretty picture at
> http://developer.android.com/reference/android/app/Activity.
> html#ActivityLifecycle the onCreate should ever be called once.

Within that Activity instance this is true. But it may be possible that another instance of the activity could be created within the same Application lifetime, I believe.

Comment 5

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Comment on attachment 665440 [details] [diff] [review]
Avoid unnecessary GeckoApplication initialization in services

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

If we end up with two instances of GeckoApp I think we'll have much bigger problems. But I guess this won't hurt.

::: mobile/android/base/GeckoApplication.java
@@ +28,5 @@
>          GeckoNetworkManager.getInstance().init(getApplicationContext());
>          MemoryMonitor.getInstance().init(getApplicationContext());
> +        mInited = true;
> +
> +        android.util.Log.i("SNORP", "Initialized GeckoApplication");

Take out log line

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71e1ca0f2da3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71e1ca0f2da3

Comment 8

[Gian-Carlo Pascutto [:gcp]]

This code is sitting *before* GeckoAppShell.registerGlobalExceptionHandler();

Doesn't this mean that if it is buggy, it will never show up in crash reports?

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #8)
> This code is sitting *before* GeckoAppShell.registerGlobalExceptionHandler();
> 
> Doesn't this mean that if it is buggy, it will never show up in crash
> reports?

Probably so. I was just trying to maintain the current flow, I guess.

Comment 10

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #8)
> This code is sitting *before* GeckoAppShell.registerGlobalExceptionHandler();
> 
> Doesn't this mean that if it is buggy, it will never show up in crash
> reports?

Filed bug 799472 for this.