Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1275)

Issue 8603012: Fix race in PluginDataRemoverImpl going away while it's still being used on the IO thread... (Closed)

Created:
9 years, 1 month ago by jam
Modified:
9 years, 1 month ago
Reviewers:
Bernhard Bauer
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, dpranke-watch+content_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 3

Patch Set 3 : Move Context class to cc file #

Unified diffs Side-by-side diffs Delta from patch set Stats (+171 lines, -156 lines) Patch
M content/browser/plugin_data_remover_impl.h View 1 2 2 chunks +7 lines, -43 lines 0 comments Download
M content/browser/plugin_data_remover_impl.cc View 1 2 2 chunks +153 lines, -104 lines 0 comments Download
M content/browser/plugin_service.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/plugin_service_browsertest.cc View 1 2 3 chunks +10 lines, -9 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
jam
9 years, 1 month ago (2011-11-19 22:58:32 UTC) #1
jam
actually set title
9 years, 1 month ago (2011-11-19 22:59:08 UTC) #2
Bernhard Bauer
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 content/browser/plugin_data_remover_impl.h:30: class Context : public PluginProcessHost::Client, Is there any advantage ...
9 years, 1 month ago (2011-11-21 14:33:59 UTC) #3
jam
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 content/browser/plugin_data_remover_impl.h:30: class Context : public PluginProcessHost::Client, On 2011/11/21 14:33:59, Bernhard ...
9 years, 1 month ago (2011-11-21 17:29:18 UTC) #4
Bernhard Bauer
LGTM. 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 content/browser/plugin_data_remover_impl.h:30: class Context : public PluginProcessHost::Client, On 2011/11/21 17:29:18, ...
9 years, 1 month ago (2011-11-21 18:07:09 UTC) #5
jam
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 > File content/browser/plugin_data_remover_impl.h (right): ...
9 years, 1 month ago (2011-11-21 18:10:04 UTC) #6
Bernhard Bauer
On 2011/11/21 18:10:04, John Abd-El-Malek wrote: > On 2011/11/21 18:07:09, Bernhard Bauer wrote: > > ...
9 years, 1 month ago (2011-11-21 18:30:35 UTC) #7
jam
9 years, 1 month ago (2011-11-21 18:34:36 UTC) #8
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...
>

Powered by Google App Engine
This is Rietveld 408576698