Slow Reports: Set a minimum time in between reports, and check for OTR sessions

Slow Reports, Chrome-side vetoes: Set a minimum time in between reports, check for OTR sessions, and be paranoid about crashes.

R=dsinclair,shatch
BUG=496243
Committed: https://crrev.com/386fc9eafc77b4f0ea3606a5d440b5fb9563cca5
Cr-Commit-Position: refs/heads/master@{#333230}

[oystein (OOO til 10th of July), 2015-06-03 18:01:25]

ptal!

[oystein (OOO til 10th of July), 2015-06-03 18:01:31]

Patchset #1 (id:1) has been deleted

[dsinclair, 2015-06-04 14:54:00]

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
File chrome/browser/tracing/chrome_tracing_delegate.cc (right):

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:23: const int
kMinDaysAllowedSinceLastUpload = 7;
kMinDaysUntilNextUpload?

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:28:
registry->RegisterInt64Pref(prefs::kBackgroundTracingLastUpload, 0);
I don't know how the registry works, does this handle not setting the value if
it already exists?

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:44: }
nit: Don't need braces.

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:56: base::Time*
next_allowed_time) {
We don't currently use next_allowed_time. Should we just remove it for now and
add it in when we start doing the re-schedule work?

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:60: DCHECK(profile);
If we leave it as a *, DHCECK(config)

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:63: if
(profile->GetLastSessionExitType() == Profile::EXIT_CRASHED)
Why does the session crashing mean that background tracing did it? Or, just if
it crashed assume we _could_ have caused it?

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:93: bool
requires_anonymized_data) {
DCHECK(config)

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
File chrome/browser/tracing/chrome_tracing_delegate.h (right):

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.h:25:
content::BackgroundTracingConfig* config,
Can this be passed by reference?

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.h:29: bool
IsAllowedToFinalizeBackgroundScenario(
Would End be better the Finalize? This doesn't really tell me that tracing is
going to be stopped after this method is executed.

https://codereview.chromium.org/1164023002/diff/20001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1164023002/diff/20001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:108: config.get(),
requires_anonymized_data, NULL)) {
nit: nullptr

[oystein (OOO til 10th of July), 2015-06-04 20:18:43]

Thanks dan!

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
File chrome/browser/tracing/chrome_tracing_delegate.cc (right):

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:23: const int
kMinDaysAllowedSinceLastUpload = 7;
On 2015/06/04 14:53:59, dsinclair wrote:
> kMinDaysUntilNextUpload?

Done.

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:28:
registry->RegisterInt64Pref(prefs::kBackgroundTracingLastUpload, 0);
On 2015/06/04 14:53:59, dsinclair wrote:
> I don't know how the registry works, does this handle not setting the value if
> it already exists?

Yeah this is just registering the name (and a default), not setting an actual
value. Needs to be done before the value can be either get or set.

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:56: base::Time*
next_allowed_time) {
On 2015/06/04 14:53:59, dsinclair wrote:
> We don't currently use next_allowed_time. Should we just remove it for now and
> add it in when we start doing the re-schedule work?

Sure thing.

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.cc:63: if
(profile->GetLastSessionExitType() == Profile::EXIT_CRASHED)
On 2015/06/04 14:53:59, dsinclair wrote:
> Why does the session crashing mean that background tracing did it? Or, just if
> it crashed assume we _could_ have caused it?

Yeah, it's a paranoid safeguard in case of crashloops, since a scenario could be
triggered on startup. We're fine with a very low number of reports per install,
so we can afford to do some pretty rough vetoing I think.

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
File chrome/browser/tracing/chrome_tracing_delegate.h (right):

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.h:25:
content::BackgroundTracingConfig* config,
On 2015/06/04 14:53:59, dsinclair wrote:
> Can this be passed by reference?

Done.

https://codereview.chromium.org/1164023002/diff/20001/chrome/browser/tracing/...
chrome/browser/tracing/chrome_tracing_delegate.h:29: bool
IsAllowedToFinalizeBackgroundScenario(
On 2015/06/04 14:53:59, dsinclair wrote:
> Would End be better the Finalize? This doesn't really tell me that tracing is
> going to be stopped after this method is executed.

Done.

https://codereview.chromium.org/1164023002/diff/20001/content/browser/tracing...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1164023002/diff/20001/content/browser/tracing...
content/browser/tracing/background_tracing_manager_impl.cc:108: config.get(),
requires_anonymized_data, NULL)) {
On 2015/06/04 14:53:59, dsinclair wrote:
> nit: nullptr

Done.

[dsinclair, 2015-06-04 20:23:50]

lgtm

[oystein (OOO til 10th of July), 2015-06-04 20:30:40]

oysteine@chromium.org changed reviewers:
+ gab@chromium.org, sievers@chromium.org

[oystein (OOO til 10th of July), 2015-06-04 20:31:31]

Thanks dan!

+sievers for content/public
+gab for chrome/browser/prefs

[no sievers, 2015-06-04 22:55:34]

content/public lgtm

https://codereview.chromium.org/1164023002/diff/80001/content/public/browser/...
File content/public/browser/tracing_delegate.h (right):

https://codereview.chromium.org/1164023002/diff/80001/content/public/browser/...
content/public/browser/tracing_delegate.h:35: bool requires_anonymized_data) =
0;
nit: Usually the interfaces to be implemented by the embedder have default
implementation stubs, though I don't know if it makes a lot of sense to *not*
implement any of these three functions here in this specific case

