994273 - Home panel sessions not being properly closed

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Sola Ogunsakin [:sola]]

Home panel sessions are not being properly closed. Currently homepanel sessions stay open if the user closes or minimizes the app or goes into settings.

Comment 1

[Sola Ogunsakin [:sola]]

Attachment: bug-994273.patch

Comment 2

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

Comment on attachment 8404168 [details] [diff] [review]
bug-994273.patch

I guess the Viewpager.onPause/onResume does not get called automatically by Android?

Pinging Lucas for feedback too.

Comment 3

[Sola Ogunsakin [:sola]]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 8404168 [details] [diff] [review]
> bug-994273.patch
> 
> I guess the Viewpager.onPause/onResume does not get called automatically by
> Android?

Yeah, no they're not from ViewPager. I added them to allow HomePager to piggyback off Android's onPause/onResume calls to BrowserApp.

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 8404168 [details] [diff] [review]
bug-994273.patch

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

This seems to speak to a bigger problem than HomePager sessions - don't all sessions need to deal with this? I'd like to see an approach that centralizes all of this onPause stuff, instead of requiring every single probe/session to explicitly pause/restart itself.

Can you think of a different approach that's more general than this?

Clearing lucasr because I think he'd have similar opinions.

Comment 5

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

Some food for thought:
* Session serve two purposes:
  1. Track the duration of a "session"
  2. Associate session context with an event

#1 is the main concern here, I think.
#2 should work fine, even without the onPause/onResume patch

* Newman mentioned that FHR has more robust handling of sessions. Maybe we could reuse some ideas there... as long as it doesn't lead down a rabbit hole of complexity and pain.

Comment 6

[Richard Newman [:rnewman]]

Note that there are two or three kinds of sessions:

* Ones that explicitly last until ended. For example, if we're "downloading", and we share a page, our activity will be paused while the share handler runs. We don't want to end the "downloading" session until we're done with the download.

* Ones that end when the activity pauses, and do not resume when the activity resumes.

* Ones that implicitly end when the activity pauses, but resume when the activity resumes. That's probably a function of the activity itself -- perhaps the activity starts the right sessions in onResume, and so only ending the sessions is necessary.


This is both an engineering and an ontological question. You need to know what kind of sessions we're creating before you can decide what happens.

I could imagine us having a "close these when you pause" set hanging off the topmost activity, or adopting that paradigm in general.

Comment 7

[Sola Ogunsakin [:sola]]

Attachment: Alternate Implementation

Alternative Implementation:
I think a good approach might be to let the Telemetry API manage the currently active sessions, stopping the sessions when the app is minimized (reason=APP_MINIMIZED) and restarting them when the app is restored. 

Implementation:
Looking through the code, looks like we had already implemented a reliable way to detect if the app is currently in the background. I did some testing and GeckoApplication#isApplicationInBackground seems to always reflect the correct state of the app. I use this, together with ActivityLifecycleCallbacks, to stop active sessions when the app is backgrounded and restart them when the app is restored.

Pros:
* Easier to maintain probes. When writing session probes we don't have to explicitly handle the case where the app is minimized.

