Double variations crash key size

Double variations crash key size

This is done by introducing a new crash key size (kHugeSize), which requires raising kLargestValueAllowed.

BUG=689045
Review-Url: https://codereview.chromium.org/2671383002
Cr-Commit-Position: refs/heads/master@{#449064}
Committed: https://chromium.googlesource.com/chromium/src/+/3d62ffd98b8739d42ac7cd34cd4e04be35929f62

[manzagop (departed), 2017-02-06 15:05:48]

manzagop@chromium.org changed reviewers:
+ ivanpe@chromium.org, mark@chromium.org, scottmg@chromium.org

[manzagop (departed), 2017-02-06 15:05:49]

Hi,
Does this make sense for a short term fix?
Thanks,
Pierre

[manzagop (departed), 2017-02-06 15:19:20]

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

[commit-bot: I haz the power, 2017-02-06 15:19:33]

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

[scottmg, 2017-02-06 15:41:22]

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

[scottmg, 2017-02-06 15:41:23]

I don't know, but lgtm to try. There's still multiple copies of the keys though.
You should change this one too:

https://cs.chromium.org/chromium/src/chrome/app/chrome_crash_reporter_client_...

(I sort of thought there was 3 copies, but maybe there's only 2 now. Ananta do
you know of another copy?)

[manzagop (departed), 2017-02-06 15:44:28]

On 2017/02/06 15:41:23, scottmg wrote:
> I don't know, but lgtm to try. There's still multiple copies of the keys
though.
> You should change this one too:
> 
>
https://cs.chromium.org/chromium/src/chrome/app/chrome_crash_reporter_client_...
> 
> (I sort of thought there was 3 copies, but maybe there's only 2 now. Ananta do
> you know of another copy?)

Ah looks like there's at least also another in
src/ios/chrome/browser/crash_report/crash_keys.cc

[Sigurður Ásgeirsson, 2017-02-06 15:44:56]

siggi@chromium.org changed reviewers:
+ siggi@chromium.org

[Sigurður Ásgeirsson, 2017-02-06 15:44:57]

https://codereview.chromium.org/2671383002/diff/1/components/crash/core/commo...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/2671383002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.cc:96:
variations_string.reserve(kHugeSize);
is it possible to test that this actually works? There's an associated unittest
I haven't looked at...

[commit-bot: I haz the power, 2017-02-06 15:46:30]

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

[commit-bot: I haz the power, 2017-02-06 15:46:31]

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

[manzagop (departed), 2017-02-06 15:48:09]

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

[commit-bot: I haz the power, 2017-02-06 15:48:23]

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, 2017-02-06 16:37:52]

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

[commit-bot: I haz the power, 2017-02-06 16:37:53]

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

[manzagop (departed), 2017-02-06 16:44:19]

I'll write a test. 

Looks like I'll also need to raise  kLargestValueAllowed.

https://codereview.chromium.org/2671383002/diff/1/components/crash/core/commo...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/2671383002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.cc:96:
variations_string.reserve(kHugeSize);
On 2017/02/06 15:44:56, Sigurður Ásgeirsson wrote:
> is it possible to test that this actually works? There's an associated
unittest
> I haven't looked at...

Ack.

[manzagop (departed), 2017-02-06 17:26:16]

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

[commit-bot: I haz the power, 2017-02-06 17:26:37]

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

[manzagop (departed), 2017-02-06 17:27:37]

I added some kind of test. Tell me what you think. 

Should we add a function like GetCrashKeysForCommandLineSwitches for variations?

[commit-bot: I haz the power, 2017-02-06 17:34:37]

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

[commit-bot: I haz the power, 2017-02-06 17:34:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[manzagop (departed), 2017-02-06 17:40:47]

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

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

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, 2017-02-06 17:50:37]

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

[commit-bot: I haz the power, 2017-02-06 17:50:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[manzagop (departed), 2017-02-06 18:38:39]

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

[commit-bot: I haz the power, 2017-02-06 18:39:01]

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, 2017-02-06 20:00:40]

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

