872206 - Add telemetry submission for existing SHUTDOWN_OK from Metro front-end code

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Brian R. Bondy [:bbondy]]

SHUTDOWN_OK doesn't get sent up from Metro Firefox, I think because of our custom sessionstore code.

The existing code can be found here:
browser/components/sessionstore/src/nsSessionStartup.js
Services.telemetry.getHistogramById("SHUTDOWN_OK").add(!lastSessionCrashed);

We should do this same thing our our session store code.


p=1

Comment 1

[Brian R. Bondy [:bbondy]]

This data will help us learn if we have a lot more crashes than Desktop, and also by comparing it to Desktop we can see if Windows kills us or if a user is using the left application bar to close us.  Based on this info we can decide to put more resources on crashes.

Comment 3

[Allison Naaktgeboren :ally]

Attachment: sending SHUTDOWN_OK

left some comments in the code. I don't understand why the desktop has a giant && statement, and I notice it used == instead of ===

Comment 4

[Brian R. Bondy [:bbondy]]

Comment on attachment 764445 [details] [diff] [review]
sending SHUTDOWN_OK

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

::: browser/metro/components/SessionStore.js
@@ +154,3 @@
>        case "final-ui-startup":
>          observerService.removeObserver(this, "final-ui-startup");
>          this.init();

Have you tested this with manually killing the process? It seems like _init will always set this._loadState = STATE_STOPPED so I think we'll always get the same telemetry value as is.

@@ +163,5 @@
> +        //  anaaktge: also, shouldnt that be === ?
> +        /* let lastSessionCrashed = this._initialState && this._initialState.session &&
> +                                 this._initialState.session.state &&
> +                                 this._initialState.session.state == STATE_RUNNING_STR;
> +        */

I don't really understand this comment. browser's SessionStore.jsm has a _initialState but this SessionStore.js does not. I think browser's does it because it may be null.  Please remove all of the above comments.

@@ +164,5 @@
> +        /* let lastSessionCrashed = this._initialState && this._initialState.session &&
> +                                 this._initialState.session.state &&
> +                                 this._initialState.session.state == STATE_RUNNING_STR;
> +        */
> +        let lastSessionCrashed = this.loadState === STATE_RUNNING;

There is a this._loadState but not a this.loadState

@@ +165,5 @@
> +                                 this._initialState.session.state &&
> +                                 this._initialState.session.state == STATE_RUNNING_STR;
> +        */
> +        let lastSessionCrashed = this.loadState === STATE_RUNNING;
> +        Cu.reportError("anaaktge: code for probe about to run lastSessionCrash: "+lastSessionCrashed);

nit: Seems like debug code please remove.

Comment 5

[Allison Naaktgeboren :ally]

:bbondy & I talked this over offline, and it seems that while desktop sessionstore saves the shutdown start from the prior run, the metro one does not. 

Why does metrofx have a custom session store? Is this an artifact of fennec? A deliberate decision?
 
