Clean up PluginGroup and related code.

Clean up PluginGroup and related code.

To avoid data races, do not pass pointers to PluginGroup around. Instead, create copies as plain objects.

BUG=61210
TEST=existing unit tests still work; TSan no longer reports the race (see bug)

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

[Jakob Kummerow, 2010-12-03 13:48:05]

Please review. 
Also, please (compile and) run this on Win and/or Mac for a manual sanity check
(no crashes; basic functionality such as dis/enabling plugins working). I'm
still waiting for my own Windows machine...
(Currently the patch is based on trunk@67709, I'll rebase it soon.)

[Bernhard Bauer, 2010-12-03 16:13:28]

There's still some stuff to be done (see the trybot logs for the compile errors
on other platforms), but the CL already looks pretty good!

http://codereview.chromium.org/5516004/diff/1/chrome/browser/mock_plugin_exce...
File chrome/browser/mock_plugin_exceptions_table_model.cc (right):

http://codereview.chromium.org/5516004/diff/1/chrome/browser/mock_plugin_exce...
chrome/browser/mock_plugin_exceptions_table_model.cc:10:
plugins_.push_back(plugins[i]);
I think you could just assign the vector.

http://codereview.chromium.org/5516004/diff/1/chrome/browser/plugin_exception...
File chrome/browser/plugin_exceptions_table_model.cc (right):

http://codereview.chromium.org/5516004/diff/1/chrome/browser/plugin_exception...
chrome/browser/plugin_exceptions_table_model.cc:138: std::vector<PluginGroup>
plugins(GetPlugins());
Are we sure that the compiler optimizes any unnecessary copying away (on all
platforms)?

http://codereview.chromium.org/5516004/diff/1/chrome/browser/ui/views/content...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

http://codereview.chromium.org/5516004/diff/1/chrome/browser/ui/views/content...
chrome/browser/ui/views/content_setting_bubble_contents.cc:234: if
(NPAPI::PluginList::Singleton()->GetPluginGroup(false, *it, &groups))
There's something wrong here. I guess that's why it doesn't compile ;-)

http://codereview.chromium.org/5516004/diff/1/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/5516004/diff/1/chrome/renderer/render_view.h#n...
chrome/renderer/render_view.h:1004: PluginGroup& group);
Reference parameters should be const.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:174: DCHECK(other.web_plugin_infos_.size()
== other.web_plugin_positions_.size());
You could pull this and the version_ constructor calls above out into an Init
method.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:248: const PluginMap* plugin_groups,
If you remove the const here, you can get rid of the const_cast below.

Is this method used anywhere else beside PluginList? If not, you could inline it
there, which could maybe simplify the code.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:464: enabled_ = enable &&
enabled_plugin_exists;
Nit: I think you can leave out the |enable &&| here.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:465: LOG(WARNING) << "Group " <<
GetGroupName() << " is now enabled: " << enabled_;
Nit: Remove logging plz :-)

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_group.h
File webkit/glue/plugins/plugin_group.h (right):

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.h:52: typedef std::map<std::string,
PluginGroup*> PluginMap;
Do we need this typedef outside of PluginGroup or can we make it private?

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.h:58: PluginGroup(const PluginGroup& other);
Nit: make the single-argument constructor explicit.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.h:65: // except by PluginList and unit tests.
PluginList is already a friend, so just declare this method private. 

