Fix race in PluginDataRemoverImpl going away while it's still being used on the IO thread...

Fix race in PluginDataRemoverImpl going away while it's still being used on the IO thread, which was introduced in 110530. This also fix the incorrect usage of PluginService::OpenChannelToNpapiPlugin on the UI thread which existed before.

BUG=104553, cros:23179
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110979

[jam, 2011-11-19 22:59:08]

actually set title

[Bernhard Bauer, 2011-11-21 14:33:59]

http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
File content/browser/plugin_data_remover_impl.h (right):

http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
content/browser/plugin_data_remover_impl.h:30: class Context : public
PluginProcessHost::Client,
Is there any advantage to using a inner Context class instead of just making
PluginDataRemover refcounted again?

[jam, 2011-11-21 17:29:18]

http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
File content/browser/plugin_data_remover_impl.h (right):

http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
content/browser/plugin_data_remover_impl.h:30: class Context : public
PluginProcessHost::Client,
On 2011/11/21 14:33:59, Bernhard Bauer wrote:
> Is there any advantage to using a inner Context class instead of just making
> PluginDataRemover refcounted again?

Yes, keeping this simpler for embedders. Now they can destroy
content::PluginDataRemove on the UI thread and not have to worry that it's being
used elsewhere. It's also better to hide the implementation details (i.e. that
it's refcounted) from the embedder.

[Bernhard Bauer, 2011-11-21 18:07:09]

LGTM.

http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
File content/browser/plugin_data_remover_impl.h (right):

http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
content/browser/plugin_data_remover_impl.h:30: class Context : public
PluginProcessHost::Client,
On 2011/11/21 17:29:18, John Abd-El-Malek wrote:
> On 2011/11/21 14:33:59, Bernhard Bauer wrote:
> > Is there any advantage to using a inner Context class instead of just making
> > PluginDataRemover refcounted again?
> 
> Yes, keeping this simpler for embedders. Now they can destroy
> content::PluginDataRemove on the UI thread and not have to worry that it's
being
> used elsewhere. It's also better to hide the implementation details (i.e. that
> it's refcounted) from the embedder.

OK. The destruction thing could be achieved with DeleteOnIOThread though.

[jam, 2011-11-21 18:10:04]

On 2011/11/21 18:07:09, Bernhard Bauer wrote:
> LGTM.
> 
>
http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
> File content/browser/plugin_data_remover_impl.h (right):
> 
>
http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
> content/browser/plugin_data_remover_impl.h:30: class Context : public
> PluginProcessHost::Client,
> On 2011/11/21 17:29:18, John Abd-El-Malek wrote:
> > On 2011/11/21 14:33:59, Bernhard Bauer wrote:
> > > Is there any advantage to using a inner Context class instead of just
making
> > > PluginDataRemover refcounted again?
> > 
> > Yes, keeping this simpler for embedders. Now they can destroy
> > content::PluginDataRemove on the UI thread and not have to worry that it's
> being
> > used elsewhere. It's also better to hide the implementation details (i.e.
that
> > it's refcounted) from the embedder.
> 
> OK. The destruction thing could be achieved with DeleteOnIOThread though.

it's error prone to tell embedders that you have to create an object on thread
X, but delete on thread Y :)

[Bernhard Bauer, 2011-11-21 18:30:35]

On 2011/11/21 18:10:04, John Abd-El-Malek wrote:
> On 2011/11/21 18:07:09, Bernhard Bauer wrote:
> > LGTM.
> > 
> >
>
http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
> > File content/browser/plugin_data_remover_impl.h (right):
> > 
> >
>
http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_...
> > content/browser/plugin_data_remover_impl.h:30: class Context : public
> > PluginProcessHost::Client,
> > On 2011/11/21 17:29:18, John Abd-El-Malek wrote:
> > > On 2011/11/21 14:33:59, Bernhard Bauer wrote:
> > > > Is there any advantage to using a inner Context class instead of just
> making
> > > > PluginDataRemover refcounted again?
> > > 
> > > Yes, keeping this simpler for embedders. Now they can destroy
> > > content::PluginDataRemove on the UI thread and not have to worry that it's
> > being
> > > used elsewhere. It's also better to hide the implementation details (i.e.
> that
> > > it's refcounted) from the embedder.
> > 
> > OK. The destruction thing could be achieved with DeleteOnIOThread though.
> 
> it's error prone to tell embedders that you have to create an object on thread
> X, but delete on thread Y :)

Sorry, I meant deriving from RefCountedThreadSafe<T, DeleteOnIOThread<T> >. That
should do it automatically, right?

Another small thing: we could move the definition of the Context class to the
implementation file.

[jam, 2011-11-21 18:34:36]

On Mon, Nov 21, 2011 at 10:30 AM, <bauerb@chromium.org> wrote:

> On 2011/11/21 18:10:04, John Abd-El-Malek wrote:
>
>  On 2011/11/21 18:07:09, Bernhard Bauer wrote:
>> > LGTM.
>> >
>> >
>>
>
> http://codereview.chromium.**org/8603012/diff/1010/content/**
>
browser/plugin_data_remover_**impl.h<http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_remover_impl.h>
>
>> > File content/browser/plugin_data_**remover_impl.h (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/8603012/diff/1010/content/**
>
browser/plugin_data_remover_**impl.h#newcode30<http://codereview.chromium.org/8603012/diff/1010/content/browser/plugin_data_remover_impl.h#newcode30>
>
>> > content/browser/plugin_data_**remover_impl.h:30: class Context : public
>> > PluginProcessHost::Client,
>> > On 2011/11/21 17:29:18, John Abd-El-Malek wrote:
>> > > On 2011/11/21 14:33:59, Bernhard Bauer wrote:
>> > > > Is there any advantage to using a inner Context class instead of
>> just
>> making
>> > > > PluginDataRemover refcounted again?
>> > >
>> > > Yes, keeping this simpler for embedders. Now they can destroy
>> > > content::PluginDataRemove on the UI thread and not have to worry that
>> it's
>> > being
>> > > used elsewhere. It's also better to hide the implementation details
>> (i.e.
>> that
>> > > it's refcounted) from the embedder.
>> >
>> > OK. The destruction thing could be achieved with DeleteOnIOThread
>> though.
>>
>
>  it's error prone to tell embedders that you have to create an object on
>> thread
>> X, but delete on thread Y :)
>>
>
> Sorry, I meant deriving from RefCountedThreadSafe<T, DeleteOnIOThread<T>
> >. That
> should do it automatically, right?
>

If the impl derives from RefCountedThreadSafe, and the interface doesn't,
then the embedder will delete this object directly (instead of going
through Release()), which will cause bugs (since other places could be
holding on to references).


> Another small thing: we could move the definition of the Context class to
> the
> implementation file.
>

good point, i'll do that.


>
http://codereview.chromium.**org/8603012/<http://codereview.chromium.org/8603...
>