Treat PPAPI disconnect as a crash for plugin metrics.

Treat PPAPI disconnect as a crash for plugin metrics.

This normalizes the behavior the user sees, which is a
IDS_PLUGIN_CRASHED_PROMPT infobar, with the metrics reported by
PluginMetricsProvider - which was only recording a crash for when
the process terminated with TERMINATION_STATUS_PROCESS_CRASHED or
TERMINATION_STATUS_ABNORMAL_TERMINATION.

Each time the infobar pops, there should now be a plugin crash
recorded in the SystemProfile.

BUG=509139
TEST=manual, see bug

Committed: https://crrev.com/ba99407642c5f7306c3a3edc0de59ad17db821ff
Cr-Commit-Position: refs/heads/master@{#341566}

[Will Harris, 2015-07-31 16:28:58]

wfh@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Will Harris, 2015-07-31 16:29:02]

PTAL

[Alexei Svitkine (slow), 2015-07-31 16:42:33]

Could you also add a test?

[Will Harris, 2015-08-03 16:51:30]

On 2015/07/31 16:42:33, Alexei Svitkine wrote:
> Could you also add a test?

Done, sorry it took so long I thought I'd found a huge bug in metrics but then
realized it was a typo.

PTAL.

[Alexei Svitkine (slow), 2015-08-03 16:57:07]

https://codereview.chromium.org/1259493003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/1259493003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/plugin_metrics_provider_unittest.cc:177:
provider.BrowserChildProcessCrashed(child_process_data, 2);
Hmm, is this a realistic scenario? Seems like if there's only two instances and
we get those 3 messages, we'd be double counting crashes? e.g. if an instance
both crashed and disconnected presumably we only want to count it once?

I mean, maybe that's not a thing that would happen in practice. But in that
case, the test shouldn't be simulating it. But if it is a thing might actually
happen in practice, then maybe the class needs extra logic to de-dup them (e.g.
by having the disconnect message take the instance id and keeping track of which
ones already crashed?).

[Will Harris, 2015-08-03 17:01:49]

On 2015/08/03 16:57:07, Alexei Svitkine wrote:
>
https://codereview.chromium.org/1259493003/diff/20001/chrome/browser/metrics/...
> File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right):
> 
>
https://codereview.chromium.org/1259493003/diff/20001/chrome/browser/metrics/...
> chrome/browser/metrics/plugin_metrics_provider_unittest.cc:177:
> provider.BrowserChildProcessCrashed(child_process_data, 2);
> Hmm, is this a realistic scenario? Seems like if there's only two instances
and
> we get those 3 messages, we'd be double counting crashes? e.g. if an instance
> both crashed and disconnected presumably we only want to count it once?
> 
> I mean, maybe that's not a thing that would happen in practice. But in that
> case, the test shouldn't be simulating it. But if it is a thing might actually
> happen in practice, then maybe the class needs extra logic to de-dup them
(e.g.
> by having the disconnect message take the instance id and keeping track of
which
> ones already crashed?).

no - this exact situation should never happen. I wanted to test that a plugin
with the same name would have two crashes recorded. I don't think we need to 
complicate the logic inside PluginMetricsProvider to handle this case.

I can just change the test to be more realistic, i.e. two instances, one crash
and one disconnect. WDYT?

[Alexei Svitkine (slow), 2015-08-03 17:03:17]

That sounds good!

On Mon, Aug 3, 2015 at 1:01 PM, <wfh@chromium.org> wrote:

> On 2015/08/03 16:57:07, Alexei Svitkine wrote:
>
>
>
https://codereview.chromium.org/1259493003/diff/20001/chrome/browser/metrics/...
>
>> File chrome/browser/metrics/plugin_metrics_provider_unittest.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1259493003/diff/20001/chrome/browser/metrics/...
>
>> chrome/browser/metrics/plugin_metrics_provider_unittest.cc:177:
>> provider.BrowserChildProcessCrashed(child_process_data, 2);
>> Hmm, is this a realistic scenario? Seems like if there's only two
>> instances
>>
> and
>
>> we get those 3 messages, we'd be double counting crashes? e.g. if an
>> instance
>> both crashed and disconnected presumably we only want to count it once?
>>
>
> I mean, maybe that's not a thing that would happen in practice. But in that
>> case, the test shouldn't be simulating it. But if it is a thing might
>> actually
>> happen in practice, then maybe the class needs extra logic to de-dup them
>>
> (e.g.
>
>> by having the disconnect message take the instance id and keeping track of
>>
> which
>
>> ones already crashed?).
>>
>
> no - this exact situation should never happen. I wanted to test that a
> plugin
> with the same name would have two crashes recorded. I don't think we need
> to
> complicate the logic inside PluginMetricsProvider to handle this case.
>
> I can just change the test to be more realistic, i.e. two instances, one
> crash
> and one disconnect. WDYT?
>
> https://codereview.chromium.org/1259493003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Will Harris, 2015-08-03 17:08:24]

done. PTAL

[Alexei Svitkine (slow), 2015-08-03 17:11:05]

lgtm

[Will Harris, 2015-08-03 17:13:12]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2015-08-03 17:13:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259493003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259493003/40001

[commit-bot: I haz the power, 2015-08-03 18:43:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-03 18:43:38]

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