Use the correct product/version for postmortem reports

Use the correct product/version for postmortem reports

BUG=620813
Committed: https://crrev.com/8014d5ff93657cc93ada6a91762d2dea1943ac74
Cr-Commit-Position: refs/heads/master@{#422311}

[manzagop (departed), 2016-09-26 15:13:58]

Patchset #1 (id:1) has been deleted

[manzagop (departed), 2016-09-26 15:18:50]

manzagop@chromium.org changed reviewers:
+ grt@chromium.org

[manzagop (departed), 2016-09-26 15:18:51]

Hi grt!
I've switched from using version_info's product/version to those found in the
executable.
Additionally, I've done the minimal change to avoid repeating the underlying
call as, per discussion, it should be avoided (for consistency and stability).
Please have a look!
Thanks,
Pierre

[grt (UTC plus 2), 2016-09-26 18:59:06]

https://codereview.chromium.org/2372633002/diff/20001/chrome/app/chrome_crash...
File chrome/app/chrome_crash_reporter_client_win.cc (right):

https://codereview.chromium.org/2372633002/diff/20001/chrome/app/chrome_crash...
chrome/app/chrome_crash_reporter_client_win.cc:269: if (exe_path_ != exe_path)
don't DCHECK and then handle the DCHECKed case as per
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md....
either DCHECK and move on, or CHECK. i propose adding DCHECK(exe_path_.empty()
|| exe_path_ == exe_path); after line 265 and getting rid of this defensive
block.

https://codereview.chromium.org/2372633002/diff/20001/chrome/app/chrome_crash...
File chrome/app/chrome_crash_reporter_client_win.h (right):

https://codereview.chromium.org/2372633002/diff/20001/chrome/app/chrome_crash...
chrome/app/chrome_crash_reporter_client_win.h:56: // Cached to ensure
consistency across calls and because the functions that
while i think this caching is ultimately the right thing, i'm not sure it's
needed right now. this choice means that you must get the one true CCRC for the
process in your own code. as per my comment in the other file, i'm not sure you
can. without trying to solve this problem here, you could call install_static
directly. it may be the case that the magic install_static function will
automagically do the once-per-process thing. since crash isn't the only consumer
of these values, i think this is the wrong place to do the caching anyway.

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
File components/browser_watcher/postmortem_minidump_writer_win.cc (right):

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:200: {"product",
minidump_info.product_name + "_Postmortem"},
shouldn't this be "prod"
(https://cs.chromium.org/chromium/src/components/crash/content/app/crash_keys_...

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:201: {"version",
minidump_info.version_number},
"ver"
(https://cs.chromium.org/chromium/src/components/crash/content/app/crash_keys_...

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
File components/browser_watcher/postmortem_report_collector.cc (right):

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:193:
CrashReporterClient* reporter_client = GetCrashReporterClient();
does this compile? GetCrashReporterClient is within a CRASH_IMPLEMENTATION
block, so i thought it was unreachable by client code.

[manzagop (departed), 2016-09-27 19:09:51]

Patchset #2 (id:40001) has been deleted

[manzagop (departed), 2016-09-27 19:37:52]

I switched things around... Please have another look!

https://codereview.chromium.org/2372633002/diff/20001/chrome/app/chrome_crash...
File chrome/app/chrome_crash_reporter_client_win.cc (right):

https://codereview.chromium.org/2372633002/diff/20001/chrome/app/chrome_crash...
chrome/app/chrome_crash_reporter_client_win.cc:269: if (exe_path_ != exe_path)
On 2016/09/26 18:59:06, grt (UTC plus 2) wrote:
> don't DCHECK and then handle the DCHECKed case as per
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md....
> either DCHECK and move on, or CHECK. i propose adding DCHECK(exe_path_.empty()
> || exe_path_ == exe_path); after line 265 and getting rid of this defensive
> block.

Thanks for the pointer!

Due to the changes, comment no longer applies. Ack.

https://codereview.chromium.org/2372633002/diff/20001/chrome/app/chrome_crash...
File chrome/app/chrome_crash_reporter_client_win.h (right):

https://codereview.chromium.org/2372633002/diff/20001/chrome/app/chrome_crash...
chrome/app/chrome_crash_reporter_client_win.h:56: // Cached to ensure
consistency across calls and because the functions that
I've made the switch to directly call install static. Because that's in chrome/,
I couldn't do so in the component and I ended up writing a derived collector in
chrome/chrome_watcher that implements some pure functions to get the details.

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
File components/browser_watcher/postmortem_minidump_writer_win.cc (right):

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:200: {"product",
minidump_info.product_name + "_Postmortem"},
On 2016/09/26 18:59:06, grt (UTC plus 2) wrote:
> shouldn't this be "prod"
>
(https://cs.chromium.org/chromium/src/components/crash/content/app/crash_keys_...

IIUC those are the deprecated keys, but it's probably better to be consistent
across chrome. Done.

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
components/browser_watcher/postmortem_minidump_writer_win.cc:201: {"version",
minidump_info.version_number},
On 2016/09/26 18:59:06, grt (UTC plus 2) wrote:
> "ver"
>
(https://cs.chromium.org/chromium/src/components/crash/content/app/crash_keys_...

Done.

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
File components/browser_watcher/postmortem_report_collector.cc (right):

https://codereview.chromium.org/2372633002/diff/20001/components/browser_watc...
components/browser_watcher/postmortem_report_collector.cc:193:
CrashReporterClient* reporter_client = GetCrashReporterClient();
On 2016/09/26 18:59:06, grt (UTC plus 2) wrote:
> does this compile? GetCrashReporterClient is within a CRASH_IMPLEMENTATION
> block, so i thought it was unreachable by client code.

It did. Not sure why though. Removed this code as per other comment.

[grt (UTC plus 2), 2016-09-27 20:43:10]

i think this can be simplified; please see below. thanks.

https://codereview.chromium.org/2372633002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2372633002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:504:
base::WrapUnique(new chrome_watcher::ChromePostmortemReportCollector()),
base::MakeUnique<chrome_watcher::ChromePostmortemReportCollector>()

that aside, i think that making Chrome aware of PRC is unfortunate due to the
dependency leakage. since what is really needed here is a way for Chrome to pass
three values to the watcher, can you either pass the values directly here, or
provide a callback that returns them when needed? for example, in an anonymous
namespace in this file:

void GetTheValues(std::string* one, std::string* two, std::string* three) {
  // use install_static to fetch the data
}

and then here:
  base::Bind(&GetTheValues)

and then inside of the WMPW:

  callback_.Run(&one, &two, &three);

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/B...
File chrome/chrome_watcher/BUILD.gn (right):

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/B...
chrome/chrome_watcher/BUILD.gn:80:
static_library("chrome_postmortem_report_collector") {
i'm wondering if this is the right thing. some questions:
- since the CPRC is only used in browser/metrics, why not put the code for it
there?
- if not, why not in "client" above?
- if you think making this its own target is really the right thing, then i
think it should be a source_set rather than a static_library

please see my other comment about using a simple callback instead

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/B...
chrome/chrome_watcher/BUILD.gn:89:
"//components/browser_watcher:stability_report_proto",
this is an unfortunate side effect of making cprc a subclass of prc. i think
it's best to avoid this.

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/c...
File chrome/chrome_watcher/chrome_postmortem_report_collector.h (right):

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/c...
chrome/chrome_watcher/chrome_postmortem_report_collector.h:18:
~ChromePostmortemReportCollector() override = default;
is this needed?

[manzagop (departed), 2016-09-28 16:27:50]

Patchset #3 (id:80001) has been deleted

[manzagop (departed), 2016-09-28 16:29:17]

Thanks for being patient and providing explanations. I'm progressing in my
understanding of what should go where and the considerations.

Please have another look.

https://codereview.chromium.org/2372633002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/2372633002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:504:
base::WrapUnique(new chrome_watcher::ChromePostmortemReportCollector()),
On 2016/09/27 20:43:09, grt (UTC plus 2) wrote:
> base::MakeUnique<chrome_watcher::ChromePostmortemReportCollector>()
> 
> that aside, i think that making Chrome aware of PRC is unfortunate due to the
> dependency leakage. since what is really needed here is a way for Chrome to
pass
> three values to the watcher, can you either pass the values directly here, or
> provide a callback that returns them when needed? for example, in an anonymous
> namespace in this file:
> 
> void GetTheValues(std::string* one, std::string* two, std::string* three) {
>   // use install_static to fetch the data
> }
> 
> and then here:
>   base::Bind(&GetTheValues)
> 
> and then inside of the WMPW:
> 
>   callback_.Run(&one, &two, &three);

Agreed that is better. Done.

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/B...
File chrome/chrome_watcher/BUILD.gn (right):

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/B...
chrome/chrome_watcher/BUILD.gn:80:
static_library("chrome_postmortem_report_collector") {
On 2016/09/27 20:43:09, grt (UTC plus 2) wrote:
> i'm wondering if this is the right thing. some questions:
> - since the CPRC is only used in browser/metrics, why not put the code for it
> there?
> - if not, why not in "client" above?
> - if you think making this its own target is really the right thing, then i
> think it should be a source_set rather than a static_library
> 
> please see my other comment about using a simple callback instead

Went with using a callback. Theoretically, the collector could be used by any
embedder. Keeping the questions about relocation in mind.

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/B...
chrome/chrome_watcher/BUILD.gn:89:
"//components/browser_watcher:stability_report_proto",
On 2016/09/27 20:43:09, grt (UTC plus 2) wrote:
> this is an unfortunate side effect of making cprc a subclass of prc. i think
> it's best to avoid this.

Acknowledged.

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/c...
File chrome/chrome_watcher/chrome_postmortem_report_collector.h (right):

https://codereview.chromium.org/2372633002/diff/60001/chrome/chrome_watcher/c...
chrome/chrome_watcher/chrome_postmortem_report_collector.h:18:
~ChromePostmortemReportCollector() override = default;
On 2016/09/27 20:43:09, grt (UTC plus 2) wrote:
> is this needed?

Ack.

[grt (UTC plus 2), 2016-09-28 19:42:45]

https://codereview.chromium.org/2372633002/diff/120001/components/browser_wat...
File components/browser_watcher/postmortem_report_collector.h (right):

https://codereview.chromium.org/2372633002/diff/120001/components/browser_wat...
components/browser_watcher/postmortem_report_collector.h:34: using
GetExecutableDetailsCallback =
wdyt of getting rid of this callback and instead have the ctor take the three
strings? the value of the callback is more clear for WatcherMetricsProviderWin
since it allows the extraction of those values to be deferred until
CollectPostmortemReports. for the collector, though, i don't see that it's
really needed.

now that we've simplified this much, is there a reason not to pass the three
strings into WatcherMetricsProviderWin's ctor directly? are there cases where
they're never needed, or will they always be fetched eventually. is it costly
enough that it shouldn't happen within ChromeMetricsServiceClient::Initialize?

https://codereview.chromium.org/2372633002/diff/120001/components/browser_wat...
File components/browser_watcher/watcher_metrics_provider_win.h (right):

https://codereview.chromium.org/2372633002/diff/120001/components/browser_wat...
components/browser_watcher/watcher_metrics_provider_win.h:23: using
GetExecutableDetailsCallback =
please provide sufficient documentation here for an implementer to know what to
do.

[manzagop (departed), 2016-09-29 15:23:14]

Thanks! Another look?

https://codereview.chromium.org/2372633002/diff/120001/components/browser_wat...
File components/browser_watcher/postmortem_report_collector.h (right):

https://codereview.chromium.org/2372633002/diff/120001/components/browser_wat...
components/browser_watcher/postmortem_report_collector.h:34: using
GetExecutableDetailsCallback =
On 2016/09/28 19:42:45, grt (UTC plus 2) wrote:
> wdyt of getting rid of this callback and instead have the ctor take the three
> strings? the value of the callback is more clear for WatcherMetricsProviderWin
> since it allows the extraction of those values to be deferred until
> CollectPostmortemReports. for the collector, though, i don't see that it's
> really needed.

Done.

> now that we've simplified this much, is there a reason not to pass the three
> strings into WatcherMetricsProviderWin's ctor directly? are there cases where
> they're never needed, or will they always be fetched eventually. is it costly
> enough that it shouldn't happen within ChromeMetricsServiceClient::Initialize?

It's indeed not always needed. I have a bad sense of the involved cost, but work
in ChromeMetricsServiceClient::Initialize should be avoided as it's on the UI
thread's startup path.

https://codereview.chromium.org/2372633002/diff/120001/components/browser_wat...
File components/browser_watcher/watcher_metrics_provider_win.h (right):

https://codereview.chromium.org/2372633002/diff/120001/components/browser_wat...
components/browser_watcher/watcher_metrics_provider_win.h:23: using
GetExecutableDetailsCallback =
On 2016/09/28 19:42:45, grt (UTC plus 2) wrote:
> please provide sufficient documentation here for an implementer to know what
to
> do.

Done.

[grt (UTC plus 2), 2016-09-29 19:49:42]

simple! i like it! lgtm.

[manzagop (departed), 2016-09-30 13:22:11]

Thanks for the review!

[manzagop (departed), 2016-09-30 13:23:01]

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

[manzagop (departed), 2016-09-30 13:23:02]

Hi Rob!

I'm looking for an OWNERS' review for the change in
chrome_metrics_service_client.cc. 

Thanks!

Pierre

[rkaplow, 2016-09-30 14:41:54]

lgtm

https://codereview.chromium.org/2372633002/diff/140001/components/browser_wat...
File components/browser_watcher/postmortem_minidump_writer.h (right):

https://codereview.chromium.org/2372633002/diff/140001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer.h:24: std::string
channel_name;    // The name of the channel.
Can we point to any authoritative source on what these string/identifiers can
be? For ex. I don't know just looking at this if I should expect something more
like 'W' or 'Windows', or what

[manzagop (departed), 2016-09-30 18:52:32]

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

[commit-bot: I haz the power, 2016-09-30 18:53:02]

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

[manzagop (departed), 2016-09-30 18:54:20]

Thanks for the review!

https://codereview.chromium.org/2372633002/diff/140001/components/browser_wat...
File components/browser_watcher/postmortem_minidump_writer.h (right):

https://codereview.chromium.org/2372633002/diff/140001/components/browser_wat...
components/browser_watcher/postmortem_minidump_writer.h:24: std::string
channel_name;    // The name of the channel.
On 2016/09/30 14:41:54, rkaplow wrote:
> Can we point to any authoritative source on what these string/identifiers can
> be? For ex. I don't know just looking at this if I should expect something
more
> like 'W' or 'Windows', or what

I was more specific in the comment, though more on the provenance than the
content.

[commit-bot: I haz the power, 2016-09-30 19:48:12]

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

[commit-bot: I haz the power, 2016-09-30 19:48:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[manzagop (departed), 2016-09-30 21:10:26]

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

[commit-bot: I haz the power, 2016-09-30 21:11:06]

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

[manzagop (departed), 2016-09-30 21:14:27]

Patchset #7 (id:180001) has been deleted

[manzagop (departed), 2016-09-30 21:14:38]

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

[commit-bot: I haz the power, 2016-09-30 21:15:08]

Dry run: 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, 2016-09-30 23:48:08]

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

[commit-bot: I haz the power, 2016-09-30 23:48:09]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[manzagop (departed), 2016-10-01 11:50:57]

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

[commit-bot: I haz the power, 2016-10-01 11:51:05]

Dry run: 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, 2016-10-01 12:33:42]

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

[commit-bot: I haz the power, 2016-10-01 12:33:43]

Dry run: This issue passed the CQ dry run.

[manzagop (departed), 2016-10-01 12:41:32]

FYI I had to update a test (due to 'product' -> 'prod' crash key change) and
address clang compile errors. I don't think another look is required though.

[manzagop (departed), 2016-10-01 12:42:03]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2016-10-01 12:42:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2372633002/#ps220001
(title: "Address clang errors")

[commit-bot: I haz the power, 2016-10-01 12:42:12]

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, 2016-10-01 12:47:04]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2016-10-01 12:48:57]

Description was changed from

==========
Use the correct product/version for postmortem reports

BUG=620813
==========

to

==========
Use the correct product/version for postmortem reports

BUG=620813

Committed: https://crrev.com/8014d5ff93657cc93ada6a91762d2dea1943ac74
Cr-Commit-Position: refs/heads/master@{#422311}
==========

[commit-bot: I haz the power, 2016-10-01 12:48:58]

Patchset 8 (id:??) landed as
https://crrev.com/8014d5ff93657cc93ada6a91762d2dea1943ac74
Cr-Commit-Position: refs/heads/master@{#422311}

[grt (UTC plus 2), 2016-10-01 19:51:13]

lgtm