785960 - "Don't keep activities" developer option gets counted as OOM in telemetry

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Bug 779687 added telemetry to track OOM kills. However this code also gets run whenever the user dismisses the awesomebar when they have the "don't keep activities" developer option set. It would be good to detect when this option is set and not count that as an OOM kill.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Don't count OOM when restoring activities

As far as I can tell this avoids the bug and still triggers in all the real OOM cases (in those Gecko also gets killed).

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Rework telemetry name, add for "dont keep activities"

We're adding FENNEC_ to all our new telemetry, so now that we likely will start analysis on this one afresh, use the opportunity to rename it.

Add a flag to see if the user is running with "Dont keep activities" enabled. Might come in useful for performance analysis.

Comment 3

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

Comment on attachment 669175 [details] [diff] [review]
Patch 1. Don't count OOM when restoring activities

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

LGTM

Comment 4

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

Comment on attachment 669176 [details] [diff] [review]
Patch 2. Rework telemetry name, add for "dont keep activities"

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

To me the name "FENNEC_WAS_KILLED" implies that the histogram will count all instances of fennec being killed, including e.g. if somebody runs "kill" from adb shell or swipes away the app from the JB app switcher. However the histogram doesn't actually include those instances. I would prefer if this were renamed to "FENNEC_OOM_KILLED" since that's really what we're measuring here, AFAICT.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

>To me the name "FENNEC_WAS_KILLED" implies that the histogram will count all 
>instances of fennec being killed, including e.g. if somebody runs "kill" from adb 
>shell or swipes away the app from the JB app switcher. However the histogram 
>doesn't actually include those instances.

It *will* count a "kill" from the adb shell or a kill from the Task Manager if the application was in the foreground when it happened. That's why I renamed it - there are a few cases that are indistinguishable from an OOM kill.

The most descriptive name would probably be FENNEC_WAS_KILLED_IN_FOREGROUND.

Comment 6

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

Comment on attachment 669176 [details] [diff] [review]
Patch 2. Rework telemetry name, add for "dont keep activities"

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

Ah hmm. However it will also count a kill from an OutOfMemoryError thrown while it is in the background, right? The uncaught exception handler sets the GeckoApp.PREFS_OOM_EXCEPTION shared pref which will increment the histogram on the next run.

Maybe FENNEC_WAS_KILLED is good enough.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

>However it will also count a kill from an OutOfMemoryError thrown while it is in 
>the background, right?

It's going to be harder to OOM in Java code while the activity is suspended :-)

Comment 8

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0b4572696b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c80a02b9d478

Comment 9

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

(In reply to Gian-Carlo Pascutto (:gcp) from comment #7)
> It's going to be harder to OOM in Java code while the activity is suspended
> :-)

Hard, but not impossible. :) Gecko could still be running stuff and sending messages to Java while the activity is suspended.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0b4572696b6
https://hg.mozilla.org/mozilla-central/rev/c80a02b9d478