Also, please explain that the caller takes ownership of the returned object.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list.cc
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.cc:161: next_priority_(0) {
I have a slight suspicion that we might be able to get rid of priorities anyway,
because they're always monotonically increasing in a plugin group and in the
plugin list anyway.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.cc:292: // AddToPluginGroups(plugin_info) .)
Nit: Wrap the code in bars |like this|, then it's still kind of readable if you
put punctuation after it.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.cc:492: return plugin_groups;
Remember back in plugin_exceptions_table_model.cc when I mentioned that I'm not
sure if the compiler optimizes all unnecessary copying away? Here I'm pretty
sure it doesn't ;-)

I think you should change the interface to take a second parameter, and write to
it.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list.h
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.h:165: PluginGroup GetPluginGroup(const
WebPluginInfo& web_plugin_info);
I think you could return a const pointer instead of copying the PluginGroup.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.h:169: string16 GetPluginGroupName(std::string
identifier);
I think you could also return a const PluginGroup* here, which would make this
interface useful to get additional information about a PluginGroup in the
future.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.h:216: // Creates a PluginGroup for the given
WebPluginInfo and adds it to the cache.
// Adds the given WebPluginInfo to its corresponding group, creating it if
necessary, and returns the group.

[Jakob Kummerow, 2010-12-06 18:21:12]

Addressed your comments and (hopefully) fixed the problems reported by the
trybots. Please review again.

http://codereview.chromium.org/5516004/diff/1/chrome/browser/mock_plugin_exce...
File chrome/browser/mock_plugin_exceptions_table_model.cc (right):

http://codereview.chromium.org/5516004/diff/1/chrome/browser/mock_plugin_exce...
chrome/browser/mock_plugin_exceptions_table_model.cc:10:
plugins_.push_back(plugins[i]);
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> I think you could just assign the vector.

Done.

http://codereview.chromium.org/5516004/diff/1/chrome/browser/plugin_exception...
File chrome/browser/plugin_exceptions_table_model.cc (right):

http://codereview.chromium.org/5516004/diff/1/chrome/browser/plugin_exception...
chrome/browser/plugin_exceptions_table_model.cc:138: std::vector<PluginGroup>
plugins(GetPlugins());
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> Are we sure that the compiler optimizes any unnecessary copying away (on all
> platforms)?

Done. (Changed the interface.)

http://codereview.chromium.org/5516004/diff/1/chrome/browser/ui/views/content...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

http://codereview.chromium.org/5516004/diff/1/chrome/browser/ui/views/content...
chrome/browser/ui/views/content_setting_bubble_contents.cc:234: if
(NPAPI::PluginList::Singleton()->GetPluginGroup(false, *it, &groups))
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> There's something wrong here. I guess that's why it doesn't compile ;-)

Done.
This should of course match the code in content_setting_bubble_gtk.cc.

http://codereview.chromium.org/5516004/diff/1/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/5516004/diff/1/chrome/renderer/render_view.h#n...
chrome/renderer/render_view.h:1004: PluginGroup& group);
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> Reference parameters should be const.

Done.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:174: DCHECK(other.web_plugin_infos_.size()
== other.web_plugin_positions_.size());
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> You could pull this and the version_ constructor calls above out into an Init
> method.

Done. Good idea.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:248: const PluginMap* plugin_groups,
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> If you remove the const here, you can get rid of the const_cast below.
> 
> Is this method used anywhere else beside PluginList? If not, you could inline
it
> there, which could maybe simplify the code.

Done.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:464: enabled_ = enable &&
enabled_plugin_exists;
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> Nit: I think you can leave out the |enable &&| here.

Done.
I agree that |enable &&| is superfluous; I left it there because I thought it
enhanced readability (in the sense of easily understanding what's being done
here).

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:465: LOG(WARNING) << "Group " <<
GetGroupName() << " is now enabled: " << enabled_;
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> Nit: Remove logging plz :-)

Done.
(Of course -- that was just a leftover I forgot to remove.)

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_group.h
File webkit/glue/plugins/plugin_group.h (right):

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.h:52: typedef std::map<std::string,
PluginGroup*> PluginMap;
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> Do we need this typedef outside of PluginGroup or can we make it private?

Done. (Made private.)

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.h:58: PluginGroup(const PluginGroup& other);
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> Nit: make the single-argument constructor explicit.

Nope. Let me quote the style guide:
"We require all single argument constructors to be explicit. [...]
The exception is copy constructors, which, in the rare cases when we allow them,
should probably not be explicit."
In fact, it doesn't compile if this copy constructor is explicit.
The only alternative would be to never store plain PluginGroups (as opposed to
pointers) in std::vectors or similar containers, and get rid of this constructor
altogether, but then we'd still have the same threading/concurrency problems
this cleanup is intended to get rid of.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.h:65: // except by PluginList and unit tests.
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> PluginList is already a friend, so just declare this method private. 

Done.
This required adding several more friend classes (which is why I didn't make the
method private before).

