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

VERIFIED FIXED in Firefox 30

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: bbondy, Assigned: emtwo)

Tracking

Trunk
Firefox 30
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=1 s=it-30c-29a-28b.1 r=ff30, )

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

6 years ago
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
Reporter

Comment 1

6 years ago
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.
Reporter

Updated

6 years ago
Duplicate of this bug: 872203
Blocks: 855945, 855930
No longer blocks: metrov1defect&change
Summary: Add telemetry submission for existing SHUTDOWN_OK from Metro front-end code → Work - Add telemetry submission for existing SHUTDOWN_OK from Metro front-end code
Whiteboard: feature=work
Priority: -- → P2
Whiteboard: feature=work → [shovel-ready] feature=work
Assignee: nobody → ally
Posted patch sending SHUTDOWN_OK (obsolete) — Splinter Review
left some comments in the code. I don't understand why the desktop has a giant && statement, and I notice it used == instead of ===
Attachment #764445 - Flags: review?(netzen)
Reporter

Comment 4

6 years ago
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.
Attachment #764445 - Flags: review?(netzen)
: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.
Flags: needinfo?(mark.finkle)
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.
Flags: needinfo?(mark.finkle)
(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.
Reporter

Comment 8

6 years ago
We moved bug 886336 to v2 in triage so doing the same for this.
Group: mozilla-corporation-confidential
Whiteboard: [shovel-ready] feature=work → feature=work
Reporter

Updated

6 years ago
Group: mozilla-corporation-confidential
well if its v2, I'd better drop it for now.
Assignee: ally → nobody
No longer blocks: metrov2planning
Summary: Work - Add telemetry submission for existing SHUTDOWN_OK from Metro front-end code → Add telemetry submission for existing SHUTDOWN_OK from Metro front-end code
Whiteboard: feature=work → [feature] p=0

Updated

6 years ago
Blocks: 956798
Reporter

Updated

6 years ago
Whiteboard: [feature] p=0 → [feature] p=1
Priority: P2 → --
Priority: -- → P1
Target Milestone: --- → Firefox 30
Assignee

Updated

6 years ago
Assignee: nobody → msamuel
Status: NEW → ASSIGNED
Priority: P1 → P2
QA Contact: jbecerra
Whiteboard: [feature] p=1 → [feature] p=1 s=it-30c-29a-28b.1
Assignee

Comment 10

6 years ago
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.
Attachment #764445 - Attachment is obsolete: true
Attachment #8370825 - Flags: feedback?(mbrubeck)
Comment on attachment 8370825 [details] [diff] [review]
Part 1: v1: Create a separate CrashMonitor file for Metro

Seems reasonable to me.
Attachment #8370825 - Flags: feedback?(mbrubeck) → feedback+
Assignee

Comment 12

6 years ago
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!
Attachment #8371021 - Flags: feedback?(mbrubeck)
Attachment #8371021 - Flags: feedback?(mbrubeck) → feedback+
Assignee

Updated

5 years ago
Attachment #8370825 - Flags: review?(mbrubeck)
Assignee

Updated

5 years ago
Attachment #8371021 - Flags: review?(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.
Attachment #8370825 - Flags: review?(mbrubeck) → review?(dteller)
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.
Attachment #8371021 - Flags: review?(mbrubeck) → review+
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.
Attachment #8370825 - Flags: review?(dteller) → feedback+
Assignee

Comment 16

5 years ago
Added a metro folder
Attachment #8370825 - Attachment is obsolete: true
Attachment #8372517 - Flags: review?(dteller)
Whiteboard: [feature] p=1 s=it-30c-29a-28b.1 → p=1 s=it-30c-29a-28b.1 r=ff30
Target Milestone: Firefox 30 → ---
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.
Flags: needinfo?(dteller)
I'll try and do this by tomorrow evening.
Flags: needinfo?(dteller)
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.
Attachment #8372517 - Flags: review?(dteller)
Assignee

Comment 20

5 years ago
Ah, thanks for pointing that out.
Attachment #8372517 - Attachment is obsolete: true
Attachment #8374328 - Flags: review?(dteller)
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.
Attachment #8374328 - Flags: review?(dteller) → review+
Assignee

Comment 22

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/c0ea0b4d69f7
https://hg.mozilla.org/integration/fx-team/rev/77e7f75b502d
Whiteboard: p=1 s=it-30c-29a-28b.1 r=ff30 → [fixed-in-fx-team] p=1 s=it-30c-29a-28b.1 r=ff30
https://hg.mozilla.org/mozilla-central/rev/c0ea0b4d69f7
https://hg.mozilla.org/mozilla-central/rev/77e7f75b502d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team] p=1 s=it-30c-29a-28b.1 r=ff30 → p=1 s=it-30c-29a-28b.1 r=ff30
Target Milestone: --- → Firefox 30
Could anyone please give guidance in order for the QA to verify this?
Flags: needinfo?(msamuel)
Assignee

Comment 25

5 years ago
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.
Flags: needinfo?(msamuel)
(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. " ?
Flags: needinfo?(msamuel)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(msamuel)
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.