Enable UMA reporting for crashpad_handler on macOS

Enable UMA reporting for crashpad_handler on macOS

This should have been working already, but a functioning
crash_reporter::CrashReporterClient::GetCrashMetricsLocation() for macOS
was unintentionally absent.

BUG=crashpad:100, chromium:678720
Review-Url: https://codereview.chromium.org/2610883006
Cr-Commit-Position: refs/heads/master@{#442888}
Committed: https://chromium.googlesource.com/chromium/src/+/c8e4f85cf19acdf6e1e61aa3ed0cde6a06a84487

[Mark Mentovai, 2017-01-05 22:20:29]

The CQ bit was checked by mark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 22:21:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark Mentovai, 2017-01-05 22:26:49]

mark@chromium.org changed reviewers:
+ scottmg@chromium.org

[Mark Mentovai, 2017-01-05 22:26:49]

This seems to be working in that when I start crashpad_handler manually like
this

crashpad_handler --mach-service=blah --database=/tmp/crashpad_database
--metrics-dir=/tmp

I see /tmp/CrashpadMetrics-active.pma show up as expected.

When I run Chrome now, I do see crashpad_handler being started with
--metrics-dir=(user_data_dir), but I don’t see
(user_data_dir)/CrashpadMetrics-active.pma. So I’m taking that to mean that
crashpad_handler has created it but the browser found and deleted it. Is that
right? Should it be gone? Or does this mean that something’s losing a race and
we still won’t get UMA reports?

This was months ago, I don’t remember the details of the shared memory design
and what the expected lifetimes of these files should be.

[scottmg, 2017-01-05 22:41:36]

On 2017/01/05 22:26:49, Mark Mentovai wrote:
> This seems to be working in that when I start crashpad_handler manually like
> this
> 
> crashpad_handler --mach-service=blah --database=/tmp/crashpad_database
> --metrics-dir=/tmp
> 
> I see /tmp/CrashpadMetrics-active.pma show up as expected.
> 
> When I run Chrome now, I do see crashpad_handler being started with
> --metrics-dir=(user_data_dir), but I don’t see
> (user_data_dir)/CrashpadMetrics-active.pma. So I’m taking that to mean that
> crashpad_handler has created it but the browser found and deleted it. Is that
> right? Should it be gone? Or does this mean that something’s losing a race and
> we still won’t get UMA reports?
> 
> This was months ago, I don’t remember the details of the shared memory design
> and what the expected lifetimes of these files should be.

I have a CrashpadMetrics-active.pma while the browser is running on Windows
which then gets removed on shutdown. Lemme look at the code again a bit, I kind
of just did what Brian said to do.

[scottmg, 2017-01-05 22:41:40]

https://codereview.chromium.org/2610883006/diff/1/chrome/app/chrome_crash_rep...
File chrome/app/chrome_crash_reporter_client_mac.mm (right):

https://codereview.chromium.org/2610883006/diff/1/chrome/app/chrome_crash_rep...
chrome/app/chrome_crash_reporter_client_mac.mm:24: #if
defined(GOOGLE_CHROME_BUILD)
The #if feels like unnecessary clutter here.

[scottmg, 2017-01-05 22:42:31]

Either way, this seems better than the current state, so lgtm.

[Mark Mentovai, 2017-01-05 22:44:34]

On 2017/01/05 22:41:40, scottmg wrote:
>
https://codereview.chromium.org/2610883006/diff/1/chrome/app/chrome_crash_rep...
> File chrome/app/chrome_crash_reporter_client_mac.mm (right):
> 
>
https://codereview.chromium.org/2610883006/diff/1/chrome/app/chrome_crash_rep...
> chrome/app/chrome_crash_reporter_client_mac.mm:24: #if
> defined(GOOGLE_CHROME_BUILD)
> The #if feels like unnecessary clutter here.

What’s the sense in littering Chromium builds with metrics files? I didn’t think
that Chromium ran metrics, so the pma files would just collect dust in the
user-data-dir.

I’ll take it out if we’ve got that detritus on Windows Chromium builds, though.
Consistency!

[Mark Mentovai, 2017-01-05 22:47:55]

The CQ bit was checked by mark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 22:48:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2017-01-05 22:48:42]

