Bug 144399 – REGRESSION: js/dom/navigator-plugins-crash.html asserts a lot

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Alexey Proskuryakov]

Only happening on Apple Yosemite Debug WK2.

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=js%2Fdom%2Fnavigator-plugins-crash.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000001153eee17 WTFCrash + 39
1   com.apple.WebCore             	0x0000000116ae5a99 WTF::CrashOnOverflow::overflowed() + 9
2   com.apple.WebCore             	0x000000011717d791 WTF::Vector<WebCore::PluginInfo, 0ul, WTF::CrashOnOverflow, 16ul>::at(unsigned long) + 81 (Vector.h:660)
3   com.apple.WebCore             	0x000000011717d65d WTF::Vector<WebCore::PluginInfo, 0ul, WTF::CrashOnOverflow, 16ul>::operator[](unsigned long) + 29 (Vector.h:679)
4   com.apple.WebCore             	0x000000011717cfdf WebCore::DOMPlugin::pluginInfo() const + 79 (DOMPlugin.cpp:61)
5   com.apple.WebCore             	0x000000011717cf67 WebCore::DOMPlugin::name() const + 39 (DOMPlugin.cpp:41)
6   com.apple.WebCore             	0x00000001179e6ff7 WebCore::jsDOMPluginName(JSC::ExecState*, JSC::JSObject*, long long, JSC::PropertyName) + 71 (JSDOMPlugin.cpp:240)

[Alexey Proskuryakov]

I don't know why this started now, but the code is somewhat new too, as it was added in  <http://trac.webkit.org/changeset/181562>.

I don't think that there is any guarantee of m_index remaining a valid index.

[Radar WebKit Bug Importer]

<rdar://problem/20750473>

[Alexey Proskuryakov]

Marked the test as flakily crashing in r185598.

[Conrad Shultz]

Created attachment 255023 [details]
Patch

[Darin Adler]

Comment on attachment 255023 [details]
Patch

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

> Source/WebCore/plugins/DOMPlugin.cpp:31
> +    , m_pluginInfo(pluginInfo)

I think this could be WTF::move(pluginInfo) for slightly better performance.

> Source/WebCore/plugins/DOMPlugin.h:36
> +    static Ref<DOMPlugin> create(PluginData* pluginData, Frame* frame, PluginInfo pluginInfo) { return adoptRef(*new DOMPlugin(pluginData, frame, pluginInfo)); }

I think this could use WTF::move(pluginInfo) for slightly better performance.

Should take a PluginData& argument instead of PluginData*.

> Source/WebCore/plugins/DOMPluginArray.cpp:90
>      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);
> +            return DOMPlugin::create(data, m_frame, plugins[i]);
>      }

Should be rewritten as a modern for loop:

    for (auto& plugin : data->webVisiblePlugins()) {
        if (plugin.name == propertyName)
            return DOMPlugin::create(data, m_frame, plugin);
    }

The loop used the old fashioned form only because it wanted an "i" to pass to DOMPlugin::create.

> Source/WebCore/plugins/PluginData.h:76
> +inline bool operator==(PluginInfo a, PluginInfo b)

Better to take const PluginInfo& arguments instead of copying the PluginInfo objects just to compare them.

[David Kilzer (:ddkilzer)]

Please revert the change in r185598 when this lands.

[Conrad Shultz]

Comment on attachment 255023 [details]
Patch

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

Thanks, Darin!

>> Source/WebCore/plugins/DOMPlugin.cpp:31
>> +    , m_pluginInfo(pluginInfo)
> 
> I think this could be WTF::move(pluginInfo) for slightly better performance.

Done.

>> Source/WebCore/plugins/DOMPlugin.h:36
>> +    static Ref<DOMPlugin> create(PluginData* pluginData, Frame* frame, PluginInfo pluginInfo) { return adoptRef(*new DOMPlugin(pluginData, frame, pluginInfo)); }
> 
> I think this could use WTF::move(pluginInfo) for slightly better performance.
> 
> Should take a PluginData& argument instead of PluginData*.

I adopted WTF::move(), but changing PluginData is outside the scope of this bug and has wider ramifications, so I'd prefer to address that separately.

>> Source/WebCore/plugins/DOMPluginArray.cpp:90
>>      }
> 
> Should be rewritten as a modern for loop:
> 
>     for (auto& plugin : data->webVisiblePlugins()) {
>         if (plugin.name == propertyName)
>             return DOMPlugin::create(data, m_frame, plugin);
>     }
> 
> The loop used the old fashioned form only because it wanted an "i" to pass to DOMPlugin::create.

Good catch. Fixed.

>> Source/WebCore/plugins/PluginData.h:76
>> +inline bool operator==(PluginInfo a, PluginInfo b)
> 
> Better to take const PluginInfo& arguments instead of copying the PluginInfo objects just to compare them.

Yup, done.

[Conrad Shultz]

Committed r185717: <http://trac.webkit.org/changeset/185717>

[Conrad Shultz]

(In reply to comment #6)
> Please revert the change in r185598 when this lands.

Done in r185718.