Log when a browser plugin hangs

Log when a browser plugin hangs

Add an API to webplugin_delegate_impl to get the plugin version from the window, similar to the way to get the plugin name. Update hung_plugin_action to log when a browser plugin hangs and log the version of the plugin is Google Talk Plugin. 

BUG=
TEST=try bot and manual test on Google Talk Plugin hangs


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

[jam, 2012-04-03 16:18:02]

some nits

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
webkit/plugins/npapi/webplugin_delegate_impl.h:99: string16* plugin_name);
nit: plugin_version

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
File webkit/plugins/npapi/webplugin_delegate_impl_win.cc (right):

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
webkit/plugins/npapi/webplugin_delegate_impl_win.cc:270: if (NULL ==
plugin_atom_property || NULL == plugin_property) {
nit: this isn't necessary, if someone passes in NULL we'll get a crash dump and
quickly fix it

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
webkit/plugins/npapi/webplugin_delegate_impl_win.cc:300:
GetPluginPropertyFromWindow(
nit: here and below, indentation is 4 spaces

[James Cook, 2012-04-03 16:34:08]

Did you mean a different James?  I'm not familiar with this code.

[whyuan, 2012-04-03 17:35:07]

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
File webkit/plugins/npapi/webplugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
webkit/plugins/npapi/webplugin_delegate_impl.h:99: string16* plugin_name);
On 2012/04/03 16:18:02, John Abd-El-Malek wrote:
> nit: plugin_version

Done.

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
File webkit/plugins/npapi/webplugin_delegate_impl_win.cc (right):

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
webkit/plugins/npapi/webplugin_delegate_impl_win.cc:270: if (NULL ==
plugin_atom_property || NULL == plugin_property) {
On 2012/04/03 16:18:02, John Abd-El-Malek wrote:
> nit: this isn't necessary, if someone passes in NULL we'll get a crash dump
and
> quickly fix it

Done.

https://chromiumcodereview.appspot.com/9958104/diff/1/webkit/plugins/npapi/we...
webkit/plugins/npapi/webplugin_delegate_impl_win.cc:300:
GetPluginPropertyFromWindow(
On 2012/04/03 16:18:02, John Abd-El-Malek wrote:
> nit: here and below, indentation is 4 spaces

Done.

[jar (doing other things), 2012-04-03 19:20:38]

Please do not land (per comments below).

Ilya: Do you have comments or suggestions that might better fit in with your
protobuf plans?

Another (long standing) TODO is to implement "Sparse Slow Histograms using
locks" to handle cases like this.  That is currently on the plausible TODO list
for several developers, including Jeff, Raman, or possibly Kai... but has not
been scheduled.

https://chromiumcodereview.appspot.com/9958104/diff/4001/chrome/browser/hang_...
File chrome/browser/hang_monitor/hung_plugin_action.cc (right):

https://chromiumcodereview.appspot.com/9958104/diff/4001/chrome/browser/hang_...
chrome/browser/hang_monitor/hung_plugin_action.cc:136:
UMA_HISTOGRAM_ENUMERATION("GTalkPlugin.Hung_Terminate_Process",
We don't have a sparse histogram, and so the cost (in memory) of this histogram
will be very large.  With about 4 bytes per bucket for the sample counts, and 4
bytes per bucket for the ranges, this totals 80KB.  In addition, the snapshot of
each upload will cost about 40KB, for a grand total of 120KB of data space.

Until we have a slow/sparse histogram implemented, I think you should avoid
using histograms in this way.

We do  send up a list of plugins (and versions), but historically we did that
only in the initial log.  You could decide here that it was the GTalkPlugin, and
histogram failures, but leave it to secondary analysis to identify the cause. 
It is also possible that the newer proto-buf approach may include additional
plugin info in each log transmission... and that might make this a lot simpler. 
Perhaps we need to (at a minimum) upload version info for Google plugins all the
time, if you don't want to have to process this out after the fact.

https://chromiumcodereview.appspot.com/9958104/diff/4001/chrome/browser/hang_...
chrome/browser/hang_monitor/hung_plugin_action.cc:143: gtalk_plugin_version,
kMaxGTalkPluginVersion + 1);
This would be another 120KB :-/.

[Ilya Sherman, 2012-04-03 19:31:17]

Rather than creating histograms for this, have you considered adding a hang
count to the PluginStability message in the UMA SystemProfile message [1]?
[1]
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/metrics/proto/s...

(Note that this change would also a require a server-side change to update the
protocol buffer definition.  If you decide to pursue this approach, I can help
you get the server-side changes in place.)

[whyuan, 2012-04-09 06:57:57]

limit to log 10 versions of gtalk plugin
PTAL
 On Apr 3, 2012 12:20 PM, <jar@chromium.org> wrote:

> Please do not land (per comments below).
>
> Ilya: Do you have comments or suggestions that might better fit in with
> your
> protobuf plans?
>
> Another (long standing) TODO is to implement "Sparse Slow Histograms using
> locks" to handle cases like this.  That is currently on the plausible TODO
> list
> for several developers, including Jeff, Raman, or possibly Kai... but has
> not
> been scheduled.
>
>
> https://chromiumcodereview.**appspot.com/9958104/diff/4001/**
>
chrome/browser/hang_monitor/**hung_plugin_action.cc<https://chromiumcodereview.appspot.com/9958104/diff/4001/chrome/browser/hang_monitor/hung_plugin_action.cc>
> File chrome/browser/hang_monitor/**hung_plugin_action.cc (right):
>
> https://chromiumcodereview.**appspot.com/9958104/diff/4001/**
>
chrome/browser/hang_monitor/**hung_plugin_action.cc#**newcode136<https://chromiumcodereview.appspot.com/9958104/diff/4001/chrome/browser/hang_monitor/hung_plugin_action.cc#newcode136>
> chrome/browser/hang_monitor/**hung_plugin_action.cc:136:
> UMA_HISTOGRAM_ENUMERATION("**GTalkPlugin.Hung_Terminate_**Process",
> We don't have a sparse histogram, and so the cost (in memory) of this
> histogram will be very large.  With about 4 bytes per bucket for the
> sample counts, and 4 bytes per bucket for the ranges, this totals 80KB.
> In addition, the snapshot of each upload will cost about 40KB, for a
> grand total of 120KB of data space.
>
> Until we have a slow/sparse histogram implemented, I think you should
> avoid using histograms in this way.
>
> We do  send up a list of plugins (and versions), but historically we did
> that only in the initial log.  You could decide here that it was the
> GTalkPlugin, and histogram failures, but leave it to secondary analysis
> to identify the cause.  It is also possible that the newer proto-buf
> approach may include additional plugin info in each log transmission...
> and that might make this a lot simpler.  Perhaps we need to (at a
> minimum) upload version info for Google plugins all the time, if you
> don't want to have to process this out after the fact.
>
> https://chromiumcodereview.**appspot.com/9958104/diff/4001/**
>
chrome/browser/hang_monitor/**hung_plugin_action.cc#**newcode143<https://chromiumcodereview.appspot.com/9958104/diff/4001/chrome/browser/hang_monitor/hung_plugin_action.cc#newcode143>
> chrome/browser/hang_monitor/**hung_plugin_action.cc:143:
> gtalk_plugin_version, kMaxGTalkPluginVersion + 1);
> This would be another 120KB :-/.
>
>
https://chromiumcodereview.**appspot.com/9958104/<https://chromiumcodereview....
>

[jar (doing other things), 2012-04-11 22:26:59]

Small nits below.  I'm now fine with the use of histograms, but you should get
an L G T M from the from jam (or folks more familiar with the driving code).

https://chromiumcodereview.appspot.com/9958104/diff/7001/chrome/browser/hang_...
File chrome/browser/hang_monitor/hung_plugin_action.cc (right):

https://chromiumcodereview.appspot.com/9958104/diff/7001/chrome/browser/hang_...
chrome/browser/hang_monitor/hung_plugin_action.cc:51: } else if
(gtalk_plugin_version > GTALK_PLUGIN_VERSION_MAX) {
nit: Since you return, you don't need else clauses or curlies.

https://chromiumcodereview.appspot.com/9958104/diff/7001/chrome/browser/hang_...
chrome/browser/hang_monitor/hung_plugin_action.cc:96: GTALK_PLUGIN_VERSION_MAX +
1);
This will create a histogram with the full 37 buckets (which is not too big to
concern me).  Given that you have values 0... 37, you *could* just as well list
them explicitly as map them all to "below your listed minimum."

[whyuan, 2012-04-12 21:02:18]

https://chromiumcodereview.appspot.com/9958104/diff/7001/chrome/browser/hang_...
File chrome/browser/hang_monitor/hung_plugin_action.cc (right):

https://chromiumcodereview.appspot.com/9958104/diff/7001/chrome/browser/hang_...
chrome/browser/hang_monitor/hung_plugin_action.cc:51: } else if
(gtalk_plugin_version > GTALK_PLUGIN_VERSION_MAX) {
On 2012/04/11 22:26:59, jar wrote:
> nit: Since you return, you don't need else clauses or curlies.

Done.

https://chromiumcodereview.appspot.com/9958104/diff/7001/chrome/browser/hang_...
chrome/browser/hang_monitor/hung_plugin_action.cc:96: GTALK_PLUGIN_VERSION_MAX +
1);
Reduced to 11
On 2012/04/11 22:26:59, jar wrote:
> This will create a histogram with the full 37 buckets (which is not too big to
> concern me).  Given that you have values 0... 37, you *could* just as well
list
> them explicitly as map them all to "below your listed minimum."

[jar (doing other things), 2012-04-12 23:10:51]

histogram changes L G T M.... so please get jam to ok it.

Thanks!

[jam, 2012-04-17 23:17:14]

lgtm

[commit-bot: I haz the power, 2012-04-20 16:39:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whyuan@google.com/9958104/16001

[commit-bot: I haz the power, 2012-04-20 16:39:23]

Presubmit check for 9958104-16001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    chrome/browser/hang_monitor/hung_plugin_action.cc:162
    chrome/browser/hang_monitor/hung_plugin_action.cc:163
    chrome/browser/hang_monitor/hung_plugin_action.h:41
    chrome/browser/hang_monitor/hung_plugin_action.h:42
    webkit/plugins/npapi/webplugin_delegate_impl_win.cc:725

Presubmit checks took 1.4s to calculate.

[whyuan, 2012-04-20 18:19:54]

Replace std::wstring with string16. PTAL

On 2012/04/17 23:17:14, John Abd-El-Malek wrote:
> lgtm

[jam, 2012-04-20 19:15:52]

lgtm

[commit-bot: I haz the power, 2012-04-20 19:40:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whyuan@google.com/9958104/32001

[commit-bot: I haz the power, 2012-04-20 23:02:31]

Change committed as 133292