Add the most recent crash report IDs to feedback reports

Add the most recent crash report IDs to feedback reports

Adds the IDs of crashes that occurred within the last hour before sending the
feedback report, making sure that these IDs don't end up in the system_logs.txt
file for privacy reasons.

BUG=613606
Committed: https://crrev.com/f376884e2dff8960d6683c63abb490e4ee080d20
Cr-Commit-Position: refs/heads/master@{#418351}

[afakhry, 2016-09-07 23:17:25]

Description was changed from

==========
Add the most recent crash report ID to feedback reports

BUG=613606
==========

to

==========
Add the most recent crash report IDs to feedback reports

Adds the IDs of crashes that occurred within the last hour before sending the
feedback report, making sure that these IDs don't end up in the system_logs.txt
file for privacy reasons.

BUG=613606
==========

[afakhry, 2016-09-07 23:18:58]

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

[commit-bot: I haz the power, 2016-09-07 23:19:37]

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

[afakhry, 2016-09-07 23:20:02]

afakhry@chromium.org changed reviewers:
+ steel@chromium.org

[afakhry, 2016-09-07 23:20:03]

Rahul, Please take a look. Thanks!

[commit-bot: I haz the power, 2016-09-08 00:36:09]

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

[commit-bot: I haz the power, 2016-09-08 00:36:10]

Dry run: This issue passed the CQ dry run.

[Rahul Chaturvedi, 2016-09-08 19:58:24]

First pass.

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
File chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc
(right):

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc:30:
crash_upload_list_(CreateCrashUploadList(this)),
What happens if the system logs fetcher is deleted before we get a callback on
OnUploadListAvailable?
Should this be a weak_ptr instead?

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
File chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h (right):

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h:39: using
Requests = base::Closure;
This seems, not very useful. base::Closure is barely a few more characters than
'Requests' and makes it very clear what the type is.

https://codereview.chromium.org/2242833003/diff/40001/components/feedback/fee...
File components/feedback/feedback_common.cc (right):

https://codereview.chromium.org/2242833003/diff/40001/components/feedback/fee...
components/feedback/feedback_common.cc:45: constexpr char kCrashReportIds[] =
"crash_report_ids";
Can this be made global and used from the crash reporter source? Relying on two
constants having the same value is fragile. Particularly since this is privacy
sensitive code.

[afakhry, 2016-09-09 22:20:10]

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
File chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc
(right):

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc:30:
crash_upload_list_(CreateCrashUploadList(this)),
On 2016/09/08 19:58:24, Rahul Chaturvedi wrote:
> What happens if the system logs fetcher is deleted before we get a callback on
> OnUploadListAvailable?
> Should this be a weak_ptr instead?

We can make it a weak_ptr, but is it really needed?
The SystemLogsFetcher is a self-deleting object that gets deleted only when all
the responses from all log sources have been received:
https://cs.chromium.org/chromium/src/chrome/browser/feedback/system_logs/syst...

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
File chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h (right):

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h:39: using
Requests = base::Closure;
On 2016/09/08 19:58:24, Rahul Chaturvedi wrote:
> This seems, not very useful. base::Closure is barely a few more characters
than
> 'Requests' and makes it very clear what the type is.

Makes sense ... Done.

https://codereview.chromium.org/2242833003/diff/40001/components/feedback/fee...
File components/feedback/feedback_common.cc (right):

https://codereview.chromium.org/2242833003/diff/40001/components/feedback/fee...
components/feedback/feedback_common.cc:45: constexpr char kCrashReportIds[] =
"crash_report_ids";
On 2016/09/08 19:58:24, Rahul Chaturvedi wrote:
> Can this be made global and used from the crash reporter source? Relying on
two
> constants having the same value is fragile. Particularly since this is privacy
> sensitive code.

I couldn't agree more! Thanks! Done. :)

[Rahul Chaturvedi, 2016-09-12 22:31:21]

lgtm % nits

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
File chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc
(right):

https://codereview.chromium.org/2242833003/diff/40001/chrome/browser/feedback...
chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc:30:
crash_upload_list_(CreateCrashUploadList(this)),
On 2016/09/09 22:20:10, afakhry wrote:
> On 2016/09/08 19:58:24, Rahul Chaturvedi wrote:
> > What happens if the system logs fetcher is deleted before we get a callback
on
> > OnUploadListAvailable?
> > Should this be a weak_ptr instead?
> 
> We can make it a weak_ptr, but is it really needed?
> The SystemLogsFetcher is a self-deleting object that gets deleted only when
all
> the responses from all log sources have been received:
>
https://cs.chromium.org/chromium/src/chrome/browser/feedback/system_logs/syst...

Ah - in that case just document that below where we call
LoadUploadListAsynchronously(), that we currently outlive that call and need to
in the future too.

https://codereview.chromium.org/2242833003/diff/60001/chrome/browser/feedback...
File chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc
(right):

https://codereview.chromium.org/2242833003/diff/60001/chrome/browser/feedback...
chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc:53:
crash_ids_list_.reserve(kMaxCrashesCountToRetrieve);
kMaxCrashesCountToRetrieve * (kCrashIdStringSize + 2)?
Also, reserve isn't really much of a gain here since we don't really know how
many crashes we'll actually end up adding. I would just skip it.

https://codereview.chromium.org/2242833003/diff/60001/components/feedback/fee...
File components/feedback/feedback_report.h (right):

https://codereview.chromium.org/2242833003/diff/60001/components/feedback/fee...
components/feedback/feedback_report.h:36: static const char
kCrashReportIdsKey[];
constexpr? That should also allow you to initialize here instead of in the
definition.

[afakhry, 2016-09-13 16:58:32]

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

[commit-bot: I haz the power, 2016-09-13 16:59:00]

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

[afakhry, 2016-09-13 16:59:46]

afakhry@chromium.org changed reviewers:
+ thestig@chromium.org

[afakhry, 2016-09-13 16:59:46]

Thanks, Rahul!

thestig for BUILD.gn

https://codereview.chromium.org/2242833003/diff/60001/chrome/browser/feedback...
File chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc
(right):

https://codereview.chromium.org/2242833003/diff/60001/chrome/browser/feedback...
chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc:53:
crash_ids_list_.reserve(kMaxCrashesCountToRetrieve);
On 2016/09/12 22:31:21, Rahul Chaturvedi wrote:
> kMaxCrashesCountToRetrieve * (kCrashIdStringSize + 2)?
> Also, reserve isn't really much of a gain here since we don't really know how
> many crashes we'll actually end up adding. I would just skip it.

Wow, thanks for catching that! I think I confused reserving the std::vector
|crashes| with the string. 

Done.

https://codereview.chromium.org/2242833003/diff/60001/components/feedback/fee...
File components/feedback/feedback_report.h (right):

https://codereview.chromium.org/2242833003/diff/60001/components/feedback/fee...
components/feedback/feedback_report.h:36: static const char
kCrashReportIdsKey[];
On 2016/09/12 22:31:21, Rahul Chaturvedi wrote:
> constexpr? That should also allow you to initialize here instead of in the
> definition.

That's the first thing I tried, and it gave me link errors. I was hitting this
issue:
http://stackoverflow.com/questions/8016780/undefined-reference-to-static-cons...

I didn't realize we still define the static member in the cpp file without
initializing it.

Done.

[afakhry, 2016-09-13 17:00:13]

afakhry@chromium.org changed reviewers:
- thestig@chromium.org

[afakhry, 2016-09-13 17:01:07]

afakhry@chromium.org changed reviewers:
+ sky@chromium.org

[afakhry, 2016-09-13 17:01:08]

thestig is OOO. sky@ for BUILD.gn

[commit-bot: I haz the power, 2016-09-13 17:36:01]

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

[commit-bot: I haz the power, 2016-09-13 17:36:02]

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

[afakhry, 2016-09-13 17:46:37]

https://codereview.chromium.org/2242833003/diff/80001/components/feedback/fee...
File components/feedback/feedback_report.h (right):

https://codereview.chromium.org/2242833003/diff/80001/components/feedback/fee...
components/feedback/feedback_report.h:36: static constexpr char
kCrashReportIdsKey[] = "crash_report_ids";
This would still cause compile errors on Windows:

FAILED: obj/chrome/browser/browser/crash_ids_source.obj 
ninja -t msvc -e environment.x64 -- E:\b\c\cipd\goma/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\VC\bin\amd64/cl.exe"
/nologo /showIncludes /FC @obj/chrome/browser/browser/crash_ids_source.obj.rsp
/c ../../chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc
/Foobj/chrome/browser/browser/crash_ids_source.obj
/Fd"obj/chrome/browser/browser_cc.pdb"
e:\b\c\b\win\src\components\feedback\feedback_report.h(36): error C2131:
expression did not evaluate to a constant
e:\b\c\b\win\src\components\feedback\feedback_report.h(36): note: failure was
caused by unevaluable pointer value

[Rahul Chaturvedi, 2016-09-13 18:02:55]

https://codereview.chromium.org/2242833003/diff/80001/components/feedback/fee...
File components/feedback/feedback_report.h (right):

https://codereview.chromium.org/2242833003/diff/80001/components/feedback/fee...
components/feedback/feedback_report.h:36: static constexpr char
kCrashReportIdsKey[] = "crash_report_ids";
On 2016/09/13 17:46:37, afakhry wrote:
> This would still cause compile errors on Windows:
> 
> FAILED: obj/chrome/browser/browser/crash_ids_source.obj 
> ninja -t msvc -e environment.x64 -- E:\b\c\cipd\goma/gomacc.exe
>
"E:\b\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\VC\bin\amd64/cl.exe"
> /nologo /showIncludes /FC @obj/chrome/browser/browser/crash_ids_source.obj.rsp
> /c ../../chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc
> /Foobj/chrome/browser/browser/crash_ids_source.obj
> /Fd"obj/chrome/browser/browser_cc.pdb"
> e:\b\c\b\win\src\components\feedback\feedback_report.h(36): error C2131:
> expression did not evaluate to a constant
> e:\b\c\b\win\src\components\feedback\feedback_report.h(36): note: failure was
> caused by unevaluable pointer value

Weird. I am also not seeing any usages of this in current code, maybe it is just
broken.
const should be fine, this was a nit anyway.

[afakhry, 2016-09-13 18:46:52]

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

[commit-bot: I haz the power, 2016-09-13 18:47:32]

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-13 19:59:45]

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