On 2017/01/05 22:41:36, scottmg wrote:
> On 2017/01/05 22:26:49, Mark Mentovai wrote:
> > This seems to be working in that when I start crashpad_handler manually like
> > this
> > 
> > crashpad_handler --mach-service=blah --database=/tmp/crashpad_database
> > --metrics-dir=/tmp
> > 
> > I see /tmp/CrashpadMetrics-active.pma show up as expected.
> > 
> > When I run Chrome now, I do see crashpad_handler being started with
> > --metrics-dir=(user_data_dir), but I don’t see
> > (user_data_dir)/CrashpadMetrics-active.pma. So I’m taking that to mean that
> > crashpad_handler has created it but the browser found and deleted it. Is
that
> > right? Should it be gone? Or does this mean that something’s losing a race
and
> > we still won’t get UMA reports?
> > 
> > This was months ago, I don’t remember the details of the shared memory
design
> > and what the expected lifetimes of these files should be.
> 
> I have a CrashpadMetrics-active.pma while the browser is running on Windows
> which then gets removed on shutdown. Lemme look at the code again a bit, I
kind
> of just did what Brian said to do.

I think the .pma should stick around for the lifetime of crashpad_handler so
stuff can get written to it.

The startup procedure moves -active to unsuffixed, and then that gets slurped by
the browser and deleted.

Is it possible that you're hitting the !metrics_reporting_enabled path? I think
that causes it to delete the .pma enthusiastically to make sure nothing gets
reported incorrectly.

[Mark Mentovai, 2017-01-05 22:49:11]

In Chromium, I get CrashpadMetrics-active.pma while the browser is running, and
it disappears when I quit, so it seems like that’s working, and cruft doesn’t
collect. Good, I’ll get rid of the branding check.

But in branded Google Chrome (where it counts!), I don’t see the .pma file where
I’m expecting it while the browser is running. That’s concerning…

[Mark Mentovai, 2017-01-05 22:49:54]

On 2017/01/05 22:48:42, scottmg wrote:
> On 2017/01/05 22:41:36, scottmg wrote:
> > On 2017/01/05 22:26:49, Mark Mentovai wrote:
> > > This seems to be working in that when I start crashpad_handler manually
like
> > > this
> > > 
> > > crashpad_handler --mach-service=blah --database=/tmp/crashpad_database
> > > --metrics-dir=/tmp
> > > 
> > > I see /tmp/CrashpadMetrics-active.pma show up as expected.
> > > 
> > > When I run Chrome now, I do see crashpad_handler being started with
> > > --metrics-dir=(user_data_dir), but I don’t see
> > > (user_data_dir)/CrashpadMetrics-active.pma. So I’m taking that to mean
that
> > > crashpad_handler has created it but the browser found and deleted it. Is
> that
> > > right? Should it be gone? Or does this mean that something’s losing a race
> and
> > > we still won’t get UMA reports?
> > > 
> > > This was months ago, I don’t remember the details of the shared memory
> design
> > > and what the expected lifetimes of these files should be.
> > 
> > I have a CrashpadMetrics-active.pma while the browser is running on Windows
> > which then gets removed on shutdown. Lemme look at the code again a bit, I
> kind
> > of just did what Brian said to do.
> 
> I think the .pma should stick around for the lifetime of crashpad_handler so
> stuff can get written to it.
> 
> The startup procedure moves -active to unsuffixed, and then that gets slurped
by
> the browser and deleted.
> 
> Is it possible that you're hitting the !metrics_reporting_enabled path? I
think
> that causes it to delete the .pma enthusiastically to make sure nothing gets
> reported incorrectly.

Metrics are definitely on. As to which path I’m hitting…let’s see…

[scottmg, 2017-01-05 22:50:26]

On 2017/01/05 22:44:34, Mark Mentovai wrote:
> On 2017/01/05 22:41:40, scottmg wrote:
> >
>
https://codereview.chromium.org/2610883006/diff/1/chrome/app/chrome_crash_rep...
> > File chrome/app/chrome_crash_reporter_client_mac.mm (right):
> > 
> >
>
https://codereview.chromium.org/2610883006/diff/1/chrome/app/chrome_crash_rep...
> > chrome/app/chrome_crash_reporter_client_mac.mm:24: #if
> > defined(GOOGLE_CHROME_BUILD)
> > The #if feels like unnecessary clutter here.
> 
> What’s the sense in littering Chromium builds with metrics files? I didn’t
think
> that Chromium ran metrics, so the pma files would just collect dust in the
> user-data-dir.
> 
> I’ll take it out if we’ve got that detritus on Windows Chromium builds,
though.
> Consistency!