We have a couple of options:
1) get rid of the custom session store if we have no compelling reason for having our own. 
2) modify the custom one to save the state of loadState at shutdown to be used to send the probe on restart (mirroring desktop's session store behavior)

bbondy thinks either mbrubeck or mfinkle might know why we have a special store and be able to shed more light on the custom store.

Comment 6

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

Fennec created a forked SessionStore.js because we wanted control over the data being saved and when it was saved. Mobile devices have slow file systems and saving a lot of data (like desktop's can) would hurt Fennec's performance and responsiveness.

Another reason is desktop's nsSessionStore.js is somewhat tied to the browser.js and browser.xul and tabbrowser.xml used by desktop. Since Fennec has very different code, we couldn't just use the desktop impl without making a fair bit of code changes.

We have talked about creating Session Utilities, which could be used by all SessionStore impls, making it easier to share code.

The telemetry code added by this patch did not exist in desktop's SessionStore impl when we forked, or we'd have it in Fennec's too. These days, "shutdown" is rarely ever clean on Android since we run until the user kills us or android kills us. Neither is clean.

Comment 7

[Allison Naaktgeboren :ally]

(In reply to Mark Finkle (:mfinkle) from comment #6)

Those sound like they qualify as compelling reasons for the time being (we can revisit if a Session Utilities comes into being, but that seems to be outside the scope of this bug.)

So it sounds like option (2) represents the path forward.

Comment 8

[Brian R. Bondy [:bbondy]]

We moved bug 886336 to v2 in triage so doing the same for this.

Comment 9

[Allison Naaktgeboren :ally]

well if its v2, I'd better drop it for now.

Comment 10

[Marina Samuel [:emtwo]]

Attachment: Part 1: v1: Create a separate CrashMonitor file for Metro

Since bug 887780, Desktop is using CrashMonitor to decide what to send for SHUTDOWN_OK, so I think we should do the same. However, I think if we share the same file as desktop then a crash that happens on desktop while switching to metro will be perceived as a metro crash and vice versa so I think it makes the most sense to create a separate file for ourselves to keep track of our crashes only.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8370825 [details] [diff] [review]
Part 1: v1: Create a separate CrashMonitor file for Metro

Seems reasonable to me.

Comment 12

[Marina Samuel [:emtwo]]

Attachment: Part 2: v1: Use CrashMonitor to identify crashes in Metro for sending a SHUTDOWN_OK telemetry probe

Note: I pulled this.saveState() out of the if statement in this patch because it was not getting called on every close, only when a save request had been made earlier but still not completed.

Tested that the checkpoint files were updated correctly with "sessionstore-final-state-write-complete"

Also, for part 1, is there someone on another team who I should be requesting a review from? Thanks!

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8370825 [details] [diff] [review]
Part 1: v1: Create a separate CrashMonitor file for Metro

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

Handing off review to Yoric.

Yoric, Metro Firefox is in the unusual position of sharing a profile with desktop Firefox; hence the need to differentiate certain data stored in the profile.  We're open to suggestions if you can think of a better way to do this.

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8371021 [details] [diff] [review]
Part 2: v1: Use CrashMonitor to identify crashes in Metro for sending a SHUTDOWN_OK telemetry probe

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

One minor change:

::: browser/metro/components/SessionStore.js
@@ +617,5 @@
>        return;
>  
> +    if (Services.startup.shuttingDown) {
> +      Services.obs.notifyObservers(null, "sessionstore-final-state-write-complete", "");
> +    }

I think this should be moved a few lines down, into the NetUtil.asyncCopy callback.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8370825 [details] [diff] [review]
Part 1: v1: Create a separate CrashMonitor file for Metro

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

No strong objection, but I would prefer if this could be in a metro/ subdirectory, to avoid increasing promiscuity.

Comment 16

[Marina Samuel [:emtwo]]

Attachment: Part 1: v2: Create a separate CrashMonitor file for Metro

Added a metro folder

Comment 17

[Marco Mucci [:MarcoM]]

Hi David, will you have time to review the patch this week?  Our iteration ends this Friday and I'm looking to determine if this bug will be completed by then.  Thanks.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

I'll try and do this by tomorrow evening.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8372517 [details] [diff] [review]
Part 1: v2: Create a separate CrashMonitor file for Metro

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

::: toolkit/components/crashmonitor/CrashMonitor.jsm
@@ +89,5 @@
>     * disc to reflect the information in |checkpoints|.
>     */
> +
> +  path: Services.metro && Services.metro.immersive ?
> +      FileUtils.getDir("ProfD", ["metro"], true).path + "\\metroSessionCheckpoints.json" :

No. That's going to cause main thread I/O during startup, which is pretty bad.

If you want to create the directory, in init(), before returning, call OS.File.makeDir(...).

Also, you don't need to call this metroSessionCheckpoints.json if this is already in directory "metro". Just sessionCheckpoints.json will do.

Comment 20

[Marina Samuel [:emtwo]]

Attachment: Part 1: v3: Create a separate CrashMonitor file for Metro

Ah, thanks for pointing that out.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8374328 [details] [diff] [review]
Part 1: v3: Create a separate CrashMonitor file for Metro

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

r=me with the changes below

::: toolkit/components/crashmonitor/CrashMonitor.jsm
@@ +88,5 @@
>     * Each time a new notification is received, this file is written to
>     * disc to reflect the information in |checkpoints|.
>     */
> +  path: OS.Path.join(OS.Constants.Path.profileDir, Services.metro && Services.metro.immersive ?
> +                     "metro\\sessionCheckpoints.json" : "sessionCheckpoints.json"),

Please don't hardcode "\\" in the path. This is not the only way to specify paths under Windows, so hardcoding backslash is a good recipe for errors.

Rather

path: (Services.metro && Services.metro.immersive)?
  OS.Path.join(OS.Constants.Path.profileDir,
               "metro",
               "sessionCheckPoints.json"):
  OS.Path.join(OS.Constants.Path.profileDir,
               "sessionCheckPoints.json")
  
Also please add a small comment explaining why we don't want the same file for metro and non-metro.

Comment 22

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/c0ea0b4d69f7
https://hg.mozilla.org/integration/fx-team/rev/77e7f75b502d

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c0ea0b4d69f7
https://hg.mozilla.org/mozilla-central/rev/77e7f75b502d

Comment 24

[Manuela Muntean [Away]]

Could anyone please give guidance in order for the QA to verify this?

Comment 25

[Marina Samuel [:emtwo]]

This bug and bug 967126 can be verified in the same way. Go to http://telemetry.mozilla.org/#nightly/30/SHUTDOWN_OK/saved_session/OTHER. There should be a visible graph for SHUTDOWN_OK but there will be no graph if you switch to Nightly 29. 

This is because bug 967126 separates telemetry logging for Metro and places it under OTHER instead of under Firefox. This bug adds the SHUTDOWN_OK probe which is the graph seen there.

Comment 26

[Manuela Muntean [Away]]

(In reply to Marina Samuel [:emtwo] from comment #25)
> This bug and bug 967126 can be verified in the same way. Go to
> http://telemetry.mozilla.org/#nightly/30/SHUTDOWN_OK/saved_session/OTHER.
> There should be a visible graph for SHUTDOWN_OK but there will be no graph
> if you switch to Nightly 29. 
> 
> This is because bug 967126 separates telemetry logging for Metro and places
> it under OTHER instead of under Firefox. This bug adds the SHUTDOWN_OK probe
> which is the graph seen there.

- on win 8 64-bit:

a) with latest Nightly 30.0a1  when loading http://telemetry.mozilla.org/#nightly/30/SHUTDOWN_OK/saved_session/OTHER I can see the graph, but I can also see it if I switch to Nightly 29 from the dropdown

b) with the Nightly 29.0a1 from 2014-01-08 (build ID: 20140108030203) when loading http://telemetry.mozilla.org/#nightly/30/SHUTDOWN_OK/saved_session/OTHER I can see the graph, but I can't see it anymore if I switch to Nightly 29 from the dropdown

Marina, what exactly did you mean by "There should be a visible graph for SHUTDOWN_OK but there will be no graph if you switch to Nightly 29. " ?

Comment 27

[Manuela Muntean [Away]]

I've retested this on a Win 8 64-bit machine, with latest Nightly, and I get the following:

  * http://telemetry.mozilla.org/#nightly/30/SHUTDOWN_OK/saved_session/OTHER  shows a graph (without text on it)

  * http://telemetry.mozilla.org/#nightly/29/SHUTDOWN_OK/saved_session/OTHER shows a graph with the text "No Data Available" on it
      
Marking this verified as fixed.