Adding shutdown tracing capabilities

Adding shutdown tracing capabilities

use "--trace-shutdown" to enable the feature to start profiling when the user has pressed "shutdown" and specify "--trace-shutdown-file=<name>" to specify the file where you want to dump to. Additionally you can specify which modules to trace with e.g. "--trace-shutdown=base,net".

That said: NOTE that the dumping will cost time since it has to be done after the shutdown of Chrome is finished. As such it takes time and will make it impossible to get a correct reading of time from shutdown till a new startup is finished.

Note: This is similar to the trace-startup - with the exception that upon shutdown it is not possible to rely on threads for IO anymore.

BUG=281524
TEST=visual tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221631

[Mr4D (OOO till 08-26), 2013-08-30 21:47:01]

Please have a look!

[DaveMoore, 2013-08-30 22:51:15]

+nduca

https://codereview.chromium.org/23691025/diff/20001/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/23691025/diff/20001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:133:
base::debug::TraceLog::RECORD_UNTIL_FULL);
Be sure to turn off tracing if the attempt to exit fails.

https://codereview.chromium.org/23691025/diff/20001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:135: TRACE_EVENT0("shutdown",
"AttemptUserExit");
It sure would be good to get system tracing on ChromeOS too. This complicates
things quite a bit though, so it probably should come on a subsequent cl.

