Include PPAPI plugin crashes in the plugin crash data.

Include PPAPI plugin crashes in the plugin crash data.

This should enable us to see PPAPI plugin crashes in the plugin crashes column
on the stability dashboard. Note that each plugin (PPAPI or regular) is included
separately in the metrics log and aggregated at the server side. This allows us
to slice and dice the data later, if necessary.

Also:

- Remove a couple of obsolete FRIEND_TEST declarations.

- Use base file name as plugin name when the name of the PPAPI plugin to be
  launched is empty. This is mostly so that the OutOfProcessPPAPITests can pass
  but also matches the logic in PepperPluginInfo::ToWebPluginInfo().

BUG=chromium-os:22465
TEST=git try, unit tests, kill -SEGV ppapi process, check that it's counted in
the plugin prefs and transmitted in the metrics log:
<pluginstability filename="nfGm7uk2q0xBpC3X5CA95A==" launchcount="3"
                 instancecount="1" crashcount="2"/>


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109627

[petkov, 2011-11-08 15:45:35]

PTAL. Reason for including each reviewer:

piman: because the patch is Chrome OS / PPAPI related.

jar: because the patch is metrics related.

jam: because the patch touches content/browser/ppapi_plugin_process_host.cc.

cc'ing rtc since he requested this crash bucketing for Chrome OS.

[jam, 2011-11-08 18:21:57]

On 2011/11/08 15:45:35, petkov wrote:
> PTAL. Reason for including each reviewer:
> 
> piman: because the patch is Chrome OS / PPAPI related.
> 
> jar: because the patch is metrics related.
> 
> jam: because the patch touches content/browser/ppapi_plugin_process_host.cc.
> 
> cc'ing rtc since he requested this crash bucketing for Chrome OS.

It's not clear to me why the chrome code should be modified to suit the
dashboard needs for cros. If you really want to count both as plugin, then do it
in the dashboard. It's much easier to sum two numbers in a script, then to later
want to split them up.

[petkov, 2011-11-08 18:44:27]

On 2011/11/08 18:21:57, John Abd-El-Malek wrote:
> On 2011/11/08 15:45:35, petkov wrote:
> > PTAL. Reason for including each reviewer:
> > 
> > piman: because the patch is Chrome OS / PPAPI related.
> > 
> > jar: because the patch is metrics related.
> > 
> > jam: because the patch touches content/browser/ppapi_plugin_process_host.cc.
> > 
> > cc'ing rtc since he requested this crash bucketing for Chrome OS.
> 
> It's not clear to me why the chrome code should be modified to suit the
> dashboard needs for cros. If you really want to count both as plugin, then do
it
> in the dashboard. It's much easier to sum two numbers in a script, then to
later
> want to split them up.

1. This patch splits off the PPAPI crashes from the rest of the crashes that are
already grouped together and adds them to the plugin bucket instead. For
example, right now we can't distinguish GPU crashes from PPAPI plugin crashes on
the server side.

2. The only Chrome OS non-PPAPI plugin is GTalk (and my understanding is that
this will change).

So, for Chrome OS, the plugin bucket is basically empty right now and we move
the PPAPI plugin crashes into it.

[jam, 2011-11-08 18:49:14]

On 2011/11/08 18:44:27, petkov wrote:
> On 2011/11/08 18:21:57, John Abd-El-Malek wrote:
> > On 2011/11/08 15:45:35, petkov wrote:
> > > PTAL. Reason for including each reviewer:
> > > 
> > > piman: because the patch is Chrome OS / PPAPI related.
> > > 
> > > jar: because the patch is metrics related.
> > > 
> > > jam: because the patch touches
content/browser/ppapi_plugin_process_host.cc.
> > > 
> > > cc'ing rtc since he requested this crash bucketing for Chrome OS.
> > 
> > It's not clear to me why the chrome code should be modified to suit the
> > dashboard needs for cros. If you really want to count both as plugin, then
do
> it
> > in the dashboard. It's much easier to sum two numbers in a script, then to
> later
> > want to split them up.
> 
> 1. This patch splits off the PPAPI crashes from the rest of the crashes that
are
> already grouped together and adds them to the plugin bucket instead. For
> example, right now we can't distinguish GPU crashes from PPAPI plugin crashes
on
> the server side.

