Making the MemoryDumpManager less aggressive in disabling dump providers on failures.

Making the MemoryDumpManager less aggressive in disabling dump providers on failures.

The dump providers are disabled after it fails for once. But in some
cases the dump failure doesn't mean that it will fail permanently.
This CL adds a failure count to the dump provider and disables it only
if it fails consecutively for n times.

BUG=490783
Committed: https://crrev.com/4656df694b1754047d53f301eea7a5a8b3cd0052
Cr-Commit-Position: refs/heads/master@{#335119}

[ssid, 2015-06-02 12:39:39]

ssid@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-06-10 23:37:07]

Don't you have also to update the unittest that checks the disabling logic? How
come that still passes (does it)?

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:320: const int kMaxFailuresCount = 3;
+consecutive

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:323: LOG(ERROR) << "The memory dumper
failed, possibly due to sandboxing "
you want to give this message only if it fails N time, not each time

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:328:
dump_providers_.find(mdp)->second.failures++;
Let's just lookup once, and keep the ptr to the mdpinfo

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:386: it->second.disabled = false;
and you want to reset the count here, otherwise once they fail will be disabled
forever

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:93: int failures;  // Number of times the
provider failed (to disable the MDPs).
nit consecutive_failures

[ssid, 2015-06-12 14:37:00]

Made changes, PTAL at the test.

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:320: const int kMaxFailuresCount = 3;
On 2015/06/10 23:37:07, Primiano Tucci wrote:
> +consecutive

Done.

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:323: LOG(ERROR) << "The memory dumper
failed, possibly due to sandboxing "
On 2015/06/10 23:37:07, Primiano Tucci wrote:
> you want to give this message only if it fails N time, not each time

Done.

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:328:
dump_providers_.find(mdp)->second.failures++;
On 2015/06/10 23:37:07, Primiano Tucci wrote:
> Let's just lookup once, and keep the ptr to the mdpinfo

Done.

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:386: it->second.disabled = false;
On 2015/06/10 23:37:07, Primiano Tucci wrote:
> and you want to reset the count here, otherwise once they fail will be
disabled
> forever

Done.

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/1152743003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:93: int failures;  // Number of times the
provider failed (to disable the MDPs).
On 2015/06/10 23:37:07, Primiano Tucci wrote:
> nit consecutive_failures

Done.

[Primiano Tucci (use gerrit), 2015-06-16 11:05:47]

LGTM with 1 change request.

https://codereview.chromium.org/1152743003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1152743003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:263: const int kFailureCount =
3;
At this point I think this constant should be a private static of
MemoryDUmpManager (of which this class is already a friend)

[picksi, 2015-06-16 12:56:43]

picksi@google.com changed reviewers:
+ picksi@google.com

[picksi, 2015-06-16 12:56:44]

https://codereview.chromium.org/1152743003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1152743003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:335: }
nit: Removing the '!' and swapping the if/else bodies around would make this
(slightly) more readable.

[ssid, 2015-06-16 14:52:36]

Thanks made changes.

https://codereview.chromium.org/1152743003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1152743003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:335: }
On 2015/06/16 12:56:44, picksi wrote:
> nit: Removing the '!' and swapping the if/else bodies around would make this
> (slightly) more readable.

Done.

https://codereview.chromium.org/1152743003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1152743003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:263: const int kFailureCount =
3;
On 2015/06/16 11:05:47, Primiano Tucci wrote:
> At this point I think this constant should be a private static of
> MemoryDUmpManager (of which this class is already a friend)

Done.

[picksi, 2015-06-16 16:01:38]

lgtm!

[ssid, 2015-06-18 19:44:44]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-06-18 19:44:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, picksi@google.com
Link to the patchset: https://codereview.chromium.org/1152743003/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-06-18 19:46:30]

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

[commit-bot: I haz the power, 2015-06-18 21:10:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-18 21:11:20]

Patchset 4 (id:??) landed as
https://crrev.com/4656df694b1754047d53f301eea7a5a8b3cd0052
Cr-Commit-Position: refs/heads/master@{#335119}