https://codereview.chromium.org/23691025/diff/20001/content/browser/browser_m...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/23691025/diff/20001/content/browser/browser_m...
content/browser/browser_main_runner.cc:52: void WriteTracesToDisc(base::FilePath
file_name) {
What happens if the trace buffer fills up before this happens?

https://codereview.chromium.org/23691025/diff/20001/content/public/common/con...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/23691025/diff/20001/content/public/common/con...
content/public/common/content_switches.h:230: extern const char
kTraceShutdown[];
If you're accessing these from chrome (application_lifetime) they'll need to be
exported.

[James Cook, 2013-08-30 22:54:02]

I might also mention in the commit description that this is similar to
--trace-startup

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_loop.cc:718: TRACE_EVENT0("shutdown",
You don't need this one - this controller doesn't do any work on shutdown.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_loop.cc:863: TRACE_EVENT0("shutdown",
"BrowserMainLoop::Subsystem:DeleteDataSources");
This seems like a lot of TRACE_EVENT macros in this function. Do you need this
many?  Could things be broken into groups somehow?

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:41: class
BrowserShutdownProfilerAndDumper {
I think this class is big enough to go in a separate file, even though it's only
used here.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:59: LOG(ERROR) << "Flushing shutdown
traces to disc. The buffer is %" <<
Could this be a VLOG?  Or is this slow enough that logging is needed on normal
devices?

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:91: return
base::FilePath().AppendASCII("chrometrace.log");
FYI - We usually use the suffix ".json" for trace files.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:111: return dump_file_ && (0 ==
ferror(dump_file_));
ferror(dump_file_) == 0

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:117: 
no blank line

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:137: fclose(dump_file_);
Use file_util::CloseFile()

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:252: {
Put a blank line above this, or move the comment from 253 above it.  At first I
thought this scope was part of the if() statement.

https://codereview.chromium.org/23691025/diff/1/content/public/common/content...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/23691025/diff/1/content/public/common/content...
content/public/common/content_switches.cc:799: // flag results in the first call
to BeginTracing() to receive all trace events
Is the part of the comment about BeginTracing() accurate?

https://codereview.chromium.org/23691025/diff/1/content/public/common/content...
content/public/common/content_switches.cc:805: // omitted the default will be
used "chrometrace.log" in the current directory.
Update this to match the other filename if you change it

[Mr4D (OOO till 08-26), 2013-08-31 02:03:44]

Please have another look!

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_loop.cc:718: TRACE_EVENT0("shutdown",
Since that might change I thought it is better to have them all. But - we see
the gap ;). Done.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_loop.cc:863: TRACE_EVENT0("shutdown",
"BrowserMainLoop::Subsystem:DeleteDataSources");
I was trying first to move that into the functions - but unfortunately that does
lead to incorrect results since several functions had scoped pointers which get
destroyed after the trace statements are finished. Many of these were extremely
expensive. Since this function is "the destroyer" it gets loads of them, but it
is nice to seem them sequentially here. So I would leave it this way.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:41: class
BrowserShutdownProfilerAndDumper {
On 2013/08/30 22:54:02, James Cook wrote:
> I think this class is big enough to go in a separate file, even though it's
only
> used here.

Done.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:59: LOG(ERROR) << "Flushing shutdown
traces to disc. The buffer is %" <<
I did never run VLOG yet, but since you asked for it I use that now (it is not
an error after all ;) ).
 Done.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:91: return
base::FilePath().AppendASCII("chrometrace.log");
I used the same default name as for the startup case.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:111: return dump_file_ && (0 ==
ferror(dump_file_));
On 2013/08/30 22:54:02, James Cook wrote:
> ferror(dump_file_) == 0

Done.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:117: 
On 2013/08/30 22:54:02, James Cook wrote:
> no blank line

Done.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:137: fclose(dump_file_);
On 2013/08/30 22:54:02, James Cook wrote:
> Use file_util::CloseFile()

Done.

https://codereview.chromium.org/23691025/diff/1/content/browser/browser_main_...
content/browser/browser_main_runner.cc:252: {
Did that already in the following patch. Done.

https://codereview.chromium.org/23691025/diff/1/content/public/common/content...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/23691025/diff/1/content/public/common/content...
content/public/common/content_switches.cc:799: // flag results in the first call
to BeginTracing() to receive all trace events
On 2013/08/30 22:54:02, James Cook wrote:
> Is the part of the comment about BeginTracing() accurate?

Done.

https://codereview.chromium.org/23691025/diff/1/content/public/common/content...
content/public/common/content_switches.cc:805: // omitted the default will be
used "chrometrace.log" in the current directory.
Somehow .json for logfiles makes no sense. ".json" is a javascript exchange
format which makes every programmer think that it must have something to do with
with some Javascript program - bit surely not with a log file. Since the startup
default name is also .log - this should also remain this way.

https://codereview.chromium.org/23691025/diff/20001/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/23691025/diff/20001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:133:
base::debug::TraceLog::RECORD_UNTIL_FULL);
Good one. I haven't figured out yet how it can fail. Added a todo and will
address that later.
Note: This is a debug flag and even if the user would cancel, the buffer
capturing would stop once the buffer is full and the next time we would come
here it would start again.

Nevertheless - will ask sky about all possibilities of all OS'es since they can
vary here.

https://codereview.chromium.org/23691025/diff/20001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:135: TRACE_EVENT0("shutdown",
"AttemptUserExit");
We can add "system events" also - in a later CL as suggested.

https://codereview.chromium.org/23691025/diff/20001/content/browser/browser_m...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/23691025/diff/20001/content/browser/browser_m...
content/browser/browser_main_runner.cc:52: void WriteTracesToDisc(base::FilePath
file_name) {
As I understood it it will stop tracing when the buffer is full - but we cannot
do much about that state since there is no background writing... 
Since the fill levels I have seen were 0.00xx% I think that if it really gets
filled it is better to write out the content we have since it will show the
culprit.
Added a comment towards that.

https://codereview.chromium.org/23691025/diff/20001/content/public/common/con...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/23691025/diff/20001/content/public/common/con...
content/public/common/content_switches.h:230: extern const char
kTraceShutdown[];
On 2013/08/30 22:51:15, DaveMoore wrote:
> If you're accessing these from chrome (application_lifetime) they'll need to
be
> exported.

Done.

[Mr4D (OOO till 08-26), 2013-09-03 16:46:39]

Moved the shutdown trace start to a later point when no user intervention is
possible anymore.

Please have another look!

[DaveMoore, 2013-09-03 17:02:47]

https://codereview.chromium.org/23691025/diff/61001/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/23691025/diff/61001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:127: void AttemptUserExit() {
Why can't you always start shutdown tracing here?

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.cc (right):

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:75: blocks_++;
Nit: ++blocks_;

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:76:
WriteString(events_str->data());
What you're doing currently is taking the char* return from data() which may not
be terminated and calling WriteString() which creates a new std::string() out of
that data (potentially walking off the end) which then calls WriteChars() with
the char* data. 

Instead you should just call WriteString() with events_str.

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.h (right):

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:24: //
"--trace-shutdown-file=<name>" parameter - or - to "chrometrace.log".
What directory is chrometrace.log located in?

https://codereview.chromium.org/23691025/diff/61001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/23691025/diff/61001/content/content_browser.g...
content/content_browser.gypi:320: 'browser/browser_shutdown_profile_dumper.h',
Not alphabetized.

[Mr4D (OOO till 08-26), 2013-09-03 17:34:33]

Addressed (most of it).

Please have another look!

https://codereview.chromium.org/23691025/diff/61001/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/23691025/diff/61001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:127: void AttemptUserExit() {
As you have said yourself:
On other OS'ses then ChromeOS the user / running apps can stop the logging. If
there is a user interaction stopping the shutdown it does not make much sense to
trace that.

For ChromeOS on the other hand the state->CommitPendingWrite below might be of
interest.

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.cc (right):

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:75: blocks_++;
On 2013/09/03 17:02:48, DaveMoore wrote:
> Nit: ++blocks_;

Done.

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:76:
WriteString(events_str->data());
Actually... RefCountedString does return it's std::string - not a char*. As such
the length as well as the string should be properly handled.

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.h (right):

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:24: //
"--trace-shutdown-file=<name>" parameter - or - to "chrometrace.log".
Added a comment. But following the way as startup tracing has done it.

https://codereview.chromium.org/23691025/diff/61001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/23691025/diff/61001/content/content_browser.g...
content/content_browser.gypi:320: 'browser/browser_shutdown_profile_dumper.h',
On 2013/09/03 17:02:48, DaveMoore wrote:
> Not alphabetized.

Done.

[DaveMoore, 2013-09-03 20:24:10]

https://codereview.chromium.org/23691025/diff/61001/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/23691025/diff/61001/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:127: void AttemptUserExit() {
I would want that info in the trace, and then abort the write if the user ends
up not exiting.
On 2013/09/03 17:34:34, Mr4D wrote:
> As you have said yourself:
> On other OS'ses then ChromeOS the user / running apps can stop the logging. If
> there is a user interaction stopping the shutdown it does not make much sense
to
> trace that.
> 
> For ChromeOS on the other hand the state->CommitPendingWrite below might be of
> interest.

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.cc (right):

https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:76:
WriteString(events_str->data());
You are right.
On 2013/09/03 17:34:34, Mr4D wrote:
> Actually... RefCountedString does return it's std::string - not a char*. As
such
> the length as well as the string should be properly handled.

[DaveMoore, 2013-09-03 21:17:39]

On 2013/09/03 20:24:10, DaveMoore wrote:
>
https://codereview.chromium.org/23691025/diff/61001/chrome/browser/lifetime/a...
> File chrome/browser/lifetime/application_lifetime.cc (right):
> 
>
https://codereview.chromium.org/23691025/diff/61001/chrome/browser/lifetime/a...
> chrome/browser/lifetime/application_lifetime.cc:127: void AttemptUserExit() {
> I would want that info in the trace, and then abort the write if the user ends
> up not exiting.
> On 2013/09/03 17:34:34, Mr4D wrote:
> > As you have said yourself:
> > On other OS'ses then ChromeOS the user / running apps can stop the logging.
If
> > there is a user interaction stopping the shutdown it does not make much
sense
> to
> > trace that.
> > 
> > For ChromeOS on the other hand the state->CommitPendingWrite below might be
of
> > interest.
> 
>
https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
> File content/browser/browser_shutdown_profile_dumper.cc (right):
> 
>
https://codereview.chromium.org/23691025/diff/61001/content/browser/browser_s...
> content/browser/browser_shutdown_profile_dumper.cc:76:
> WriteString(events_str->data());
> You are right.
> On 2013/09/03 17:34:34, Mr4D wrote:
> > Actually... RefCountedString does return it's std::string - not a char*. As
> such
> > the length as well as the string should be properly handled.

Stefan feels that --trace-shutdown implies from the time we know we're shutting
down, not from the request. I don't really agree, but it appears to be pretty
fragile to catch the cases where something stops us from shutting down, so LGTM.

You'll need one from Nat as well though.

[James Cook, 2013-09-03 21:23:35]

lgtm with optional nits

https://codereview.chromium.org/23691025/diff/21011/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/23691025/diff/21011/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:160: void StartShutdownTracing()
{
optional nit: MaybeStartShutdownTracing()?

https://codereview.chromium.org/23691025/diff/21011/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:169: TRACE_EVENT0("shutdown",
"AttemptUserExit");
Are you sure you want to do this here, since sometimes this is called from
OnShutdownStarting?

[Mr4D (OOO till 08-26), 2013-09-03 22:18:58]

Addressed!

@Nduca - could you please do the owners review?

Thanks!

https://codereview.chromium.org/23691025/diff/21011/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/23691025/diff/21011/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:160: void StartShutdownTracing()
{
For what Chrome is concerned the tracing gets started after this call. The only
reason why it does not get started is that the user didn't want it or later on
someone cannot write the log.

Adding a "Maybe" here would require a "Maybe" in a lot of other places. As such
I don't do it. :)

https://codereview.chromium.org/23691025/diff/21011/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:169: TRACE_EVENT0("shutdown",
"AttemptUserExit");
Indeed - Done.

[nduca, 2013-09-04 23:01:30]

lgtm but please have wangxianzhu lg as well for compatibility with the new
thread local trace buffers.

[Xianzhu, 2013-09-04 23:21:11]

lgtm.

My CL changes TraceLog::Flush() to be async, so the new call site added in this
CL needs to be changed accordingly. However I've delayed CQ of my change and
will update the call site in my change after this change is submitted.

[commit-bot: I haz the power, 2013-09-04 23:38:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/23691025/91001

[Mr4D (OOO till 08-26), 2013-09-04 23:48:30]

Sky, can you do please an owners review?

Thanks!

[sky, 2013-09-05 00:00:13]

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_m...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_m...
content/browser/browser_main_runner.cc:137: DCHECK(initialization_started_);
I would actually keep these three above your changes since they are general
preconditions.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.cc (right):

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
13

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:32: VLOG(1) << "Flushing
shutdown traces to disc. The buffer is %" <<
DVLOG?

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:80: return dump_file_ &&
(ferror(dump_file_) == 0);
Do you really need to check ferror?

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:100: if (!IsFileValid())
Should this close even if ferror returns 0? Meaning maybe it should only check
dump_file_?

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.h (right):

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:26: // Use the class with a
scoped_ptr to automatically write the dump when the
Instead of this I think you want. Files is written to disk in the destructor.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:34: virtual
~BrowserShutdownProfileDumper();
no virtual.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:37: // Writes all traces which
happened to disc.
Isn't it disk?

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:38: void
WriteTracesToDisc(base::FilePath file_name);
const base::FilePath&

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:64: };
DISALLOW_...

[Mr4D (OOO till 08-26), 2013-09-05 00:13:40]

Addressed! Please have another look!

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_m...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_m...
content/browser/browser_main_runner.cc:137: DCHECK(initialization_started_);
On 2013/09/05 00:00:14, sky wrote:
> I would actually keep these three above your changes since they are general
> preconditions.

Done.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.cc (right):

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/09/05 00:00:14, sky wrote:
> 13

Done.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:32: VLOG(1) << "Flushing
shutdown traces to disc. The buffer is %" <<
On 2013/09/05 00:00:14, sky wrote:
> DVLOG?

Done.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:80: return dump_file_ &&
(ferror(dump_file_) == 0);
Yes - even if it is only for the file opening.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:100: if (!IsFileValid())
That is a good one! Done.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.h (right):

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:26: // Use the class with a
scoped_ptr to automatically write the dump when the
On 2013/09/05 00:00:14, sky wrote:
> Instead of this I think you want. Files is written to disk in the destructor.

Done.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:34: virtual
~BrowserShutdownProfileDumper();
On 2013/09/05 00:00:14, sky wrote:
> no virtual.

Done.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:37: // Writes all traces which
happened to disc.
On 2013/09/05 00:00:14, sky wrote:
> Isn't it disk?

Done.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:38: void
WriteTracesToDisc(base::FilePath file_name);
On 2013/09/05 00:00:14, sky wrote:
> const base::FilePath&

Done.

https://codereview.chromium.org/23691025/diff/91001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.h:64: };
On 2013/09/05 00:00:14, sky wrote:
> DISALLOW_...

Done.

[sky, 2013-09-05 15:45:13]

LGTM

[commit-bot: I haz the power, 2013-09-05 15:53:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/23691025/53001

[commit-bot: I haz the power, 2013-09-05 16:06:17]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Mr4D (OOO till 08-26), 2013-09-05 16:12:49]

@piman - could you please do an owners review?

Many thanks!

[piman, 2013-09-05 17:28:39]

LGTM+nit

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.cc (right):

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:20: dump_file_(0) {
nit: NULL

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:31: // what we have then
nothing since we might see from the amount of events
nit: s/then/than/

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:32: // who caused the
problem.
nit: s/who/that/
Don't personify events, they hate that :)

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:104: dump_file_ = 0;
nit: NULL

[Mr4D (OOO till 08-26), 2013-09-05 18:55:17]

Addressed! Thanks!

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
File content/browser/browser_shutdown_profile_dumper.cc (right):

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:20: dump_file_(0) {
On 2013/09/05 17:28:40, piman wrote:
> nit: NULL

Done.

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:31: // what we have then
nothing since we might see from the amount of events
On 2013/09/05 17:28:40, piman wrote:
> nit: s/then/than/

Done.

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:32: // who caused the
problem.
On 2013/09/05 17:28:40, piman wrote:
> nit: s/who/that/
> Don't personify events, they hate that :)

Done.

https://codereview.chromium.org/23691025/diff/53001/content/browser/browser_s...
content/browser/browser_shutdown_profile_dumper.cc:104: dump_file_ = 0;
On 2013/09/05 17:28:40, piman wrote:
> nit: NULL

Done.

[commit-bot: I haz the power, 2013-09-05 18:57:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/23691025/128001

[commit-bot: I haz the power, 2013-09-06 07:47:09]

Change committed as 221631