Bug 112834

Summary: [WebKit2] Plugins without a MIME Type fail to load
Product: WebKit Reporter: Xan Lopez <xan.lopez>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, cgarcia, dglazkov, mrobinson, sam, thorton, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Rebased patch
benjamin: review-, webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64
none
Updated patch benjamin: review+

Description Xan Lopez 2013-03-20 13:38:50 PDT
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.
Comment 1 Carlos Garcia Campos 2013-03-24 09:36:35 PDT
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+.
Comment 2 Carlos Garcia Campos 2013-03-24 09:44:26 PDT
Created attachment 194756 [details]
Patch
Comment 3 Carlos Garcia Campos 2013-03-24 11:20:12 PDT
Created attachment 194759 [details]
Rebased patch

Patch rebased to apply on current git master.
Comment 4 WebKit Review Bot 2013-03-24 12:05:47 PDT
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
Comment 5 WebKit Review Bot 2013-03-24 12:05:51 PDT
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
Comment 6 Anders Carlsson 2013-04-18 08:25:58 PDT
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.
Comment 7 Benjamin Poulain 2013-04-18 13:20:51 PDT
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.
Comment 8 Carlos Garcia Campos 2013-04-18 23:37:21 PDT
(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.
Comment 9 Carlos Garcia Campos 2013-04-20 12:39:48 PDT
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.
Comment 10 Benjamin Poulain 2013-04-20 14:03:19 PDT
(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 :)
Comment 11 Carlos Garcia Campos 2013-04-21 01:45:41 PDT
Created attachment 198961 [details]
Updated patch

GetPluginPath message has been renamed ad FindPlugin and plugins/no-mime-with-valid-extension.html unskipped.
Comment 12 Anders Carlsson 2013-04-21 20:14:57 PDT
(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!
Comment 13 Carlos Garcia Campos 2013-04-22 00:24:17 PDT
Committed r148860: <http://trac.webkit.org/changeset/148860>