Add Unclean Exit Watching to CPM

Add Unclean Exit Watching to CPM

Adds in ability to watch for unclean exits. These will be recognized
retroactively the next time the profile signs in, but the time should be within
two minutes of accuracy to the shutdown.

BUG=130212
TEST=Included Browsertests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147094

[Devlin, 2012-07-03 20:17:58]

Yoyo,
Also this one :)

[Yoyo Zhou, 2012-07-10 02:35:06]

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
File chrome/browser/performance_monitor/constants.cc (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/constants.cc:21: const char kStateProfile[] =
"profile";
This should probably have "Prefix" in its name.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
File chrome/browser/performance_monitor/performance_monitor.cc (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.cc:38: bool
TimeToString(base::Time time, std::string* output) {
It doesn't make sense that conversion in this direction could fail. This should
just return a string.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.cc:194: // this case, we
find if they exited uncleanly here.
This comment is written backwards. I'd say
"At init time, we find if profiles exited uncleanly in the case that they were
first loaded prior to the init..."

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.cc:195: void
PerformanceMonitor::CheckForUncleanExit() {
CheckForUncleanExits

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.cc:227: // that someone
has backdated the system time, so that Chrome won't crash in
This seems like a case that could actually happen, more than one might think. It
seems safe to proceed anyway though.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:374:
TwoProfileUncleanExit) {
It looks like these tests are only testing one of the 2 paths by which we might
add an unclean exit event.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/test/data/pe...
File chrome/test/data/performance_monitor/unclean_exit_prefs (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/test/data/pe...
chrome/test/data/performance_monitor/unclean_exit_prefs:2: "profile": {
3 space indent?

[lynn_cobs.com.sg, 2012-07-10 02:37:16]

Pls remove ivan@cobs.com.sg & lynn@cobs.com.sg from your list now

-----Original Message-----
From: yoz@chromium.org [mailto:yoz@chromium.org] 
Sent: Tuesday, 10 July, 2012 10:35 AM
To: rdevlin.cronin@chromium.org
Cc: chromium-reviews@chromium.org
Subject: [chromium-reviews] Re: Add Unclean Exit Watching to CPM (issue
10703078)


https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/per
formance_monitor/constants.cc
File chrome/browser/performance_monitor/constants.cc (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/per
formance_monitor/constants.cc#newcode21
chrome/browser/performance_monitor/constants.cc:21: const char
kStateProfile[] = "profile"; This should probably have "Prefix" in its name.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/per
formance_monitor/performance_monitor.cc
File chrome/browser/performance_monitor/performance_monitor.cc (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/per
formance_monitor/performance_monitor.cc#newcode38
chrome/browser/performance_monitor/performance_monitor.cc:38: bool
TimeToString(base::Time time, std::string* output) { It doesn't make sense
that conversion in this direction could fail. This should just return a
string.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/per
formance_monitor/performance_monitor.cc#newcode194
chrome/browser/performance_monitor/performance_monitor.cc:194: // this case,
we find if they exited uncleanly here.
This comment is written backwards. I'd say "At init time, we find if
profiles exited uncleanly in the case that they were first loaded prior to
the init..."

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/per
formance_monitor/performance_monitor.cc#newcode195
chrome/browser/performance_monitor/performance_monitor.cc:195: void
PerformanceMonitor::CheckForUncleanExit() { CheckForUncleanExits

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/per
formance_monitor/performance_monitor.cc#newcode227
chrome/browser/performance_monitor/performance_monitor.cc:227: // that
someone has backdated the system time, so that Chrome won't crash in This
seems like a case that could actually happen, more than one might think. It
seems safe to proceed anyway though.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/per
formance_monitor/performance_monitor_browsertest.cc
File
chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/per
formance_monitor/performance_monitor_browsertest.cc#newcode374
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:374:
TwoProfileUncleanExit) {
It looks like these tests are only testing one of the 2 paths by which we
might add an unclean exit event.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/test/data/p
erformance_monitor/unclean_exit_prefs
File chrome/test/data/performance_monitor/unclean_exit_prefs (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/test/data/p
erformance_monitor/unclean_exit_prefs#newcode2
chrome/test/data/performance_monitor/unclean_exit_prefs:2: "profile": {
3 space indent?

https://chromiumcodereview.appspot.com/10703078/

[Devlin, 2012-07-10 17:18:32]

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
File chrome/browser/performance_monitor/constants.cc (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/constants.cc:21: const char kStateProfile[] =
"profile";
On 2012/07/10 02:35:06, Yoyo Zhou wrote:
> This should probably have "Prefix" in its name.

Done.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
File chrome/browser/performance_monitor/performance_monitor.cc (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.cc:38: bool
TimeToString(base::Time time, std::string* output) {
On 2012/07/10 02:35:06, Yoyo Zhou wrote:
> It doesn't make sense that conversion in this direction could fail. This
should
> just return a string.

Done.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.cc:194: // this case, we
find if they exited uncleanly here.
On 2012/07/10 02:35:06, Yoyo Zhou wrote:
> This comment is written backwards. I'd say
> "At init time, we find if profiles exited uncleanly in the case that they were
> first loaded prior to the init..."

Done.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.cc:195: void
PerformanceMonitor::CheckForUncleanExit() {
On 2012/07/10 02:35:06, Yoyo Zhou wrote:
> CheckForUncleanExits

Done.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.cc:227: // that someone
has backdated the system time, so that Chrome won't crash in
On 2012/07/10 02:35:06, Yoyo Zhou wrote:
> This seems like a case that could actually happen, more than one might think.
It
> seems safe to proceed anyway though.

Yeah, I wasn't sure about this one...it won't break anything to continue on
(hence the DCHECK, not CHECK), but it's not necessarily good, either. In
retrospect, though, if someone's messing with the system time and using chrome,
it's quite possible they *are* debugging, so this would get very annoying very
quickly. Removed it.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:374:
TwoProfileUncleanExit) {
On 2012/07/10 02:35:06, Yoyo Zhou wrote:
> It looks like these tests are only testing one of the 2 paths by which we
might
> add an unclean exit event.

I assume that by the "Two paths" you mean either at startup or through adding
the profile after startup (if this isn't right, please correct me). The second
test (TwoProfileUncleanExit) tests both of these paths. We init both profiles to
have an unclean exit, but only the first ("Default") is loaded at startup
(testing the first path). When we call ProfileManager::GetProfile(), it loads
the second profile and causes a PROFILE_ADDED notification, which is the second
path.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/test/data/pe...
File chrome/test/data/performance_monitor/unclean_exit_prefs (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/test/data/pe...
chrome/test/data/performance_monitor/unclean_exit_prefs:2: "profile": {
On 2012/07/10 02:35:06, Yoyo Zhou wrote:
> 3 space indent?

3-space indent is standard for the Preferences file (and other JSON-formatted
files), I thought?

[Yoyo Zhou, 2012-07-12 18:13:44]

LGTM

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:374:
TwoProfileUncleanExit) {
On 2012/07/10 17:18:32, D Cronin wrote:
> On 2012/07/10 02:35:06, Yoyo Zhou wrote:
> > It looks like these tests are only testing one of the 2 paths by which we
> might
> > add an unclean exit event.
> 
> I assume that by the "Two paths" you mean either at startup or through adding
> the profile after startup (if this isn't right, please correct me). The second
> test (TwoProfileUncleanExit) tests both of these paths. We init both profiles
to
> have an unclean exit, but only the first ("Default") is loaded at startup
> (testing the first path). When we call ProfileManager::GetProfile(), it loads
> the second profile and causes a PROFILE_ADDED notification, which is the
second
> path.

Yes, that's what I meant. Apparently I didn't read the test carefully enough, so
never mind; this is good.

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/test/data/pe...
File chrome/test/data/performance_monitor/unclean_exit_prefs (right):

https://chromiumcodereview.appspot.com/10703078/diff/2002/chrome/test/data/pe...
chrome/test/data/performance_monitor/unclean_exit_prefs:2: "profile": {
On 2012/07/10 17:18:32, D Cronin wrote:
> On 2012/07/10 02:35:06, Yoyo Zhou wrote:
> > 3 space indent?
> 
> 3-space indent is standard for the Preferences file (and other JSON-formatted
> files), I thought?

You're right; for Preferences, apparently it is. (A bunch of source files that
end in .json are 2-space.)

https://chromiumcodereview.appspot.com/10703078/diff/14002/chrome/browser/per...
File chrome/browser/performance_monitor/performance_monitor.h (right):

https://chromiumcodereview.appspot.com/10703078/diff/14002/chrome/browser/per...
chrome/browser/performance_monitor/performance_monitor.h:26: typedef
base::Callback<void(const std::string&)> StateValueCallback;
Nice, this makes things more readable.

https://chromiumcodereview.appspot.com/10703078/diff/14002/chrome/browser/per...
chrome/browser/performance_monitor/performance_monitor.h:83: void
AddUncleanExitEvent(std::string profile_name);
Just noticed this one should also be a const std::string&

[commit-bot: I haz the power, 2012-07-17 18:26:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10703078/2...

[commit-bot: I haz the power, 2012-07-17 19:46:45]

Try job failure for 10703078-25001 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-17 19:50:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10703078/1...

[commit-bot: I haz the power, 2012-07-17 21:12:48]

Change committed as 147094