Bug 28677 – Allow excluding certain plugins from loading

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Marius Renn]

Created attachment 38475 [details]
Proposed patch to implement the plugin loading delegation described in the feature request.

(Note, this is a re-post as the original got lost in a bugzilla crash).

It is well-known that plugins can cause problems, in the worst case leading to a crash. Unfortunately, this can happen as early as plugin-loading, as code may be executed on the plugin during this stage. One way this can be worked around is by using the recently introduced patch to change the plugin directory. However, in some applications it may not be desirable to switch to a different directory. Instead, a client may wish to filter the list of plugins to load.

Therefore I propose adding a client callback class (or delegate), which allows filtering plugins based on its path or other attributes such as name, version, etc. I have included a proposed patch that implements this functionality, by introducing an abstract base class PluginDatabaseClient with two pure virtual methods shouldLoadPluginAtPath() and shouldLoadPluginPackage(). These are called in sequence, and if any of these return false, the current plugin is not loaded. For instance, if shouldLoadPluginAtPath() returns false, no further inspection of the plugin is performed. Such a client can be set (optionally) by calling setClient() on the PluginDatabase singleton instance. Note, that the instance must be created without populating as the following example shows (where myPluginLoaderClient denote some PluginDatabaseClient implementation):

PluginDatabase* db = PluginDatabase::installedPlugins(false); // Do not load plugins
db->setClient(myPluginLoaderClient);	// Set client instance
db->setPluginDirectories(PluginDatabase::defaultPluginDirectories());
db->refresh();	// Load plugins with client in place

[Eric Seidel (no email)]

Comment on attachment 38475 [details]
Proposed patch to implement the plugin loading delegation described in the feature request.

I would probably have broken:
   if (!m_client || m_client->shouldLoadPluginAtPath(*it)) {
 121             RefPtr<PluginPackage> package = PluginPackage::createPackage(*it, lastModified);
 122             if (package) {
 123                 if ((!m_client || m_client->shouldLoadPluginPackage(package)) && add(package))
 124                     pluginSetChanged = true;
 125                 package.release();
 126             }
 127         }
out into a little function to make early return possible (for when !package) for instance.  You'd have to pass the Client* and the path, and likey return a PassRefPtr<PluginPackage>  It would get rid of the need for explicit release though.

Probably would look something like this:

RefPtr<PluginPackage> package = loadPluginAtPath(*it, m_client);
if (addPluginPackage(package.release(), this))
    pluginSetChanged = true;

Actually, I'm not sure that's cleaner since you have to pass "this" in oder to get access to "add()"

Anyway, you can re-work that function if you'd like, or it's OK as is.  But I'm going to r- this for the wrong copyright here:

+ * Copyright (C) 2007 Apple Inc. All rights reserved.  Should show your copyright

Otherwise this looks fine to me.  I'll CC the plugin experts in case they have comment.

[Eric Seidel (no email)]

CCing plugin people in case they have comment.  This in general looks fine to me.

[Marius Renn]

Created attachment 38675 [details]
Revised Patch (fixed copyright)

[Eric Seidel (no email)]

Comment on attachment 38675 [details]
Revised Patch (fixed copyright)

Hearing no complaints from the plugin experts, this still looks fine to me.

[Eric Seidel (no email)]

Comment on attachment 38675 [details]
Revised Patch (fixed copyright)

Clearing flags on attachment: 38675

Committed r47918: <http://trac.webkit.org/changeset/47918>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Looks like this broke the windows build. :(

Looks like the ChangeLog was also missing a bug number.  I should have caught that.

I'm looking into the build break now, although I might just roll this out.

[Eric Seidel (no email)]

Committed r47920: <http://trac.webkit.org/changeset/47920>

[Marius Renn]

(In reply to comment #8)
> Committed r47920: <http://trac.webkit.org/changeset/47920>

Woha, sorry about that. I have to admit I hadn't tried compiling in Windows. I will be more careful in the future...

Anyway, I took a look at the patch to fix the Windows issue. I am a bit concerned about the release() call within the if() statement. If the expression '(!m_client || m_client->shouldLoadPluginPackage(package.get()))' evaluates to false, then will we not leak memory (as the add(package.release()) will not be called? Correct me if I am wrong though.

[Eric Seidel (no email)]

Your understanding is incorrect. :)  (Although you're certainly not alone in this misunderstanding!)

I suggest you read:
http://webkit.org/coding/RefPtr.html

[Marius Renn]

Ah, that certainly clears up a few things. Thanks!

[Eric Seidel (no email)]

Sadly that document is now out of date.  RefCounted starts with a refCount() of 1 these days.  Hence all the PassRefPtr<Foo> Foo::create() methods you see.

[Marius Renn]

Hmmm... it seems the documentation may be more up-to-date than you give it credit for :-)

"However, for efficiency and simplicity, the RefCounted class doesn't use a reference count of 0 at all. Objects are created with a reference count of 1."

[Marius Renn]

Created attachment 39720 [details]
Qt patch to allow overriding plugin loading

I have extended the plugin load delegation functionality to the Qt API layer, where I have added two virtual methods shouldLoadPluginAtPath() and shouldLoadPluginWithInfo() to the QWebPluginDatabase class. While the default implementation simply returns true, a subclass may do something more sophisticated, such as excluding certain plugins.

[Kenneth Rohde Christiansen]

New patch has been added, so I'm reopening.

[Eric Seidel (no email)]

Generally we do one fix per bug.  Please open a new bug for new patches.

[Marius Renn]

(In reply to comment #16)
> Generally we do one fix per bug.  Please open a new bug for new patches.

Got it. Shall I open a new bug for this one then? (Not sure as this is currently still open).

[Adam Barth]

Please make a new bug for the new patch.

[Eric Seidel (no email)]

Comment on attachment 39720 [details]
Qt patch to allow overriding plugin loading

Removing r? now that this bug is closed.