Changeset 185717 in webkit

Timestamp:

Jun 18, 2015, 12:32:15 PM (11 years ago)

Author:

Conrad Shultz

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.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• plugins/DOMMimeType.cpp (modified) (4 diffs)
• plugins/DOMMimeType.h (modified) (1 diff)
• plugins/DOMPlugin.cpp (modified) (2 diffs)
• plugins/DOMPlugin.h (modified) (2 diffs)
• plugins/DOMPluginArray.cpp (modified) (2 diffs)
• plugins/PluginData.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-06-17  Conrad Shultz  <conrad_shultz@apple.com>
+
+        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  <achristensen@webkit.org>
 

trunk/Source/WebCore/plugins/DOMMimeType.cpp

@@ -33 +33 @@
     : 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]];
 }
 
@@ -43 +47 @@
 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;
@@ -61 +65 @@
 String DOMMimeType::description() const
 {
-    return mimeClassInfo().desc;
-}
-
-MimeClassInfo DOMMimeType::mimeClassInfo() const
-{
-    Vector<MimeClassInfo> mimes;
-    Vector<size_t> mimePluginIndices;
-    m_pluginData->getWebVisibleMimesAndPluginIndices(mimes, mimePluginIndices);
-    return mimes[m_index];
+    return m_mimeClassInfo.desc;
 }
 
@@ -80 +76 @@
     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);
 }
 

trunk/Source/WebCore/plugins/DOMMimeType.h

@@ -43 +43 @@
 
 private:
-    MimeClassInfo mimeClassInfo() const;
-
     DOMMimeType(PassRefPtr<PluginData>, Frame*, unsigned index);
+    MimeClassInfo m_mimeClassInfo;
     RefPtr<PluginData> m_pluginData;
-    unsigned m_index;
+    PluginInfo m_pluginInfo;
 };
 

trunk/Source/WebCore/plugins/DOMPlugin.cpp

@@ -26 +26 @@
 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))
 {
 }
@@ -39 +39 @@
 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();
-}
-
-PluginInfo DOMPlugin::pluginInfo() const
-{
-    return m_pluginData->webVisiblePlugins()[m_index];
+    return m_pluginInfo.mimes.size();
 }
 
 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);
     }

trunk/Source/WebCore/plugins/DOMPlugin.h

@@ -34 +34 @@
 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();
 
@@ -48 +48 @@
 
 private:
-    PluginInfo pluginInfo() const;
-
-    DOMPlugin(PluginData*, Frame*, unsigned index);
+    DOMPlugin(PluginData*, Frame*, PluginInfo);
     RefPtr<PluginData> m_pluginData;
-    unsigned m_index;
+    PluginInfo m_pluginInfo;
 };
 

trunk/Source/WebCore/plugins/DOMPluginArray.cpp

@@ -62 +62 @@
     if (index >= plugins.size())
         return 0;
-    return DOMPlugin::create(data, m_frame, index);
+    return DOMPlugin::create(data, m_frame, plugins[index]);
 }
 
@@ -84 +84 @@
         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;

trunk/Source/WebCore/plugins/PluginData.h

@@ -74 +74 @@
 };
 
+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> {