[commit-bot: I haz the power, 2017-02-06 20:00:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[manzagop (departed), 2017-02-06 20:08:58]

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

[commit-bot: I haz the power, 2017-02-06 20:09:36]

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

[manzagop (departed), 2017-02-06 20:11:08]

Description was changed from

==========
Double variations crash key size

BUG=689045
==========

to

==========
Double variations crash key size

This is done by introducing a new crash key size (kHugeSize), which requires
raising kLargestValueAllowed.

BUG=689045
==========

[manzagop (departed), 2017-02-06 20:15:57]

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

[manzagop (departed), 2017-02-06 20:15:58]

Hi rsesek@, sdefresne@,

Could you have an OWNERS' look?

rsesek@: chrome\common\crash_keys.cc
sdefresne@: ios\chrome\browser\crash_report\crash_keys.cc

Thanks!

Pierre

[commit-bot: I haz the power, 2017-02-06 21:17:28]

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

[commit-bot: I haz the power, 2017-02-06 21:17:29]

Dry run: This issue passed the CQ dry run.

[manzagop (departed), 2017-02-06 21:40:50]

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

[manzagop (departed), 2017-02-06 21:40:51]

+sdefresne@ for realz

[sdefresne, 2017-02-07 08:51:16]

ios/ lgtm

[Robert Sesek, 2017-02-07 19:51:07]

LGTM

[manzagop (departed), 2017-02-07 20:28:28]

Thanks for the reviews!

[manzagop (departed), 2017-02-07 20:28:37]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2017-02-07 20:28:38]

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/2671383002/#ps100001
(title: "Remove an expectation based on chunk size")

[commit-bot: I haz the power, 2017-02-07 20:29:32]

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-02-07 20:37:28]

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

[commit-bot: I haz the power, 2017-02-07 20:37:30]

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

[manzagop (departed), 2017-02-08 00:02:41]

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

[manzagop (departed), 2017-02-08 00:02:41]

Hi Gab!

Could you have an OWNERS's look at:
base/debug/crash_logging.cc

Thanks!

Pierre-Antoine

[gab, 2017-02-08 16:05:17]

On 2017/02/08 00:02:41, manzagop wrote:
> Hi Gab!
> 
> Could you have an OWNERS's look at:
> base/debug/crash_logging.cc

base/ r-s lgtm given scottmg's

> 
> Thanks!
> 
> Pierre-Antoine

[manzagop (departed), 2017-02-08 16:09:33]

On 2017/02/08 16:05:17, gab wrote:
> On 2017/02/08 00:02:41, manzagop wrote:
> > Hi Gab!
> > 
> > Could you have an OWNERS's look at:
> > base/debug/crash_logging.cc
> 
> base/ r-s lgtm given scottmg's
> 
> > 
> > Thanks!
> > 
> > Pierre-Antoine

Thanks!

[manzagop (departed), 2017-02-08 16:09:43]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2017-02-08 16:10:06]

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-02-08 17:48:26]

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

[commit-bot: I haz the power, 2017-02-08 17:48:28]

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

[manzagop (departed), 2017-02-08 18:40:58]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2017-02-08 18:41:50]

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-02-08 19:41:42]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486579258291470,
"parent_rev": "07f10c1ad12b4ccb681a27131a353c074431997d", "commit_rev":
"3d62ffd98b8739d42ac7cd34cd4e04be35929f62"}

[commit-bot: I haz the power, 2017-02-08 19:42:31]

Description was changed from

==========
Double variations crash key size

This is done by introducing a new crash key size (kHugeSize), which requires
raising kLargestValueAllowed.

BUG=689045
==========

to

==========
Double variations crash key size

This is done by introducing a new crash key size (kHugeSize), which requires
raising kLargestValueAllowed.

BUG=689045

Review-Url: https://codereview.chromium.org/2671383002
Cr-Commit-Position: refs/heads/master@{#449064}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d62ffd98b8739d42ac7cd34cd4e...
==========

[commit-bot: I haz the power, 2017-02-08 19:42:35]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/3d62ffd98b8739d42ac7cd34cd4e...