Finish moving plugin probing out of process.

Finish moving plugin probing out of process.

This moves all browser-side synchronous callers to use the asynchronous
PluginService interface.

BUG=17863, 95114
TEST=Covered by tests. Plugins still work.

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

[jam, 2011-10-04 22:42:30]

The code lgtm, but it's hard to follow every place where each of the old
functions were called and would plugins if it were stale with now, where it
would not. I'm worried that where before we might have done disk io on the wrong
thread, now we break. Can we add CHECKs to make sure that when these methods are
called the plugins exist?

http://codereview.chromium.org/8071013/diff/5001/content/browser/plugin_servi...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/8071013/diff/5001/content/browser/plugin_servi...
content/browser/plugin_service.h:58: struct ForwardOpenChannelParams;
nit: seems a little odd to forward declare stuff in an anonymous namespace. if
you want to forward declare it, usually we put make it a private member

[Bernhard Bauer, 2011-10-05 13:00:45]

http://codereview.chromium.org/8071013/diff/5001/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover_helper.cc (right):

http://codereview.chromium.org/8071013/diff/5001/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover_helper.cc:21: // The internal class is
refcounted so it can outlive PluginDataRemoverHelper.
If we're doing everything on the UI thread now, this class isn't necessary
anymore. Can you add a TODO(bauerb) to replace it in favor of using a
WeakPtrFactory?

http://codereview.chromium.org/8071013/diff/5001/content/browser/plugin_servi...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/8071013/diff/5001/content/browser/plugin_servi...
content/browser/plugin_service.h:201: // because more arity is needed
<http://crbug.com/98542>. This just forwards.
I'm running into the same issue in http://codereview.chromium.org/7990005/.
Could we maybe pull out the parameters that are only passed as context to the
PluginServiceFilter (i.e. render_process_id, render_view_id, page_url,
resource_context) so I can use the same struct?

[Robert Sesek, 2011-10-05 18:27:16]

On 2011/10/04 22:42:30, John Abd-El-Malek wrote:
> The code lgtm, but it's hard to follow every place where each of the old
> functions were called and would plugins if it were stale with now, where it
> would not. I'm worried that where before we might have done disk io on the
wrong
> thread, now we break. Can we add CHECKs to make sure that when these methods
are
> called the plugins exist?

Hmm. Where would you add the CHECK, though? The return value for
GetPluginInfoByPath et al indicates (plugins_loaded && plugin_found). If the
plugin is disabled, then that CHECK would trip.

http://codereview.chromium.org/8071013/diff/5001/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover_helper.cc (right):

http://codereview.chromium.org/8071013/diff/5001/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover_helper.cc:21: // The internal class is
refcounted so it can outlive PluginDataRemoverHelper.
On 2011/10/05 13:00:46, Bernhard Bauer wrote:
> If we're doing everything on the UI thread now, this class isn't necessary
> anymore. Can you add a TODO(bauerb) to replace it in favor of using a
> WeakPtrFactory?

Done.

http://codereview.chromium.org/8071013/diff/5001/content/browser/plugin_servi...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/8071013/diff/5001/content/browser/plugin_servi...
content/browser/plugin_service.h:201: // because more arity is needed
<http://crbug.com/98542>. This just forwards.
On 2011/10/05 13:00:46, Bernhard Bauer wrote:
> I'm running into the same issue in http://codereview.chromium.org/7990005/.
> Could we maybe pull out the parameters that are only passed as context to the
> PluginServiceFilter (i.e. render_process_id, render_view_id, page_url,
> resource_context) so I can use the same struct?

Done.

[Bernhard Bauer, 2011-10-06 10:10:13]

On 2011/10/05 18:27:16, rsesek wrote:
> On 2011/10/04 22:42:30, John Abd-El-Malek wrote:
> > The code lgtm, but it's hard to follow every place where each of the old
> > functions were called and would plugins if it were stale with now, where it
> > would not. I'm worried that where before we might have done disk io on the
> wrong
> > thread, now we break. Can we add CHECKs to make sure that when these methods
> are
> > called the plugins exist?
> 
> Hmm. Where would you add the CHECK, though? The return value for
> GetPluginInfoByPath et al indicates (plugins_loaded && plugin_found). If the
> plugin is disabled, then that CHECK would trip.