Yeah, I was mostly concerned about consistency since it's not in the _win
version. I haven't done a non-CHROME build in a while tbh and don't have one
handy.

... OK, how about adding the same #if to the _win version instead then?

[scottmg, 2017-01-05 22:52:12]

On 2017/01/05 22:50:26, scottmg wrote:
> On 2017/01/05 22:44:34, Mark Mentovai wrote:
> > On 2017/01/05 22:41:40, scottmg wrote:
> > >
> >
>
https://codereview.chromium.org/2610883006/diff/1/chrome/app/chrome_crash_rep...
> > > File chrome/app/chrome_crash_reporter_client_mac.mm (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2610883006/diff/1/chrome/app/chrome_crash_rep...
> > > chrome/app/chrome_crash_reporter_client_mac.mm:24: #if
> > > defined(GOOGLE_CHROME_BUILD)
> > > The #if feels like unnecessary clutter here.
> > 
> > What’s the sense in littering Chromium builds with metrics files? I didn’t
> think
> > that Chromium ran metrics, so the pma files would just collect dust in the
> > user-data-dir.
> > 
> > I’ll take it out if we’ve got that detritus on Windows Chromium builds,
> though.
> > Consistency!
> 
> Yeah, I was mostly concerned about consistency since it's not in the _win
> version. I haven't done a non-CHROME build in a while tbh and don't have one
> handy.
> 
> ... OK, how about adding the same #if to the _win version instead then?

(I guess the general confusion about different behaviour in Chrome vs. Chromium
is the best reason for not having the #if, it's one more thing to confuse our
future selves.)

[Mark Mentovai, 2017-01-05 22:53:20]

On 2017/01/05 22:49:54, Mark Mentovai wrote:
> On 2017/01/05 22:48:42, scottmg wrote:
> > On 2017/01/05 22:41:36, scottmg wrote:
> > > On 2017/01/05 22:26:49, Mark Mentovai wrote:
> > > > This seems to be working in that when I start crashpad_handler manually
> like
> > > > this
> > > > 
> > > > crashpad_handler --mach-service=blah --database=/tmp/crashpad_database
> > > > --metrics-dir=/tmp
> > > > 
> > > > I see /tmp/CrashpadMetrics-active.pma show up as expected.
> > > > 
> > > > When I run Chrome now, I do see crashpad_handler being started with
> > > > --metrics-dir=(user_data_dir), but I don’t see
> > > > (user_data_dir)/CrashpadMetrics-active.pma. So I’m taking that to mean
> that
> > > > crashpad_handler has created it but the browser found and deleted it. Is
> > that
> > > > right? Should it be gone? Or does this mean that something’s losing a
race
> > and
> > > > we still won’t get UMA reports?
> > > > 
> > > > This was months ago, I don’t remember the details of the shared memory
> > design
> > > > and what the expected lifetimes of these files should be.
> > > 
> > > I have a CrashpadMetrics-active.pma while the browser is running on
Windows
> > > which then gets removed on shutdown. Lemme look at the code again a bit, I
> > kind
> > > of just did what Brian said to do.
> > 
> > I think the .pma should stick around for the lifetime of crashpad_handler so
> > stuff can get written to it.
> > 
> > The startup procedure moves -active to unsuffixed, and then that gets
slurped
> by
> > the browser and deleted.
> > 
> > Is it possible that you're hitting the !metrics_reporting_enabled path? I
> think
> > that causes it to delete the .pma enthusiastically to make sure nothing gets
> > reported incorrectly.
> 
> Metrics are definitely on. As to which path I’m hitting…let’s see…

Weird. When I turn ON “Automatically send usage statistics and crash reports to
Google”, the .pma file disappears. When it’s OFF, the .pma file sticks around
for the life of the browser process, just like un-branded Chromium where
reporting is always off.

Say, do YOU have reporting on?

[scottmg, 2017-01-05 22:56:44]

On 2017/01/05 22:53:20, Mark Mentovai wrote:
> On 2017/01/05 22:49:54, Mark Mentovai wrote:
> > On 2017/01/05 22:48:42, scottmg wrote:
> > > On 2017/01/05 22:41:36, scottmg wrote:
> > > > On 2017/01/05 22:26:49, Mark Mentovai wrote:
> > > > > This seems to be working in that when I start crashpad_handler
manually
> > like
> > > > > this
> > > > > 
> > > > > crashpad_handler --mach-service=blah --database=/tmp/crashpad_database
> > > > > --metrics-dir=/tmp
> > > > > 
> > > > > I see /tmp/CrashpadMetrics-active.pma show up as expected.
> > > > > 
> > > > > When I run Chrome now, I do see crashpad_handler being started with
> > > > > --metrics-dir=(user_data_dir), but I don’t see
> > > > > (user_data_dir)/CrashpadMetrics-active.pma. So I’m taking that to mean
> > that
> > > > > crashpad_handler has created it but the browser found and deleted it.
Is
> > > that
> > > > > right? Should it be gone? Or does this mean that something’s losing a
> race
> > > and
> > > > > we still won’t get UMA reports?
> > > > > 
> > > > > This was months ago, I don’t remember the details of the shared memory
> > > design
> > > > > and what the expected lifetimes of these files should be.
> > > > 
> > > > I have a CrashpadMetrics-active.pma while the browser is running on
> Windows
> > > > which then gets removed on shutdown. Lemme look at the code again a bit,
I
> > > kind
> > > > of just did what Brian said to do.
> > > 
> > > I think the .pma should stick around for the lifetime of crashpad_handler
so
> > > stuff can get written to it.
> > > 
> > > The startup procedure moves -active to unsuffixed, and then that gets
> slurped
> > by
> > > the browser and deleted.
> > > 
> > > Is it possible that you're hitting the !metrics_reporting_enabled path? I
> > think
> > > that causes it to delete the .pma enthusiastically to make sure nothing
gets
> > > reported incorrectly.
> > 
> > Metrics are definitely on. As to which path I’m hitting…let’s see…
> 
> Weird. When I turn ON “Automatically send usage statistics and crash reports
to
> Google”, the .pma file disappears. When it’s OFF, the .pma file sticks around
> for the life of the browser process, just like un-branded Chromium where
> reporting is always off.
> 
> Say, do YOU have reporting on?

Good question! But yes, I seem to. Canary -- chrome://crashes thinks so,
chrome://settings thinks so, and crashpad_database_util --database
"C:\Users\scott\AppData\Local\Google\Chrome SxS\User Data\Crashpad"
--show-uploads-enabled thinks so too.

[commit-bot: I haz the power, 2017-01-05 23:53:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 23:53:20]

Dry run: This issue passed the CQ dry run.

[Mark Mentovai, 2017-01-06 15:13:59]

Brian, what do you think about this?

I’m seeing the Chrome browser start crashpad_handler, then crashpad_handler
initializes a GlogalHistogramAllocator which creates CrashpadMetrics-active.pma,
and shortly thereafter, the Chrome browser removes this newly created file. It
looks like it slurps up whatever’s been written to it by that point, which isn’t
much because everything’s still starting up, and won’t ever read anything from
that file again because it’s been deleted. All of the Crashpad metrics written
to that file from that point on are lost.

Is my understanding correct? I don’t actually know if this is working or not,
but I know that Scott says that he sees CrashpadMetrics-active.pma while Chrome
is running and reporting is on, and I’m not seeing it on macOS.

Is there some ordering expectation that we’re not meeting, therefore we wind up
hitting this race?

Is it possible that we’re subject to this same race on Windows too, and that
we’re under-reporting Crashpad metrics via UMA?

Here’s the backtrace in Chrome where I see CrashpadMetrics-active.pma being
deleted during browser startup:

(lldb) p (char*)$rdi
(char *) $1 = 0x000000010e727970 "/Users/mark/Library/Application
Support/Google/Chrome/CrashpadMetrics-active.pma"
(lldb) bt
* thread #27: tid = 0x42a6c4, 0x00007fff9050a9be libsystem_kernel.dylib`unlink,
name = 'BrowserBlockingWorker1/51203', stop reason = breakpoint 1.3
  * frame #0: 0x00007fff9050a9be libsystem_kernel.dylib`unlink
    frame #1: 0x0000000102990aae Google Chrome
Framework`base::DeleteFile(path=0x000000010e727920, recursive=false) + 206 at
file_util_posix.cc:214 [opt]
    frame #2: 0x0000000104013a99 Google Chrome
Framework`metrics::FileMetricsProvider::CheckAndMergeMetricSourcesOnTaskRunner(sources=0x0000000100624490
size=0) + 201 at file_metrics_provider.cc:272 [opt]
    frame #3: 0x00000001029f1968 Google Chrome Framework`base::(anonymous
namespace)::PostTaskAndReplyRelay::RunTaskAndPostReply() [inlined]
base::internal::RunMixin<base::Callback<void (), (base::internal::CopyMode)1,
(base::internal::RepeatMode)1> >::Run() const + 24 at callback.h:85 [opt]
    frame #4: 0x00000001029f1962 Google Chrome Framework`base::(anonymous
