- Revision
- 185717
- Author
- [email protected]
- Date
- 2015-06-18 12:32:15 -0700 (Thu, 18 Jun 2015)
Log Message
REGRESSION: js/dom/navigator-plugins-crash.html asserts a lot
https://bugs.webkit.org/show_bug.cgi?id=144399
Reviewed by Darin Adler.
Earlier work made the array of web-visible plug-ins dynamic, but allowed DOMPlugin (and, indirectly by extension,
DOMMimeType) to continue keeping a reference to a plug-in in terms of an index into that array. This superficially
appeared correct since DOMPlugin immutably holds onto a PluginData instance, which in turn immutably holds onto a
Page instance. PluginStrategy::getWebVisiblePluginInfo() is passed this Page, which is used to determine the contents
of the plugin array. The expectation was that keeping an index would still be safe since the Page is not changing,
but this is not strictly correct since relevant attributes of the Page and/or the available plugins may still change.
It's not entirely clear why the test failures are intermittent and occur only on certain configurations, but address
them by eliminating the incorrect storage of indexes in favor of keeping copies of the relevant plugin info itself.
* plugins/DOMMimeType.cpp:
(WebCore::DOMMimeType::DOMMimeType):
Instead of storing the MIME type index, retrieve and store the MIME class info and plugin info.
(WebCore::DOMMimeType::type):
Directly access the m_mimeClassInfo member.
(WebCore::DOMMimeType::suffixes):
Ditto.
(WebCore::DOMMimeType::description):
Ditto.
(WebCore::DOMMimeType::enabledPlugin):
Directly access the m_pluginInfo member.
(WebCore::DOMMimeType::mimeClassInfo): Deleted.
* plugins/DOMMimeType.h:
Update member variables.
* plugins/DOMPlugin.cpp:
(WebCore::DOMPlugin::DOMPlugin):
Instead of storing the plugin index, store the plugin info directly.
(WebCore::DOMPlugin::name):
Directly access m_pluginInfo.
(WebCore::DOMPlugin::filename):
Ditto.
(WebCore::DOMPlugin::description):
Ditto.
(WebCore::DOMPlugin::length):
Ditto.
(WebCore::DOMPlugin::item):
Access m_pluginInfo directly; find the matching plug-in based on matching PluginInfo (for which an overloaded
comparator is supplied below).
(WebCore::DOMPlugin::pluginInfo): Deleted.
* plugins/DOMPlugin.h:
Update member variables.
(WebCore::DOMPlugin::create):
Accept a PluginInfo instead of a plugin index.
* plugins/DOMPluginArray.cpp:
(WebCore::DOMPluginArray::item):
(WebCore::DOMPluginArray::namedItem):
* plugins/PluginData.h:
(WebCore::operator==):
Added; compare PluginInfo structs on the basis of member equality.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (185716 => 185717)
--- trunk/Source/WebCore/ChangeLog 2015-06-18 19:21:22 UTC (rev 185716)
+++ trunk/Source/WebCore/ChangeLog 2015-06-18 19:32:15 UTC (rev 185717)
@@ -1,3 +1,65 @@
+2015-06-17 Conrad Shultz <[email protected]>
+
+ REGRESSION: js/dom/navigator-plugins-crash.html asserts a lot
+ https://bugs.webkit.org/show_bug.cgi?id=144399
+
+ Reviewed by Darin Adler.
+
+ Earlier work made the array of web-visible plug-ins dynamic, but allowed DOMPlugin (and, indirectly by extension,
+ DOMMimeType) to continue keeping a reference to a plug-in in terms of an index into that array. This superficially
+ appeared correct since DOMPlugin immutably holds onto a PluginData instance, which in turn immutably holds onto a
+ Page instance. PluginStrategy::getWebVisiblePluginInfo() is passed this Page, which is used to determine the contents
+ of the plugin array. The expectation was that keeping an index would still be safe since the Page is not changing,
+ but this is not strictly correct since relevant attributes of the Page and/or the available plugins may still change.
+
+ It's not entirely clear why the test failures are intermittent and occur only on certain configurations, but address
+ them by eliminating the incorrect storage of indexes in favor of keeping copies of the relevant plugin info itself.
+
+ * plugins/DOMMimeType.cpp:
+ (WebCore::DOMMimeType::DOMMimeType):
+ Instead of storing the MIME type index, retrieve and store the MIME class info and plugin info.
+ (WebCore::DOMMimeType::type):
+ Directly access the m_mimeClassInfo member.
+ (WebCore::DOMMimeType::suffixes):
+ Ditto.
+ (WebCore::DOMMimeType::description):
+ Ditto.
+ (WebCore::DOMMimeType::enabledPlugin):
+ Directly access the m_pluginInfo member.
+ (WebCore::DOMMimeType::mimeClassInfo): Deleted.
+
+ * plugins/DOMMimeType.h:
+ Update member variables.
+
+ * plugins/DOMPlugin.cpp:
+ (WebCore::DOMPlugin::DOMPlugin):
+ Instead of storing the plugin index, store the plugin info directly.
+ (WebCore::DOMPlugin::name):
+ Directly access m_pluginInfo.
+ (WebCore::DOMPlugin::filename):
+ Ditto.
+ (WebCore::DOMPlugin::description):
+ Ditto.
+ (WebCore::DOMPlugin::length):
+ Ditto.
+ (WebCore::DOMPlugin::item):
+ Access m_pluginInfo directly; find the matching plug-in based on matching PluginInfo (for which an overloaded
+ comparator is supplied below).
+ (WebCore::DOMPlugin::pluginInfo): Deleted.
+
+ * plugins/DOMPlugin.h:
+ Update member variables.
+ (WebCore::DOMPlugin::create):
+ Accept a PluginInfo instead of a plugin index.
+
+ * plugins/DOMPluginArray.cpp:
+ (WebCore::DOMPluginArray::item):
+ (WebCore::DOMPluginArray::namedItem):
+
+ * plugins/PluginData.h:
+ (WebCore::operator==):
+ Added; compare PluginInfo structs on the basis of member equality.
+
2015-06-17 Alex Christensen <[email protected]>
[Content Extensions] Log blocked loads to the WebInspector console
Modified: trunk/Source/WebCore/plugins/DOMMimeType.cpp (185716 => 185717)
--- trunk/Source/WebCore/plugins/DOMMimeType.cpp 2015-06-18 19:21:22 UTC (rev 185716)
+++ trunk/Source/WebCore/plugins/DOMMimeType.cpp 2015-06-18 19:32:15 UTC (rev 185717)
@@ -32,8 +32,12 @@
DOMMimeType::DOMMimeType(PassRefPtr<PluginData> pluginData, Frame* frame, unsigned index)
: FrameDestructionObserver(frame)
, m_pluginData(pluginData)
- , m_index(index)
{
+ Vector<MimeClassInfo> mimes;
+ Vector<size_t> mimePluginIndices;
+ m_pluginData->getWebVisibleMimesAndPluginIndices(mimes, mimePluginIndices);
+ m_mimeClassInfo = mimes[index];
+ m_pluginInfo = m_pluginData->webVisiblePlugins()[mimePluginIndices[index]];
}
DOMMimeType::~DOMMimeType()
@@ -42,12 +46,12 @@
String DOMMimeType::type() const
{
- return mimeClassInfo().type;
+ return m_mimeClassInfo.type;
}
String DOMMimeType::suffixes() const
{
- const Vector<String>& extensions = mimeClassInfo().extensions;
+ const Vector<String>& extensions = m_mimeClassInfo.extensions;
StringBuilder builder;
for (size_t i = 0; i < extensions.size(); ++i) {
@@ -60,17 +64,9 @@
String DOMMimeType::description() const
{
- return mimeClassInfo().desc;
+ return m_mimeClassInfo.desc;
}
-MimeClassInfo DOMMimeType::mimeClassInfo() const
-{
- Vector<MimeClassInfo> mimes;
- Vector<size_t> mimePluginIndices;
- m_pluginData->getWebVisibleMimesAndPluginIndices(mimes, mimePluginIndices);
- return mimes[m_index];
-}
-
PassRefPtr<DOMPlugin> DOMMimeType::enabledPlugin() const
{
if (!m_frame || !m_frame->page() || !m_frame->page()->mainFrame().loader().subframeLoader().allowPlugins(NotAboutToInstantiatePlugin))
@@ -79,7 +75,7 @@
Vector<MimeClassInfo> mimes;
Vector<size_t> mimePluginIndices;
m_pluginData->getWebVisibleMimesAndPluginIndices(mimes, mimePluginIndices);
- return DOMPlugin::create(m_pluginData.get(), m_frame, mimePluginIndices[m_index]);
+ return DOMPlugin::create(m_pluginData.get(), m_frame, m_pluginInfo);
}
} // namespace WebCore
Modified: trunk/Source/WebCore/plugins/DOMMimeType.h (185716 => 185717)
--- trunk/Source/WebCore/plugins/DOMMimeType.h 2015-06-18 19:21:22 UTC (rev 185716)
+++ trunk/Source/WebCore/plugins/DOMMimeType.h 2015-06-18 19:32:15 UTC (rev 185717)
@@ -42,11 +42,10 @@
PassRefPtr<DOMPlugin> enabledPlugin() const;
private:
- MimeClassInfo mimeClassInfo() const;
-
DOMMimeType(PassRefPtr<PluginData>, Frame*, unsigned index);
+ MimeClassInfo m_mimeClassInfo;
RefPtr<PluginData> m_pluginData;
- unsigned m_index;
+ PluginInfo m_pluginInfo;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/plugins/DOMPlugin.cpp (185716 => 185717)
--- trunk/Source/WebCore/plugins/DOMPlugin.cpp 2015-06-18 19:21:22 UTC (rev 185716)
+++ trunk/Source/WebCore/plugins/DOMPlugin.cpp 2015-06-18 19:32:15 UTC (rev 185717)
@@ -25,10 +25,10 @@
namespace WebCore {
-DOMPlugin::DOMPlugin(PluginData* pluginData, Frame* frame, unsigned index)
+DOMPlugin::DOMPlugin(PluginData* pluginData, Frame* frame, PluginInfo pluginInfo)
: FrameDestructionObserver(frame)
, m_pluginData(pluginData)
- , m_index(index)
+ , m_pluginInfo(WTF::move(pluginInfo))
{
}
@@ -38,41 +38,37 @@
String DOMPlugin::name() const
{
- return pluginInfo().name;
+ return m_pluginInfo.name;
}
String DOMPlugin::filename() const
{
- return pluginInfo().file;
+ return m_pluginInfo.file;
}
String DOMPlugin::description() const
{
- return pluginInfo().desc;
+ return m_pluginInfo.desc;
}
unsigned DOMPlugin::length() const
{
- return pluginInfo().mimes.size();
+ return m_pluginInfo.mimes.size();
}
-PluginInfo DOMPlugin::pluginInfo() const
-{
- return m_pluginData->webVisiblePlugins()[m_index];
-}
-
PassRefPtr<DOMMimeType> DOMPlugin::item(unsigned index)
{
- if (index >= pluginInfo().mimes.size())
+ if (index >= m_pluginInfo.mimes.size())
return 0;
- MimeClassInfo mime = pluginInfo().mimes[index];
+ MimeClassInfo mime = m_pluginInfo.mimes[index];
Vector<MimeClassInfo> mimes;
Vector<size_t> mimePluginIndices;
+ Vector<PluginInfo> plugins = m_pluginData->webVisiblePlugins();
m_pluginData->getWebVisibleMimesAndPluginIndices(mimes, mimePluginIndices);
for (unsigned i = 0; i < mimes.size(); ++i) {
- if (mimes[i] == mime && mimePluginIndices[i] == m_index)
+ if (mimes[i] == mime && plugins[mimePluginIndices[i]] == m_pluginInfo)
return DOMMimeType::create(m_pluginData.get(), m_frame, i);
}
return 0;
Modified: trunk/Source/WebCore/plugins/DOMPlugin.h (185716 => 185717)
--- trunk/Source/WebCore/plugins/DOMPlugin.h 2015-06-18 19:21:22 UTC (rev 185716)
+++ trunk/Source/WebCore/plugins/DOMPlugin.h 2015-06-18 19:32:15 UTC (rev 185717)
@@ -33,7 +33,7 @@
class DOMPlugin : public ScriptWrappable, public RefCounted<DOMPlugin>, public FrameDestructionObserver {
public:
- static Ref<DOMPlugin> create(PluginData* pluginData, Frame* frame, unsigned index) { return adoptRef(*new DOMPlugin(pluginData, frame, index)); }
+ static Ref<DOMPlugin> create(PluginData* pluginData, Frame* frame, PluginInfo pluginInfo) { return adoptRef(*new DOMPlugin(pluginData, frame, WTF::move(pluginInfo))); }
~DOMPlugin();
String name() const;
@@ -47,11 +47,9 @@
PassRefPtr<DOMMimeType> namedItem(const AtomicString& propertyName);
private:
- PluginInfo pluginInfo() const;
-
- DOMPlugin(PluginData*, Frame*, unsigned index);
+ DOMPlugin(PluginData*, Frame*, PluginInfo);
RefPtr<PluginData> m_pluginData;
- unsigned m_index;
+ PluginInfo m_pluginInfo;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/plugins/DOMPluginArray.cpp (185716 => 185717)
--- trunk/Source/WebCore/plugins/DOMPluginArray.cpp 2015-06-18 19:21:22 UTC (rev 185716)
+++ trunk/Source/WebCore/plugins/DOMPluginArray.cpp 2015-06-18 19:32:15 UTC (rev 185717)
@@ -61,7 +61,7 @@
const Vector<PluginInfo>& plugins = data->webVisiblePlugins();
if (index >= plugins.size())
return 0;
- return DOMPlugin::create(data, m_frame, index);
+ return DOMPlugin::create(data, m_frame, plugins[index]);
}
bool DOMPluginArray::canGetItemsForName(const AtomicString& propertyName)
@@ -83,10 +83,9 @@
if (!data)
return 0;
- const Vector<PluginInfo>& plugins = data->webVisiblePlugins();
- for (unsigned i = 0; i < plugins.size(); ++i) {
- if (plugins[i].name == propertyName)
- return DOMPlugin::create(data, m_frame, i);
+ for (auto& plugin : data->webVisiblePlugins()) {
+ if (plugin.name == propertyName)
+ return DOMPlugin::create(data, m_frame, plugin);
}
return 0;
}
Modified: trunk/Source/WebCore/plugins/PluginData.h (185716 => 185717)
--- trunk/Source/WebCore/plugins/PluginData.h 2015-06-18 19:21:22 UTC (rev 185716)
+++ trunk/Source/WebCore/plugins/PluginData.h 2015-06-18 19:32:15 UTC (rev 185717)
@@ -73,6 +73,15 @@
#endif
};
+inline bool operator==(PluginInfo& a, PluginInfo& b)
+{
+ bool result = a.name == b.name && a.file == b.file && a.desc == b.desc && a.mimes == b.mimes && a.isApplicationPlugin == b.isApplicationPlugin && a.clientLoadPolicy == b.clientLoadPolicy;
+#if PLATFORM(MAC)
+ result = result && a.bundleIdentifier == b.bundleIdentifier && a.versionString == b.versionString;
+#endif
+ return result;
+}
+
// FIXME: merge with PluginDatabase in the future
class PluginData : public RefCounted<PluginData> {
public:
