880109 - Begin a new session when environment changes

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Android, defect, P1)

Description

[Richard Newman [:rnewman]]

Currently we don't do so, but we should.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

This seems to work.

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 761095 [details] [diff] [review]
Proposed patch. v1

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

r+ if your claim about error states is correct.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +142,5 @@
> +            final boolean wasOOM = false;
> +            final boolean wasStopped = true;
> +            final long wallStartTime = System.currentTimeMillis();
> +            final long realStartTime = android.os.SystemClock.elapsedRealtime();
> +            Log.d(LOG_TAG, "Recording runtime session transition: " +

-1 on log spew.

@@ +341,5 @@
>              this.state = State.INITIALIZATION_FAILED;
>              return;
>          }
> +
> +        final int env = ensureEnvironment();

nit: shadowing this.env is lame. newEnv? next?

@@ +342,5 @@
>              return;
>          }
> +
> +        final int env = ensureEnvironment();
> +        

nit: whitespace.

@@ +597,5 @@
> +    /**
> +     * Invoked in the background whenever the environment transitions between
> +     * two valid values.
> +     *
> +     * This will never be called without a valid storage handle, so it does not

Why can't this race with storage shutdown?  Runnable gets pushed to background thread, lots of things happen, storage shutdown starts (and finishes), then runnable executes?

@@ +603,5 @@
> +     */
> +    protected void onEnvironmentTransition(int prev, int env) {
> +        final SharedPreferences prefs = GeckoApp.getAppSharedPreferences();
> +        final SharedPreferences.Editor editor = prefs.edit();
> +

nit: less newlines and less this.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #2)

> Why can't this race with storage shutdown?  Runnable gets pushed to
> background thread, lots of things happen, storage shutdown starts (and
> finishes), then runnable executes?

I think my logic is: BrowserHealthRecorder.close is only ever called on the background thread. If our runnable starts and it's INITIALIZED, it'll continue to be valid until the next runnable. I'm checking on this, though, and might add the extra safety regardless.

Comment 4

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/5fd1ed6740f8


Note for verification: with this patch you'll see stuff like

08-01 14:03:54.022 D/GeckoHealthRec(30441): Add-on changed: {0fa2149e-bb2c-4ac2-a8d3-479599819475}
08-01 14:03:54.082 D/GeckoHealthRec(30441): Recording session end: E
08-01 14:03:54.122 D/GeckoSessInfo(30441): Recording session done: 1375390877883
08-01 14:03:54.122 D/GeckoSessInfo(30441): Recording runtime session transition: 1375391034126, 406917130
08-01 14:03:54.122 D/GeckoSessInfo(30441): Recording start of session: 1375391034126

when you install an add-on -- that is, we'll end the current session and start another.


Flagging for tracking 24, because it's a little interrelated with Bug 900694, and it will improve FHR's accuracy with little risk, but needs to bake for a brief period on Nightly before I request uplift.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fd1ed6740f8

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 761095 [details] [diff] [review]
Proposed patch. v1

Drat, forgot to flag this. Dependency for Part 2 of Bug 900694.

Should be safe, and well-tested by hand.

Comment 7

[bhavana bajaj [:bajaj]]

Comment on attachment 761095 [details] [diff] [review]
Proposed patch. v1

[Triage Comment]

a=bajaj for beta as Fx24 is beta now

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d4d13a2b279