namespace)::PostTaskAndReplyRelay::RunTaskAndPostReply(this=0x0000000100624600)
+ 18 at post_task_and_reply_impl.cc:46 [opt]
    frame #5: 0x00000001029f2e17 Google Chrome
Framework`base::SequencedWorkerPool::Inner::ThreadLoop(base::SequencedWorkerPool::Worker*)
[inlined] base::internal::RunMixin<base::Callback<void (),
(base::internal::CopyMode)1, (base::internal::RepeatMode)1> >::Run() const +
3287 at callback.h:85 [opt]
    frame #6: 0x00000001029f2e0e Google Chrome
Framework`base::SequencedWorkerPool::Inner::ThreadLoop(this=0x00000001003125e0,
this_worker=0x000000010e725a70) + 3278 at sequenced_worker_pool.cc:1021 [opt]
    frame #7: 0x00000001029f20f6 Google Chrome
Framework`base::SequencedWorkerPool::Worker::Run(this=0x000000010e725a70) + 134
at sequenced_worker_pool.cc:611 [opt]
    frame #8: 0x00000001029f5b10 Google Chrome
Framework`base::SimpleThread::ThreadMain(this=0x000000010e725a70) + 128 at
simple_thread.cc:68 [opt]
    frame #9: 0x00000001029f17c7 Google Chrome Framework`base::(anonymous