Whether a plug-in is disabled or not shouldn't figure into this anymore, as that
is now stored in PluginPrefs. PluginList always returns all plug-ins it knows
about.

>
http://codereview.chromium.org/8071013/diff/5001/chrome/browser/plugin_data_r...
> File chrome/browser/plugin_data_remover_helper.cc (right):
> 
>
http://codereview.chromium.org/8071013/diff/5001/chrome/browser/plugin_data_r...
> chrome/browser/plugin_data_remover_helper.cc:21: // The internal class is
> refcounted so it can outlive PluginDataRemoverHelper.
> On 2011/10/05 13:00:46, Bernhard Bauer wrote:
> > If we're doing everything on the UI thread now, this class isn't necessary
> > anymore. Can you add a TODO(bauerb) to replace it in favor of using a
> > WeakPtrFactory?
> 
> Done.
> 
>
http://codereview.chromium.org/8071013/diff/5001/content/browser/plugin_servi...
> File content/browser/plugin_service.h (right):
> 
>
http://codereview.chromium.org/8071013/diff/5001/content/browser/plugin_servi...
> content/browser/plugin_service.h:201: // because more arity is needed
> <http://crbug.com/98542>. This just forwards.
> On 2011/10/05 13:00:46, Bernhard Bauer wrote:
> > I'm running into the same issue in http://codereview.chromium.org/7990005/.
> > Could we maybe pull out the parameters that are only passed as context to
the
> > PluginServiceFilter (i.e. render_process_id, render_view_id, page_url,
> > resource_context) so I can use the same struct?
> 
> Done.

[Robert Sesek, 2011-10-06 18:08:17]

On 2011/10/06 10:10:13, Bernhard Bauer wrote:
> On 2011/10/05 18:27:16, rsesek wrote:
> > On 2011/10/04 22:42:30, John Abd-El-Malek wrote:
> > > The code lgtm, but it's hard to follow every place where each of the old
> > > functions were called and would plugins if it were stale with now, where
it
> > > would not. I'm worried that where before we might have done disk io on the
> > wrong
> > > thread, now we break. Can we add CHECKs to make sure that when these
methods
> > are
> > > called the plugins exist?
> > 
> > Hmm. Where would you add the CHECK, though? The return value for
> > GetPluginInfoByPath et al indicates (plugins_loaded && plugin_found). If the
> > plugin is disabled, then that CHECK would trip.
> 
> Whether a plug-in is disabled or not shouldn't figure into this anymore, as
that
> is now stored in PluginPrefs. PluginList always returns all plug-ins it knows
> about.

Could we avoid this whole issue by kicking off a startup task to load the
plugins initially? We can do it delayed by a few seconds to ensure we don't
regress startup time, but that way plugins can't ever be in the unloaded state.

[Bernhard Bauer, 2011-10-07 09:11:23]

On 2011/10/06 18:08:17, rsesek wrote:
> On 2011/10/06 10:10:13, Bernhard Bauer wrote:
> > On 2011/10/05 18:27:16, rsesek wrote:
> > > On 2011/10/04 22:42:30, John Abd-El-Malek wrote:
> > > > The code lgtm, but it's hard to follow every place where each of the old
> > > > functions were called and would plugins if it were stale with now, where
> it
> > > > would not. I'm worried that where before we might have done disk io on
the
> > > wrong
> > > > thread, now we break. Can we add CHECKs to make sure that when these
> methods
> > > are
> > > > called the plugins exist?
> > > 
> > > Hmm. Where would you add the CHECK, though? The return value for
> > > GetPluginInfoByPath et al indicates (plugins_loaded && plugin_found). If
the
> > > plugin is disabled, then that CHECK would trip.
> > 
> > Whether a plug-in is disabled or not shouldn't figure into this anymore, as
> that
> > is now stored in PluginPrefs. PluginList always returns all plug-ins it
knows
> > about.
> 
> Could we avoid this whole issue by kicking off a startup task to load the
> plugins initially? We can do it delayed by a few seconds to ensure we don't
> regress startup time, but that way plugins can't ever be in the unloaded
state.