> Also, please explain that the caller takes ownership of the returned object.

Done.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list.cc
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.cc:161: next_priority_(0) {
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> I have a slight suspicion that we might be able to get rid of priorities
anyway,
> because they're always monotonically increasing in a plugin group and in the
> plugin list anyway.

Well, yes -- it might go away someday. Doesn't have to be in this CL, though.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.cc:292: // AddToPluginGroups(plugin_info) .)
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> Nit: Wrap the code in bars |like this|, then it's still kind of readable if
you
> put punctuation after it.

Done. ("kind of" being the key word in that sentence.)

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.cc:492: return plugin_groups;
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> Remember back in plugin_exceptions_table_model.cc when I mentioned that I'm
not
> sure if the compiler optimizes all unnecessary copying away? Here I'm pretty
> sure it doesn't ;-)
> 
> I think you should change the interface to take a second parameter, and write
to
> it.

Done.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list.h
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.h:165: PluginGroup GetPluginGroup(const
WebPluginInfo& web_plugin_info);
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> I think you could return a const pointer instead of copying the PluginGroup.

Done.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.h:169: string16 GetPluginGroupName(std::string
identifier);
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> I think you could also return a const PluginGroup* here, which would make this
> interface useful to get additional information about a PluginGroup in the
> future.

As discussed offline, we'll leave it as it is for the time being.

http://codereview.chromium.org/5516004/diff/1/webkit/glue/plugins/plugin_list...
webkit/glue/plugins/plugin_list.h:216: // Creates a PluginGroup for the given
WebPluginInfo and adds it to the cache.
On 2010/12/03 16:13:28, Bernhard Bauer wrote:
> // Adds the given WebPluginInfo to its corresponding group, creating it if
> necessary, and returns the group.

Done.

[Bernhard Bauer, 2010-12-07 10:32:05]

http://codereview.chromium.org/5516004/diff/20001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_group.h (right):

http://codereview.chromium.org/5516004/diff/20001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.h:120:
FRIEND_TEST_ALL_PREFIXES(PluginGroupTest, PluginGroupDefinition);
I think this is unnecessary now.

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.h:125: static size_t
GetPluginGroupDefinitionsSize();
As discussed offline, please move these to PluginList.

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.cc:528: AutoLock lock(lock_);
As discussed offline, please move this lock to the callers of this method (and
maybe add a comment here).

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:164: // Beware: when calling this from the
Browser prrocess, the group that the
Nit: process

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:165: // returned pointer points to might
disappear suddenly.
Also, please elaborate on under what circumstances the group might disappear (if
RefreshPlugins is called and then GetPlugins/GetEnabledPlugins/GetPluginGroups),
and that it's the caller's responsibility to make sure that doesn't happen.

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:213: // Creates PluginGroups for the group
definitions in PluginGroup.cc, and adds
Nit: The file is called plugin_group.cc.

[Jakob Kummerow, 2010-12-07 11:43:08]

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_group.h (right):

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.h:125: static size_t
GetPluginGroupDefinitionsSize();
On 2010/12/07 10:32:05, Bernhard Bauer wrote:
> As discussed offline, please move these to PluginList.

Done.

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.cc:528: AutoLock lock(lock_);
On 2010/12/07 10:32:05, Bernhard Bauer wrote:
> As discussed offline, please move this lock to the callers of this method (and
> maybe add a comment here).

Done.

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:164: // Beware: when calling this from the
Browser prrocess, the group that the
On 2010/12/07 10:32:05, Bernhard Bauer wrote:
> Nit: process

Done.

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:165: // returned pointer points to might
disappear suddenly.
On 2010/12/07 10:32:05, Bernhard Bauer wrote:
> Also, please elaborate on under what circumstances the group might disappear
(if
> RefreshPlugins is called and then
GetPlugins/GetEnabledPlugins/GetPluginGroups),
> and that it's the caller's responsibility to make sure that doesn't happen.

Done.

http://codereview.chromium.org/5516004/diff/33001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:213: // Creates PluginGroups for the group
definitions in PluginGroup.cc, and adds
On 2010/12/07 10:32:05, Bernhard Bauer wrote:
> Nit: The file is called plugin_group.cc.

Done.

[Bernhard Bauer, 2010-12-07 12:09:50]

LGTM with a nit I forgot earlier.

http://codereview.chromium.org/5516004/diff/38001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5516004/diff/38001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.cc:103: DCHECK(other.web_plugin_infos_.size()
== other.web_plugin_positions_.size());
Nit: Use DCHECK_EQ(expected, actual) for nicer error messages.