If the problem that we're grouping all these crashes in the server ping, then it
seems that the solution should be to list ppapi crashes separately.

> 
> 2. The only Chrome OS non-PPAPI plugin is GTalk (and my understanding is that
> this will change).

I think this is orthogonal.

> 
> So, for Chrome OS, the plugin bucket is basically empty right now and we move
> the PPAPI plugin crashes into it.

ditto.


Another important point, which I believe is already communicated to the cros
team, is that we really want to minimize CHROME_OS ifdefs in chrome code. In
this case, I don't see any strong reasons why the chrome code needs to do
anything differently for cros. if cros' dashboard wants better data, then let's
send pappi crashes apart from gpu crashes (which seems like a good change) and
update cros' dashboard.

[jar (doing other things), 2011-11-08 22:16:31]

http://codereview.chromium.org/8501005/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/8501005/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:1528: case
ChildProcessInfo::PPAPI_PLUGIN_PROCESS:
Could you just use a traditional logical ||, and not bother with the ifdef
OS_CHROMEOS?  I think the process type enum is defined in both cases, and the
code would be much more readable.

http://codereview.chromium.org/8501005/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service_unittest.cc (right):

http://codereview.chromium.org/8501005/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service_unittest.cc:43:
MetricsService::IsPluginProcess(ChildProcessInfo::PPAPI_PLUGIN_PROCESS));
This ifdef should also go away when you use the logical or in the definition.

[petkov, 2011-11-09 14:21:07]

Thanks for the reviews. There might be little downside to doing what jar@
suggested and always assign the PPAPI plugin stats into separate plugin metrics
elements, regardless of the platform. Note that the stats for each plugin (PPAPI
or regular) are reported separately to the server. Currently, the server will
aggregate both PPAPI and regular plugin crashes into the plugin bucket but this
can be changed on the server side, if needed. This way we send maximum detail
from the client and avoid ifdefs. We may still want to send a plugin type bit at
some point but this doesn't seem necessary right now.

I've updated the patch along with the description and 
summary. PTAL and let me know what you think.

http://codereview.chromium.org/8501005/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/8501005/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:1528: case
ChildProcessInfo::PPAPI_PLUGIN_PROCESS:
On 2011/11/08 22:16:31, jar wrote:
> Could you just use a traditional logical ||, and not bother with the ifdef
> OS_CHROMEOS?  I think the process type enum is defined in both cases, and the
> code would be much more readable.

Done.

http://codereview.chromium.org/8501005/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service_unittest.cc (right):

http://codereview.chromium.org/8501005/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service_unittest.cc:43:
MetricsService::IsPluginProcess(ChildProcessInfo::PPAPI_PLUGIN_PROCESS));
On 2011/11/08 22:16:31, jar wrote:
> This ifdef should also go away when you use the logical or in the definition.

Done.

[piman, 2011-11-09 16:38:17]

lgtm

[jam, 2011-11-09 17:42:27]

lgtm

[jar (doing other things), 2011-11-09 20:07:25]

Metrics code LGTM

[commit-bot: I haz the power, 2011-11-11 13:21:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petkov@chromium.org/8501005/4003

[commit-bot: I haz the power, 2011-11-11 14:30:45]

Change committed as 109627

[laforge, 2011-11-16 18:55:50]

On 2011/11/11 14:30:45, I haz the power (commit-bot) wrote:
> Change committed as 109627

Could you add a histogram so that we have a means of counting the ppapi counts
separately (see contribution numbers)?

[petkov, 2011-11-16 19:23:56]

On 2011/11/16 18:55:50, laforge wrote:
> On 2011/11/11 14:30:45, I haz the power (commit-bot) wrote:
> > Change committed as 109627
> 
> Could you add a histogram so that we have a means of counting the ppapi counts
> separately (see contribution numbers)?

Done already -- ChildProcess.Crashed. Note that the existing
ChildProcess.Crashes is wrong because the bucket intervals are exponential
rather than linear -- see http://codereview.chromium.org/8356042/