namespace)::ThreadFunc(params=<unavailable>) + 87 at platform_thread_posix.cc:71
[opt]
    frame #10: 0x00007fff905f5aab libsystem_pthread.dylib`_pthread_body + 180
    frame #11: 0x00007fff905f59f7 libsystem_pthread.dylib`_pthread_start + 286
    frame #12: 0x00007fff905f51fd libsystem_pthread.dylib`thread_start + 13

[bcwhite, 2017-01-06 16:12:03]

> I’m seeing the Chrome browser start crashpad_handler, then crashpad_handler
> initializes a GlogalHistogramAllocator which creates
CrashpadMetrics-active.pma,
> and shortly thereafter, the Chrome browser removes this newly created file. It
> looks like it slurps up whatever’s been written to it by that point, which
isn’t
> much because everything’s still starting up, and won’t ever read anything from
> that file again because it’s been deleted. All of the Crashpad metrics written
> to that file from that point on are lost.

I'm assuming that CrashPad would be expected to save things after a crash and so
should operate like this:

During startup, an existing -active file should be renamed to not have that
qualifier.  A new -active file is then created for the current run and
histograms get stored there.  That is the case:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/handler/ha...

Chrome should then slurp and delete the base file (not the active one) and
upload the metrics from that as "stability metrics" which get associated with
the previous run. That is also the case:
https://cs.chromium.org/chromium/src/chrome/browser/metrics/chrome_metrics_se...

HOWEVER, directly after that it also registers the active_file for immediate
collect-and-delete. Lines 216-227 should be removed. It looks like they got left
behind after some refactoring of the CL (something I should have caught but did
not).

Alternatively, if useful, those lines can use SOURCE_HISTOGRAMS_ACTIVE_FILE
which will continuously get any new values and report them during the run
leaving only final changes to be reported as "stability metrics" during the next
run.

[Mark Mentovai, 2017-01-06 16:39:33]

