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.
Created attachment 172436 [details] Patch
(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.
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?
(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.
Created attachment 174542 [details] Patch
r? ping?
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
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
(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...
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?
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
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.
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.
Created attachment 176580 [details] Patch
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
(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...
(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*?
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
(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.
Created attachment 177090 [details] Patch
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.
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".
(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.
Created attachment 179033 [details] Patch
Created attachment 179036 [details] Patch
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??
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
Created attachment 179046 [details] Patch
sorry...i was not able to see the last few comments from antonio before...I will have to make more changes from current patch.
Created attachment 179076 [details] Patch
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?
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.
Created attachment 179303 [details] Patch
Comment on attachment 179303 [details] Patch Clearing flags on attachment: 179303 Committed r137627: <http://trac.webkit.org/changeset/137627>
All reviewed patches have been landed. Closing bug.
reopening for a build error
Created attachment 179487 [details] Patch
Comment on attachment 179487 [details] Patch Clearing flags on attachment: 179487 Committed r137753: <http://trac.webkit.org/changeset/137753>