[Jakob Kummerow, 2010-12-07 17:08:56]

http://codereview.chromium.org/5516004/diff/38001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5516004/diff/38001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.cc:103: DCHECK(other.web_plugin_infos_.size()
== other.web_plugin_positions_.size());
On 2010/12/07 12:09:51, Bernhard Bauer wrote:
> Nit: Use DCHECK_EQ(expected, actual) for nicer error messages.

Done.

[Timur Iskhodzhanov, 2010-12-07 19:28:50]

This CL has made almost all the memory waterfall red.
Please revert if possible or/and investigate & fix/suppress the reports ASAP

[Jakob Kummerow, 2010-12-07 20:18:28]

On 2010/12/07 19:28:50, Timur Iskhodzhanov wrote:
> This CL has made almost all the memory waterfall red.

Damn. Sorry about that. PluginList is a singleton that doesn't clean up after
itself. We knew this, and didn't care since it doesn't hurt in the real world,
but we didn't consider that it'd make the valgrind bots cry :-(

> Please revert if possible or/and investigate & fix/suppress the reports ASAP

I'm at home and about to go to bed; I can't do anything until tomorrow morning
(European time), about 12 hours from now. Please feel free to revert yourself if
you don't want to wait that long.

[Hironori Bono, 2010-12-08 04:18:21]

On 2010/12/07 20:18:28, jkummerow wrote:
> Damn. Sorry about that. PluginList is a singleton that doesn't
> clean up after itself. We knew this, and didn't care since it
> doesn't hurt in the real world, but we didn't consider that it'd
> make the valgrind bots cry :-(

This is just for your information.
Our LazyInstance<T> class adds the destructor of the class T to AtExitManager,
i.e. we call the destructor 'T::~T()' when Chrome exits. So, it would be
definitely helpful to add code that cleans up the resources used by a singleton
in its destructor, as listed in the change of leiz
<http://codereview.chromium.org/5574005>.

Regards,

Hironori Bono

[willchan no longer on Chromium, 2010-12-11 05:04:36]

On Tue, Dec 7, 2010 at 8:18 PM, <hbono@chromium.org> wrote:

> On 2010/12/07 20:18:28, jkummerow wrote:
>
>> Damn. Sorry about that. PluginList is a singleton that doesn't
>> clean up after itself. We knew this, and didn't care since it
>> doesn't hurt in the real world, but we didn't consider that it'd
>> make the valgrind bots cry :-(
>>
>
> This is just for your information.
> Our LazyInstance<T> class adds the destructor of the class T to
> AtExitManager,
> i.e. we call the destructor 'T::~T()' when Chrome exits. So, it would be
> definitely helpful to add code that cleans up the resources used by a
> singleton
> in its destructor, as listed in the change of leiz
> <http://codereview.chromium.org/5574005>.
>

That shutdown code looks slightly weird.  It only runs if we're running in
Valgrind.  This is wrong.

We should say that we either care or don't care about leaking the object.
 If we don't care, use LeakySingletonTraits or LeakyLazyInstanceTraits.  If
we do care, then make sure the destructor cleans up.

Why would one care?  Well, ChromeFrame loads some Chrome code into IE's
address space.  If you don't delete the global, then when ChromeFrame
unloads, it will be leaking in IE's address space.  That'd be lame of us.

So, the moral of the story is that using RUNNING_IN_VALGRIND makes no sense.
 You either care or you don't care.  If you don't care, then you should use
Leaky*Traits, which shouldn't cause valgrind leaks (although I've seen it
happen before for LazyInstance, in which case you should just add a
suppression until that issue is fixed).


> Regards,
>
> Hironori Bono
>
>
> http://codereview.chromium.org/5516004/
>