On 2017/01/06 16:12:03, bcwhite wrote:
> > I’m seeing the Chrome browser start crashpad_handler, then crashpad_handler
> > initializes a GlogalHistogramAllocator which creates
> CrashpadMetrics-active.pma,
> > and shortly thereafter, the Chrome browser removes this newly created file.
It
> > looks like it slurps up whatever’s been written to it by that point, which
> isn’t
> > much because everything’s still starting up, and won’t ever read anything
from
> > that file again because it’s been deleted. All of the Crashpad metrics
written
> > to that file from that point on are lost.
> 
> I'm assuming that CrashPad would be expected to save things after a crash and
so
> should operate like this:
> 
> During startup, an existing -active file should be renamed to not have that
> qualifier.  A new -active file is then created for the current run and
> histograms get stored there.  That is the case:
>
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/handler/ha...
> 
> Chrome should then slurp and delete the base file (not the active one) and
> upload the metrics from that as "stability metrics" which get associated with
> the previous run. That is also the case:
>
https://cs.chromium.org/chromium/src/chrome/browser/metrics/chrome_metrics_se...
> 
> HOWEVER, directly after that it also registers the active_file for immediate
> collect-and-delete. Lines 216-227 should be removed. It looks like they got
left
> behind after some refactoring of the CL (something I should have caught but
did
> not).
> 
> Alternatively, if useful, those lines can use SOURCE_HISTOGRAMS_ACTIVE_FILE
> which will continuously get any new values and report them during the run
> leaving only final changes to be reported as "stability metrics" during the
next
> run.

Thanks. I had found SOURCE_HISTOGRAMS_ACTIVE_FILE and was wondering why we
weren’t using it. I can switch to that, it’ll eliminate some of the collection
lag.

But I’m also concerned that we’re seeing a different behavior between Mac and
Windows. Scott reports that CrashpadMetrics-active.pma is present in his
user-data-dir even with CreateFileMetricsProvider() using
SOURCE_HISTOGRAMS_ATOMIC_FILE as currently checked-in. That implies to me that
CreateFileMetricsProvider() isn’t actually picking that file up, otherwise it’d
delete it. So even switching to SOURCE_HISTOGRAMS_ACTIVE_FILE, we wouldn’t
really get live reporting if the file isn’t there.

[Mark Mentovai, 2017-01-06 16:59:25]

Description was changed from

==========
Enable UMA reporting for crashpad_handler on macOS

This should have been working already, but a functioning
crash_reporter::CrashReporterClient::GetCrashMetricsLocation() for macOS
was unintentionally absent.

BUG=crashpad:100,chromium:678720
==========

to

==========
Enable UMA reporting for crashpad_handler on macOS

This should have been working already, but a functioning
crash_reporter::CrashReporterClient::GetCrashMetricsLocation() for macOS
was unintentionally absent.

BUG=crashpad:100,chromium:678720
==========

[Mark Mentovai, 2017-01-06 16:59:25]

mark@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Mark Mentovai, 2017-01-06 17:00:43]

+Robert for metrics OWNERS.

Robert, do you think that this is right? I’m not totally convinced given some of
the behavior differences noted above.

[Mark Mentovai, 2017-01-06 17:01:30]

The CQ bit was checked by mark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 17:02:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2017-01-06 17:35:49]

On 2017/01/06 16:39:33, Mark Mentovai wrote:
> On 2017/01/06 16:12:03, bcwhite wrote:
> > > I’m seeing the Chrome browser start crashpad_handler, then
crashpad_handler
> > > initializes a GlogalHistogramAllocator which creates
> > CrashpadMetrics-active.pma,
> > > and shortly thereafter, the Chrome browser removes this newly created
file.
> It
> > > looks like it slurps up whatever’s been written to it by that point, which
> > isn’t
> > > much because everything’s still starting up, and won’t ever read anything
> from
> > > that file again because it’s been deleted. All of the Crashpad metrics
> written
> > > to that file from that point on are lost.
> > 
> > I'm assuming that CrashPad would be expected to save things after a crash
and
> so
> > should operate like this:
> > 
> > During startup, an existing -active file should be renamed to not have that
> > qualifier.  A new -active file is then created for the current run and
> > histograms get stored there.  That is the case:
> >
>
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/handler/ha...
> > 
> > Chrome should then slurp and delete the base file (not the active one) and
> > upload the metrics from that as "stability metrics" which get associated
with
> > the previous run. That is also the case:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/metrics/chrome_metrics_se...
> > 
> > HOWEVER, directly after that it also registers the active_file for immediate
> > collect-and-delete. Lines 216-227 should be removed. It looks like they got
> left
> > behind after some refactoring of the CL (something I should have caught but
> did
> > not).
> > 
> > Alternatively, if useful, those lines can use SOURCE_HISTOGRAMS_ACTIVE_FILE
> > which will continuously get any new values and report them during the run
> > leaving only final changes to be reported as "stability metrics" during the
> next
> > run.
> 
> Thanks. I had found SOURCE_HISTOGRAMS_ACTIVE_FILE and was wondering why we
> weren’t using it. I can switch to that, it’ll eliminate some of the collection
> lag.
> 
> But I’m also concerned that we’re seeing a different behavior between Mac and
> Windows. Scott reports that CrashpadMetrics-active.pma is present in his
> user-data-dir even with CreateFileMetricsProvider() using
> SOURCE_HISTOGRAMS_ATOMIC_FILE as currently checked-in. That implies to me that
> CreateFileMetricsProvider() isn’t actually picking that file up, otherwise
it’d
> delete it. So even switching to SOURCE_HISTOGRAMS_ACTIVE_FILE, we wouldn’t
> really get live reporting if the file isn’t there.