Cons:
* ActivityLifecycleCallbacks is not available for pre-ICS devices :(

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8405685 [details] [diff] [review]
Alternate Implementation

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

I think this is failing to address some intrinsic complexity here, as I mentioned in Comment 6. You haven't given yourself a way out for modeling sessions that don't end only when we have no frontmost activities.

This patch doesn't try to provide something applicable to other contexts, as Chenxia alluded to in Comment 4, and it's probably wrong for some common scenarios (what happens if the browser activity is paused and finished when the user enters settings? AFAICS from this patch, open sessions aren't closed unless all activities are closed!).

Here's something to consider. Activities establish sessions, either when they begin or during use. Activities are paused and finished. Some sessions are tied to that lifecycle. Some are not, or have a more complex lifecycle -- for example, persisting across the launch of and return from nested activities.

A pattern you're perhaps looking for here, then, is scoping: when you establish a session that needs to end no later than activity pause, put it into a scope, and finalize the scope (and all of its outstanding sessions) in onPause. This need only be one line of code. If you need a session to last even when you open a nested activity, we have a place for that hook, too. And if you need a session to truly last for the duration of the Application (which we probably don't), then the Application can handle both ends.

::: mobile/android/base/Telemetry.java
@@ +119,5 @@
>          Log.d(LOGTAG, "StartUISession: " + sessionName);
>          GeckoEvent event = GeckoEvent.createTelemetryUISessionStartEvent(sessionName, realtime());
>          GeckoAppShell.sendEventToGecko(event);
> +        if (updateActiveSessions) {
> +            activeSessions.add(sessionName);

I think what you want is to have two sets of concepts here: start/stop and pause/resume. Boolean arguments are a red flag.

@@ +164,5 @@
>          sendUIEvent(action, null, realtime(), null);
>      }
> +
> +    public static void endActiveUITelemetrySessions() {
> +        for(String sessionName: activeSessions) {

Nit: spaces after for (here and below).

Comment 9

[Sola Ogunsakin [:sola]]

(In reply to Richard Newman [:rnewman] from comment #8)
Spoke with Chenxia and mfinkle today. They think we should go ahead with the use of ActivityLifecycleCallbacks. I see your point about tying sessions to activities such that they are closed when the activity is paused/closed. This will be helpful for sessions with less well defined exit paths (e.g urlbar, homepanel.1). I'll update the original patch to introduce the concept of managed and unmanaged sessions. When started, managed sessions are bound to the top-most activity so if that activity is paused/closed, all managed sessions started while it was in the foreground will be closed. Managed sessions are also restarted when the app is restored from being in the background. Unmanaged sessions work just like the ones we have now. I think we should be able to do this with the ActivityLifecycleCallbacks. 

I think this addresses the 3 types of sessions from comment#6:

* Ones that explicitly last until ended. For example, if we're "downloading", and we share a page, our activity will be paused while the share handler runs. We don't want to end the "downloading" session until we're done with the download.

This can be an unmanaged session.

* Ones that end when the activity pauses, and do not resume when the activity resumes.

This can be an also unmanaged session.

* Ones that implicitly end when the activity pauses, but resume when the activity resumes. That's probably a function of the activity itself -- perhaps the activity starts the right sessions in onResume, and so only ending the sessions is necessary.

This can be a managed session.

Comment 10

[Sola Ogunsakin [:sola]]

(In reply to Sola Ogunsakin [:sola] from comment #9)
> (In reply to Richard Newman [:rnewman] from comment #8)
> Spoke with Chenxia and mfinkle today.
oops yesterday

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8405685 [details] [diff] [review]
Alternate Implementation

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

You'll need to add unmanaged sessions.

::: mobile/android/base/GeckoApplication.java
@@ +128,5 @@
>          HomeConfigInvalidator.getInstance().init(getApplicationContext());
> +        registerActivityLifecycleCallbacks(new ActivityLifecycleCallbacks() {
> +
> +            @Override
> +            public void onActivityStopped(Activity activity) {

Nit: list these in the lifecycle order.

@@ +137,5 @@
> +            }
> +
> +            @Override
> +            public void onActivityResumed(Activity activity) {
> +                if (mApplicationWasPaused) {

I think this should be under Started, no? Pause/Resume are paired, Stop/(Re)Start are paired (at least with the Activity lifecycle).

::: mobile/android/base/Telemetry.java
@@ +119,5 @@
>          Log.d(LOGTAG, "StartUISession: " + sessionName);
>          GeckoEvent event = GeckoEvent.createTelemetryUISessionStartEvent(sessionName, realtime());
>          GeckoAppShell.sendEventToGecko(event);
> +        if (updateActiveSessions) {
> +            activeSessions.add(sessionName);

rnewman: I'm hesitant to group starting/stopping session with pausing/resuming, because those seem like cases that a probe should explicitly handle. Telemetry should definitely have the capability to manage the pausing/resuming sessions so probes don't have to handle application backgrounding if they don't want to, but we could also have a different class of "unmanaged" sessions where the probe is responsible for starting/stopping itself (if it cares about session times).

@@ +163,5 @@
>      public static void sendUIEvent(String action) {
>          sendUIEvent(action, null, realtime(), null);
>      }
> +
> +    public static void endActiveUITelemetrySessions() {

Maybe pause instead of end?

@@ +169,5 @@
> +            stopUISession(sessionName, TelemetryContract.Reason.APP_MINIMIZED, false);
> +        }
> +    }
> +
> +    public static void startPreviousUITelemetrySessions() {

I don't like this method name for some reason. Maybe you can pair the naming with the "pause/stop" method better?

::: mobile/android/base/TelemetryContract.java
@@ +61,5 @@
>       * Holds reasons for stopping a session. Intended for use in
>       * Telemetry.stopUISession() as the "reason" parameter.
>       */
> +    public interface Reason {
> +        public static final String APP_MINIMIZED = "appminimized.1";

I would prefer app.backgrounded or something - minimized is such a desktop term!

Comment 12

[Sola Ogunsakin [:sola]]

Attachment: bug-994273.patch

Introduced 3 types of sessions
* Unbounded sessions: don't stop until explicitly stopped.
* Activity-bound sessions: open until it is explicitly stopped or the current activity is stopped. stopped temporarily when the app is backgrounded restarted when the app is restored. e.g homepanel sessions, browsersearch.
* Application-bound sessions: open as long as the app is open and will be paused when the app is backgrounded. can be explicitly stopped. e.g device orientation.

Comment 13

[Sola Ogunsakin [:sola]]

Attachment: bug-994273.patch

- add comments
- formatting

Comment 14

[Sola Ogunsakin [:sola]]

Attachment: Part 2: Update existing sessions

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8410441 [details] [diff] [review]
bug-994273.patch

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

I don't really like the added complexity of this new revision of the API, and I'd like to see some better naming than Unbound/ActivityBoundUI/ApplicationBoundUI - it seems too complex, and will make it harder for people to understand and use!

Specifically
- Do we want to handle the case of activity-bound probes? This seems to add a lot of complexity, and not much value.

(Please also see the review comments from my previous review. I don't think you addressed some of those issues.)

::: mobile/android/base/GeckoApplication.java
@@ +31,5 @@
>      private boolean mInBackground;
>      private boolean mPausedGecko;
>  
> +    private boolean mApplicationWasPaused = false;
> +    private ArrayList<ApplicationStatusListener> mApplicationStatusListeners = new ArrayList<ApplicationStatusListener>();

The variable type should be List - ArrayList is the implementation type.

@@ +131,5 @@
>          GeckoLoader.loadMozGlue();
>          HomeConfigInvalidator.getInstance().init(getApplicationContext());
> +        registerActivityLifecycleCallbacks(new ActivityLifecycleCallbacks() {
> +            @Override
> +            public void onActivityStarted(Activity activity) {}

See comment on method ordering from my previous review.

@@ +142,5 @@
> +
> +                if (mApplicationWasPaused) {
> +                    // App was restored from background. Notify ApplicationStatusListeners.
> +                    mApplicationWasPaused = false;
> +                    for(ApplicationStatusListener l: mApplicationStatusListeners) {

To change throughout:
Spacing - around : and after for (see previous comment by rnewman, and make sure you correct this style in future patches)
Nit: listener, not l

@@ +161,5 @@
> +                    l.onActivityStopped(activity);
> +                }
> +
> +                if (isApplicationInBackground()) {
> +                    // App entered the background. Notify ApplicationStatusListeners.

"entered the background" sounds weird to me - maybe "App was backgrounded, notify..."?

::: mobile/android/base/Telemetry.java
@@ +27,5 @@
>  @RobocopTarget
>  public class Telemetry {
>      private static final String LOGTAG = "Telemetry";
>  
> +    private static HashMap<String, HashSet<String>> mActivitySessions;

Don't use the implementation type in declarations! Always use the interface, unless you have a good reason to explicitly need that specific implementation.

@@ +30,5 @@
>  
> +    private static HashMap<String, HashSet<String>> mActivitySessions;
> +    private static HashSet<String> mApplicationSessions;
> +    static {
> +        mActivitySessions = new HashMap<String, HashSet<String>>();

Interface, not implementation.

@@ +188,5 @@
> +     * stopActivityBoundUISession}) or the current activity is stopped.
> +     * The session will be stopped temporarily when the app is backgrounded
> +     * and restarted when the app is restored.
> +     *
> +     * @param sessionName name of session to be started

Why do we want an activity-bound session?

@@ +205,5 @@
> +     * Start an unbound session. Session will be open until explicitly stopped.
> +     *
> +     * @param sessionName name of session to be started
> +     */
> +    public static void startUnboundUISession(String sessionName) {

Can we call this unmanaged?

Comment 16

[Sola Ogunsakin [:sola]]

(In reply to Chenxia Liu [:liuche] from comment #15)
> I don't really like the added complexity of this new revision of the API,
> and I'd like to see some better naming than
> Unbound/ActivityBoundUI/ApplicationBoundUI - it seems too complex, and will
> make it harder for people to understand and use!

hmm. not too sure. how about UnmanagedSession/ActivitySession/ApplicationSession?

> Specifically
> - Do we want to handle the case of activity-bound probes? This seems to add
> a lot of complexity, and not much value.

activity-bound sessions will stop when the activity stops and resume when the activity resumes. For e.g if you're in a homepanel and you open settings. Telemetry knows that homepanel.1 is an activity-bound session so it stops it temporarily while settings is open. When you close settings and return to about:home, telemetry will automatically restart the homepanel.1 session as well as any active session that was bound to BrowserApp before it was paused. I think this is a very useful feature to have. Otherwise to probe writer will have to handle this exit/re-entry scenario every time they make a session. I think it resolves your concerns from comment #4.

> @@ +142,5 @@
> > +
> > +                if (mApplicationWasPaused) {
> > +                    // App was restored from background. Notify ApplicationStatusListeners.
> > +                    mApplicationWasPaused = false;
> > +                    for(ApplicationStatusListener l: mApplicationStatusListeners) {
> 
> To change throughout:
> Spacing - around : and after for (see previous comment by rnewman, and make
> sure you correct this style in future patches)

this wont benefit me much now but I think for future it would be nice to have a style guide for these nuanced nits. Would certainly help prevent a some of the functionally inconsequential back and forths on bugs that tend to slow the dev process.

> Nit: listener, not l
> 
> @@ +161,5 @@
> > +                    l.onActivityStopped(activity);
> > +                }
> > +
> > +                if (isApplicationInBackground()) {
> > +                    // App entered the background. Notify ApplicationStatusListeners.
> 
> "entered the background" sounds weird to me - maybe "App was backgrounded,
> notify..."?
sounds good. i'll change that
> ::: mobile/android/base/Telemetry.java
> @@ +27,5 @@
> >  @RobocopTarget
> >  public class Telemetry {
> >      private static final String LOGTAG = "Telemetry";
> >  
> > +    private static HashMap<String, HashSet<String>> mActivitySessions;
> 
> Don't use the implementation type in declarations! Always use the interface,
> unless you have a good reason to explicitly need that specific
> implementation.
this would be nice to have in a style guide
> @@ +30,5 @@
> >  
> > +    private static HashMap<String, HashSet<String>> mActivitySessions;
> > +    private static HashSet<String> mApplicationSessions;
> > +    static {
> > +        mActivitySessions = new HashMap<String, HashSet<String>>();
> 
> Interface, not implementation.
ditto

Comment 17

[Chenxia Liu [:liuche]]

Comment on attachment 8410441 [details] [diff] [review]
bug-994273.patch

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

Ok, notes from a second pass on this review, because I understand what you are trying to do here now. More comments :)

+1 for the Telemetry.*Session naming you suggested.

::: mobile/android/base/GeckoApplication.java
@@ +31,5 @@
>      private boolean mInBackground;
>      private boolean mPausedGecko;
>  
> +    private boolean mApplicationWasPaused = false;
> +    private ArrayList<ApplicationStatusListener> mApplicationStatusListeners = new ArrayList<ApplicationStatusListener>();

I would definitely also comment these listeners, and describe the situations under which they're called.

@@ +134,5 @@
> +            @Override
> +            public void onActivityStarted(Activity activity) {}
> +
> +            @Override
> +            public void onActivityResumed(Activity activity) {

General question: why put this in onResume/onStop instead of onStart/onStop?

@@ +139,5 @@
> +                for(ActivityLifecycleListener l: mActivityLifecycleListeners) {
> +                    l.onActivityResumed(activity);
> +                }
> +
> +                if (mApplicationWasPaused) {

Nit: I would call the application level listeners here before the activity-level ones, because it makes more sense in terms of nesting.

@@ +190,5 @@
>      public void prepareLightweightTheme() {
>          mLightweightTheme = new LightweightTheme(this);
>      }
> +
> +    public void registerLifecycleListener(ActivityLifecycleListener listener) {

Shouldn't this be called registerActivityLifecycleListener?

::: mobile/android/base/Telemetry.java
@@ +130,5 @@
> +            // Stop sessions that are bound to this activity.
> +            if (mActivitySessions.containsKey(activityName)) {
> +                final HashSet<String> activeSessions = mActivitySessions.get(activityName);
> +                for (String session: activeSessions) {
> +                    stopUISession(session, null);

I'd use "activity.backgrounded.1" or something here as a reason.

@@ +227,5 @@
> +        mApplicationSessions.remove(sessionName);
> +    }
> +
> +    public static void stopApplicationBoundUISession(String sessionName) {
> +        stopApplicationBoundUISession(sessionName, null);

I'd also include a reason here.

Comment 18

[Chenxia Liu [:liuche]]

Also, once we have this API hammered out, you'll need a patch to update the docs. Thanks for keeping on through this back and forth!

Comment 19

[Sola Ogunsakin [:sola]]

(In reply to Chenxia Liu [:liuche] from comment #17)
> @@ +134,5 @@
> > +            @Override
> > +            public void onActivityStarted(Activity activity) {}
> > +
> > +            @Override
> > +            public void onActivityResumed(Activity activity) {
> 
> General question: why put this in onResume/onStop instead of onStart/onStop?

right, that should be the case. i'll move the onResume logic to onStart :)

Comment 20

[Sola Ogunsakin [:sola]]

Attachment: bug-994273.patch

Comment 21

[Sola Ogunsakin [:sola]]

Attachment: Part 2: Update existing sessions

Comment 22

[Sola Ogunsakin [:sola]]

Attachment: Part 2: Update existing sessions

-bump version number on sessions

Comment 23

[Chenxia Liu [:liuche]]

Comment on attachment 8411466 [details] [diff] [review]
bug-994273.patch

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

Good job addressing all the comments. I'd think about thread safety, since you might be registering lifecyle listeners from multiple threads. You should see what suggestions rnewman has on this.

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8411466 [details] [diff] [review]
bug-994273.patch

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

Lots of questions inline!

(Also commit message is missing a.)

::: mobile/android/base/GeckoApplication.java
@@ +33,5 @@
>      private boolean mInBackground;
>      private boolean mPausedGecko;
>  
> +    // Tracks transition between background and foreground states.
> +    private boolean mApplicationWasPaused = false;

What's wrong with mInBackground?

@@ +139,5 @@
>          HomeConfigInvalidator.getInstance().init(getApplicationContext());
> +
> +        // ActivityLifecycleCallbacks only available for v14+.
> +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH) {
> +            registerActivityLifecycleCallbacks(new ActivityLifecycleCallbacksListener());

So what's our plan for pre-ICS devices? What happens on those devices with this patch?

@@ +157,5 @@
>          mLightweightTheme = new LightweightTheme(this);
>      }
> +
> +    public void registerActivityLifecycleListener(ActivityLifecycleListener listener) {
> +        mActivityLifecycleListeners.add(listener);

These methods are a threading bug waiting to happen. You're using non-thread-safe data structures with no guards at all, and no documentation about thread behavior.

@@ +200,5 @@
> +         */
> +        public void onApplicationRestored();
> +    }
> +
> +    public class ActivityLifecycleCallbacksListener implements ActivityLifecycleCallbacks {

Code reading suggests that these are only ever called on the UI thread. Document this and add ThreadUtils assertions to verify.

@@ +207,5 @@
> +
> +        @Override
> +        public void onActivityStarted(Activity activity) {
> +            // Notify ActivityLifecycleListeners of starting activity.
> +            for (ActivityLifecycleListener listener : mActivityLifecycleListeners) {

Thread bug.

@@ +211,5 @@
> +            for (ActivityLifecycleListener listener : mActivityLifecycleListeners) {
> +                listener.onActivityStarted(activity);
> +            }
> +
> +            if (mApplicationWasPaused) {

Can mApplicationWasPaused ever be touched from multiple threads? If so, this needs to be reworked.

@@ +212,5 @@
> +                listener.onActivityStarted(activity);
> +            }
> +
> +            if (mApplicationWasPaused) {
> +                // App was restored from background. Notify ApplicationStatusListeners.

"Notify..." seems redundant, given the code below :)

@@ +215,5 @@
> +            if (mApplicationWasPaused) {
> +                // App was restored from background. Notify ApplicationStatusListeners.
> +                mApplicationWasPaused = false;
> +                for (ApplicationStatusListener listener : mApplicationStatusListeners) {
> +                    listener.onApplicationRestored();

Is this something we want to block the UI thread for? More elsewhere.

::: mobile/android/base/Telemetry.java
@@ +30,5 @@
>  public class Telemetry {
>      private static final String LOGTAG = "Telemetry";
>  
> +    // Active sessions for a given activity.
> +    private static Map<String, Set<String>> sActivitySessions;

Nit: no 's' prefix.

@@ +40,5 @@
> +    private static String sCurrentActivity;
> +
> +    static {
> +        sActivitySessions = new HashMap<String, Set<String>>();
> +        sApplicationSessions = new HashSet<String>();

These are the wrong data structures to use for this. You probably want Concurrent*, and for the embedded Sets to be Concurrent, too, but consider two racing clients trying to insert the first session for the same activity...

I might be inclined to flatten activitySessions into a linear sequence of pairs, and just walk the whole thing. We only ever have one or two activities alive at a time.

@@ +42,5 @@
> +    static {
> +        sActivitySessions = new HashMap<String, Set<String>>();
> +        sApplicationSessions = new HashSet<String>();
> +
> +        GeckoApplication.get().registerActivityLifecycleListener(new TelemetryActivityLifecycleListener());

I'm slightly perturbed by the use of that singleton accessor inside a static initializer. Seems like a very strict lifecycle coupling.

@@ +43,5 @@
> +        sActivitySessions = new HashMap<String, Set<String>>();
> +        sApplicationSessions = new HashSet<String>();
> +
> +        GeckoApplication.get().registerActivityLifecycleListener(new TelemetryActivityLifecycleListener());
> +        GeckoApplication.get().registerApplicationStatusListener(new TelemetryApplicationStatusListener());

Why get() twice?

@@ +133,5 @@
> +    public static class TelemetryActivityLifecycleListener
> +                    implements GeckoApplication.ActivityLifecycleListener {
> +        @Override
> +        public void onActivityStarted(Activity activity) {
> +            sCurrentActivity = activity.getClass().getName();

Why not use the Class itself as a key?

@@ +136,5 @@
> +        public void onActivityStarted(Activity activity) {
> +            sCurrentActivity = activity.getClass().getName();
> +            // Restart sessions that are bound to this activity.
> +            if (sActivitySessions.containsKey(sCurrentActivity)) {
> +                final Set<String> activeSessions = sActivitySessions.get(sCurrentActivity);

Just get-and-check rather than using containsKey.

@@ +138,5 @@
> +            // Restart sessions that are bound to this activity.
> +            if (sActivitySessions.containsKey(sCurrentActivity)) {
> +                final Set<String> activeSessions = sActivitySessions.get(sCurrentActivity);
> +                for (String session : activeSessions) {
> +                    startUISession(session);

Shouldn't this have some metadata to denote that it was resumed?

@@ +156,5 @@
> +            }
> +        }
> +    }
> +
> +    public static class TelemetryApplicationStatusListener

Why make these two classes? Just one is enough, implementing both interfaces.

@@ +214,5 @@
> +     *
> +     * @param sessionName name of session to be started
> +     */
> +    public static void startUnmanagedUISession(String sessionName) {
> +        startUISession(sessionName);

This doesn't seem like a win to me. Just put that comment above startUISession!

Comment 25

[Sola Ogunsakin [:sola]]

(In reply to Richard Newman [:rnewman] from comment #24)
> ::: mobile/android/base/GeckoApplication.java
> @@ +33,5 @@
> >      private boolean mInBackground;
> >      private boolean mPausedGecko;
> >  
> > +    // Tracks transition between background and foreground states.
> > +    private boolean mApplicationWasPaused = false;
> 
> What's wrong with mInBackground?

mInBackground can't track the transition from background to foreground. By the time we get our first callback after returning to the foreground, mInBackground will already be false.

> @@ +139,5 @@
> >          HomeConfigInvalidator.getInstance().init(getApplicationContext());
> > +
> > +        // ActivityLifecycleCallbacks only available for v14+.
> > +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH) {
> > +            registerActivityLifecycleCallbacks(new ActivityLifecycleCallbacksListener());
> 
> So what's our plan for pre-ICS devices? What happens on those devices with
> this patch?

we decided we wouldn't analyze session duration on pre-ICS devices. With this patch, ActivityLifecycleCallbacks will never be called on those devices so every session will act like an unmanaged one.

> @@ +157,5 @@
> >          mLightweightTheme = new LightweightTheme(this);
> >      }
> > +
> > +    public void registerActivityLifecycleListener(ActivityLifecycleListener listener) {
> > +        mActivityLifecycleListeners.add(listener);
> 
> These methods are a threading bug waiting to happen. You're using
> non-thread-safe data structures with no guards at all, and no documentation
> about thread behavior.

ActivityLifecycle callbacks all occur on the main thread. I don't believe there are any issues with concurrency.

> @@ +207,5 @@
> > +
> > +        @Override
> > +        public void onActivityStarted(Activity activity) {
> > +            // Notify ActivityLifecycleListeners of starting activity.
> > +            for (ActivityLifecycleListener listener : mActivityLifecycleListeners) {
> 
> Thread bug.

ditto

> @@ +211,5 @@
> > +            for (ActivityLifecycleListener listener : mActivityLifecycleListeners) {
> > +                listener.onActivityStarted(activity);
> > +            }
> > +
> > +            if (mApplicationWasPaused) {
> 
> Can mApplicationWasPaused ever be touched from multiple threads? If so, this
> needs to be reworked.

ditto

> @@ +215,5 @@
> > +            if (mApplicationWasPaused) {
> > +                // App was restored from background. Notify ApplicationStatusListeners.
> > +                mApplicationWasPaused = false;
> > +                for (ApplicationStatusListener listener : mApplicationStatusListeners) {
> > +                    listener.onApplicationRestored();
> 
> Is this something we want to block the UI thread for? More elsewhere.

didn't notice any impact on UI. we're just sending gecko messages. there is only one listener (for now).
I'll add a comment to the interface mentioning that very little work should be done in these callbacks.

Only issue I see here is a potentially large list of sessions to restart/stop each time. I guess we could create batch "start session" and "stop session" telemetry gecko messages to avoid sending O(n) messages each time.

> @@ +40,5 @@
> > +    private static String sCurrentActivity;
> > +
> > +    static {
> > +        sActivitySessions = new HashMap<String, Set<String>>();
> > +        sApplicationSessions = new HashSet<String>();
> 
> These are the wrong data structures to use for this. You probably want
> Concurrent*, and for the embedded Sets to be Concurrent, too, but consider
> two racing clients trying to insert the first session for the same
> activity...

there are no race conditions I believe. its all the same thread.

> I might be inclined to flatten activitySessions into a linear sequence of
> pairs, and just walk the whole thing. We only ever have one or two
> activities alive at a time.

what is the advantage to this? seems like more code :/

> @@ +42,5 @@
> > +    static {
> > +        sActivitySessions = new HashMap<String, Set<String>>();
> > +        sApplicationSessions = new HashSet<String>();
> > +
> > +        GeckoApplication.get().registerActivityLifecycleListener(new TelemetryActivityLifecycleListener());
> 
> I'm slightly perturbed by the use of that singleton accessor inside a static
> initializer. Seems like a very strict lifecycle coupling.

not sure what you mean here :/

> @@ +138,5 @@
> > +            // Restart sessions that are bound to this activity.
> > +            if (sActivitySessions.containsKey(sCurrentActivity)) {
> > +                final Set<String> activeSessions = sActivitySessions.get(sCurrentActivity);
> > +                for (String session : activeSessions) {
> > +                    startUISession(session);
> 
> Shouldn't this have some metadata to denote that it was resumed?

believe this to be a separate session. the user has restarted their interaction with the UI.

Comment 26

[Richard Newman [:rnewman]]

(In reply to Sola Ogunsakin [:sola] from comment #25)

> > What's wrong with mInBackground?
> 
> mInBackground can't track the transition from background to foreground. By
> the time we get our first callback after returning to the foreground,
> mInBackground will already be false.

So can we use mApplicationWasPaused instead of mInBackground, or combine the logic to get coverage on all API revisions?


> we decided we wouldn't analyze session duration on pre-ICS devices. With
> this patch, ActivityLifecycleCallbacks will never be called on those devices
> so every session will act like an unmanaged one.

Write up a big comment to that effect, and document it in the Sphinx docs, particularly how we tell the two apart!


> ActivityLifecycle callbacks all occur on the main thread. I don't believe
> there are any issues with concurrency.

These aren't the AL callbacks. This is you registering the listeners. If you're always registering those on the main thread, great! Add assertions to ensure this!


> > Thread bug.
> 
> ditto

Ditto about the assertion :)

It's a bug waiting to happen unless you make sure it can't happen!


> Only issue I see here is a potentially large list of sessions to
> restart/stop each time. I guess we could create batch "start session" and
> "stop session" telemetry gecko messages to avoid sending O(n) messages each
> time.

That's a good idea. Message sending isn't free. File a follow-up?


> there are no race conditions I believe. its all the same thread.

What's to stop me starting a session from a background task ("download underway", say) and the UI thread? Concurrent access to these two data structures. You either need to document and assert these conditions, or make them thread-safe.

You're building an API for other code to use. You have to make it robust.

 
> > I might be inclined to flatten activitySessions into a linear sequence of
> > pairs, and just walk the whole thing. We only ever have one or two
> > activities alive at a time.
> 
> what is the advantage to this? seems like more code :/

Firstly, fewer concurrency concerns. Secondly, there's usually only one activity, so it removes some pointless indirection. Thirdly, it's quicker to walk a list than to iterate over the elements of a map. Fourthly, a list is more space efficient and likely to be cheaper for both insertions and removals.

And it shouldn't be more code -- you can get rid of the whole is-there-an-entry-oh-better-make-one thing. Just push and walk.


> > > +                    startUISession(session);
> > 
> > Shouldn't this have some metadata to denote that it was resumed?
> 
> believe this to be a separate session. the user has restarted their
> interaction with the UI.

It will appear identical to the original session, just with a later timestamp, so we can't distinguish between "this was automatically begun due to resuming an activity" and "this began because the user did something".

Imagine you're trying to figure out how many times a user entered Reader Mode. If you do nothing special here, you cannot answer that question by simply looking for how many Reader Mode sessions started, because every time the user switches tasks, you'll get a new session.

Comment 27

[Chenxia Liu [:liuche]]

I initially had some reservations about how much code complexity we're adding, as well as the complexity we're adding to the UI Telemetry API - since this seems to include threading complexity as well, I wonder if we should take a step back and look at how much benefit we'll get from the effort we're putting in. How important is having accurate timing for session data? It would certainly be useful for seeing how long people are spending in a particular part of the app, but if we instrument the user actions that we're interested in, we'll still be able to learn about the context where the users are doing the interesting actions. In general, I'm leaning towards keeping things more simple.

Comment 28

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

I am also thinking we should put this work down for a bit and focus on landing more Event and simple Session patches.

After we start getting some data, we can determine how to move ahead with more exact Session lifecyle handling.

Comment 29

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.