Begin a new session when environment changes

RESOLVED FIXED in Firefox 24

Status

Firefox Health Report
Client: Android
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

unspecified
Firefox 25
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24+ fixed, firefox25 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Currently we don't do so, but we should.
(Assignee)

Updated

5 years ago
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Depends on: 868445
Priority: -- → P1
(Assignee)

Comment 1

5 years ago
Created attachment 761095 [details] [diff] [review]
Proposed patch. v1

This seems to work.
Attachment #761095 - Flags: review?(nalexander)
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.
Attachment #761095 - Flags: review?(nalexander) → review+
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
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.
tracking-firefox24: --- → ?
Depends on: 900694
Target Milestone: --- → Firefox 25

Updated

5 years ago
status-firefox24: --- → affected
tracking-firefox24: ? → +
https://hg.mozilla.org/mozilla-central/rev/5fd1ed6740f8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

5 years ago
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.
Attachment #761095 - Flags: approval-mozilla-aurora?
Comment on attachment 761095 [details] [diff] [review]
Proposed patch. v1

[Triage Comment]

a=bajaj for beta as Fx24 is beta now
Attachment #761095 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/4d4d13a2b279
status-firefox24: affected → fixed
status-firefox25: --- → fixed
You need to log in before you can comment on or make changes to this bug.