Bug 112834 – [WebKit2] Plugins without a MIME Type fail to load

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Xan Lopez]

The flash player in PodOmatic won't show in WebKit2 with the flash player properly installed. For example: http://mmnemosine.podomatic.com/entry/2013-03-14T18_16_33-07_00 (but just check any podcast in that site).

Seems to work fine in Firefox or other browsers.

[Carlos Garcia Campos]

The problem is that when the plugin is created the MIME Type is empty, and the MIME Type guessed in the UI process using the plugin extension is not passed to the WebProcess. When the plugin is instantiated, the value of NPMIMEType parameter passed to NPP_New is NULL, and NPERR_INVALID_INSTANCE_ERROR is returned. I don't think this is specific to GTK+.

[Carlos Garcia Campos]

Created attachment 194756 [details]
Patch

[Carlos Garcia Campos]

Created attachment 194759 [details]
Rebased patch

Patch rebased to apply on current git master.

[WebKit Review Bot]

Comment on attachment 194759 [details]
Rebased patch

Attachment 194759 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17288322

New failing tests:
fast/css/font-family-pictograph.html

[WebKit Review Bot]

Created attachment 194760 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Anders Carlsson]

Comment on attachment 194759 [details]
Rebased patch

I think this is the right approach, but it'd like to see a layout test as well. I can help with the Mac plug-in modifications if needed.

[Benjamin Poulain]

Comment on attachment 194759 [details]
Rebased patch

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

Som comments bellow + I think this deserve a test.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1333
> -void WebPageProxy::getPluginPath(const String& mimeType, const String& urlString, const String& frameURLString, const String& pageURLString, String& pluginPath, uint32_t& pluginLoadPolicy)
> +void WebPageProxy::getPluginPath(const String& mimeType, const String& urlString, const String& frameURLString, const String& pageURLString, String& pluginPath, String& newMimeType, uint32_t& pluginLoadPolicy)

The method become badly named with the addition.
I think it is okay to have the new MIME type returned here, but not from a function named getPluginPath.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1337
> -    String newMimeType = mimeType.lower();
> +    newMimeType = mimeType.lower();

A bit unrelated but I am surprised you need to call lower() here. Usually, when handling MIME types in WebCore, we make them lower case as soon as we get the input for the user space.

[Carlos Garcia Campos]

(In reply to comment #7)
> (From update of attachment 194759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194759&action=review
> 
> Som comments bellow + I think this deserve a test.

Sure, that's what's Anders told me, I was already looking at it.

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1333
> > -void WebPageProxy::getPluginPath(const String& mimeType, const String& urlString, const String& frameURLString, const String& pageURLString, String& pluginPath, uint32_t& pluginLoadPolicy)
> > +void WebPageProxy::getPluginPath(const String& mimeType, const String& urlString, const String& frameURLString, const String& pageURLString, String& pluginPath, String& newMimeType, uint32_t& pluginLoadPolicy)
> 
> The method become badly named with the addition.
> I think it is okay to have the new MIME type returned here, but not from a function named getPluginPath.

I agree, it probably become badly named when the policy parameter was added. What about findPlugin? What we are doing is calling PluginInfoStore::findPlugin() in the end.

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1337
> > -    String newMimeType = mimeType.lower();
> > +    newMimeType = mimeType.lower();
> 
> A bit unrelated but I am surprised you need to call lower() here. Usually, when handling MIME types in WebCore, we make them lower case as soon as we get the input for the user space.

Note that I haven't added the .lower(), it was already there, I don't know if it's needed, but I agree it should probably be done before.

[Carlos Garcia Campos]

I've noticed there's a layout test for this already, plugins/no-mime-with-valid-extension.html which  is listed as 'Unexplained plugin failures' in wk2/TestExpectations. Unfortunately I can't test it right now because the plugin process is currently broken in GTK (see bug #114901), but I bet my patch fixes that test.

[Benjamin Poulain]

(In reply to comment #9)
> I've noticed there's a layout test for this already, plugins/no-mime-with-valid-extension.html which  is listed as 'Unexplained plugin failures' in wk2/TestExpectations. Unfortunately I can't test it right now because the plugin process is currently broken in GTK (see bug #114901), but I bet my patch fixes that test.

Unskip that test, push the patch again, and EWS will tell you if the test is failing :)

[Carlos Garcia Campos]

Created attachment 198961 [details]
Updated patch

GetPluginPath message has been renamed ad FindPlugin and plugins/no-mime-with-valid-extension.html unskipped.

[Anders Carlsson]

(In reply to comment #11)
> Created an attachment (id=198961) [details]
> Updated patch
> 
> GetPluginPath message has been renamed ad FindPlugin and plugins/no-mime-with-valid-extension.html unskipped.

Yay, looks great!

[Carlos Garcia Campos]

Committed r148860: <http://trac.webkit.org/changeset/148860>