Bug 24988 – The Webkit plugin mime type matching code should allow the wildcards

• Component: Plug-ins
• Status: RESOLVED WONTFIX

[Ananta Iyengar]

The Webkit plugin mime type matching code should allow plugins to specify the wildcard mime type (*) on the same lines as Firefox and Chromium. Firefox uses the npnul32 plugin which specifies its mime type as * for NPAPI plugin installation. Chromium does the same. The wildcard mime type is also useful for NPAPI spy applications, which would be very useful for debugging.

[Ananta Iyengar]

Created attachment 29166 [details]
Patch for supporting wildcards in the plugin mime type matching code

[Ananta Iyengar]

Comment on attachment 29166 [details]
Patch for supporting wildcards in the plugin mime type matching code

Will upload a new patch.

[Ananta Iyengar]

Created attachment 29194 [details]
Updated patch

[Darin Adler]

Comment on attachment 29194 [details]
Updated patch

> -bool PluginData::supportsMimeType(const String& mimeType) const
> +bool PluginData::supportsMimeType(const String& mimeType, bool allowWildCard) const

I suggest two separate functions rather than adding a boolean argument to this function. The boolean is extremely unclear at the call site. You see a "false" or "true" and have no idea what it means, requiring a comment at the call site. Instead the version that doesn't allow wild cards could be named supportsMimeTypeWithoutConsideringWildCards, perhaps.

Other aspects of this patch look good.

[Darin Adler]

There's a tab in the ChangeLog too. That should be fixed.

[Darin Adler]

Comment on attachment 29194 [details]
Updated patch

> +        if (m_mimes[i]->type == mimeType) {
> +            return true;
> +        } else if (allowWildCard && m_mimes[i]->type == "*") {
>              return true;
> +        }

Single line if statements bodies don't get braces in the WebKit coding style (documented in the style document).

No else after return in the WebKit coding style.

> +    PluginPackage* pluginPackageSupportingWildcards = NULL;

WebKit coding style uses 0, not NULL.

> +            // We check for exact mime type matches and wildcard (*) matches. 
> +            if ((mimeiter->first == key) || (mimeiter->first == "*"))

We normally don't use the extra parentheses as done here.

> -    if (pluginChoices.isEmpty())
> +    if (pluginChoices.isEmpty()) {
>          return 0;
> +    }

This changes code that's correct according to the WebKit style to a different style, perhaps one you prefer. But we have a standard you should adhere to.

>      } else if (fileName() == "npmozax.dll")
>          // Bug 15217: Mozilla ActiveX control complains about missing xpcom_core.dll
>          return true;
> +      else if (fileName() == "npnul32.dll")
> +        // We don't want to load the Mozilla NULL plugin, which is used to install
> +        // NPAPI plugins.
> +        return true;

These multiple line bodies need braces to fit the style. Both the npmozax.dll case that's already.

I don't think the name of the plug-in is "NULL". You can call it the "null plug-in" instead of the "NULL plugin".

> -    return pluginData && !type.isEmpty() && pluginData->supportsMimeType(type);
> +    // We pass in true to the supportsMimeType function in PluginData to allow
> +    // plugins which support the wildcard mime type * to be instantiated.
> +    return pluginData && !type.isEmpty() && pluginData->supportsMimeType(type, true);

We want to avoid having to write two-line comments just to explain the boolean, so please take my suggestion about not using a boolean.

Do we want other styles of wildcard to work, such as "text/*", or is it just "*" that we need?

[Ananta Iyengar]

Comment on attachment 29194 [details]
Updated patch

Will upload a new patch addressing the comments on this one.

[Ananta Iyengar]

Created attachment 29206 [details]
Updated patch

Hi Darin

Thanks for the review. I have uploaded a new patch which addresses your comments. We only need to match the * mime type on the same lines as Firefox.

Thanks
Ananta

[Ananta Iyengar]

Comment on attachment 29206 [details]
Updated patch

Will upload a new patch emulating the wildcard plugin matching behavior on the Mac.

[Ananta Iyengar]

Created attachment 29209 [details]
Updated patch with the wildcard mime type matching behavior on the mac.

Hi Darin

I have uploaded a new patch containing similar changes on the Mac plugin branch.

Thanks
Ananta

[Darin Adler]

Comment on attachment 29209 [details]
Updated patch with the wildcard mime type matching behavior on the mac.

> -        if ([[[plugin performSelector:enumeratorSelector] allObjects] containsObject:key]) {
> +        if ([[[plugin performSelector:enumeratorSelector] allObjects] containsObject:key] ||
> +            [[[plugin performSelector:enumeratorSelector] allObjects] containsObject:@"*"]) {

This is inefficient. We don't want to call allObjects twice. We should put the result of allObjects into a local variable and then use it twice.

[Ananta Iyengar]

Created attachment 29221 [details]
Updated patch with review comments addressed

I have uploaded a new patch which addresses the comments on the earlier patch.

-Ananta

[Ananta Iyengar]

Created attachment 29244 [details]
Updated patch with the layout test LayoutTests/plugins/plugin-javascript-access.html rebaselined

Hi Darin

Please take a look at the updated patch. I had to rebaseline the LayoutTests/plugins/plugin-javascript-access.html file as we support an additional mime type * in the layout test plugin.

Thanks
Ananta

[Darin Fisher (:fishd, Google)]

How about the slightly shorter name supportsMimeTypeIgnoringWildCards instead of supportsMimeTypeWithoutConsideringWildCards?

nit: "mimeiter" should probably be "mimeIter" or maybe just "iter"

otherwise, LGTM

[Ananta Iyengar]

Created attachment 29320 [details]
Updated patch with review comments addressed

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/42321

[Darin Fisher (:fishd, Google)]

Reverted because layout tests broke

http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/205/steps/layout-test/logs/stdio
http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Tests%29/builds/163/steps/layout-test/logs/stdio
http://build.webkit.org/builders/Windows%20Release%20%28Tests%29/builds/46/steps/layout-test/logs/stdio
http://build.webkit.org/builders/Windows%20Debug%20%28Tests%29/builds/52/steps/layout-test/logs/stdio

[Darin Fisher (:fishd, Google)]

The revert was http://trac.webkit.org/changeset/42324

[David Levin]

Comment on attachment 29320 [details]
Updated patch with review comments addressed

Cleared r+ to move out of commit queue (since it was landed and then backed out due to layout tests regressions).

[David Levin]

Comment on attachment 29206 [details]
Updated patch

Clear r+ to move out of commit queue.

[Ahmad Saleem]

NPAPI Plugin support has been removed from Safari 14 onward (and for JAVA, it was dropped in 12). Further, Webkit Plugin is not support since Safari 5.1 (based on below link).

Can this be marked as "RESOLVED WONTFIX"? If I am wrong, please ignore my comment. Thanks!

Link - https://developer.apple.com/library/archive/documentation/InternetWeb/Conceptual/WebKit_PluginProgTopic/WebKitPluginTopics.html

Although - if I search Webkit GitHub, I do see "PluginData.cpp":

https://github.com/WebKit/WebKit/blob/c68178ddc01939b4bce3cdb9718c62087649dad4/Source/WebCore/plugins/PluginData.cpp

and "SupportsMIMEType" etc.

Is it something which can be enhanced via this patch in current code? If yes, please ignore my "WONTFIX" suggestion. Thanks!