Closed
Bug 872206
Opened 10 years ago
Closed 9 years ago
Add telemetry submission for existing SHUTDOWN_OK from Metro front-end code
Categories
(Firefox for Metro Graveyard :: General, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 30
People
(Reporter: bbondy, Assigned: emtwo)
References
()
Details
(Whiteboard: p=1 s=it-30c-29a-28b.1 r=ff30)
Attachments
(2 files, 3 obsolete files)
4.21 KB,
patch
|
mbrubeck
:
review+
mbrubeck
:
feedback+
|
Details | Diff | Splinter Review |
1.67 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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•10 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.
Updated•10 years ago
|
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
Updated•10 years ago
|
Priority: -- → P2
Whiteboard: feature=work → [shovel-ready] feature=work
Updated•10 years ago
|
Assignee: nobody → ally
Comment 3•10 years ago
|
||
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•10 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)
Comment 5•10 years ago
|
||
: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)
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
We moved bug 886336 to v2 in triage so doing the same for this.
Blocks: metrov2planning
Group: mozilla-corporation-confidential
Whiteboard: [shovel-ready] feature=work → feature=work
Reporter | ||
Updated•10 years ago
|
Group: mozilla-corporation-confidential
Updated•10 years ago
|
Updated•10 years ago
|
Blocks: metrobacklog
Updated•10 years ago
|
No longer blocks: metrov2planning
Updated•10 years ago
|
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
Reporter | ||
Updated•10 years ago
|
Whiteboard: [feature] p=0 → [feature] p=1
Updated•10 years ago
|
Priority: P2 → --
Updated•10 years ago
|
Priority: -- → P1
Updated•10 years ago
|
Target Milestone: --- → Firefox 30
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → msamuel
Updated•10 years ago
|
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•10 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 11•10 years ago
|
||
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•10 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)
Updated•10 years ago
|
Attachment #8371021 -
Flags: feedback?(mbrubeck) → feedback+
Assignee | ||
Updated•9 years ago
|
Attachment #8370825 -
Flags: review?(mbrubeck)
Assignee | ||
Updated•9 years ago
|
Attachment #8371021 -
Flags: review?(mbrubeck)
Comment 13•9 years ago
|
||
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 14•9 years ago
|
||
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•9 years ago
|
||
Added a metro folder
Attachment #8370825 -
Attachment is obsolete: true
Attachment #8372517 -
Flags: review?(dteller)
Updated•9 years ago
|
Whiteboard: [feature] p=1 s=it-30c-29a-28b.1 → p=1 s=it-30c-29a-28b.1 r=ff30
Target Milestone: Firefox 30 → ---
Comment 17•9 years ago
|
||
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•9 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•9 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: 9 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
Comment 24•9 years ago
|
||
Could anyone please give guidance in order for the QA to verify this?
Flags: needinfo?(msamuel)
Assignee | ||
Comment 25•9 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)
Comment 26•9 years ago
|
||
(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)
Comment 27•9 years ago
|
||
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)
Updated•9 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•