[commit-bot: I haz the power, 2016-09-13 19:59:46]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-13 20:00:35]

BUILD.gn LGTM

[afakhry, 2016-09-13 20:28:34]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2016-09-13 20:28:35]

The patchset sent to the CQ was uploaded after l-g-t-m from steel@chromium.org
Link to the patchset: https://codereview.chromium.org/2242833003/#ps100001
(title: "revert back to a const rather than a constexpr")

[commit-bot: I haz the power, 2016-09-13 20:29:07]

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-13 20:34:51]

Description was changed from

==========
Add the most recent crash report IDs to feedback reports

Adds the IDs of crashes that occurred within the last hour before sending the
feedback report, making sure that these IDs don't end up in the system_logs.txt
file for privacy reasons.

BUG=613606
==========

to

==========
Add the most recent crash report IDs to feedback reports

Adds the IDs of crashes that occurred within the last hour before sending the
feedback report, making sure that these IDs don't end up in the system_logs.txt
file for privacy reasons.

BUG=613606
==========

[commit-bot: I haz the power, 2016-09-13 20:34:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-13 20:37:19]

Description was changed from

==========
Add the most recent crash report IDs to feedback reports

Adds the IDs of crashes that occurred within the last hour before sending the
feedback report, making sure that these IDs don't end up in the system_logs.txt
file for privacy reasons.

BUG=613606
==========

to

==========
Add the most recent crash report IDs to feedback reports

Adds the IDs of crashes that occurred within the last hour before sending the
feedback report, making sure that these IDs don't end up in the system_logs.txt
file for privacy reasons.

BUG=613606

Committed: https://crrev.com/f376884e2dff8960d6683c63abb490e4ee080d20
Cr-Commit-Position: refs/heads/master@{#418351}
==========

[commit-bot: I haz the power, 2016-09-13 20:37:21]

Patchset 6 (id:??) landed as
https://crrev.com/f376884e2dff8960d6683c63abb490e4ee080d20
Cr-Commit-Position: refs/heads/master@{#418351}