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
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.
CCing plugin people in case they have comment. This in general looks fine to me.
Created attachment 38675 [details] Revised Patch (fixed copyright)
Comment on attachment 38675 [details] Revised Patch (fixed copyright) Hearing no complaints from the plugin experts, this still looks fine to me.
Comment on attachment 38675 [details] Revised Patch (fixed copyright) Clearing flags on attachment: 38675 Committed r47918: <http://trac.webkit.org/changeset/47918>
All reviewed patches have been landed. Closing bug.
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.
Committed r47920: <http://trac.webkit.org/changeset/47920>
(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.
Your understanding is incorrect. :) (Although you're certainly not alone in this misunderstanding!) I suggest you read: http://webkit.org/coding/RefPtr.html
Ah, that certainly clears up a few things. Thanks!
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.
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."
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.
New patch has been added, so I'm reopening.
Generally we do one fix per bug. Please open a new bug for new patches.
(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).
Please make a new bug for the new patch.
Comment on attachment 39720 [details] Qt patch to allow overriding plugin loading Removing r? now that this bug is closed.