I'll do some more debugging on Windows to find out what's going on. Could be
that the delete fails on Windows because of file locking so it works by
accident?

[commit-bot: I haz the power, 2017-01-06 18:48:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 18:48:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rkaplow, 2017-01-10 19:25:19]

rkaplow@chromium.org changed reviewers:
+ asvitkine@chromium.org

[rkaplow, 2017-01-10 19:25:20]

lgtm

Sorry, I'm not sure what may be causing the differences in how the handler is
treating windows/mac differently. Maybe alexei knows more. May be worth filing a
bug 

For the change, LG since brian suggested it

[Mark Mentovai, 2017-01-10 19:31:37]

OK, we’ll go with this for now and see what happens.

[Mark Mentovai, 2017-01-10 19:31:42]

The CQ bit was checked by mark@chromium.org

[Mark Mentovai, 2017-01-10 19:31:43]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2610883006/#ps40001
(title: "Use SOURCE_HISTOGRAMS_ACTIVE_FILE")

[commit-bot: I haz the power, 2017-01-10 19:32:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 20:57:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 20:57:45]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Alexei Svitkine (slow), 2017-01-10 21:58:10]

asvitkine@chromium.org changed reviewers:
- asvitkine@chromium.org

[Mark Mentovai, 2017-01-10 22:01:30]

The CQ bit was checked by mark@chromium.org

[commit-bot: I haz the power, 2017-01-10 22:02:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 23:29:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 23:29:52]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Mark Mentovai, 2017-01-10 23:31:36]

The CQ bit was checked by mark@chromium.org

[commit-bot: I haz the power, 2017-01-10 23:33:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 00:52:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 00:53:01]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Mark Mentovai, 2017-01-11 03:01:50]

The CQ bit was checked by mark@chromium.org

[commit-bot: I haz the power, 2017-01-11 03:02:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 04:32:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 04:32:49]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Mark Mentovai, 2017-01-11 12:55:14]

The CQ bit was checked by mark@chromium.org

[commit-bot: I haz the power, 2017-01-11 12:55:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 13:21:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484139314765810,
"parent_rev": "5b86dfe66171ac6b9641ea596c74369e515de917", "commit_rev":
"c8e4f85cf19acdf6e1e61aa3ed0cde6a06a84487"}

[commit-bot: I haz the power, 2017-01-11 13:22:21]

Description was changed from

==========
Enable UMA reporting for crashpad_handler on macOS

This should have been working already, but a functioning
crash_reporter::CrashReporterClient::GetCrashMetricsLocation() for macOS
was unintentionally absent.

BUG=crashpad:100,chromium:678720
==========

to

==========
Enable UMA reporting for crashpad_handler on macOS

This should have been working already, but a functioning
crash_reporter::CrashReporterClient::GetCrashMetricsLocation() for macOS
was unintentionally absent.

BUG=crashpad:100,chromium:678720

Review-Url: https://codereview.chromium.org/2610883006
Cr-Commit-Position: refs/heads/master@{#442888}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8e4f85cf19acdf6e1e61aa3ed0c...
==========

[commit-bot: I haz the power, 2017-01-11 13:22:23]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c8e4f85cf19acdf6e1e61aa3ed0c...