As long as we don't immediately load plug-ins at startup (which we won't do),
there's still a time window where we don't have the list available. Loading them
delayed would only decrease the window, which would also decrease our chances of
finding out that we access the plug-in list before it's loaded.

[Bernhard Bauer, 2011-10-07 09:12:54]

On 2011/10/07 09:11:23, Bernhard Bauer wrote:
> On 2011/10/06 18:08:17, rsesek wrote:
> > On 2011/10/06 10:10:13, Bernhard Bauer wrote:
> > > On 2011/10/05 18:27:16, rsesek wrote:
> > > > On 2011/10/04 22:42:30, John Abd-El-Malek wrote:
> > > > > The code lgtm, but it's hard to follow every place where each of the
old
> > > > > functions were called and would plugins if it were stale with now,
where
> > it
> > > > > would not. I'm worried that where before we might have done disk io on
> the
> > > > wrong
> > > > > thread, now we break. Can we add CHECKs to make sure that when these
> > methods
> > > > are
> > > > > called the plugins exist?
> > > > 
> > > > Hmm. Where would you add the CHECK, though? The return value for
> > > > GetPluginInfoByPath et al indicates (plugins_loaded && plugin_found). If
> the
> > > > plugin is disabled, then that CHECK would trip.
> > > 
> > > Whether a plug-in is disabled or not shouldn't figure into this anymore,
as
> > that
> > > is now stored in PluginPrefs. PluginList always returns all plug-ins it
> knows
> > > about.
> > 
> > Could we avoid this whole issue by kicking off a startup task to load the
> > plugins initially? We can do it delayed by a few seconds to ensure we don't
> > regress startup time, but that way plugins can't ever be in the unloaded
> state.
> 
> As long as we don't immediately load plug-ins at startup (which we won't do),
> there's still a time window where we don't have the list available. Loading
them
> delayed would only decrease the window, which would also decrease our chances
of
> finding out that we access the plug-in list before it's loaded.

(I'd be perfectly fine with doing the CHECK in an additional CL though; I don't
want to block this one.)

[jam, 2011-10-07 16:10:07]

On 2011/10/06 18:08:17, rsesek wrote:
> On 2011/10/06 10:10:13, Bernhard Bauer wrote:
> > On 2011/10/05 18:27:16, rsesek wrote:
> > > On 2011/10/04 22:42:30, John Abd-El-Malek wrote:
> > > > The code lgtm, but it's hard to follow every place where each of the old
> > > > functions were called and would plugins if it were stale with now, where
> it
> > > > would not. I'm worried that where before we might have done disk io on
the
> > > wrong
> > > > thread, now we break. Can we add CHECKs to make sure that when these
> methods
> > > are
> > > > called the plugins exist?
> > > 
> > > Hmm. Where would you add the CHECK, though? The return value for
> > > GetPluginInfoByPath et al indicates (plugins_loaded && plugin_found). If
the
> > > plugin is disabled, then that CHECK would trip.
> > 
> > Whether a plug-in is disabled or not shouldn't figure into this anymore, as
> that
> > is now stored in PluginPrefs. PluginList always returns all plug-ins it
knows
> > about.
> 
> Could we avoid this whole issue by kicking off a startup task to load the
> plugins initially? We can do it delayed by a few seconds to ensure we don't
> regress startup time, but that way plugins can't ever be in the unloaded
state.

This already happens from MetricsService. It's after 30s I believe. We wouldn't
want to do it too quick, in case that interfers with profile loading etc.
However, no matter how soon we start loading them, we want to make sure that all
code paths that get the cached version must have went through an async path
previously to wait on plugin loading.

btw if it wasn't clear, this is lgtm as is, this is just something to think
about and do in a future cl