[gab, 2015-06-05 14:02:42]

Looking solely at prefs.

https://codereview.chromium.org/1164023002/diff/100001/chrome/browser/tracing...
File chrome/browser/tracing/chrome_tracing_delegate.h (right):

https://codereview.chromium.org/1164023002/diff/100001/chrome/browser/tracing...
chrome/browser/tracing/chrome_tracing_delegate.h:13: void
RegisterTracingPrefs(PrefRegistrySimple* registry);
Make this a static call of ChromeTracingDelegate.

i.e. ChromeTracingDelegate::RegisterPrefs(PrefRegistrySimple* registry);

https://codereview.chromium.org/1164023002/diff/100001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/1164023002/diff/100001/chrome/common/pref_nam...
chrome/common/pref_names.cc:2283: }  // namespace prefs
nit: empty line above.

[oystein (OOO til 10th of July), 2015-06-05 21:14:43]

Thanks everyone!

https://codereview.chromium.org/1164023002/diff/80001/content/public/browser/...
File content/public/browser/tracing_delegate.h (right):

https://codereview.chromium.org/1164023002/diff/80001/content/public/browser/...
content/public/browser/tracing_delegate.h:35: bool requires_anonymized_data) =
0;
On 2015/06/04 22:55:34, sievers wrote:
> nit: Usually the interfaces to be implemented by the embedder have default
> implementation stubs, though I don't know if it makes a lot of sense to *not*
> implement any of these three functions here in this specific case

Fair point; I've made them default to false, as a safeguard.

https://codereview.chromium.org/1164023002/diff/100001/chrome/browser/tracing...
File chrome/browser/tracing/chrome_tracing_delegate.h (right):

https://codereview.chromium.org/1164023002/diff/100001/chrome/browser/tracing...
chrome/browser/tracing/chrome_tracing_delegate.h:13: void
RegisterTracingPrefs(PrefRegistrySimple* registry);
On 2015/06/05 14:02:41, gab wrote:
> Make this a static call of ChromeTracingDelegate.
> 
> i.e. ChromeTracingDelegate::RegisterPrefs(PrefRegistrySimple* registry);

Done.

https://codereview.chromium.org/1164023002/diff/100001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/1164023002/diff/100001/chrome/common/pref_nam...
chrome/common/pref_names.cc:2283: }  // namespace prefs
On 2015/06/05 14:02:42, gab wrote:
> nit: empty line above.

Done.

[gab, 2015-06-06 01:09:16]

prefs lgtm w/ nit

https://codereview.chromium.org/1164023002/diff/160001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1164023002/diff/160001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:266:
ChromeTracingDelegate::RegisterPrefs(registry);
nit: Sort with the C's now that this no longer starts with an R.

[oystein (OOO til 10th of July), 2015-06-06 05:33:11]

Thanks!

https://codereview.chromium.org/1164023002/diff/160001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1164023002/diff/160001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:266:
ChromeTracingDelegate::RegisterPrefs(registry);
On 2015/06/06 01:09:16, gab wrote:
> nit: Sort with the C's now that this no longer starts with an R.

Done.

[oystein (OOO til 10th of July), 2015-06-06 05:33:26]

The CQ bit was checked by oysteine@chromium.org

[oystein (OOO til 10th of July), 2015-06-06 05:33:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, sievers@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1164023002/#ps170001
(title: "Alphabetize")

[commit-bot: I haz the power, 2015-06-06 05:33:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164023002/170001

[commit-bot: I haz the power, 2015-06-06 05:51:37]

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

[commit-bot: I haz the power, 2015-06-06 05:51:38]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[oystein (OOO til 10th of July), 2015-06-06 05:54:35]

The CQ bit was checked by oysteine@chromium.org

[oystein (OOO til 10th of July), 2015-06-06 05:54:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, gab@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1164023002/#ps190001
(title: "Buildfix")

[commit-bot: I haz the power, 2015-06-06 05:54:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164023002/190001

[commit-bot: I haz the power, 2015-06-06 06:09:11]

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

[commit-bot: I haz the power, 2015-06-06 06:09:12]

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

[oystein (OOO til 10th of July), 2015-06-07 17:52:55]

The CQ bit was checked by oysteine@chromium.org

[oystein (OOO til 10th of July), 2015-06-07 17:52:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, gab@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1164023002/#ps210001
(title: "Buildfixes")

[commit-bot: I haz the power, 2015-06-07 17:53:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164023002/210001

[commit-bot: I haz the power, 2015-06-07 18:45:52]

Committed patchset #11 (id:210001)

[commit-bot: I haz the power, 2015-06-07 18:46:53]

Patchset 11 (id:??) landed as
https://crrev.com/386fc9eafc77b4f0ea3606a5d440b5fb9563cca5
Cr-Commit-Position: refs/heads/master@{#333230}