Bug 101274 – Allow plugins to be disabled by shared library filename

• Component: Plug-ins
• Status: RESOLVED FIXED

[Max Feil]

We need a mechanism to allow plugins to be disabled by specifying their shared library filenames, for example "libflashplayer.so". Once disabled, a plugin's shared library should never be loaded. This will save memory at runtime. Other mechanisms based on mime type, etc, require loading of the shared library just to determine its supported mime type(s).

This bug covers the Source/WebCore and Source/WebKit changes for this fix. There are corresponding changes in the libwebview and browser repositories, which are covered by PR201152.

[Max Feil]

Created attachment 172436 [details]
Patch

[Max Feil]

(In reply to comment #1)
> Created an attachment (id=172436) [details]
> Patch

It would be nice to get this patch reviewed. If there are problems with it, I can work on them.

[Antonio Gomes]

Comment on attachment 172436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172436&action=review

I can review it later today, but I've seen some nits to be addressed anyways.

> Source/WebCore/ChangeLog:3
> +        [BlackBerry] Allow plugins to be disabled by shared library filename

The prefix [BB] here is not right. It change webcore cross platform code, and affects blackberry. So we should remove the prefix and add 'blackberry' keyword available on bugzilla.

> Source/WebCore/plugins/PluginDatabase.cpp:322
> +    HashSet<String>::const_iterator fIt;
> +    for (fIt = m_disabledPluginFiles.begin(); fIt != filesEnd; ++fIt) {

I would name it 'it'

> Source/WebCore/plugins/PluginDatabase.cpp:328
> +    if (fIt != filesEnd)
> +        return false;
> +    return true;

return fIt == filesEnd;

> Source/WebCore/plugins/PluginDatabase.cpp:335
> +        if (!fileExistsAndIsEnabled((*it)->path()))

I did not like much this updated method name, but am out of suggestions. Maybe fileExistsAndIsNotBlackListed?

> Source/WebCore/plugins/PluginDatabase.cpp:379
> +bool PluginDatabase::updateDisabledPluginFiles(const String& fileName, bool disabled)

Same. ::updatePluginsBlackListedFileList?

> Source/WebKit/blackberry/Api/WebPage.cpp:5236
> +    PluginDatabase* database = PluginDatabase::installedPlugins(true);
> +
> +    if (!database)

no new line needed (for consistency).

> Source/WebKit/blackberry/Api/WebPage.cpp:5250
> +        d->m_page->refreshPlugins(false);

what does "false" mean here?

> Source/WebKit/blackberry/Api/WebPage.cpp:5253
> +        if (d->m_webSettings->arePluginsEnabled())
> +            database->refresh();

should not this be the first things checked in this method?

[Max Feil]

(In reply to comment #3)
> The prefix [BB] here is not right. It change webcore cross platform code, and affects blackberry. So we should remove the prefix and add 'blackberry' keyword available on bugzilla.

Done, thanks.
 
> I would name it 'it'

I have replaced it with it, hope you like it :)

> return fIt == filesEnd;

Much cleaner. Done.
 
> > +        if (!fileExistsAndIsEnabled((*it)->path()))
> I did not like much this updated method name, but am out of suggestions. Maybe fileExistsAndIsNotBlackListed?

Since the function used to disable plugin library files is called 'updateDisabledPluginFiles', and the member variable used to track the set of disabled files is called 'm_disabledPluginFiles', then 'NotDisabled' is more self documenting than 'NotBlackListed'. I'll change it to that for now...

> > +bool PluginDatabase::updateDisabledPluginFiles(const String& fileName, bool disabled)
> 
> Same. ::updatePluginsBlackListedFileList?

I don't think this is any better. Sorry...
 
> no new line needed (for consistency).

Fixed...

> > +        d->m_page->refreshPlugins(false);
> 
> what does "false" mean here?

True only needs to be passed here if we want to reload each frame in the page's frame tree. I am passing false for minimum disruption, and because this does exactly what we need and nothing more: refresh the plugin data. See Page.cpp.

> > +        if (d->m_webSettings->arePluginsEnabled())
> > +            database->refresh();
> 
> should not this be the first things checked in this method?

I don't think so. There's no requirement that plugins be enabled in order to change their internal configuration (list of directories, disabled library filenames, etc). In fact certain calls like refreshPlugins() MUST be done even if plugins are disabled, because this is not done when plugins get re-enabled.

[Max Feil]

Created attachment 174542 [details]
Patch

[Max Feil]

r? ping?

[Yong Li]

Comment on attachment 174542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174542&action=review

> Source/WebCore/plugins/PluginDatabase.cpp:390
> +        if (!m_disabledPluginFiles.contains(fileName)) {
> +            m_disabledPluginFiles.add(fileName);
> +            changed = true;
> +        }
> +    } else {
> +        if (m_disabledPluginFiles.contains(fileName)) {
> +            m_disabledPluginFiles.remove(fileName);
> +            changed = true;
> +        }
> +    }

Calling m_disabledPluginFiles.contains is probably not necessary. At least add() returns a value indicating if it is new entry. Not sure but probably HashSet::remove also returns something

> Source/WebKit/blackberry/Api/WebPage.cpp:5240
> +    if (path.length() && !pluginDirectories.contains(path.c_str())) {

Early return is preferred.

!empty() is usually better than length(). No need to call c_str()? Platform::String can be converted to WTF::String implicitly.

> Source/WebKit/blackberry/Api/WebPage.cpp:5241
> +        pluginDirectories.append(path.c_str());

here, too

> Source/WebKit/blackberry/Api/WebPage.cpp:5262
> +    if (database->updateDisabledPluginFiles(String(fileName.c_str()), disabled)) {

early return is better

[Yong Li]

Comment on attachment 174542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174542&action=review

> Source/WebCore/plugins/PluginDatabase.cpp:324
> +        if (fileName.endsWith(*it))
> +            break;

Why endsWith()? Also I would return false here rather than checking it==filesEnd

[Max Feil]

(In reply to comment #8)
> (From update of attachment 174542 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174542&action=review
> 
> > Source/WebCore/plugins/PluginDatabase.cpp:324
> > +        if (fileName.endsWith(*it))
> > +            break;
> 
> Why endsWith()? Also I would return false here rather than checking it==filesEnd

The "endsWith" is so that we don't have to specify an entire path name, just the important part of the path which is at the end. It's probably better to isolate everything after the last slash for comparison though...

[Yong Li]

Comment on attachment 174542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174542&action=review

>>> Source/WebCore/plugins/PluginDatabase.cpp:324
>>> +            break;
>> 
>> Why endsWith()? Also I would return false here rather than checking it==filesEnd
> 
> The "endsWith" is so that we don't have to specify an entire path name, just the important part of the path which is at the end. It's probably better to isolate everything after the last slash for comparison though...

You can use pathGetFileName(). See WebCore/platform/FileSystem.h

Also I think checking the disabledPlugin list first may be faster in most cases. fileExists() might be slow?

[Yong Li]

Comment on attachment 174542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174542&action=review

> Source/WebCore/ChangeLog:3
> +        [BlackBerry] Allow plugins to be disabled by shared library filename

Probably we should remove [BlackBerry] here, as it changes cross-platform code

[Yong Li]

Comment on attachment 174542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174542&action=review

>>>> Source/WebCore/plugins/PluginDatabase.cpp:324
>>>> +    HashSet<String>::const_iterator filesEnd = m_disabledPluginFiles.end();
>>>> +    HashSet<String>::const_iterator it;
>>>> +    for (it = m_disabledPluginFiles.begin(); it != filesEnd; ++it) {
>>>> +        if (fileName.endsWith(*it))
>>>> +            break;
>>> 
>>> Why endsWith()? Also I would return false here rather than checking it==filesEnd
>> 
>> The "endsWith" is so that we don't have to specify an entire path name, just the important part of the path which is at the end. It's probably better to isolate everything after the last slash for comparison though...
> 
> You can use pathGetFileName(). See WebCore/platform/FileSystem.h
> 
> Also I think checking the disabledPlugin list first may be faster in most cases. fileExists() might be slow?

If fileName can be a path, we can use pathGetFileName. Then we just need "return !m_disabledPluginFiles.contain(fileName)". That is also faster than walking through the list.

[Max Feil]

Thanks for the review comments Yong! I will address your comments one by one below:

(In reply to comment #7)
> Calling m_disabledPluginFiles.contains is probably not necessary. At least add() returns a value indicating if it is new entry. Not sure but probably HashSet::remove also returns something

Good point. I changed the code to use the "isNewEntry" bool returned by HashSet::add. The HashSet::remove method returns void, so I kept that part the same.
 
> > Source/WebKit/blackberry/Api/WebPage.cpp:5240
> > +    if (path.length() && !pluginDirectories.contains(path.c_str())) {
> 
> Early return is preferred.
> !empty() is usually better than length(). No need to call c_str()? Platform::String can be converted to WTF::String implicitly.

That is not my code. It was written by somebody else, and reviewed by somebody else long ago. I prefer not to create unrelated diffs in other people's reviewed code. If you feel strongly about this you can open a separate bug, but as it stands this code is not being touched by this bug (see webkit/89a69f88 and PR201152).

> > Source/WebKit/blackberry/Api/WebPage.cpp:5241
> > +        pluginDirectories.append(path.c_str());
> 
> here, too

That is not my code. It was written by somebody else, and reviewed by somebody else long ago. I prefer not to create unrelated diffs in other people's reviewed code. If you feel strongly about this you can open a separate bug, but as it stands this code is not being touched by this bug (see webkit/89a69f88 and PR201152).

> > Source/WebKit/blackberry/Api/WebPage.cpp:5262
> > +    if (database->updateDisabledPluginFiles(String(fileName.c_str()), disabled)) {
> 
> early return is better

Ok, I changed this to early return.

(In reply to comment #8)
> Why endsWith()? Also I would return false here rather than checking it==filesEnd

Ok, I changed the code to return false.

(In reply to comment #10)
> You can use pathGetFileName(). See WebCore/platform/FileSystem.h
> Also I think checking the disabledPlugin list first may be faster in most cases. fileExists() might be slow?

Good points. I implemented both suggestions.

(In reply to comment #11)
> Probably we should remove [BlackBerry] here, as it changes cross-platform code

Yes, I missed those...

(In reply to comment #12)
> If fileName can be a path, we can use pathGetFileName. Then we just need "return !m_disabledPluginFiles.contain(fileName)". That is also faster than walking through the list.

Another good point. I updated my code. Patch coming momentarily.

[Max Feil]

Created attachment 176580 [details]
Patch

[Yong Li]

Comment on attachment 176580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176580&action=review

Basically looks good to me if updateDisabledPluginFiles() is changed to setDisabledPluginFile()

> Source/WebCore/plugins/PluginDatabase.cpp:385
> +    bool changed = false;
> +    if (disabled) {
> +        if (m_disabledPluginFiles.add(fileName).isNewEntry)
> +            changed = true;
> +    } else {
> +        if (m_disabledPluginFiles.contains(fileName)) {
> +            m_disabledPluginFiles.remove(fileName);
> +            changed = true;
> +        }
> +    }
> +    return changed;
> +}

Minor: this function can be further simplified by:
if (disabled)
    return m_disabledPluginFiles.add(fileName).isNewEntry;
if (!m_disabledPluginFiles.contains(fileName))
    return false;
....

> Source/WebCore/plugins/PluginDatabase.h:77
> +        bool updateDisabledPluginFiles(const String& fileName, bool disabled);

It is called update...Files, but it only changes one file

[Max Feil]

(In reply to comment #15)
> Basically looks good to me if updateDisabledPluginFiles() is changed to setDisabledPluginFile()
> It is called update...Files, but it only changes one file

The name is plural because the "disabled plugin files" are a hash set of multiple files, and you are updating it by adding or removing a filename. So I don't think setDisabledPluginFile works...

[Yong Li]

(In reply to comment #16)
> (In reply to comment #15)
> > Basically looks good to me if updateDisabledPluginFiles() is changed to setDisabledPluginFile()
> > It is called update...Files, but it only changes one file
> 
> The name is plural because the "disabled plugin files" are a hash set of multiple files, and you are updating it by adding or removing a filename. So I don't think setDisabledPluginFile works...

I see. But it wouldn't confuse me if it was updateOneFileInDisabledPluginFiles() :)

Or just addDisabledPluginFile/remove*?

[Yong Li]

Comment on attachment 176580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176580&action=review

> Source/WebKit/blackberry/Api/WebPage.cpp:5147
> +    if (!database->updateDisabledPluginFiles(String(fileName.c_str()), disabled))

Also I can't find this in our existing code. Can you please clean them up? String(fileName.c_str()) can just be replaced with fileName

[Max Feil]

(In reply to comment #17)
> I see. But it wouldn't confuse me if it was updateOneFileInDisabledPluginFiles() :)
> 
> Or just addDisabledPluginFile/remove*?

I like the add/remove idea, but it would mean plumbing two API calls through QNXWebView. I still want to avoid that.

(In reply to comment #18)
> > Source/WebKit/blackberry/Api/WebPage.cpp:5147
> > +    if (!database->updateDisabledPluginFiles(String(fileName.c_str()), disabled))
> 
> Also I can't find this in our existing code. Can you please clean them up? String(fileName.c_str()) can just be replaced with fileName

Ok.

[Max Feil]

Created attachment 177090 [details]
Patch

[Antonio Gomes]

Comment on attachment 177090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177090&action=review

some minor nits.

> Source/WebKit/blackberry/Api/WebPage.cpp:5117
> +    PluginDatabase* database = PluginDatabase::installedPlugins(true);

what is the bool? /**/

> Source/WebKit/blackberry/Api/WebPage.cpp:5142
> +    PluginDatabase* database = PluginDatabase::installedPlugins(true);

ditto

> Source/WebKit/blackberry/Api/WebPage.cpp:5148
> +        return; // No change

comment is not a sentence (no . at the end).

> Source/WebKit/blackberry/Api/WebPage.cpp:5156
> +    d->m_page->refreshPlugins(false);

what is the bool?

> Source/WebKit/blackberry/Api/WebPage.cpp:5158
> +    // Refresh the plugin database if necessary

unneeded comment.

[Max Feil]

Assigning to Yong. He offered to complete the upstreaming. In my discussions with him the only item preventing his r+ was the unsatisfactory function name "updateDisabledPluginFiles".

[Yong Li]

(In reply to comment #22)
> Assigning to Yong. He offered to complete the upstreaming. In my discussions with him the only item preventing his r+ was the unsatisfactory function name "updateDisabledPluginFiles".

OK. I would also rewrite the function with early return to follow webkit style if not create a pair of add/remove methods.

[Parth Patel]

Created attachment 179033 [details]
Patch

[Parth Patel]

Created attachment 179036 [details]
Patch

[Antonio Gomes]

Comment on attachment 179036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179036&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests are needed.

please explain why

> Source/WebCore/plugins/PluginDatabase.cpp:317
> +    // Skip plugin files that are disabled by filename

dot at end

> Source/WebCore/plugins/PluginDatabase.cpp:383
> +    return m_disabledPluginFiles.add(fileName).isNewEntry

does it build??

[Antonio Gomes]

Comment on attachment 179036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179036&action=review

>> Source/WebCore/ChangeLog:11
>> +        No new tests are needed.
> 
> please explain why

please explain why

>> Source/WebCore/plugins/PluginDatabase.cpp:317
>> +    // Skip plugin files that are disabled by filename
> 
> dot at end

dot at end

>> Source/WebCore/plugins/PluginDatabase.cpp:383
>> +    return m_disabledPluginFiles.add(fileName).isNewEntry
> 
> does it build??

does it build??

> Source/WebCore/plugins/PluginDatabase.h:80
> +        bool removeDisabledPluginFile(const String& fileName);
> +
> +        bool addDisabledPluginFile(const String& fileName);

remove extra line

> Source/WebKit/blackberry/Api/WebPage.cpp:5156
> +void WebPage::setExtraPluginDirectory(const BlackBerry::Platform::String& path)

is blackberry:: namespace needed.

> Source/WebKit/blackberry/Api/WebPage.cpp:5160
> +    PluginDatabase* database = PluginDatabase::installedPlugins(true);
> +
> +    if (!database)

remove extra line

> Source/WebKit/blackberry/Api/WebPage.cpp:5174
> +        d->m_page->refreshPlugins(false);

what is false herE?

> Source/WebKit/blackberry/Api/WebPage.cpp:5185
> +    PluginDatabase* database = PluginDatabase::installedPlugins(true);
> +
> +    if (!database)

remove extra line

what is TRUE herE?

> Source/WebKit/blackberry/Api/WebPage.h:334
> +    void setExtraPluginDirectory(const BlackBerry::Platform::String& path);

is BlackBerry:: namespace needed?

> Source/WebKit/blackberry/Api/WebPage.h:335
> +    void updateDisabledPluginFiles(const BlackBerry::Platform::String& fileName, bool disabled);

ditto

[Parth Patel]

Created attachment 179046 [details]
Patch

[Parth Patel]

sorry...i was not able to see the last few comments from antonio before...I will have to make more changes from current patch.

[Parth Patel]

Created attachment 179076 [details]
Patch

[Yong Li]

Comment on attachment 179076 [details]
Patch

This looks good to me except the new data member should usually be added after old ones.

Antonio, Max?

[Antonio Gomes]

Comment on attachment 179076 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179076&action=review

Looks good.  Please address the comments below and any concern Yong might have.

Next, reupload the patch with "Reviewed by Antonio Gomes"  in the ChangeLog, and only request commit-queue to land it. No further review needed.

> Source/WebKit/blackberry/Api/WebPage.cpp:5158
> +    PluginDatabase* database = PluginDatabase::installedPlugins(true);

::installedPlugins(true /*whatDoesTheBoolMeanHerePlease*/);

> Source/WebKit/blackberry/Api/WebPage.cpp:5161
> +        return;
> +    // Reload

replace this "reload" comment by an empty line.

> Source/WebKit/blackberry/Api/WebPage.cpp:5178
> +    d->m_page->refreshPlugins(false);

refreshPlugins(false /*whatDoesTheBoolMeanHerePlease*/);

> Source/WebKit/blackberry/Api/WebPage.cpp:5187
> +    PluginDatabase* database = PluginDatabase::installedPlugins(true);

::installedPlugins(true /*whatDoesTheBoolMeanHerePlease*/);

> Source/WebKit/blackberry/Api/WebPage.cpp:5191
> +    if (!(disabled ? database->addDisabledPluginFile(fileName) : database->removeDisabledPluginFile(fileName)))

This line does not read well to me. Use a help variable, please.

> Source/WebKit/blackberry/Api/WebPage.cpp:5200
> +    d->m_page->refreshPlugins(false);

refreshPlugins(false /*whatDoesTheBoolMeanHerePlease*/);

> Source/WebKit/blackberry/Api/WebPage.cpp:5202
> +    // Refresh the plugin database if necessary

Add a dot at the end of the sentence.

[Parth Patel]

Created attachment 179303 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179303 [details]
Patch

Clearing flags on attachment: 179303

Committed r137627: <http://trac.webkit.org/changeset/137627>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yong Li]

reopening for a build error

[Parth Patel]

Created attachment 179487 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179487 [details]
Patch

Clearing flags on attachment: 179487

Committed r137753: <http://trac.webkit.org/changeset/137753>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.